From f4f809fe3dc45ce72b4ccfb891eb715a8b330675 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 19 Sep 2018 10:56:15 -0400 Subject: [PATCH 1/2] hs-v3: Close all SOCKS request on descriptor failure Client side, when a descriptor is finally fetched and stored in the cache, we then go over all pending SOCKS request for that descriptor. If it turns out that the intro points are unusable, we close the first SOCKS request but not the others for the same .onion. This commit makes it that we'll close all SOCKS requests so we don't let hanging the other ones. It also fixes another bug which is having a SOCKS connection in RENDDESC_WAIT state but with a descriptor in the cache. At some point, tor will expire the intro failure cache which will make that descriptor usable again. When retrying all SOCKS connection (retry_all_socks_conn_waiting_for_desc()), we won't end up in the code path where we have already the descriptor for a pending request causing a BUG(). Bottom line is that we should never have pending requests (waiting for a descriptor) with that descriptor in the cache (even if unusable). Fixees #27410. Signed-off-by: David Goulet --- changes/ticket27410 | 5 +++++ src/feature/hs/hs_client.c | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 changes/ticket27410 diff --git a/changes/ticket27410 b/changes/ticket27410 new file mode 100644 index 0000000000..a21fdde58e --- /dev/null +++ b/changes/ticket27410 @@ -0,0 +1,5 @@ + o Minor bugfixes (hidden service v3): + - Close all SOCKS request (for the same .onion) if the newly fetched + descriptor is unusable. Before that, we would close only the first one + leaving the other hanging and let to time out by themselves. Fixes bug + 27410; bugfix on 0.3.2.1-alpha. diff --git a/src/feature/hs/hs_client.c b/src/feature/hs/hs_client.c index a6384b87a3..fa7c78c8f9 100644 --- a/src/feature/hs/hs_client.c +++ b/src/feature/hs/hs_client.c @@ -1688,7 +1688,7 @@ hs_client_desc_has_arrived(const hs_ident_dir_conn_t *ident) /* We are unable to use the descriptor so remove the directory request * from the cache so the next connection can try again. */ note_connection_attempt_succeeded(edge_conn->hs_ident); - goto end; + continue; } log_info(LD_REND, "Descriptor has arrived. Launching circuits."); From cb81a69f90c82c2c2a5b41b80eadf50be5e8193d Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 19 Sep 2018 09:55:57 -0400 Subject: [PATCH 2/2] test: hs-v3 desc has arrived unit test That unit test makes sure we don't have pending SOCK request if the descriptor turns out to be unusable. Part of #27410. Signed-off-by: David Goulet --- src/feature/hs/hs_client.c | 2 +- src/feature/hs/hs_client.h | 2 + src/test/test_hs_client.c | 117 +++++++++++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+), 1 deletion(-) diff --git a/src/feature/hs/hs_client.c b/src/feature/hs/hs_client.c index fa7c78c8f9..563f6c6b3b 100644 --- a/src/feature/hs/hs_client.c +++ b/src/feature/hs/hs_client.c @@ -247,7 +247,7 @@ close_all_socks_conns_waiting_for_desc(const ed25519_public_key_t *identity_pk, /* Find all pending SOCKS connection waiting for a descriptor and retry them * all. This is called when the directory information changed. */ -static void +STATIC void retry_all_socks_conn_waiting_for_desc(void) { smartlist_t *conns = diff --git a/src/feature/hs/hs_client.h b/src/feature/hs/hs_client.h index 1ba0338dc3..fb4f9e9e9f 100644 --- a/src/feature/hs/hs_client.h +++ b/src/feature/hs/hs_client.h @@ -104,6 +104,8 @@ STATIC int handle_rendezvous2(origin_circuit_t *circ, const uint8_t *payload, MOCK_DECL(STATIC hs_client_fetch_status_t, fetch_v3_desc, (const ed25519_public_key_t *onion_identity_pk)); +STATIC void retry_all_socks_conn_waiting_for_desc(void); + #ifdef TOR_UNIT_TESTS STATIC digest256map_t *get_hs_client_auths_map(void); diff --git a/src/test/test_hs_client.c b/src/test/test_hs_client.c index c91e82ed4a..a115997c7f 100644 --- a/src/test/test_hs_client.c +++ b/src/test/test_hs_client.c @@ -523,6 +523,9 @@ mock_connection_mark_unattached_ap_(entry_connection_t *conn, int endreason, (void) line; (void) file; conn->edge_.end_reason = endreason; + /* This function ultimately will flag this so make sure we do also in the + * MOCK one so we can assess closed connections vs open ones. */ + conn->edge_.base_.marked_for_close = 1; } static void @@ -771,6 +774,117 @@ test_config_client_authorization(void *arg) UNMOCK(check_private_dir); } +static entry_connection_t * +helper_build_socks_connection(const ed25519_public_key_t *service_pk, + int conn_state) +{ + entry_connection_t *socks = entry_connection_new(CONN_TYPE_AP, AF_INET); + ENTRY_TO_EDGE_CONN(socks)->hs_ident = hs_ident_edge_conn_new(service_pk); + TO_CONN(ENTRY_TO_EDGE_CONN(socks))->state = conn_state; + smartlist_add(get_connection_array(), &socks->edge_.base_); + return socks; +} + +static void +test_desc_has_arrived_cleanup(void *arg) +{ + /* The goal of this test is to make sure we clean up everything in between + * two descriptors from the same .onion. Because intro points can change + * from one descriptor to another, once we received a new descriptor, we + * need to cleanup the remaining circuits so they aren't used or selected + * when establishing a connection with the newly stored descriptor. + * + * This test was created because of #27410. */ + + int ret; + char *desc_str = NULL; + hs_descriptor_t *desc = NULL; + const hs_descriptor_t *cached_desc; + ed25519_keypair_t signing_kp; + entry_connection_t *socks1 = NULL, *socks2 = NULL; + hs_ident_dir_conn_t hs_dir_ident; + + (void) arg; + + hs_init(); + + MOCK(networkstatus_get_live_consensus, + mock_networkstatus_get_live_consensus); + MOCK(connection_mark_unattached_ap_, + mock_connection_mark_unattached_ap_); + MOCK(router_have_minimum_dir_info, + mock_router_have_minimum_dir_info_true); + + /* Set consensus time before our time so the cache lookup can always + * validate that the entry is not expired. */ + parse_rfc1123_time("Sat, 26 Oct 1985 13:00:00 UTC", &mock_ns.valid_after); + parse_rfc1123_time("Sat, 26 Oct 1985 14:00:00 UTC", &mock_ns.fresh_until); + parse_rfc1123_time("Sat, 26 Oct 1985 16:00:00 UTC", &mock_ns.valid_until); + + /* Build a descriptor for a specific .onion. */ + ret = ed25519_keypair_generate(&signing_kp, 0); + tt_int_op(ret, OP_EQ, 0); + desc = hs_helper_build_hs_desc_with_ip(&signing_kp); + tt_assert(desc); + ret = hs_desc_encode_descriptor(desc, &signing_kp, NULL, &desc_str); + tt_int_op(ret, OP_EQ, 0); + + /* Store in the client cache. */ + ret = hs_cache_store_as_client(desc_str, &signing_kp.pubkey); + tt_int_op(ret, OP_EQ, 0); + cached_desc = hs_cache_lookup_as_client(&signing_kp.pubkey); + tt_assert(cached_desc); + hs_helper_desc_equal(desc, cached_desc); + + /* Create two SOCKS connection for the same .onion both in the waiting for a + * descriptor state. */ + socks1 = helper_build_socks_connection(&signing_kp.pubkey, + AP_CONN_STATE_RENDDESC_WAIT); + tt_assert(socks1); + socks2 = helper_build_socks_connection(&signing_kp.pubkey, + AP_CONN_STATE_RENDDESC_WAIT); + tt_assert(socks2); + + /* Now, we'll make the intro points in the current descriptor unusable so + * the hs_client_desc_has_arrived() will take the right code path that we + * want to test that is the fetched descriptor has bad intro points. */ + SMARTLIST_FOREACH_BEGIN(desc->encrypted_data.intro_points, + hs_desc_intro_point_t *, ip) { + hs_cache_client_intro_state_note(&signing_kp.pubkey, + &ip->auth_key_cert->signed_key, + INTRO_POINT_FAILURE_GENERIC); + } SMARTLIST_FOREACH_END(ip); + + /* Simulate that a new descriptor just arrived. We should have both of our + * SOCKS connection to be ended with a resolved failed. */ + hs_ident_dir_conn_init(&signing_kp.pubkey, + &desc->plaintext_data.blinded_pubkey, &hs_dir_ident); + hs_client_desc_has_arrived(&hs_dir_ident); + tt_int_op(socks1->edge_.end_reason, OP_EQ, END_STREAM_REASON_RESOLVEFAILED); + /* XXX: MUST work with OP_EQ. */ + tt_int_op(socks2->edge_.end_reason, OP_EQ, END_STREAM_REASON_RESOLVEFAILED); + + /* Now let say tor cleans up the intro state cache which resets all intro + * point failure count. */ + hs_cache_client_intro_state_purge(); + + /* Retrying all SOCKS which should basically do nothing since we don't have + * any pending SOCKS connection in AP_CONN_STATE_RENDDESC_WAIT state. */ + /* XXX: BUG() is triggered here, shouldn't if socks2 wasn't alive. */ + retry_all_socks_conn_waiting_for_desc(); + + done: + connection_free_minimal(ENTRY_TO_CONN(socks1)); + connection_free_minimal(ENTRY_TO_CONN(socks2)); + hs_descriptor_free(desc); + tor_free(desc_str); + hs_free_all(); + + UNMOCK(networkstatus_get_live_consensus); + UNMOCK(connection_mark_unattached_ap_); + UNMOCK(router_have_minimum_dir_info); +} + struct testcase_t hs_client_tests[] = { { "e2e_rend_circuit_setup_legacy", test_e2e_rend_circuit_setup_legacy, TT_FORK, NULL, NULL }, @@ -786,5 +900,8 @@ struct testcase_t hs_client_tests[] = { NULL, NULL }, { "config_client_authorization", test_config_client_authorization, TT_FORK, NULL, NULL }, + { "desc_has_arrived_cleanup", test_desc_has_arrived_cleanup, + TT_FORK, NULL, NULL }, + END_OF_TESTCASES };