From ba63c4099a491c3f96160005f94cf3a61433c145 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 25 Sep 2017 16:47:16 -0400 Subject: [PATCH 1/3] hs-v3: Don't cleanup intro point in has_closed() The hs_service_intro_circ_has_closed() was removing intro point objects if too many retries. We shouldn't cleanup those objects in that function at all but rather let cleanup_intro_points() do its job and clean it properly. This was causing an issue in #23603. Furthermore, this moves the logic of remembering failing intro points in the cleanup_intro_points() function which should really be the only function to know when to cleanup and thus when an introduction point should be remembered as a failed one. Fixes #23603 Signed-off-by: David Goulet --- src/or/hs_service.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) 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; } From e80893e51b0c0320838cbed8c46fd5b0fe608bef Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 4 Dec 2017 11:49:48 -0500 Subject: [PATCH 2/3] 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 --- changes/bug23603 | 7 +++++++ 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 ++ 6 files changed, 54 insertions(+), 13 deletions(-) create mode 100644 changes/bug23603 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..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); From 68601a85ca48bf2cd54087fe21fdcf8aaa23ebb2 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 4 Dec 2017 11:45:41 -0500 Subject: [PATCH 3/3] test: Add HSv3 unit test for expiring intro point Signed-off-by: David Goulet --- src/or/circuitlist.c | 4 +- src/or/circuitlist.h | 2 +- src/test/test_hs_service.c | 88 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 3 deletions(-) diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 45d4521c22..d442887c9e 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -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/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,