November 2019 - tor-commits - lists.torproject.org

[tor/master] hs-v3: Note client intro circuit failure
by asn＠torproject.org 27 Nov '19

27 Nov '19

commit 7f83c43594dcf13fb04352f5faa8db2cd86354c1 Author: David Goulet <dgoulet(a)torproject.org> Date: Wed Oct 30 15:56:17 2019 -0400 hs-v3: Note client intro circuit failure Report back to the v3 subsystem any introduction point client circuit failure so they can be noted down in the failure cache. Fixes #32020 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/feature/hs/hs_circuit.c | 34 ++++++++++++++++++------------ src/feature/hs/hs_client.c | 50 +++++++++++++++++++++++++++++++++++++++++++++ src/feature/hs/hs_client.h | 1 + 3 files changed, 72 insertions(+), 13 deletions(-) diff --git a/src/feature/hs/hs_circuit.c b/src/feature/hs/hs_circuit.c index 1a3767ce6..8640129f3 100644 --- a/src/feature/hs/hs_circuit.c +++ b/src/feature/hs/hs_circuit.c @@ -20,6 +20,7 @@ #include "feature/hs/hs_cell.h" #include "feature/hs/hs_circuit.h" #include "feature/hs/hs_circuitmap.h" +#include "feature/hs/hs_client.h" #include "feature/hs/hs_ident.h" #include "feature/hs/hs_service.h" #include "feature/nodelist/describe.h" @@ -619,6 +620,22 @@ setup_introduce1_data(const hs_desc_intro_point_t *ip, return ret; } +/** Helper: cleanup function for client circuit. This is for every HS version. + * It is called from hs_circ_cleanup() entry point. */ +static void +cleanup_client_circ(circuit_t *circ) +{ + tor_assert(circ); + + if (circuit_is_hs_v2(circ)) { + rend_client_circuit_cleanup(circ); + } else if (circuit_is_hs_v3(circ)) { + hs_client_circuit_cleanup(circ); + } + /* It is possible the circuit has an HS purpose but no identifier (rend_data + * or hs_ident). Thus possible that this passess through. */ +} + /* ========== */ /* Public API */ /* ========== */ @@ -1206,21 +1223,12 @@ hs_circ_cleanup(circuit_t *circ) { tor_assert(circ); - /* v2 specific circuits. */ - if (circuit_is_hs_v2(circ)) { - if (circuit_is_hs_client(circ)) { - rend_client_circuit_cleanup(circ); - } + if (circuit_purpose_is_hs_client(circ->purpose)) { + cleanup_client_circ(circ); } - /* From this point on, it is v3 specific. */ - - /* If it's a service-side intro circ, notify the HS subsystem for the intro - * point circuit closing so it can be dealt with cleanly. */ - if (circ->purpose == CIRCUIT_PURPOSE_S_ESTABLISH_INTRO || - circ->purpose == CIRCUIT_PURPOSE_S_INTRO) { - hs_service_intro_circ_has_closed(TO_ORIGIN_CIRCUIT(circ)); - } + /* Actions that MUST happen for every circuits regardless of what was done + * on it before. */ /* Clear HS circuitmap token for this circ (if any). Very important to be * done after the HS subsystem has been notified of the close else the diff --git a/src/feature/hs/hs_client.c b/src/feature/hs/hs_client.c index 787b29b57..716b6a26a 100644 --- a/src/feature/hs/hs_client.c +++ b/src/feature/hs/hs_client.c @@ -1522,6 +1522,56 @@ get_hs_client_auths_map(void) /* Public API */ /* ========== */ +/** Called when a circuit was just cleaned up. This is done right before the + * circuit is freed. */ +void +hs_client_circuit_cleanup(const circuit_t *circ) +{ + bool has_timed_out; + rend_intro_point_failure_t failure = INTRO_POINT_FAILURE_GENERIC; + const origin_circuit_t *orig_circ = NULL; + + tor_assert(circ); + tor_assert(CIRCUIT_IS_ORIGIN(circ)); + + orig_circ = CONST_TO_ORIGIN_CIRCUIT(circ); + tor_assert(orig_circ->hs_ident); + + has_timed_out = + (circ->marked_for_close_orig_reason == END_CIRC_REASON_TIMEOUT); + if (has_timed_out) { + failure = INTRO_POINT_FAILURE_TIMEOUT; + } + + switch (circ->purpose) { + case CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT: + log_info(LD_REND, "Failed v3 intro circ for service %s to intro point %s " + "(awaiting ACK). Failure code: %d", + safe_str_client(ed25519_fmt(&orig_circ->hs_ident->identity_pk)), + safe_str_client(build_state_get_exit_nickname(orig_circ->build_state)), + failure); + hs_cache_client_intro_state_note(&orig_circ->hs_ident->identity_pk, + &orig_circ->hs_ident->intro_auth_pk, + failure); + break; + case CIRCUIT_PURPOSE_C_INTRODUCING: + if (has_timed_out || !orig_circ->build_state) { + break; + } + failure = INTRO_POINT_FAILURE_UNREACHABLE; + log_info(LD_REND, "Failed v3 intro circ for service %s to intro point %s " + "(while building circuit). Marking as unreachable.", + safe_str_client(ed25519_fmt(&orig_circ->hs_ident->identity_pk)), + safe_str_client(build_state_get_exit_nickname(orig_circ->build_state))); + hs_cache_client_intro_state_note(&orig_circ->hs_ident->identity_pk, + &orig_circ->hs_ident->intro_auth_pk, + failure); + break; + default: + break; + } +} + /** A circuit just finished connecting to a hidden service that the stream * conn has been waiting for. Let the HS subsystem know about this. */ void diff --git a/src/feature/hs/hs_client.h b/src/feature/hs/hs_client.h index 04827ea92..a0f1c7758 100644 --- a/src/feature/hs/hs_client.h +++ b/src/feature/hs/hs_client.h @@ -112,6 +112,7 @@ int hs_client_send_introduce1(origin_circuit_t *intro_circ, origin_circuit_t *rend_circ); void hs_client_circuit_has_opened(origin_circuit_t *circ); +void hs_client_circuit_cleanup(const circuit_t *circ); int hs_client_receive_rendezvous_acked(origin_circuit_t *circ, const uint8_t *payload,

