From e80893e51b0c0320838cbed8c46fd5b0fe608bef Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 4 Dec 2017 11:49:48 -0500 Subject: hs-v3: Cleanup HS circuits when marking as closed First, hs_service_intro_circ_has_closed() is now called in circuit_mark_for close() because the HS subsystem needs to learn when an intro point is actually not established anymore as soon as possible. There is a time window between a close and a free. Second, when we mark for close, we also remove it from the circuitmap because between the close and the free, a service can launch an new circuit to that same intro point and thus register it which only succeeds if the intro point authentication key is not already in the map. However, we still do a remove from the circuitmap in circuit_free() in order to also cleanup the circuit if it wasn't marked for close prior to the free. Fixes #23603 Signed-off-by: David Goulet --- src/or/circuitlist.c | 26 +++++++++++++------------- src/or/hs_circuit.c | 28 ++++++++++++++++++++++++++++ src/or/hs_circuit.h | 3 +++ src/or/hs_common.c | 1 + src/or/hs_common.h | 2 ++ 5 files changed, 47 insertions(+), 13 deletions(-) (limited to 'src') diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index d37d986f17..45d4521c22 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -67,7 +67,6 @@ #include "main.h" #include "hs_circuit.h" #include "hs_circuitmap.h" -#include "hs_common.h" #include "hs_ident.h" #include "networkstatus.h" #include "nodelist.h" @@ -938,6 +937,12 @@ circuit_free(circuit_t *circ) circuit_clear_testing_cell_stats(circ); + /* Cleanup circuit from anything HS v3 related. We also do this when the + * 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); + if (CIRCUIT_IS_ORIGIN(circ)) { origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ); mem = ocirc; @@ -963,7 +968,11 @@ circuit_free(circuit_t *circ) crypto_pk_free(ocirc->intro_key); rend_data_free(ocirc->rend_data); + + /* Finally, free the identifier of the circuit and nullify it so multiple + * cleanup will work. */ hs_ident_circuit_free(ocirc->hs_ident); + ocirc->hs_ident = NULL; tor_free(ocirc->dest_address); if (ocirc->socks_username) { @@ -1022,11 +1031,6 @@ circuit_free(circuit_t *circ) /* Remove from map. */ circuit_set_n_circid_chan(circ, 0, NULL); - /* Clear HS circuitmap token from this circ (if any) */ - if (circ->hs_token) { - hs_circuitmap_remove_circuit(circ); - } - /* Clear cell queue _after_ removing it from the map. Otherwise our * "active" checks will be violated. */ cell_queue_clear(&circ->n_chan_cells); @@ -1917,6 +1921,9 @@ circuit_mark_for_close_, (circuit_t *circ, int reason, int line, } } + /* Notify the HS subsystem that this circuit is closing. */ + hs_circ_cleanup(circ); + if (circuits_pending_close == NULL) circuits_pending_close = smartlist_new(); @@ -1997,13 +2004,6 @@ circuit_about_to_free(circuit_t *circ) orig_reason); } - /* Notify the HS subsystem for any 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)); - } - 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); diff --git a/src/or/hs_circuit.c b/src/or/hs_circuit.c index ee952f4d68..a58166ccde 100644 --- a/src/or/hs_circuit.c +++ b/src/or/hs_circuit.c @@ -1178,3 +1178,31 @@ 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. */ +void +hs_circ_cleanup(circuit_t *circ) +{ + tor_assert(circ); + + /* 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)); + } + + /* 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/or/hs_circuit.h b/src/or/hs_circuit.h index 0a1186dbaa..63ff5e463c 100644 --- a/src/or/hs_circuit.h +++ b/src/or/hs_circuit.h @@ -15,6 +15,9 @@ #include "hs_service.h" +/* Cleanup function when the circuit is closed or/and freed. */ +void hs_circ_cleanup(circuit_t *circ); + /* Circuit API. */ int hs_circ_service_intro_has_opened(hs_service_t *service, hs_service_intro_point_t *ip, diff --git a/src/or/hs_common.c b/src/or/hs_common.c index a0f2af29cd..ad783a36fb 100644 --- a/src/or/hs_common.c +++ b/src/or/hs_common.c @@ -22,6 +22,7 @@ #include "hs_client.h" #include "hs_ident.h" #include "hs_service.h" +#include "hs_circuitmap.h" #include "policies.h" #include "rendcommon.h" #include "rendservice.h" diff --git a/src/or/hs_common.h b/src/or/hs_common.h index c95e59a6f8..18c4e70141 100644 --- a/src/or/hs_common.h +++ b/src/or/hs_common.h @@ -161,6 +161,8 @@ typedef struct hsdir_index_t { void hs_init(void); void hs_free_all(void); +void hs_cleanup_circ(circuit_t *circ); + int hs_check_service_private_dir(const char *username, const char *path, unsigned int dir_group_readable, unsigned int create); -- cgit v1.2.3-54-g00ecf