[tor-commits] [tor/main] hs: Don't BUG() when setting up RP congestion control

[dgoulet at torproject.org]

commit 069b27860102990b23b58e60fb1de65347f37669
Author: David Goulet <dgoulet at torproject.org>
Date:   Wed Mar 9 12:15:12 2022 -0500

    hs: Don't BUG() when setting up RP congestion control
    
    It is possible to not have the descriptor anymore by the time the
    rendezvous circuit opens. Don't BUG() on that.
    
    Instead, when sending the INTRODUCE1 cell, make sure the descriptor we
    have (or have just fetched) matches what we setup in the rendezvous
    circuit.
    
    If not, the circuit is closed and another one is opened for a retry.
    
    Fixes #40576
    
    Signed-off-by: David Goulet <dgoulet at torproject.org>
---
 changes/ticket40576            |  5 +++++
 src/feature/hs/hs_client.c     | 29 +++++++++++++++++++++++------
 src/feature/hs/hs_descriptor.c | 13 +++++++++++++
 src/feature/hs/hs_descriptor.h |  2 ++
 4 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/changes/ticket40576 b/changes/ticket40576
new file mode 100644
index 0000000000..c9e3745560
--- /dev/null
+++ b/changes/ticket40576
@@ -0,0 +1,5 @@
+  o Minor bugfixes (onion service congestion control):
+    - Avoid a non fatal assert in case we can't setup congestion control on the
+      rendezvous circuit after opening. Fixes bug 40576; bugfix on
+      0.4.7.4-alpha.
+
diff --git a/src/feature/hs/hs_client.c b/src/feature/hs/hs_client.c
index 81c0459a86..f5891e6bdc 100644
--- a/src/feature/hs/hs_client.c
+++ b/src/feature/hs/hs_client.c
@@ -624,6 +624,16 @@ send_introduce1(origin_circuit_t *intro_circ,
     goto tran_err;
   }
 
+  /* Check if the rendevous circuit was setup WITHOUT congestion control but if
+   * it is enabled and the service supports it. This can happen, see
+   * setup_rendezvous_circ_congestion_control() and so close rendezvous circuit
+   * so another one can be created. */
+  if (TO_CIRCUIT(rend_circ)->ccontrol == NULL && congestion_control_enabled()
+      && hs_desc_supports_congestion_control(desc)) {
+    circuit_mark_for_close(TO_CIRCUIT(rend_circ), END_CIRC_REASON_INTERNAL);
+    goto tran_err;
+  }
+
   /* We need to find which intro point in the descriptor we are connected to
    * on intro_circ. */
   ip = find_desc_intro_point_by_ident(intro_circ->hs_ident, desc);
@@ -760,7 +770,14 @@ client_intro_circ_has_opened(origin_circuit_t *circ)
 }
 
 /** Setup the congestion control parameters on the given rendezvous circuit.
- * This looks at the service descriptor flow control line (if any). */
+ * This looks at the service descriptor flow control line (if any).
+ *
+ * It is possible that we are unable to set congestion control on the circuit
+ * if the descriptor can't be found. In that case, the introduction circuit
+ * can't be opened without it so a fetch will be triggered.
+ *
+ * However, if the descriptor asks for congestion control but the RP circuit
+ * doesn't have it, it will be closed and a new circuit will be opened. */
 static void
 setup_rendezvous_circ_congestion_control(origin_circuit_t *circ)
 {
@@ -771,16 +788,16 @@ setup_rendezvous_circ_congestion_control(origin_circuit_t *circ)
   /* Setup congestion control parameters on the circuit. */
   const hs_descriptor_t *desc =
     hs_cache_lookup_as_client(&circ->hs_ident->identity_pk);
-  if (BUG(desc == NULL)) {
-    /* This should really never happened but in case, scream and stop. */
+  if (desc == NULL) {
+    /* This is possible because between launching the circuit and the circuit
+     * ending in opened state, the descriptor could have been removed from the
+     * cache. In this case, we just can't setup congestion control. */
     return;
   }
 
   /* Check if the service lists support for congestion control in its
    * descriptor. If not, we don't setup congestion control. */
-  if (!desc->encrypted_data.flow_control_pv ||
-      !protocol_list_supports_protocol(desc->encrypted_data.flow_control_pv,
-                                       PRT_FLOWCTRL, PROTOVER_FLOWCTRL_CC)) {
+  if (!hs_desc_supports_congestion_control(desc)) {
     return;
   }
 
diff --git a/src/feature/hs/hs_descriptor.c b/src/feature/hs/hs_descriptor.c
index 523ededf8c..15ad9d8efb 100644
--- a/src/feature/hs/hs_descriptor.c
+++ b/src/feature/hs/hs_descriptor.c
@@ -2987,3 +2987,16 @@ hs_descriptor_clear_intro_points(hs_descriptor_t *desc)
     smartlist_clear(ips);
   }
 }
+
+/** Return true iff we support the given descriptor congestion control
+ * parameters. */
+bool
+hs_desc_supports_congestion_control(const hs_descriptor_t *desc)
+{
+  tor_assert(desc);
+
+  /* Validate that we support the protocol version in the descriptor. */
+  return desc->encrypted_data.flow_control_pv &&
+         protocol_list_supports_protocol(desc->encrypted_data.flow_control_pv,
+                                         PRT_FLOWCTRL, PROTOVER_FLOWCTRL_CC);
+}
diff --git a/src/feature/hs/hs_descriptor.h b/src/feature/hs/hs_descriptor.h
index 8f5ee6a2f1..8f42b2138b 100644
--- a/src/feature/hs/hs_descriptor.h
+++ b/src/feature/hs/hs_descriptor.h
@@ -319,6 +319,8 @@ void hs_desc_superencrypted_data_free_contents(
                                         hs_desc_superencrypted_data_t *desc);
 void hs_desc_encrypted_data_free_contents(hs_desc_encrypted_data_t *desc);
 
+bool hs_desc_supports_congestion_control(const hs_descriptor_t *desc);
+
 #ifdef HS_DESCRIPTOR_PRIVATE
 
 /* Encoding. */