1 0

[tor/master] hs-v2: Move v2 circuit cleanup actions into hs_circ_cleanup()
by asn＠torproject.org 27 Nov '19

27 Nov '19

commit 00136c9430c60edcd8a4e0d725ed711e03c7c607 Author: David Goulet <dgoulet(a)torproject.org> Date: Wed Oct 30 14:48:04 2019 -0400 hs-v2: Move v2 circuit cleanup actions into hs_circ_cleanup() Refactor to decomplexify circuit_about_to_free() and finally have one single entry point into the HS subsystems (v2 and v3) for when a circuit is freed. With this, hs_circ_cleanup() becomes the one and only entry point when a circuit is freed which then routes to the right subsystem version for any actions to be taken. This moves a big chunk of code from circuituse.c to rendclient.c. No behavior change. Next commit will refactor it to reduce our technical debt. Part of #32020 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/core/or/circuitlist.c | 37 --------------------------------- src/feature/hs/hs_circuit.c | 10 +++++++++ src/feature/rend/rendclient.c | 48 +++++++++++++++++++++++++++++++++++++++++++ src/feature/rend/rendclient.h | 2 ++ 4 files changed, 60 insertions(+), 37 deletions(-) diff --git a/src/core/or/circuitlist.c b/src/core/or/circuitlist.c index 9a0b9241d..3ae482a5b 100644 --- a/src/core/or/circuitlist.c +++ b/src/core/or/circuitlist.c @@ -2343,43 +2343,6 @@ circuit_about_to_free(circuit_t *circ) orig_reason); } - if (circ->purpose == CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT) { - origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ); - int timed_out = (reason == END_CIRC_REASON_TIMEOUT); - tor_assert(circ->state == CIRCUIT_STATE_OPEN); - tor_assert(ocirc->build_state->chosen_exit); - if (orig_reason != END_CIRC_REASON_IP_NOW_REDUNDANT && - ocirc->rend_data) { - /* treat this like getting a nack from it */ - log_info(LD_REND, "Failed intro circ %s to %s (awaiting ack). %s", - safe_str_client(rend_data_get_address(ocirc->rend_data)), - safe_str_client(build_state_get_exit_nickname(ocirc->build_state)), - timed_out ? "Recording timeout." : "Removing from descriptor."); - rend_client_report_intro_point_failure(ocirc->build_state->chosen_exit, - ocirc->rend_data, - timed_out ? - INTRO_POINT_FAILURE_TIMEOUT : - INTRO_POINT_FAILURE_GENERIC); - } - } else if (circ->purpose == CIRCUIT_PURPOSE_C_INTRODUCING && - reason != END_CIRC_REASON_TIMEOUT) { - origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ); - if (ocirc->build_state->chosen_exit && ocirc->rend_data) { - if (orig_reason != END_CIRC_REASON_IP_NOW_REDUNDANT && - ocirc->rend_data) { - log_info(LD_REND, "Failed intro circ %s to %s " - "(building circuit to intro point). " - "Marking intro point as possibly unreachable.", - safe_str_client(rend_data_get_address(ocirc->rend_data)), - safe_str_client(build_state_get_exit_nickname( - ocirc->build_state))); - rend_client_report_intro_point_failure(ocirc->build_state->chosen_exit, - ocirc->rend_data, - INTRO_POINT_FAILURE_UNREACHABLE); - } - } - } - if (circ->n_chan) { circuit_clear_cell_queue(circ, circ->n_chan); /* Only send destroy if the channel isn't closing anyway */ diff --git a/src/feature/hs/hs_circuit.c b/src/feature/hs/hs_circuit.c index 9d21acf42..1a3767ce6 100644 --- a/src/feature/hs/hs_circuit.c +++ b/src/feature/hs/hs_circuit.c @@ -25,6 +25,7 @@ #include "feature/nodelist/describe.h" #include "feature/nodelist/nodelist.h" #include "feature/rend/rendservice.h" +#include "feature/rend/rendclient.h" #include "feature/stats/rephist.h" #include "lib/crypt_ops/crypto_dh.h" #include "lib/crypt_ops/crypto_rand.h" @@ -1205,6 +1206,15 @@ hs_circ_cleanup(circuit_t *circ) { tor_assert(circ); + /* v2 specific circuits. */ + if (circuit_is_hs_v2(circ)) { + if (circuit_is_hs_client(circ)) { + rend_client_circuit_cleanup(circ); + } + } + + /* From this point on, it is v3 specific. */ + /* If it's a service-side intro circ, notify the HS subsystem for the intro * point circuit closing so it can be dealt with cleanly. */ if (circ->purpose == CIRCUIT_PURPOSE_S_ESTABLISH_INTRO || diff --git a/src/feature/rend/rendclient.c b/src/feature/rend/rendclient.c index 6f4ee8b8c..971839825 100644 --- a/src/feature/rend/rendclient.c +++ b/src/feature/rend/rendclient.c @@ -1250,3 +1250,51 @@ rend_parse_service_authorization(const or_options_t *options, } return res; } + +/** The given circuit is being freed. Take appropriate action if it is of + * interest to the client subsystem. */ +void +rend_client_circuit_cleanup(circuit_t *circ) +{ + int reason = circ->marked_for_close_reason; + int orig_reason = circ->marked_for_close_orig_reason; + + tor_assert(circ); + + if (circ->purpose == CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT) { + origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ); + int timed_out = (reason == END_CIRC_REASON_TIMEOUT); + tor_assert(circ->state == CIRCUIT_STATE_OPEN); + tor_assert(ocirc->build_state->chosen_exit); + if (orig_reason != END_CIRC_REASON_IP_NOW_REDUNDANT && + ocirc->rend_data) { + /* treat this like getting a nack from it */ + log_info(LD_REND, "Failed intro circ %s to %s (awaiting ack). %s", + safe_str_client(rend_data_get_address(ocirc->rend_data)), + safe_str_client(build_state_get_exit_nickname(ocirc->build_state)), + timed_out ? "Recording timeout." : "Removing from descriptor."); + rend_client_report_intro_point_failure(ocirc->build_state->chosen_exit, + ocirc->rend_data, + timed_out ? + INTRO_POINT_FAILURE_TIMEOUT : + INTRO_POINT_FAILURE_GENERIC); + } + } else if (circ->purpose == CIRCUIT_PURPOSE_C_INTRODUCING && + reason != END_CIRC_REASON_TIMEOUT) { + origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ); + if (ocirc->build_state->chosen_exit && ocirc->rend_data) { + if (orig_reason != END_CIRC_REASON_IP_NOW_REDUNDANT && + ocirc->rend_data) { + log_info(LD_REND, "Failed intro circ %s to %s " + "(building circuit to intro point). " + "Marking intro point as possibly unreachable.", + safe_str_client(rend_data_get_address(ocirc->rend_data)), + safe_str_client(build_state_get_exit_nickname( + ocirc->build_state))); + rend_client_report_intro_point_failure(ocirc->build_state->chosen_exit, + ocirc->rend_data, + INTRO_POINT_FAILURE_UNREACHABLE); + } + } + } +} diff --git a/src/feature/rend/rendclient.h b/src/feature/rend/rendclient.h index e5f333238..dd9fa61ce 100644 --- a/src/feature/rend/rendclient.h +++ b/src/feature/rend/rendclient.h @@ -47,5 +47,7 @@ rend_service_authorization_t *rend_client_lookup_service_authorization( const char *onion_address); void rend_service_authorization_free_all(void); +void rend_client_circuit_cleanup(circuit_t *circ); + #endif /* !defined(TOR_RENDCLIENT_H) */

