hs-v3: Close client intro circuits if the descriptor is replaced

When storing a descriptor in the client cache, if we are about to replace an
existing descriptor, make sure to close every introduction circuits of the old
descriptor so we don't have leftovers lying around.

Ticket 27471 describes a situation where tor is sending an INTRODUCE1 cell on
an introduction circuit for which it doesn't have a matching intro point
object (taken from the descriptor).

The main theory is that, after a new descriptor showed up, the introduction
points changed which led to selecting an introduction circuit not used by the
service anymore thus for which we are unable to find the corresponding
introduction point within the descriptor we just fetched.

Closes #27471.

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

diff --git a/changes/ticket27471 b/changes/ticket27471
new file mode 100644 (file)
index 0000000..ffe77d2
--- /dev/null
+++ b/changes/ticket27471
@@ -0,0 +1,5 @@
+  o Minor bugfixes (hidden service v3, client):
+    - When replacing a descriptor in the client cache with a newer descriptor,
+      make sure to close all client introduction circuits of the old
+      descriptor so we don't end up with unusable leftover circuits. Fixes bug
+      27471; bugfix on 0.3.2.1-alpha.

diff --git a/src/core/or/circuitlist.c b/src/core/or/circuitlist.c
index 5ff142c15cf6b0d7e0704a79d068a9fe0b14ffea..35efc6541f6c5cd9f4efdcc6480f4ade15485ef3 100644 (file)
--- a/src/core/or/circuitlist.c
+++ b/src/core/or/circuitlist.c
@@ -1644,15 +1644,24 @@ circuit_get_ready_rend_circ_by_rend_data(const rend_data_t *rend_data)
   return NULL;
 }
 
-/** Return the first service introduction circuit originating from the global
- * circuit list after <b>start</b> or at the start of the list if <b>start</b>
- * is NULL. Return NULL if no circuit is found.
+/** Return the first introduction circuit originating from the global circuit
+ * list after <b>start</b> or at the start of the list if <b>start</b> is
+ * NULL. Return NULL if no circuit is found.
+ *
+ * If <b>want_client_circ</b> is true, then we are looking for client-side
+ * introduction circuits: A client introduction point circuit has a purpose of
+ * either CIRCUIT_PURPOSE_C_INTRODUCING, CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT
+ * or CIRCUIT_PURPOSE_C_INTRODUCE_ACKED. This does not return a circuit marked
+ * for close, but it returns circuits regardless of their circuit state.
  *
- * A service introduction point circuit has a purpose of either
- * CIRCUIT_PURPOSE_S_ESTABLISH_INTRO or CIRCUIT_PURPOSE_S_INTRO. This does not
- * return a circuit marked for close and its state must be open. */
+ * If <b>want_client_circ</b> is false, then we are looking for service-side
+ * introduction circuits: A service introduction point circuit has a purpose of
+ * either CIRCUIT_PURPOSE_S_ESTABLISH_INTRO or CIRCUIT_PURPOSE_S_INTRO. This
+ * does not return circuits marked for close, or in any state other than open.
+ */
 origin_circuit_t *
