hs-v3: Cleanup HS circuits when marking as closed

First, hs_service_intro_circ_has_closed() is now called in circuit_mark_for
close() because the HS subsystem needs to learn when an intro point is
actually not established anymore as soon as possible. There is a time window
between a close and a free.

Second, when we mark for close, we also remove it from the circuitmap because
between the close and the free, a service can launch an new circuit to that
same intro point and thus register it which only succeeds if the intro point
authentication key is not already in the map.

However, we still do a remove from the circuitmap in circuit_free() in order
to also cleanup the circuit if it wasn't marked for close prior to the free.

Fixes #23603

Signed-off-by: David Goulet <dgoulet@torproject.org>

diff --git a/changes/bug23603 b/changes/bug23603
new file mode 100644 (file)
index 0000000..dfb2052
--- /dev/null
+++ b/changes/bug23603
@@ -0,0 +1,7 @@
+  o Minor bugfixes (hidden service v3):
+    - Fix a race between the circuit close and free where the service would
+      launch a new intro circuit after the close, and then fail to register it
+      before the free of the previously closed circuit. This was making the
+      service unable to find the established intro circuit and thus not upload
+      its descriptor. It can make a service unavailable for up to 24 hours.
+      Fixes bug 23603; bugfix on 0.3.2.1-alpha.

diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c
index d37d986f176eb77e771f0dbe7996b5d7939b049b..45d4521c22bc3ce53fa0220aab1d29c756cefbfd 100644 (file)
--- a/src/or/circuitlist.c
+++ b/src/or/circuitlist.c
@@ -67,7 +67,6 @@
 #include "main.h"
 #include "hs_circuit.h"
 #include "hs_circuitmap.h"
-#include "hs_common.h"
 #include "hs_ident.h"
 #include "networkstatus.h"
 #include "nodelist.h"
@@ -938,6 +937,12 @@ circuit_free(circuit_t *circ)
 
   circuit_clear_testing_cell_stats(circ);
 
+  /* Cleanup circuit from anything HS v3 related. We also do this when the
+   * circuit is closed. This is to avoid any code path that free registered
+   * circuits without closing them before. This needs to be done before the
+   * hs identifier is freed. */
+  hs_circ_cleanup(circ);
+
   if (CIRCUIT_IS_ORIGIN(circ)) {
     origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ);
     mem = ocirc;
@@ -963,7 +968,11 @@ circuit_free(circuit_t *circ)
 
     crypto_pk_free(ocirc->intro_key);
     rend_data_free(ocirc->rend_data);
+
+    /* Finally, free the identifier of the circuit and nullify it so multiple
+     * cleanup will work. */
     hs_ident_circuit_free(ocirc->hs_ident);
+    ocirc->hs_ident = NULL;
 
     tor_free(ocirc->dest_address);
     if (ocirc->socks_username) {
@@ -1022,11 +1031,6 @@ circuit_free(circuit_t *circ)
   /* Remove from map. */
   circuit_set_n_circid_chan(circ, 0, NULL);
 
-  /* Clear HS circuitmap token from this circ (if any) */
-  if (circ->hs_token) {
-    hs_circuitmap_remove_circuit(circ);
-  }
-
   /* Clear cell queue _after_ removing it from the map.  Otherwise our
    * "active" checks will be violated. */
   cell_queue_clear(&circ->n_chan_cells);
@@ -1917,6 +1921,9 @@ circuit_mark_for_close_, (circuit_t *circ, int reason, int line,
     }
   }
 
+  /* Notify the HS subsystem that this circuit is closing. */
+  hs_circ_cleanup(circ);
+
   if (circuits_pending_close == NULL)
     circuits_pending_close = smartlist_new();
 
@@ -1997,13 +2004,6 @@ circuit_about_to_free(circuit_t *circ)
      orig_reason);
   }
 