1 0

[tor/master] hs-v2: Modernize rend_client_circuit_cleanup() code
by asn＠torproject.org 27 Nov '19

27 Nov '19

commit 1aafe3376ea08fb31fcb7e0a8a390ff5edeb8602 Author: David Goulet <dgoulet(a)torproject.org> Date: Wed Oct 30 15:17:47 2019 -0400 hs-v2: Modernize rend_client_circuit_cleanup() code Old and messy code path. Structure it in a more pleasant and readable way. No behavior change with this refactor. Part of #32020 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/feature/rend/rendclient.c | 85 +++++++++++++++++++++++++------------------ src/feature/rend/rendclient.h | 2 +- 2 files changed, 51 insertions(+), 36 deletions(-) diff --git a/src/feature/rend/rendclient.c b/src/feature/rend/rendclient.c index 971839825..632b00c85 100644 --- a/src/feature/rend/rendclient.c +++ b/src/feature/rend/rendclient.c @@ -1254,47 +1254,62 @@ rend_parse_service_authorization(const or_options_t *options, /** The given circuit is being freed. Take appropriate action if it is of * interest to the client subsystem. */ void -rend_client_circuit_cleanup(circuit_t *circ) +rend_client_circuit_cleanup(const circuit_t *circ) { - int reason = circ->marked_for_close_reason; - int orig_reason = circ->marked_for_close_orig_reason; + int reason, orig_reason; + bool has_timed_out, ip_is_redundant; + const origin_circuit_t *ocirc = NULL; tor_assert(circ); + tor_assert(CIRCUIT_IS_ORIGIN(circ)); - if (circ->purpose == CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT) { - origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ); - int timed_out = (reason == END_CIRC_REASON_TIMEOUT); + reason = circ->marked_for_close_reason; + orig_reason = circ->marked_for_close_orig_reason; + ocirc = CONST_TO_ORIGIN_CIRCUIT(circ); + tor_assert(ocirc->rend_data); + + has_timed_out = (reason == END_CIRC_REASON_TIMEOUT); + ip_is_redundant = (orig_reason == END_CIRC_REASON_IP_NOW_REDUNDANT); + + switch (circ->purpose) { + case CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT: + { + if (ip_is_redundant) { + break; + } tor_assert(circ->state == CIRCUIT_STATE_OPEN); tor_assert(ocirc->build_state->chosen_exit); - if (orig_reason != END_CIRC_REASON_IP_NOW_REDUNDANT && - ocirc->rend_data) { - /* treat this like getting a nack from it */ - log_info(LD_REND, "Failed intro circ %s to %s (awaiting ack). %s", - safe_str_client(rend_data_get_address(ocirc->rend_data)), - safe_str_client(build_state_get_exit_nickname(ocirc->build_state)), - timed_out ? "Recording timeout." : "Removing from descriptor."); - rend_client_report_intro_point_failure(ocirc->build_state->chosen_exit, - ocirc->rend_data, - timed_out ? - INTRO_POINT_FAILURE_TIMEOUT : - INTRO_POINT_FAILURE_GENERIC); - } - } else if (circ->purpose == CIRCUIT_PURPOSE_C_INTRODUCING && - reason != END_CIRC_REASON_TIMEOUT) { - origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ); - if (ocirc->build_state->chosen_exit && ocirc->rend_data) { - if (orig_reason != END_CIRC_REASON_IP_NOW_REDUNDANT && - ocirc->rend_data) { - log_info(LD_REND, "Failed intro circ %s to %s " - "(building circuit to intro point). " - "Marking intro point as possibly unreachable.", - safe_str_client(rend_data_get_address(ocirc->rend_data)), - safe_str_client(build_state_get_exit_nickname( - ocirc->build_state))); - rend_client_report_intro_point_failure(ocirc->build_state->chosen_exit, - ocirc->rend_data, - INTRO_POINT_FAILURE_UNREACHABLE); - } + /* Treat this like getting a nack from it */ + log_info(LD_REND, "Failed intro circ %s to %s (awaiting ack). %s", + safe_str_client(rend_data_get_address(ocirc->rend_data)), + safe_str_client(build_state_get_exit_nickname(ocirc->build_state)), + has_timed_out ? "Recording timeout." : "Removing from descriptor."); + rend_client_report_intro_point_failure(ocirc->build_state->chosen_exit, + ocirc->rend_data, + has_timed_out ? + INTRO_POINT_FAILURE_TIMEOUT : + INTRO_POINT_FAILURE_GENERIC); + break; + } + case CIRCUIT_PURPOSE_C_INTRODUCING: + { + /* Ignore if we were introducing and it timed out, we didn't pick an exit + * point yet (IP) or the reason indicate that it was a redundant IP. */ + if (has_timed_out || !ocirc->build_state->chosen_exit || ip_is_redundant) { + break; } + log_info(LD_REND, "Failed intro circ %s to %s " + "(building circuit to intro point). " + "Marking intro point as possibly unreachable.", + safe_str_client(rend_data_get_address(ocirc->rend_data)), + safe_str_client(build_state_get_exit_nickname( + ocirc->build_state))); + rend_client_report_intro_point_failure(ocirc->build_state->chosen_exit, + ocirc->rend_data, + INTRO_POINT_FAILURE_UNREACHABLE); + break; + } + default: + break; } } diff --git a/src/feature/rend/rendclient.h b/src/feature/rend/rendclient.h index dd9fa61ce..da6f4646d 100644 --- a/src/feature/rend/rendclient.h +++ b/src/feature/rend/rendclient.h @@ -47,7 +47,7 @@ rend_service_authorization_t *rend_client_lookup_service_authorization( const char *onion_address); void rend_service_authorization_free_all(void); -void rend_client_circuit_cleanup(circuit_t *circ); +void rend_client_circuit_cleanup(const circuit_t *circ); #endif /* !defined(TOR_RENDCLIENT_H) */

