Merge remote-tracking branch 'dgoulet/ticket27471_035_02'

8 files changed, 188 insertions, 14 deletions

diff --git a/changes/ticket27471 b/changes/ticket27471
new file mode 100644
index 0000000000..ffe77d268e
--- /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 5ff142c15c..35efc6541f 100644
--- 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 dac11431c9..cb89d1820d 100644
--- 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 b9bcb446a1..afd69e1bec 100644
--- 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 11e24a3660..dfad216abb 100644
--- 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 fb4f9e9e9f..f6fb167ea2 100644
--- 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 bae9da3fe5..d135581061 100644
--- 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. */
diff --git a/src/test/test_hs_client.c b/src/test/test_hs_client.c
index 25cb991a79..91b3ed1ec4 100644
--- a/src/test/test_hs_client.c
+++ b/src/test/test_hs_client.c
@@ -885,6 +885,107 @@ test_desc_has_arrived_cleanup(void *arg)
   UNMOCK(router_have_minimum_dir_info);
 }
 
+static void
+test_close_intro_circuits_new_desc(void *arg)
+{
+  int ret;
+  ed25519_keypair_t service_kp;
+  circuit_t *circ = NULL;
+  origin_circuit_t *ocirc = NULL;
+  hs_descriptor_t *desc1 = NULL, *desc2 = NULL;
+
+  (void) arg;
+
+  hs_init();
+
+  /* This is needed because of the client cache expiration timestamp is based
+   * on having a consensus. See cached_client_descriptor_has_expired(). */
+  MOCK(networkstatus_get_live_consensus,
+       mock_networkstatus_get_live_consensus);
+
+  /* Set consensus time */
+  parse_rfc1123_time("Sat, 26 Oct 1985 13:00:00 UTC",
+                           &mock_ns.valid_after);
+  parse_rfc1123_time("Sat, 26 Oct 1985 14:00:00 UTC",
+                           &mock_ns.fresh_until);
+  parse_rfc1123_time("Sat, 26 Oct 1985 16:00:00 UTC",
+                           &mock_ns.valid_until);
+
+  /* Generate service keypair */
+  tt_int_op(0, OP_EQ, ed25519_keypair_generate(&service_kp, 0));
+
+  /* Create and add to the global list a dummy client introduction circuits.
+   * We'll then make sure the hs_ident is attached to a dummy descriptor. */
+  circ = dummy_origin_circuit_new(0);
+  tt_assert(circ);
+  circ->purpose = CIRCUIT_PURPOSE_C_INTRODUCING;
+  ocirc = TO_ORIGIN_CIRCUIT(circ);
+
+  /* Build the first descriptor and cache it. */
+  {
+    char *encoded;
+    desc1 = hs_helper_build_hs_desc_with_ip(&service_kp);
+    tt_assert(desc1);
+    ret = hs_desc_encode_descriptor(desc1, &service_kp, NULL, &encoded);
+    tt_int_op(ret, OP_EQ, 0);
+    tt_assert(encoded);
+
+    /* Store it */
+    ret = hs_cache_store_as_client(encoded, &service_kp.pubkey);
+    tt_int_op(ret, OP_EQ, 0);
+    tor_free(encoded);
+    tt_assert(hs_cache_lookup_as_client(&service_kp.pubkey));
+  }
+
+  /* We'll pick one introduction point and associate it with the circuit. */
+  {
+    const hs_desc_intro_point_t *ip =
+      smartlist_get(desc1->encrypted_data.intro_points, 0);
+    tt_assert(ip);
+    ocirc->hs_ident = hs_ident_circuit_new(&service_kp.pubkey,
+                                           HS_IDENT_CIRCUIT_INTRO);
+    ed25519_pubkey_copy(&ocirc->hs_ident->intro_auth_pk,
+                        &ip->auth_key_cert->signed_key);
+  }
+
+  /* Before we are about to clean up the intro circuits, make sure it is
+   * actually there. */
+  tt_assert(circuit_get_next_intro_circ(NULL, true));
+
+  /* Build the second descriptor for the same service and cache it. */
+  {
+    char *encoded;
+    desc2 = hs_helper_build_hs_desc_with_ip(&service_kp);
+    tt_assert(desc2);
+    tt_mem_op(&desc1->plaintext_data.signing_pubkey, OP_EQ,
+              &desc2->plaintext_data.signing_pubkey, ED25519_PUBKEY_LEN);
+    /* To replace the existing descriptor, the revision counter needs to be
+     * bigger. */
+    desc2->plaintext_data.revision_counter =
+      desc1->plaintext_data.revision_counter + 1;
+
+    ret = hs_desc_encode_descriptor(desc2, &service_kp, NULL, &encoded);
+    tt_int_op(ret, OP_EQ, 0);
+    tt_assert(encoded);
+
+    hs_cache_store_as_client(encoded, &service_kp.pubkey);
+    tt_int_op(ret, OP_EQ, 0);
+    tor_free(encoded);
+    tt_assert(hs_cache_lookup_as_client(&service_kp.pubkey));
+  }
+
+  /* Once stored, our intro circuit should be closed because it is related to
+   * an old introduction point that doesn't exists anymore. */
+  tt_assert(!circuit_get_next_intro_circ(NULL, true));
+
+ done:
+  circuit_free(circ);
+  hs_descriptor_free(desc1);
+  hs_descriptor_free(desc2);
+  hs_free_all();
+  UNMOCK(networkstatus_get_live_consensus);
+}
+
 struct testcase_t hs_client_tests[] = {
   { "e2e_rend_circuit_setup_legacy", test_e2e_rend_circuit_setup_legacy,
     TT_FORK, NULL, NULL },
@@ -902,6 +1003,8 @@ struct testcase_t hs_client_tests[] = {
     TT_FORK, NULL, NULL },
   { "desc_has_arrived_cleanup", test_desc_has_arrived_cleanup,
     TT_FORK, NULL, NULL },
+  { "close_intro_circuits_new_desc", test_close_intro_circuits_new_desc,
+    TT_FORK, NULL, NULL },
 
   END_OF_TESTCASES
 };