-circuit_get_next_service_intro_circ(origin_circuit_t *start)
+circuit_get_next_intro_circ(const origin_circuit_t *start,
+                            bool want_client_circ)
 {
   int idx = 0;
   smartlist_t *lst = circuit_get_global_list();
@@ -1664,13 +1673,29 @@ circuit_get_next_service_intro_circ(origin_circuit_t *start)
   for ( ; idx < smartlist_len(lst); ++idx) {
     circuit_t *circ = smartlist_get(lst, idx);
 
-    /* Ignore a marked for close circuit or purpose not matching a service
-     * intro point or if the state is not open. */
-    if (circ->marked_for_close || circ->state != CIRCUIT_STATE_OPEN ||
-        (circ->purpose != CIRCUIT_PURPOSE_S_ESTABLISH_INTRO &&
-         circ->purpose != CIRCUIT_PURPOSE_S_INTRO)) {
+    /* Ignore a marked for close circuit or if the state is not open. */
+    if (circ->marked_for_close) {
       continue;
     }
+
+    /* Depending on whether we are looking for client or service circs, skip
+     * circuits with other purposes. */
+    if (want_client_circ) {
+      if (circ->purpose != CIRCUIT_PURPOSE_C_INTRODUCING &&
+          circ->purpose != CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT &&
+          circ->purpose != CIRCUIT_PURPOSE_C_INTRODUCE_ACKED) {
+        continue;
+      }
+    } else { /* we are looking for service-side circs */
+      if (circ->state != CIRCUIT_STATE_OPEN) {
+        continue;
+      }
+      if (circ->purpose != CIRCUIT_PURPOSE_S_ESTABLISH_INTRO &&
+          circ->purpose != CIRCUIT_PURPOSE_S_INTRO) {
+        continue;
+      }
+    }
+
     /* The purposes we are looking for are only for origin circuits so the
      * following is valid. */
     return TO_ORIGIN_CIRCUIT(circ);

diff --git a/src/core/or/circuitlist.h b/src/core/or/circuitlist.h
index dac11431c9494c175a609b6f2f5f8720cf93f81f..cb89d1820d80b8cf73969952c801a4a6786270fd 100644 (file)
--- a/src/core/or/circuitlist.h
+++ b/src/core/or/circuitlist.h
@@ -202,7 +202,8 @@ origin_circuit_t *circuit_get_ready_rend_circ_by_rend_data(
   const rend_data_t *rend_data);
 origin_circuit_t *circuit_get_next_by_pk_and_purpose(origin_circuit_t *start,
                                      const uint8_t *digest, uint8_t purpose);
-origin_circuit_t *circuit_get_next_service_intro_circ(origin_circuit_t *start);
+origin_circuit_t *circuit_get_next_intro_circ(const origin_circuit_t *start,
+                                              bool want_client_circ);
 origin_circuit_t *circuit_get_next_service_rp_circ(origin_circuit_t *start);
 origin_circuit_t *circuit_get_next_service_hsdir_circ(origin_circuit_t *start);
 origin_circuit_t *circuit_find_to_cannibalize(uint8_t purpose,

diff --git a/src/feature/hs/hs_cache.c b/src/feature/hs/hs_cache.c
index b9bcb446a183fa69c09ca916aabe5cd61b22fa28..afd69e1bec3b6fb0802c73bbe6faf81ecd07c571 100644 (file)
--- a/src/feature/hs/hs_cache.c
+++ b/src/feature/hs/hs_cache.c
@@ -647,6 +647,13 @@ cache_store_as_client(hs_cache_client_descriptor_t *client_desc)
     }
     /* Remove old entry. Make space for the new one! */
     remove_v3_desc_as_client(cache_entry);
+
+    /* We just removed an old descriptor and will replace it. We'll close all
+     * intro circuits related to this old one so we don't have leftovers. We
+     * leave the rendezvous circuits opened because they could be in use. */
+    hs_client_close_intro_circuits_from_desc(cache_entry->desc);
+
+    /* Free it. */
     cache_client_desc_free(cache_entry);
   }
 

diff --git a/src/feature/hs/hs_client.c b/src/feature/hs/hs_client.c
index 11e24a3660ff33a0e7088a31a95cfa20539769e0..dfad216abb8824a5318fd4656d32b6fb77849780 100644 (file)
--- a/src/feature/hs/hs_client.c
+++ b/src/feature/hs/hs_client.c
@@ -1844,6 +1844,38 @@ hs_client_reextend_intro_circuit(origin_circuit_t *circ)
   return ret;
 }
 
+/* Close all client introduction circuits related to the given descriptor.
+ * This is called with a descriptor that is about to get replaced in the
+ * client cache.
+ *
+ * Even though the introduction point might be exactly the same, we'll rebuild
+ * them if needed but the odds are very low that an existing matching
+ * introduction circuit exists at that stage. */
+void
+hs_client_close_intro_circuits_from_desc(const hs_descriptor_t *desc)
+{
+  origin_circuit_t *ocirc = NULL;
+
+  tor_assert(desc);
+
+  /* We iterate over all client intro circuits because they aren't kept in the
+   * HS circuitmap. That is probably something we want to do one day. */
+  while ((ocirc = circuit_get_next_intro_circ(ocirc, true))) {
+    if (ocirc->hs_ident == NULL) {
+      /* Not a v3 circuit, ignore it. */
+      continue;
+    }
+
+    /* Does it match any IP in the given descriptor? If not, ignore. */
+    if (find_desc_intro_point_by_ident(ocirc->hs_ident, desc) == NULL) {
+      continue;
+    }
+
+    /* We have a match. Close the circuit as consider it expired. */
+    circuit_mark_for_close(TO_CIRCUIT(ocirc), END_CIRC_REASON_FINISHED);
+  }
+}
+
 /* Release all the storage held by the client subsystem. */
 void
 hs_client_free_all(void)

diff --git a/src/feature/hs/hs_client.h b/src/feature/hs/hs_client.h
index fb4f9e9e9f022f243d7f1d08c4749e54757be247..f6fb167ea2a143b04e41b0c190dcdf5e601bc37d 100644 (file)
--- a/src/feature/hs/hs_client.h
+++ b/src/feature/hs/hs_client.h
@@ -77,6 +77,7 @@ int hs_config_client_authorization(const or_options_t *options,
                                    int validate_only);
 
 int hs_client_reextend_intro_circuit(origin_circuit_t *circ);
+void hs_client_close_intro_circuits_from_desc(const hs_descriptor_t *desc);
 
 void hs_client_purge_state(void);
 

diff --git a/src/feature/rend/rendservice.c b/src/feature/rend/rendservice.c
index bae9da3fe59b3a65ef054c38d9de915eb03c0656..d1355810617e9c9db70ee5b9e9aa4c076382d01d 100644 (file)
--- a/src/feature/rend/rendservice.c
+++ b/src/feature/rend/rendservice.c
@@ -631,7 +631,7 @@ rend_service_prune_list_impl_(void)
 
   /* For every service introduction circuit we can find, see if we have a
    * matching surviving configured service. If not, close the circuit. */
-  while ((ocirc = circuit_get_next_service_intro_circ(ocirc))) {
+  while ((ocirc = circuit_get_next_intro_circ(ocirc, false))) {
     int keep_it = 0;
     if (ocirc->rend_data == NULL) {
       /* This is a v3 circuit, ignore it. */