1 0

[tor/master] hs-v3: Give a cleanup type to hs_circ_cleanup()
by asn＠torproject.org 27 Nov '19

27 Nov '19

commit cbc495453cb522db1584525a06d26386f5819e05 Author: David Goulet <dgoulet(a)torproject.org> Date: Thu Oct 31 08:06:16 2019 -0400 hs-v3: Give a cleanup type to hs_circ_cleanup() By centralizing the circuit cleanup type that is: on close, free and repurpose, some actions on the circuit can not happen for a certain cleanup type or for all types. This passes a cleanup type so the HS subsystem (v2 and v3) can take actions based on the type of cleanup. For instance, there is slow code that we do not run on a circuit close but rather only on free. Part of #32020 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/core/or/circuitlist.c | 4 +- src/core/or/circuituse.c | 2 +- src/feature/hs/hs_circuit.c | 94 ++++++++++++++++++++++++++++++++++--------- src/feature/hs/hs_circuit.h | 6 ++- src/feature/hs/hs_client.c | 2 +- src/feature/hs/hs_client.h | 4 +- src/feature/rend/rendclient.c | 2 +- src/feature/rend/rendclient.h | 3 +- 8 files changed, 88 insertions(+), 29 deletions(-) diff --git a/src/core/or/circuitlist.c b/src/core/or/circuitlist.c index 3ae482a5b..49a63c50a 100644 --- a/src/core/or/circuitlist.c +++ b/src/core/or/circuitlist.c @@ -1137,7 +1137,7 @@ circuit_free_(circuit_t *circ) * circuit is closed. This is to avoid any code path that free registered * circuits without closing them before. This needs to be done before the * hs identifier is freed. */ - hs_circ_cleanup(circ); + hs_circ_cleanup_on_free(circ); if (CIRCUIT_IS_ORIGIN(circ)) { origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ); @@ -2261,7 +2261,7 @@ circuit_mark_for_close_, (circuit_t *circ, int reason, int line, } /* Notify the HS subsystem that this circuit is closing. */ - hs_circ_cleanup(circ); + hs_circ_cleanup_on_close(circ); if (circuits_pending_close == NULL) circuits_pending_close = smartlist_new(); diff --git a/src/core/or/circuituse.c b/src/core/or/circuituse.c index 8a667024d..ad9841cc3 100644 --- a/src/core/or/circuituse.c +++ b/src/core/or/circuituse.c @@ -3124,7 +3124,7 @@ circuit_change_purpose(circuit_t *circ, uint8_t new_purpose) /* Take specific actions if we are repurposing a hidden service circuit. */ if (circuit_purpose_is_hidden_service(circ->purpose) && !circuit_purpose_is_hidden_service(new_purpose)) { - hs_circ_cleanup(circ); + hs_circ_cleanup_on_repurpose(circ); } } diff --git a/src/feature/hs/hs_circuit.c b/src/feature/hs/hs_circuit.c index 8640129f3..a43e7f0e2 100644 --- a/src/feature/hs/hs_circuit.c +++ b/src/feature/hs/hs_circuit.c @@ -621,16 +621,16 @@ setup_introduce1_data(const hs_desc_intro_point_t *ip, } /** Helper: cleanup function for client circuit. This is for every HS version. - * It is called from hs_circ_cleanup() entry point. */ + * It is called from hs_circ_cleanup_on_free() entry point. */ static void -cleanup_client_circ(circuit_t *circ) +cleanup_on_free_client_circ(circuit_t *circ) { tor_assert(circ); if (circuit_is_hs_v2(circ)) { - rend_client_circuit_cleanup(circ); + rend_client_circuit_cleanup_on_free(circ); } else if (circuit_is_hs_v3(circ)) { - hs_client_circuit_cleanup(circ); + hs_client_circuit_cleanup_on_free(circ); } /* It is possible the circuit has an HS purpose but no identifier (rend_data * or hs_ident). Thus possible that this passess through. */ @@ -1215,29 +1215,83 @@ hs_circ_send_establish_rendezvous(origin_circuit_t *circ) return -1; } -/** We are about to close or free this circ. Clean it up from any - * related HS data structures. This function can be called multiple times - * safely for the same circuit. */ +/** Circuit cleanup strategy: + * + * What follows is a series of functions that notifies the HS subsystem of 3 + * different circuit cleanup phase: close, free and repurpose. + * + * Tor can call any of those in any orders so they have to be safe between + * each other. In other words, the free should never depend on close to be + * called before. + * + * The "on_close()" is called from circuit_mark_for_close() which is + * considered the tor fast path and thus as little work as possible should + * done in that function. Currently, we only remove the circuit from the HS + * circuit map and move on. + * + * The "on_free()" is called from circuit circuit_free_() and it is very + * important that at the end of the function, no state or objects related to + * this circuit remains alive. + * + * The "on_repurpose()" is called from circuit_change_purpose() for which we + * simply remove it from the HS circuit map. We do not have other cleanup + * requirements after that. + * + * NOTE: The onion service code, specifically the service code, cleans up + * lingering objects or state if any of its circuit disappear which is why + * our cleanup strategy doesn't involve any service specific actions. As long + * as the circuit is removed from the HS circuit map, it won't be used. + */ + +/** We are about to close this circ. Clean it up from any related HS + * data structures. This function can be called multiple times safely for the + * same circuit. */ void -hs_circ_cleanup(circuit_t *circ) +hs_circ_cleanup_on_close(circuit_t *circ) { tor_assert(circ); + /* On close, we simply remove it from the circuit map. It can not be used + * anymore. We keep this code path fast and lean. */ + + if (circ->hs_token) { + hs_circuitmap_remove_circuit(circ); + } +} + +/** We are about to free this circ. Clean it up from any related HS + * data structures. This function can be called multiple times safely for the + * same circuit. */ +void +hs_circ_cleanup_on_free(circuit_t *circ) +{ + tor_assert(circ); + + /* NOTE: Bulk of the work of cleaning up a circuit is done here. */ + if (circuit_purpose_is_hs_client(circ->purpose)) { - cleanup_client_circ(circ); + cleanup_on_free_client_circ(circ); + } + + /* We have no assurance that the given HS circuit has been closed before and + * thus removed from the HS map. This actually happens in unit tests. */ + if (circ->hs_token) { + hs_circuitmap_remove_circuit(circ); } +} + +/** We are about to repurpose this circ. Clean it up from any related + * HS data structures. This function can be called multiple times safely for + * the same circuit. */ +void +hs_circ_cleanup_on_repurpose(circuit_t *circ) +{ + tor_assert(circ); + + /* On repurpose, we simply remove it from the circuit map but we do not do + * the on_free actions since we don't treat a repurpose as something we need + * to report in the client cache failure. */ - /* Actions that MUST happen for every circuits regardless of what was done - * on it before. */ - - /* Clear HS circuitmap token for this circ (if any). Very important to be - * done after the HS subsystem has been notified of the close else the - * circuit will not be found. - * - * We do this at the close if possible because from that point on, the - * circuit is good as dead. We can't rely on removing it in the circuit - * free() function because we open a race window between the close and free - * where we can't register a new circuit for the same intro point. */ if (circ->hs_token) { hs_circuitmap_remove_circuit(circ); } diff --git a/src/feature/hs/hs_circuit.h b/src/feature/hs/hs_circuit.h index c817f3e37..42e5ca134 100644 --- a/src/feature/hs/hs_circuit.h +++ b/src/feature/hs/hs_circuit.h @@ -14,8 +14,10 @@ #include "feature/hs/hs_service.h" -/* Cleanup function when the circuit is closed or/and freed. */ -void hs_circ_cleanup(circuit_t *circ); +/* Cleanup function when the circuit is closed or freed. */ +void hs_circ_cleanup_on_close(circuit_t *circ); +void hs_circ_cleanup_on_free(circuit_t *circ); +void hs_circ_cleanup_on_repurpose(circuit_t *circ); /* Circuit API. */ int hs_circ_service_intro_has_opened(hs_service_t *service, diff --git a/src/feature/hs/hs_client.c b/src/feature/hs/hs_client.c index 716b6a26a..a2a47c1da 100644 --- a/src/feature/hs/hs_client.c +++ b/src/feature/hs/hs_client.c @@ -1525,7 +1525,7 @@ get_hs_client_auths_map(void) /** Called when a circuit was just cleaned up. This is done right before the * circuit is freed. */ void -hs_client_circuit_cleanup(const circuit_t *circ) +hs_client_circuit_cleanup_on_free(const circuit_t *circ) { bool has_timed_out; rend_intro_point_failure_t failure = INTRO_POINT_FAILURE_GENERIC; diff --git a/src/feature/hs/hs_client.h b/src/feature/hs/hs_client.h index a0f1c7758..23effc06b 100644 --- a/src/feature/hs/hs_client.h +++ b/src/feature/hs/hs_client.h @@ -10,6 +10,8 @@ #define TOR_HS_CLIENT_H #include "lib/crypt_ops/crypto_ed25519.h" + +#include "feature/hs/hs_circuit.h" #include "feature/hs/hs_descriptor.h" #include "feature/hs/hs_ident.h" @@ -112,7 +114,7 @@ int hs_client_send_introduce1(origin_circuit_t *intro_circ, origin_circuit_t *rend_circ); void hs_client_circuit_has_opened(origin_circuit_t *circ); -void hs_client_circuit_cleanup(const circuit_t *circ); +void hs_client_circuit_cleanup_on_free(const circuit_t *circ); int hs_client_receive_rendezvous_acked(origin_circuit_t *circ, const uint8_t *payload, diff --git a/src/feature/rend/rendclient.c b/src/feature/rend/rendclient.c index 632b00c85..14484f1ce 100644 --- a/src/feature/rend/rendclient.c +++ b/src/feature/rend/rendclient.c @@ -1254,7 +1254,7 @@ rend_parse_service_authorization(const or_options_t *options, /** The given circuit is being freed. Take appropriate action if it is of * interest to the client subsystem. */ void -rend_client_circuit_cleanup(const circuit_t *circ) +rend_client_circuit_cleanup_on_free(const circuit_t *circ) { int reason, orig_reason; bool has_timed_out, ip_is_redundant; diff --git a/src/feature/rend/rendclient.h b/src/feature/rend/rendclient.h index da6f4646d..63191737c 100644 --- a/src/feature/rend/rendclient.h +++ b/src/feature/rend/rendclient.h @@ -12,6 +12,7 @@ #ifndef TOR_RENDCLIENT_H #define TOR_RENDCLIENT_H +#include "feature/hs/hs_circuit.h" #include "feature/rend/rendcache.h" void rend_client_purge_state(void); @@ -47,7 +48,7 @@ rend_service_authorization_t *rend_client_lookup_service_authorization( const char *onion_address); void rend_service_authorization_free_all(void); -void rend_client_circuit_cleanup(const circuit_t *circ); +void rend_client_circuit_cleanup_on_free(const circuit_t *circ); #endif /* !defined(TOR_RENDCLIENT_H) */

