commit 79265a6fb606e416529f5a1dd31c94f15edec91b Author: David Goulet <dgoulet@torproject.org> Date: Thu Sep 20 09:32:13 2018 -0400 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 | 4 ++++ src/feature/hs/hs_circuit.c | 45 ++++++++++++++++++++++++++++++--------------- src/feature/hs/hs_client.c | 19 +++++++++++++++---- 3 files changed, 49 insertions(+), 19 deletions(-) diff --git a/changes/ticket27774 b/changes/ticket27774 new file mode 100644 index 000000000..2598c4055 --- /dev/null +++ b/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. diff --git a/src/feature/hs/hs_circuit.c b/src/feature/hs/hs_circuit.c index 541b165dd..70760e013 100644 --- a/src/feature/hs/hs_circuit.c +++ b/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)); diff --git a/src/feature/hs/hs_client.c b/src/feature/hs/hs_client.c index a6384b87a..441edc324 100644 --- a/src/feature/hs/hs_client.c +++ b/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