diff --git a/changes/bug23603 b/changes/bug23603 new file mode 100644 index 0000000000..dfb2052c9a --- /dev/null +++ b/changes/bug23603 @@ -0,0 +1,7 @@ + o Minor bugfixes (hidden service v3): + - Fix a race between the circuit close and free where the service would + launch a new intro circuit after the close, and then fail to register it + before the free of the previously closed circuit. This was making the + service unable to find the established intro circuit and thus not upload + its descriptor. It can make a service unavailable for up to 24 hours. + Fixes bug 23603; bugfix on 0.3.2.1-alpha. diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index d37d986f17..d442887c9e 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); @@ -2481,8 +2481,8 @@ assert_cpath_ok(const crypt_path_t *cp) /** Verify that circuit c has all of its invariants * correct. Trigger an assert if anything is invalid. */ -void -assert_circuit_ok(const circuit_t *c) +MOCK_IMPL(void, +assert_circuit_ok,(const circuit_t *c)) { edge_connection_t *conn; const or_circuit_t *or_circ = NULL; diff --git a/src/or/circuitlist.h b/src/or/circuitlist.h index 5d0da15ca8..4190c1f82e 100644 --- a/src/or/circuitlist.h +++ b/src/or/circuitlist.h @@ -68,7 +68,7 @@ int circuit_count_pending_on_channel(channel_t *chan); circuit_mark_for_close_((c), (reason), __LINE__, SHORT_FILE__) void assert_cpath_layer_ok(const crypt_path_t *cp); -void assert_circuit_ok(const circuit_t *c); +MOCK_DECL(void, assert_circuit_ok,(const circuit_t *c)); void circuit_free_all(void); void circuits_handle_oom(size_t current_allocation); 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); diff --git a/src/or/hs_service.c b/src/or/hs_service.c index a2082b3914..99965ddc37 100644 --- a/src/or/hs_service.c +++ b/src/or/hs_service.c @@ -1839,6 +1839,12 @@ cleanup_intro_points(hs_service_t *service, time_t now) (node == NULL) ? " fell off the consensus" : "", ip->circuit_retries); + /* We've retried too many times, remember it as a failed intro point + * so we don't pick it up again for INTRO_CIRC_RETRY_PERIOD sec. */ + if (ip->circuit_retries > MAX_INTRO_POINT_CIRCUIT_RETRIES) { + remember_failing_intro_point(ip, desc, approx_time()); + } + /* Remove intro point from descriptor map. We'll add it to the failed * map if we retried it too many times. */ MAP_DEL_CURRENT(key); @@ -2946,15 +2952,6 @@ hs_service_intro_circ_has_closed(origin_circuit_t *circ) * keeping the object in the descriptor, we'll be able to retry. */ ip->circuit_established = 0; - /* We've retried too many times, remember it as a failed intro point so we - * don't pick it up again. It will be retried in INTRO_CIRC_RETRY_PERIOD - * seconds. */ - if (ip->circuit_retries > MAX_INTRO_POINT_CIRCUIT_RETRIES) { - remember_failing_intro_point(ip, desc, approx_time()); - service_intro_point_remove(service, ip); - service_intro_point_free(ip); - } - end: return; } diff --git a/src/test/test_hs_service.c b/src/test/test_hs_service.c index 3084c6b958..8407eccfa8 100644 --- a/src/test/test_hs_service.c +++ b/src/test/test_hs_service.c @@ -778,6 +778,92 @@ test_rdv_circuit_opened(void *arg) UNMOCK(relay_send_command_from_edge_); } +static void +mock_assert_circuit_ok(const circuit_t *c) +{ + (void) c; + return; +} + +/** Test for the general mechanism for closing intro circs. + * Also a way to identify that #23603 has been fixed. */ +static void +test_closing_intro_circs(void *arg) +{ + hs_service_t *service = NULL; + hs_service_intro_point_t *ip = NULL, *entry = NULL; + origin_circuit_t *intro_circ = NULL, *tmp_circ; + int flags = CIRCLAUNCH_NEED_UPTIME | CIRCLAUNCH_IS_INTERNAL; + + (void) arg; + + MOCK(assert_circuit_ok, mock_assert_circuit_ok); + + hs_init(); + + /* Initialize service */ + service = helper_create_service(); + /* Initialize intro point */ + ip = helper_create_service_ip(); + tt_assert(ip); + service_intro_point_add(service->desc_current->intro_points.map, ip); + + /* Initialize intro circuit */ + intro_circ = origin_circuit_init(CIRCUIT_PURPOSE_S_ESTABLISH_INTRO, flags); + intro_circ->hs_ident = hs_ident_circuit_new(&service->keys.identity_pk, + HS_IDENT_CIRCUIT_INTRO); + /* Register circuit in the circuitmap . */ + hs_circuitmap_register_intro_circ_v3_service_side(intro_circ, + &ip->auth_key_kp.pubkey); + tmp_circ = + hs_circuitmap_get_intro_circ_v3_service_side(&ip->auth_key_kp.pubkey); + tt_ptr_op(tmp_circ, OP_EQ, intro_circ); + + /* Pretend that intro point has failed too much */ + ip->circuit_retries = MAX_INTRO_POINT_CIRCUIT_RETRIES+1; + + /* Now pretend we are freeing this intro circuit. We want to see that our + * destructor is not gonna kill our intro point structure since that's the + * job of the cleanup routine. */ + circuit_free(TO_CIRCUIT(intro_circ)); + intro_circ = NULL; + entry = service_intro_point_find(service, &ip->auth_key_kp.pubkey); + tt_assert(entry); + /* The free should also remove the circuit from the circuitmap. */ + tmp_circ = + hs_circuitmap_get_intro_circ_v3_service_side(&ip->auth_key_kp.pubkey); + tt_assert(!tmp_circ); + + /* Now pretend that a new intro point circ was launched and opened. Check + * that the intro point will be established correctly. */ + intro_circ = origin_circuit_init(CIRCUIT_PURPOSE_S_ESTABLISH_INTRO, flags); + intro_circ->hs_ident = hs_ident_circuit_new(&service->keys.identity_pk, + HS_IDENT_CIRCUIT_INTRO); + ed25519_pubkey_copy(&intro_circ->hs_ident->intro_auth_pk, + &ip->auth_key_kp.pubkey); + /* Register circuit in the circuitmap . */ + hs_circuitmap_register_intro_circ_v3_service_side(intro_circ, + &ip->auth_key_kp.pubkey); + tmp_circ = + hs_circuitmap_get_intro_circ_v3_service_side(&ip->auth_key_kp.pubkey); + tt_ptr_op(tmp_circ, OP_EQ, intro_circ); + tt_int_op(TO_CIRCUIT(intro_circ)->marked_for_close, OP_EQ, 0); + circuit_mark_for_close(TO_CIRCUIT(intro_circ), END_CIRC_REASON_INTERNAL); + tt_int_op(TO_CIRCUIT(intro_circ)->marked_for_close, OP_NE, 0); + /* At this point, we should not be able to find it in the circuitmap. */ + tmp_circ = + hs_circuitmap_get_intro_circ_v3_service_side(&ip->auth_key_kp.pubkey); + tt_assert(!tmp_circ); + + done: + if (intro_circ) { + circuit_free(TO_CIRCUIT(intro_circ)); + } + /* Frees the service object. */ + hs_free_all(); + UNMOCK(assert_circuit_ok); +} + /** Test sending and receiving introduce2 cells */ static void test_introduce2(void *arg) @@ -1512,6 +1598,8 @@ struct testcase_t hs_service_tests[] = { NULL, NULL }, { "intro_established", test_intro_established, TT_FORK, NULL, NULL }, + { "closing_intro_circs", test_closing_intro_circs, TT_FORK, + NULL, NULL }, { "rdv_circuit_opened", test_rdv_circuit_opened, TT_FORK, NULL, NULL }, { "introduce2", test_introduce2, TT_FORK,