1 0

[tor/master] hs-v3: Remove unused hs_service_intro_circ_has_closed()
by asn＠torproject.org 27 Nov '19

27 Nov '19

commit a423cec670140c3afa59258e302435a5a4a9791d Author: David Goulet <dgoulet(a)torproject.org> Date: Wed Nov 20 11:03:45 2019 -0500 hs-v3: Remove unused hs_service_intro_circ_has_closed() Since the removal of ip->circuit_established, this function does litterally nothing so clean it up. Part of #32020 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/feature/hs/hs_service.c | 48 ++++----------------------------------------- src/feature/hs/hs_service.h | 2 -- 2 files changed, 4 insertions(+), 46 deletions(-) diff --git a/src/feature/hs/hs_service.c b/src/feature/hs/hs_service.c index 5693cdb0f..3d152afa1 100644 --- a/src/feature/hs/hs_service.c +++ b/src/feature/hs/hs_service.c @@ -2404,12 +2404,10 @@ static void cleanup_intro_points(hs_service_t *service, time_t now) { /* List of intro points to close. We can't mark the intro circuits for close - * in the modify loop because doing so calls - * hs_service_intro_circ_has_closed() which does a digest256map_get() on the - * intro points map (that we are iterating over). This can't be done in a - * single iteration after a MAP_DEL_CURRENT, the object will still be - * returned leading to a use-after-free. So, we close the circuits and free - * the intro points after the loop if any. */ + * in the modify loop because doing so calls back into the HS subsystem and + * we need to keep that code path outside of the service/desc loop so those + * maps don't get modified during the close making us in a possible + * use-after-free situation. */ smartlist_t *ips_to_free = smartlist_new(); tor_assert(service); @@ -3684,44 +3682,6 @@ hs_service_get_num_services,(void)) return HT_SIZE(hs_service_map); } -/** Called once an introduction circuit is closed. If the circuit doesn't have - * a v3 identifier, it is ignored. */ -void -hs_service_intro_circ_has_closed(origin_circuit_t *circ) -{ - hs_service_t *service = NULL; - hs_service_intro_point_t *ip = NULL; - hs_service_descriptor_t *desc = NULL; - - tor_assert(circ); - - if (circ->hs_ident == NULL) { - /* This is not a v3 circuit, ignore. */ - goto end; - } - - get_objects_from_ident(circ->hs_ident, &service, &ip, &desc); - if (service == NULL) { - /* This is possible if the circuits are closed and the service is - * immediately deleted. */ - log_info(LD_REND, "Unable to find any hidden service associated " - "identity key %s on intro circuit %u.", - ed25519_fmt(&circ->hs_ident->identity_pk), - TO_CIRCUIT(circ)->n_circ_id); - goto end; - } - if (ip == NULL) { - /* The introduction point object has already been removed probably by our - * cleanup process so ignore. */ - goto end; - } - /* Can't have an intro point object without a descriptor. */ - tor_assert(desc); - - end: - return; -} - /** Given conn, a rendezvous edge connection acting as an exit stream, look up * the hidden service for the circuit circ, and look up the port and address * based on the connection port. Assign the actual connection address. diff --git a/src/feature/hs/hs_service.h b/src/feature/hs/hs_service.h index 6c929c7ff..1c2d9e97c 100644 --- a/src/feature/hs/hs_service.h +++ b/src/feature/hs/hs_service.h @@ -344,8 +344,6 @@ int hs_service_receive_introduce2(origin_circuit_t *circ, const uint8_t *payload, size_t payload_len); -void hs_service_intro_circ_has_closed(origin_circuit_t *circ); - char *hs_service_lookup_current_desc(const ed25519_public_key_t *pk); hs_service_add_ephemeral_status_t