-  /* Notify the HS subsystem for any intro point circuit closing so it can be
-   * dealt with cleanly. */
-  if (circ->purpose == CIRCUIT_PURPOSE_S_ESTABLISH_INTRO ||
-      circ->purpose == CIRCUIT_PURPOSE_S_INTRO) {
-    hs_service_intro_circ_has_closed(TO_ORIGIN_CIRCUIT(circ));
-  }
-
   if (circ->purpose == CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT) {
     origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ);
     int timed_out = (reason == END_CIRC_REASON_TIMEOUT);

diff --git a/src/or/hs_circuit.c b/src/or/hs_circuit.c
index ee952f4d687eacce43d812a43a87765058f0d9f1..a58166ccdec8e28b200cffb093890c26b0d5eecc 100644 (file)
--- a/src/or/hs_circuit.c
+++ b/src/or/hs_circuit.c
@@ -1178,3 +1178,31 @@ hs_circ_send_establish_rendezvous(origin_circuit_t *circ)
   return -1;
 }
 
+/* We are about to close or free this <b>circ</b>. Clean it up from any
+ * related HS data structures. This function can be called multiple times
+ * safely for the same circuit. */
+void
+hs_circ_cleanup(circuit_t *circ)
+{
+  tor_assert(circ);
+
+  /* If it's a service-side intro circ, notify the HS subsystem for the intro
+   * point circuit closing so it can be dealt with cleanly. */
+  if (circ->purpose == CIRCUIT_PURPOSE_S_ESTABLISH_INTRO ||
+      circ->purpose == CIRCUIT_PURPOSE_S_INTRO) {
+    hs_service_intro_circ_has_closed(TO_ORIGIN_CIRCUIT(circ));
+  }
+
+  /* Clear HS circuitmap token for this circ (if any). Very important to be
+   * done after the HS subsystem has been notified of the close else the
+   * circuit will not be found.
+   *
+   * We do this at the close if possible because from that point on, the
+   * circuit is good as dead. We can't rely on removing it in the circuit
+   * free() function because we open a race window between the close and free
+   * where we can't register a new circuit for the same intro point. */
+  if (circ->hs_token) {
+    hs_circuitmap_remove_circuit(circ);
+  }
+}
+

diff --git a/src/or/hs_circuit.h b/src/or/hs_circuit.h
index 0a1186dbaac0e938c4fe5cc84a4070b7d012cbff..63ff5e463ced45e63d24109a543712699f10f054 100644 (file)
--- a/src/or/hs_circuit.h
+++ b/src/or/hs_circuit.h
@@ -15,6 +15,9 @@
 
 #include "hs_service.h"
 
+/* Cleanup function when the circuit is closed or/and freed. */
+void hs_circ_cleanup(circuit_t *circ);
+
 /* Circuit API. */
 int hs_circ_service_intro_has_opened(hs_service_t *service,
                                      hs_service_intro_point_t *ip,

diff --git a/src/or/hs_common.c b/src/or/hs_common.c
index a0f2af29cd3c23a7a0ace3bb9b5a219662ce929b..ad783a36fbcbbc8aa540f20a33c4a03af05028bf 100644 (file)
--- a/src/or/hs_common.c
+++ b/src/or/hs_common.c
@@ -22,6 +22,7 @@
 #include "hs_client.h"
 #include "hs_ident.h"
 #include "hs_service.h"
+#include "hs_circuitmap.h"
 #include "policies.h"
 #include "rendcommon.h"
 #include "rendservice.h"

diff --git a/src/or/hs_common.h b/src/or/hs_common.h
index c95e59a6f89ee9ad72f77fd1a16f584471997cee..18c4e701414e07bf27630fcf0b587ecee96b77b2 100644 (file)
--- a/src/or/hs_common.h
+++ b/src/or/hs_common.h
@@ -161,6 +161,8 @@ typedef struct hsdir_index_t {
 void hs_init(void);
 void hs_free_all(void);
 
+void hs_cleanup_circ(circuit_t *circ);
+
 int hs_check_service_private_dir(const char *username, const char *path,
                                  unsigned int dir_group_readable,
                                  unsigned int create);