hs-v3: Don't BUG() if the RP node_t is invalid client side

When sending the INTRODUCE1 cell, we acquire the needed data for the cell but
if the RP node_t has invalid data, we'll fail the send and completely kill the
SOCKS connection.

Instead, close the rendezvous circuit and return a transient error meaning
that Tor can recover by selecting a new rendezvous point. We'll also do the
same when we are unable to encode the INTRODUCE1 cell for which at that point,
we'll simply take another shot at a new rendezvous point.

Fixes #27774

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

changes/ticket27774

@@ -0,0 +1,4 @@
+  o Minor bugfixes (hidden service v3):
+    - Client side would dump a stack trace if tor doesn't have the descriptor
+      for the intro point it is trying to connect to. Fixes bug 27774; bugfix
+      on 0.3.2.1-alpha.

src/feature/hs/hs_circuit.c

@@ -646,16 +646,16 @@ get_lspecs_from_node(const node_t *node, smartlist_t *lspecs)
  * already allocated intro1_data object with the needed key material and link
  * specifiers.
  *
- * If rp_node has an invalid primary address, intro1_data->link_specifiers
- * will be an empty list. Otherwise, this function can't fail. The ip
- * MUST be a valid object containing the needed keys and authentication
- * method. */
-static void
+ * Return 0 on success or a negative value if we couldn't properly filled the
+ * introduce1 data from the RP node. In other word, it means the RP node is
+ * unusable to use in the introduction. */
+static int
 setup_introduce1_data(const hs_desc_intro_point_t *ip,
                       const node_t *rp_node,
                       const uint8_t *subcredential,
                       hs_cell_introduce1_data_t *intro1_data)
 {
+  int ret = -1;
   smartlist_t *rp_lspecs;
 
   tor_assert(ip);
@@ -667,6 +667,11 @@ setup_introduce1_data(const hs_desc_intro_point_t *ip,
    * circuit that we've picked previously. */
   rp_lspecs = smartlist_new();
   get_lspecs_from_node(rp_node, rp_lspecs);
+  if (smartlist_len(rp_lspecs) == 0) {
+    /* We can't rendezvous without link specifiers. */
+    smartlist_free(rp_lspecs);
+    goto end;
+  }
 
   /* Populate the introduce1 data object. */
   memset(intro1_data, 0, sizeof(hs_cell_introduce1_data_t));
@@ -677,8 +682,17 @@ setup_introduce1_data(const hs_desc_intro_point_t *ip,
   intro1_data->auth_pk = &ip->auth_key_cert->signed_key;
   intro1_data->enc_pk = &ip->enc_key;
   intro1_data->subcredential = subcredential;
-  intro1_data->onion_pk = node_get_curve25519_onion_key(rp_node);
   intro1_data->link_specifiers = rp_lspecs;
+  intro1_data->onion_pk = node_get_curve25519_onion_key(rp_node);
+  if (intro1_data->onion_pk == NULL) {
+    /* We can't rendezvous without the curve25519 onion key. */
+    goto end;
+  }
+  /* Success, we have valid introduce data. */
+  ret = 0;
+
+ end:
+  return ret;
 }
 
 /* ========== */
@@ -1130,14 +1144,13 @@ hs_circ_send_introduce1(origin_circuit_t *intro_circ,
              "Failing.", TO_CIRCUIT(intro_circ)->n_circ_id);
     goto done;
   }
-  setup_introduce1_data(ip, exit_node, subcredential, &intro1_data);
-  /* If we didn't get any link specifiers, it's because our node was
-   * bad. */
-  if (BUG(!intro1_data.link_specifiers) ||
-      !smartlist_len(intro1_data.link_specifiers)) {
-    log_warn(LD_REND, "Unable to get link specifiers for INTRODUCE1 cell on "
-             "circuit %u.", TO_CIRCUIT(intro_circ)->n_circ_id);
-    goto done;
+
+  /* We should never select an invalid rendezvous point in theory but if we
+   * do, this function will fail to populate the introduce data. */
+  if (setup_introduce1_data(ip, exit_node, subcredential, &intro1_data) < 0) {
+    log_warn(LD_REND, "Unable to setup INTRODUCE1 data. The chosen rendezvous "
+                      "point is unusable. Closing circuit.");
+    goto close;
   }
 
   /* Final step before we encode a cell, we setup the circuit identifier which
@@ -1154,7 +1167,7 @@ hs_circ_send_introduce1(origin_circuit_t *intro_circ,
    * into payload which is then ready to be sent as is. */
   payload_len = hs_cell_build_introduce1(&intro1_data, payload);
   if (BUG(payload_len < 0)) {
-    goto done;
+    goto close;
   }
 
   if (relay_send_command_from_edge(CONTROL_CELL_ID, TO_CIRCUIT(intro_circ),
@@ -1171,6 +1184,8 @@ hs_circ_send_introduce1(origin_circuit_t *intro_circ,
   ret = 0;
   goto done;
 
+ close:
+  circuit_mark_for_close(TO_CIRCUIT(rend_circ), END_CIRC_REASON_INTERNAL);
  done:
   hs_cell_introduce1_data_clear(&intro1_data);
   memwipe(payload, 0, sizeof(payload));

src/feature/hs/hs_client.c

@@ -576,10 +576,21 @@ send_introduce1(origin_circuit_t *intro_circ,
   /* Send the INTRODUCE1 cell. */
   if (hs_circ_send_introduce1(intro_circ, rend_circ, ip,
                               desc->subcredential) < 0) {
-    /* Unable to send the cell, the intro circuit has been marked for close so
-     * this is a permanent error. */
-    tor_assert_nonfatal(TO_CIRCUIT(intro_circ)->marked_for_close);
-    goto perm_err;
+    if (TO_CIRCUIT(intro_circ)->marked_for_close) {
+      /* If the introduction circuit was closed, we were unable to send the
+       * cell for some reasons. In any case, the intro circuit has to be
+       * closed by the above function. We'll return a transient error so tor
+       * can recover and pick a new intro point. To avoid picking that same
+       * intro point, we'll note down the intro point failure so it doesn't
+       * get reused. */
+      hs_cache_client_intro_state_note(service_identity_pk,
+                                       &intro_circ->hs_ident->intro_auth_pk,
+                                       INTRO_POINT_FAILURE_GENERIC);
+    }
+    /* It is also possible that the rendezvous circuit was closed due to being
+     * unable to use the rendezvous point node_t so in that case, we also want
+     * to recover and let tor pick a new one. */
+    goto tran_err;
   }
 
   /* Cell has been sent successfully. Copy the introduction point