1 0

[tor/master] Merge branch 'tor-github/pr/1573'
by asn＠torproject.org 27 Nov '19

27 Nov '19

commit 68a00c4951666b7658064f2547739d280f912398 Merge: 96a15bece 85b984dbc Author: George Kadianakis <desnacked(a)riseup.net> Date: Wed Nov 27 15:36:26 2019 +0200 Merge branch 'tor-github/pr/1573' changes/ticket32020 | 6 +++ src/core/or/circuitlist.c | 41 +--------------- src/core/or/circuituse.c | 74 +++++++++++++++++++++------- src/core/or/circuituse.h | 9 ++++ src/feature/hs/hs_circuit.c | 106 +++++++++++++++++++++++++++++++++------- src/feature/hs/hs_circuit.h | 6 ++- src/feature/hs/hs_client.c | 50 +++++++++++++++++++ src/feature/hs/hs_client.h | 3 ++ src/feature/hs/hs_service.c | 48 ++----------------- src/feature/hs/hs_service.h | 2 - src/feature/rend/rendclient.c | 63 ++++++++++++++++++++++++ src/feature/rend/rendclient.h | 3 ++ src/test/test_hs_client.c | 109 ++++++++++++++++++++++++++++++++++++++++++ 13 files changed, 398 insertions(+), 122 deletions(-)

1 0

[tor/master] test: Add unit tests for closing intro circ on failure
by asn＠torproject.org 27 Nov '19

27 Nov '19

commit c508513014c6a6034d98e0b07f499d62d56786a5 Author: David Goulet <dgoulet(a)torproject.org> Date: Thu Oct 31 09:04:52 2019 -0400 test: Add unit tests for closing intro circ on failure Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/test/test_hs_client.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 109 insertions(+) diff --git a/src/test/test_hs_client.c b/src/test/test_hs_client.c index cf3763427..ab586a1de 100644 --- a/src/test/test_hs_client.c +++ b/src/test/test_hs_client.c @@ -1226,6 +1226,113 @@ test_socks_hs_errors(void *arg) UNMOCK(check_private_dir); } +static void +test_close_intro_circuit_failure(void *arg) +{ + char digest[DIGEST_LEN]; + circuit_t *circ = NULL; + ed25519_keypair_t service_kp, intro_kp; + origin_circuit_t *ocirc = NULL; + tor_addr_t addr; + const hs_cache_intro_state_t *entry; + + (void) arg; + + hs_init(); + + /* Generate service keypair */ + tt_int_op(0, OP_EQ, ed25519_keypair_generate(&service_kp, 0)); + tt_int_op(0, OP_EQ, ed25519_keypair_generate(&intro_kp, 0)); + + /* Create and add to the global list a dummy client introduction circuit at + * the ACK WAIT state. */ + circ = dummy_origin_circuit_new(0); + tt_assert(circ); + circ->purpose = CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT; + ocirc = TO_ORIGIN_CIRCUIT(circ); + ocirc->hs_ident = hs_ident_circuit_new(&service_kp.pubkey); + ocirc->build_state = tor_malloc_zero(sizeof(cpath_build_state_t)); + /* Code path will log this exit so build it. */ + ocirc->build_state->chosen_exit = extend_info_new("TestNickname", digest, + NULL, NULL, NULL, &addr, + 4242); + ed25519_pubkey_copy(&ocirc->hs_ident->intro_auth_pk, &intro_kp.pubkey); + + /* We'll make for close the circuit for a timeout failure. It should _NOT_ + * end up in the failure cache just yet. We do that on free() only. */ + circuit_mark_for_close(circ, END_CIRC_REASON_TIMEOUT); + tt_assert(!hs_cache_client_intro_state_find(&service_kp.pubkey, + &intro_kp.pubkey)); + /* Time to free. It should get removed. */ + circuit_free(circ); + entry = hs_cache_client_intro_state_find(&service_kp.pubkey, + &intro_kp.pubkey); + tt_assert(entry); + tt_uint_op(entry->timed_out, OP_EQ, 1); + hs_cache_client_intro_state_purge(); + + /* Again, create and add to the global list a dummy client introduction + * circuit at the INTRODUCING state. */ + circ = dummy_origin_circuit_new(0); + tt_assert(circ); + circ->purpose = CIRCUIT_PURPOSE_C_INTRODUCING; + ocirc = TO_ORIGIN_CIRCUIT(circ); + ocirc->hs_ident = hs_ident_circuit_new(&service_kp.pubkey); + ocirc->build_state = tor_malloc_zero(sizeof(cpath_build_state_t)); + /* Code path will log this exit so build it. */ + ocirc->build_state->chosen_exit = extend_info_new("TestNickname", digest, + NULL, NULL, NULL, &addr, + 4242); + ed25519_pubkey_copy(&ocirc->hs_ident->intro_auth_pk, &intro_kp.pubkey); + + /* On free, we should get an unreachable failure. */ + circuit_free(circ); + entry = hs_cache_client_intro_state_find(&service_kp.pubkey, + &intro_kp.pubkey); + tt_assert(entry); + tt_uint_op(entry->unreachable_count, OP_EQ, 1); + hs_cache_client_intro_state_purge(); + + /* Again, create and add to the global list a dummy client introduction + * circuit at the INTRODUCING state but we'll close it for timeout. It + * should not be noted as a timeout failure. */ + circ = dummy_origin_circuit_new(0); + tt_assert(circ); + circ->purpose = CIRCUIT_PURPOSE_C_INTRODUCING; + ocirc = TO_ORIGIN_CIRCUIT(circ); + ocirc->hs_ident = hs_ident_circuit_new(&service_kp.pubkey); + ocirc->build_state = tor_malloc_zero(sizeof(cpath_build_state_t)); + /* Code path will log this exit so build it. */ + ocirc->build_state->chosen_exit = extend_info_new("TestNickname", digest, + NULL, NULL, NULL, &addr, + 4242); + ed25519_pubkey_copy(&ocirc->hs_ident->intro_auth_pk, &intro_kp.pubkey); + + circuit_mark_for_close(circ, END_CIRC_REASON_TIMEOUT); + circuit_free(circ); + tt_assert(!hs_cache_client_intro_state_find(&service_kp.pubkey, + &intro_kp.pubkey)); + + /* Again, create and add to the global list a dummy client introduction + * circuit at the INTRODUCING state but without a chosen_exit. In theory, it + * can not happen but we'll make sure it doesn't end up in the failure cache + * anyway. */ + circ = dummy_origin_circuit_new(0); + tt_assert(circ); + circ->purpose = CIRCUIT_PURPOSE_C_INTRODUCING; + ocirc = TO_ORIGIN_CIRCUIT(circ); + ocirc->hs_ident = hs_ident_circuit_new(&service_kp.pubkey); + ed25519_pubkey_copy(&ocirc->hs_ident->intro_auth_pk, &intro_kp.pubkey); + + circuit_free(circ); + tt_assert(!hs_cache_client_intro_state_find(&service_kp.pubkey, + &intro_kp.pubkey)); + + done: + circuit_free(circ); + hs_free_all(); +} + struct testcase_t hs_client_tests[] = { { "e2e_rend_circuit_setup_legacy", test_e2e_rend_circuit_setup_legacy, TT_FORK, NULL, NULL }, @@ -1243,6 +1350,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_circuit_failure", test_close_intro_circuit_failure, + TT_FORK, NULL, NULL }, { "close_intro_circuits_new_desc", test_close_intro_circuits_new_desc, TT_FORK, NULL, NULL }, { "close_intro_circuits_cache_clean", test_close_intro_circuits_cache_clean,

1 0

[tor/master] hs: Add changes file for 32020
by asn＠torproject.org 27 Nov '19

27 Nov '19

commit 85b984dbc6b97df8908bd7a3fe5433ddd4dfaf5a Author: David Goulet <dgoulet(a)torproject.org> Date: Wed Nov 20 11:01:47 2019 -0500 hs: Add changes file for 32020 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- changes/ticket32020 | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/changes/ticket32020 b/changes/ticket32020 new file mode 100644 index 000000000..1cad61af8 --- /dev/null +++ b/changes/ticket32020 @@ -0,0 +1,6 @@ + o Major bugfixes (onion service): + - Report back HS circuit failure back into the HS subsytem so we take + appropriate action with regards to the client introduction point failure + cache. This improves reachability of onion services, since now clients + notice failing introduction circuits properly. Fixes bug 32020; bugfix on + 0.3.2.1-alpha;

1 0

[tpo/master] Update lego
by hiro＠torproject.org 27 Nov '19

27 Nov '19

commit 334de6eb9bd0f85ae50569318a24f04c2a370b83 Author: hiro <hiro(a)torproject.org> Date: Wed Nov 27 14:33:31 2019 +0100 Update lego --- lego | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lego b/lego index 7c0e9ee..d892ab1 160000 --- a/lego +++ b/lego @@ -1 +1 @@ -Subproject commit 7c0e9ee42a4cdef87ebb8eefa7dde2df89403c94 +Subproject commit d892ab188dbc30f5827c93fc851506e383b48715

1 0

[styleguide/master] Update lego
by hiro＠torproject.org 27 Nov '19

27 Nov '19

commit fa33bc0697727dd5f0d425eef6dcb8db4e192888 Author: hiro <hiro(a)torproject.org> Date: Wed Nov 27 14:33:16 2019 +0100 Update lego --- lego | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lego b/lego index 7c0e9ee..d892ab1 160000 --- a/lego +++ b/lego @@ -1 +1 @@ -Subproject commit 7c0e9ee42a4cdef87ebb8eefa7dde2df89403c94 +Subproject commit d892ab188dbc30f5827c93fc851506e383b48715

1 0