From 8a05e1a5d2227b0333940d004b97b1c96fa9d991 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 21 Dec 2016 13:34:45 -0500 Subject: [PATCH 1/3] circuit: Add a function to get the next service intro circuit Signed-off-by: David Goulet --- src/or/circuitlist.c | 35 +++++++++++++++++++++++++++++++++++ src/or/circuitlist.h | 1 + 2 files changed, 36 insertions(+) diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 5943e516ff..4ff5eff1d2 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -1440,6 +1440,41 @@ circuit_get_ready_rend_circ_by_rend_data(const rend_data_t *rend_data) return NULL; } +/** Return the first service introduction circuit originating from the global + * circuit list after start or at the start of the list if start + * is NULL. Return NULL if no circuit is found. + * + * A service introduction point circuit has a purpose of either + * CIRCUIT_PURPOSE_S_ESTABLISH_INTRO or CIRCUIT_PURPOSE_S_INTRO. This does not + * return a circuit marked for close and its state must be open. */ +origin_circuit_t * +circuit_get_next_service_intro_circ(origin_circuit_t *start) +{ + int idx = 0; + smartlist_t *lst = circuit_get_global_list(); + + if (start) { + idx = TO_CIRCUIT(start)->global_circuitlist_idx + 1; + } + + for ( ; idx < smartlist_len(lst); ++idx) { + circuit_t *circ = smartlist_get(lst, idx); + + /* Ignore a marked for close circuit or purpose not matching a service + * intro point or if the state is not open. */ + if (circ->marked_for_close || circ->state != CIRCUIT_STATE_OPEN || + (circ->purpose != CIRCUIT_PURPOSE_S_ESTABLISH_INTRO && + circ->purpose != CIRCUIT_PURPOSE_S_INTRO)) { + continue; + } + /* The purposes we are looking for are only for origin circuits so the + * following is valid. */ + return TO_ORIGIN_CIRCUIT(circ); + } + /* Not found. */ + return NULL; +} + /** Return the first circuit originating here in global_circuitlist after * start whose purpose is purpose, and where digest (if * set) matches the private key digest of the rend data associated with the diff --git a/src/or/circuitlist.h b/src/or/circuitlist.h index d83801a7a8..6abee37dc4 100644 --- a/src/or/circuitlist.h +++ b/src/or/circuitlist.h @@ -47,6 +47,7 @@ origin_circuit_t *circuit_get_ready_rend_circ_by_rend_data( const rend_data_t *rend_data); origin_circuit_t *circuit_get_next_by_pk_and_purpose(origin_circuit_t *start, const uint8_t *digest, uint8_t purpose); +origin_circuit_t *circuit_get_next_service_intro_circ(origin_circuit_t *start); origin_circuit_t *circuit_find_to_cannibalize(uint8_t purpose, extend_info_t *info, int flags); void circuit_mark_all_unused_circs(void); From 36b5ca2c8b07f4cbdb1b76e9fb0c10fff3d540ac Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 21 Dec 2016 13:35:22 -0500 Subject: [PATCH 2/3] hs: Move and improve the service pruning code First, this commit moves the code used to prune the service list when reloading Tor (HUP signal for instance) to a function from rend_config_services(). Second, fix bug #21054, improve the code by using the newly added circuit_get_next_service_intro_circ() function instead of poking at the global list directly and add _many_ more comments. Fixes #21054. Signed-off-by: David Goulet --- changes/bug21054 | 5 ++ src/or/rendservice.c | 172 +++++++++++++++++++++++-------------------- 2 files changed, 99 insertions(+), 78 deletions(-) create mode 100644 changes/bug21054 diff --git a/changes/bug21054 b/changes/bug21054 new file mode 100644 index 0000000000..bc499490ee --- /dev/null +++ b/changes/bug21054 @@ -0,0 +1,5 @@ + o Minor bugfixes (hidden service): + - Fix the config reload pruning of old vs new services so it actually + works when both ephemeral and non ephemeral services were configured + which lead to a BUG() stacktrace. Close #21054; Bugfix on + tor-0.3.0.1-alpha. diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 1713a84bbd..a4e6452b4a 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -542,6 +542,95 @@ rend_service_check_dir_and_add(smartlist_t *service_list, return rend_add_service(s_list, service); } +/* If this is a reload and there were hidden services configured before, + * keep the introduction points that are still needed and close the + * other ones. */ +static void +prune_services_on_reload(smartlist_t *old_service_list, + smartlist_t *new_service_list) +{ + origin_circuit_t *ocirc = NULL; + smartlist_t *surviving_services = NULL; + + tor_assert(old_service_list); + tor_assert(new_service_list); + + /* This contains all _existing_ services that survives the relaod that is + * that haven't been removed from the configuration. The difference between + * this list and the new service list is that the new list can possibly + * contain newly configured service that have no introduction points opened + * yet nor key material loaded or generated. */ + surviving_services = smartlist_new(); + + /* Preserve the existing ephemeral services. + * + * This is the ephemeral service equivalent of the "Copy introduction + * points to new services" block, except there's no copy required since + * the service structure isn't regenerated. + * + * After this is done, all ephemeral services will be: + * * Removed from old_service_list, so the equivalent non-ephemeral code + * will not attempt to preserve them. + * * Added to the new_service_list (that previously only had the + * services listed in the configuration). + * * Added to surviving_services, which is the list of services that + * will NOT have their intro point closed. + */ + SMARTLIST_FOREACH_BEGIN(old_service_list, rend_service_t *, old) { + if (rend_service_is_ephemeral(old)) { + SMARTLIST_DEL_CURRENT(old_service_list, old); + smartlist_add(surviving_services, old); + smartlist_add(new_service_list, old); + } + } SMARTLIST_FOREACH_END(old); + + /* Copy introduction points to new services. This is O(n^2), but it's only + * called on reconfigure, so it's ok performance wise. */ + SMARTLIST_FOREACH_BEGIN(new_service_list, rend_service_t *, new) { + SMARTLIST_FOREACH_BEGIN(old_service_list, rend_service_t *, old) { + /* Skip ephemeral services as we only want to copy introduction points + * from current services to newly configured one that already exists. + * The same directory means it's the same service. */ + if (rend_service_is_ephemeral(new) || rend_service_is_ephemeral(old) || + strcmp(old->directory, new->directory)) { + continue; + } + smartlist_add_all(new->intro_nodes, old->intro_nodes); + smartlist_clear(old->intro_nodes); + smartlist_add_all(new->expiring_nodes, old->expiring_nodes); + smartlist_clear(old->expiring_nodes); + /* This regular service will survive the closing IPs step after. */ + smartlist_add(surviving_services, old); + break; + } SMARTLIST_FOREACH_END(old); + } SMARTLIST_FOREACH_END(new); + + /* For every service introduction circuit we can find, see if we have a + * matching surviving configured service. If not, close the circuit. */ + while ((ocirc = circuit_get_next_service_intro_circ(ocirc))) { + int keep_it = 0; + tor_assert(ocirc->rend_data); + SMARTLIST_FOREACH_BEGIN(surviving_services, const rend_service_t *, s) { + if (rend_circuit_pk_digest_eq(ocirc, (uint8_t *) s->pk_digest)) { + /* Keep this circuit as we have a matching configured service. */ + keep_it = 1; + break; + } + } SMARTLIST_FOREACH_END(s); + if (keep_it) { + continue; + } + log_info(LD_REND, "Closing intro point %s for service %s.", + safe_str_client(extend_info_describe( + ocirc->build_state->chosen_exit)), + safe_str_client(rend_data_get_address(ocirc->rend_data))); + /* Reason is FINISHED because service has been removed and thus the + * circuit is considered old/uneeded. */ + circuit_mark_for_close(TO_CIRCUIT(ocirc), END_CIRC_REASON_FINISHED); + } + smartlist_free(surviving_services); +} + /** Set up rend_service_list, based on the values of HiddenServiceDir and * HiddenServicePort in options. Return 0 on success and -1 on * failure. (If validate_only is set, parse, warn and return as @@ -807,84 +896,11 @@ rend_config_services(const or_options_t *options, int validate_only) * keep the introduction points that are still needed and close the * other ones. */ if (old_service_list && !validate_only) { - smartlist_t *surviving_services = smartlist_new(); - - /* Preserve the existing ephemeral services. - * - * This is the ephemeral service equivalent of the "Copy introduction - * points to new services" block, except there's no copy required since - * the service structure isn't regenerated. - * - * After this is done, all ephemeral services will be: - * * Removed from old_service_list, so the equivalent non-ephemeral code - * will not attempt to preserve them. - * * Added to the new rend_service_list (that previously only had the - * services listed in the configuration). - * * Added to surviving_services, which is the list of services that - * will NOT have their intro point closed. - */ - SMARTLIST_FOREACH(old_service_list, rend_service_t *, old, { - if (rend_service_is_ephemeral(old)) { - SMARTLIST_DEL_CURRENT(old_service_list, old); - smartlist_add(surviving_services, old); - smartlist_add(rend_service_list, old); - } - }); - - /* Copy introduction points to new services. */ - /* XXXX This is O(n^2), but it's only called on reconfigure, so it's - * probably ok? */ - SMARTLIST_FOREACH_BEGIN(rend_service_list, rend_service_t *, new) { - SMARTLIST_FOREACH_BEGIN(old_service_list, rend_service_t *, old) { - if (BUG(rend_service_is_ephemeral(new)) || - BUG(rend_service_is_ephemeral(old))) { - continue; - } - if (BUG(!new->directory) || BUG(!old->directory) || - strcmp(old->directory, new->directory)) { - continue; - } - smartlist_add_all(new->intro_nodes, old->intro_nodes); - smartlist_clear(old->intro_nodes); - smartlist_add_all(new->expiring_nodes, old->expiring_nodes); - smartlist_clear(old->expiring_nodes); - smartlist_add(surviving_services, old); - break; - } SMARTLIST_FOREACH_END(old); - } SMARTLIST_FOREACH_END(new); - - /* Close introduction circuits of services we don't serve anymore. */ - /* XXXX it would be nicer if we had a nicer abstraction to use here, - * so we could just iterate over the list of services to close, but - * once again, this isn't critical-path code. */ - SMARTLIST_FOREACH_BEGIN(circuit_get_global_list(), circuit_t *, circ) { - if (!circ->marked_for_close && - circ->state == CIRCUIT_STATE_OPEN && - (circ->purpose == CIRCUIT_PURPOSE_S_ESTABLISH_INTRO || - circ->purpose == CIRCUIT_PURPOSE_S_INTRO)) { - origin_circuit_t *oc = TO_ORIGIN_CIRCUIT(circ); - int keep_it = 0; - tor_assert(oc->rend_data); - SMARTLIST_FOREACH(surviving_services, rend_service_t *, ptr, { - if (rend_circuit_pk_digest_eq(oc, (uint8_t *) ptr->pk_digest)) { - keep_it = 1; - break; - } - }); - if (keep_it) - continue; - log_info(LD_REND, "Closing intro point %s for service %s.", - safe_str_client(extend_info_describe( - oc->build_state->chosen_exit)), - rend_data_get_address(oc->rend_data)); - circuit_mark_for_close(circ, END_CIRC_REASON_FINISHED); - /* XXXX Is there another reason we should use here? */ - } - } - SMARTLIST_FOREACH_END(circ); - smartlist_free(surviving_services); - SMARTLIST_FOREACH(old_service_list, rend_service_t *, ptr, - rend_service_free(ptr)); + prune_services_on_reload(old_service_list, rend_service_list); + /* Every remaining service in the old list have been removed from the + * configuration so clean them up safely. */ + SMARTLIST_FOREACH(old_service_list, rend_service_t *, s, + rend_service_free(s)); smartlist_free(old_service_list); } From 2d1fa58fb442122a64cdef9be1d18e8f53067038 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 21 Dec 2016 15:00:02 -0500 Subject: [PATCH 3/3] test: Add unit test for prune_services_on_reload() Signed-off-by: David Goulet --- src/or/rendservice.c | 2 +- src/or/rendservice.h | 3 ++ src/test/test_hs.c | 123 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 127 insertions(+), 1 deletion(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index a4e6452b4a..2d94bb8a87 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -545,7 +545,7 @@ rend_service_check_dir_and_add(smartlist_t *service_list, /* If this is a reload and there were hidden services configured before, * keep the introduction points that are still needed and close the * other ones. */ -static void +STATIC void prune_services_on_reload(smartlist_t *old_service_list, smartlist_t *new_service_list) { diff --git a/src/or/rendservice.h b/src/or/rendservice.h index 4e6b9a2536..3bfac0bece 100644 --- a/src/or/rendservice.h +++ b/src/or/rendservice.h @@ -132,6 +132,9 @@ STATIC int rend_service_poison_new_single_onion_dir( STATIC ssize_t encode_establish_intro_cell_legacy(char *cell_body_out, crypto_pk_t *intro_key, char *rend_circ_nonce); +STATIC void prune_services_on_reload(smartlist_t *old_service_list, + smartlist_t *new_service_list); + #endif int num_rend_services(void); diff --git a/src/test/test_hs.c b/src/test/test_hs.c index ac9788ceea..fbaabe91d8 100644 --- a/src/test/test_hs.c +++ b/src/test/test_hs.c @@ -787,6 +787,126 @@ test_single_onion_poisoning(void *arg) tor_free(mock_options->DataDirectory); } +static rend_service_t * +helper_create_rend_service(const char *path) +{ + rend_service_t *s = tor_malloc_zero(sizeof(rend_service_t)); + s->ports = smartlist_new(); + s->intro_nodes = smartlist_new(); + s->expiring_nodes = smartlist_new(); + if (path) { + s->directory = tor_strdup(path); + } + return s; +} + +static void +test_prune_services_on_reload(void *arg) +{ + smartlist_t *new = smartlist_new(), *old = smartlist_new(); + /* Non ephemeral service. */ + rend_service_t *s1 = helper_create_rend_service("SomePath"); + /* Create a non ephemeral service with the _same_ path as so we can test the + * transfer of introduction point between the same services on reload. */ + rend_service_t *s2 = helper_create_rend_service(s1->directory); + /* Ephemeral service (directory is NULL). */ + rend_service_t *e1 = helper_create_rend_service(NULL); + rend_service_t *e2 = helper_create_rend_service(NULL); + + (void) arg; + + { + /* Add both services to the old list. */ + smartlist_add(old, s1); + smartlist_add(old, e1); + /* Only put the non ephemeral in the new list. */ + smartlist_add(new, s1); + prune_services_on_reload(old, new); + /* We expect that the ephemeral one is in the new list but removed from + * the old one. */ + tt_int_op(smartlist_len(old), OP_EQ, 1); + tt_assert(smartlist_get(old, 0) == s1); + tt_int_op(smartlist_len(new), OP_EQ, 2); + tt_assert(smartlist_get(new, 0) == s1); + tt_assert(smartlist_get(new, 1) == e1); + /* Cleanup for next test. */ + smartlist_clear(new); + smartlist_clear(old); + } + + { + /* This test will make sure that only the ephemeral service is kept if the + * new list is empty. The old list should contain only the non ephemeral + * one. */ + smartlist_add(old, s1); + smartlist_add(old, e1); + prune_services_on_reload(old, new); + tt_int_op(smartlist_len(old), OP_EQ, 1); + tt_assert(smartlist_get(old, 0) == s1); + tt_int_op(smartlist_len(new), OP_EQ, 1); + tt_assert(smartlist_get(new, 0) == e1); + /* Cleanup for next test. */ + smartlist_clear(new); + smartlist_clear(old); + } + + { + /* This test makes sure that the new list stays the same even from the old + * list being completely different. */ + smartlist_add(new, s1); + smartlist_add(new, e1); + prune_services_on_reload(old, new); + tt_int_op(smartlist_len(old), OP_EQ, 0); + tt_int_op(smartlist_len(new), OP_EQ, 2); + tt_assert(smartlist_get(new, 0) == s1); + tt_assert(smartlist_get(new, 1) == e1); + /* Cleanup for next test. */ + smartlist_clear(new); + } + + { + rend_intro_point_t ip1; + /* This IP should be found in the s2 service after pruning. */ + smartlist_add(s1->intro_nodes, &ip1); + /* Setup our list. */ + smartlist_add(old, s1); + smartlist_add(new, s2); + prune_services_on_reload(old, new); + tt_int_op(smartlist_len(old), OP_EQ, 1); + /* Intro nodes have been moved to the s2 in theory so it must be empty. */ + tt_int_op(smartlist_len(s1->intro_nodes), OP_EQ, 0); + tt_int_op(smartlist_len(new), OP_EQ, 1); + rend_service_t *elem = smartlist_get(new, 0); + tt_assert(elem); + tt_assert(elem == s2); + tt_int_op(smartlist_len(elem->intro_nodes), OP_EQ, 1); + tt_assert(smartlist_get(elem->intro_nodes, 0) == &ip1); + smartlist_clear(s1->intro_nodes); + smartlist_clear(s2->intro_nodes); + /* Cleanup for next test. */ + smartlist_clear(new); + smartlist_clear(old); + } + + { + /* Test two ephemeral services. */ + smartlist_add(old, e1); + smartlist_add(old, e2); + prune_services_on_reload(old, new); + /* Check if they've all been transfered. */ + tt_int_op(smartlist_len(old), OP_EQ, 0); + tt_int_op(smartlist_len(new), OP_EQ, 2); + } + + done: + rend_service_free(s1); + rend_service_free(s2); + rend_service_free(e1); + rend_service_free(e2); + smartlist_free(new); + smartlist_free(old); +} + struct testcase_t hs_tests[] = { { "hs_rend_data", test_hs_rend_data, TT_FORK, NULL, NULL }, @@ -807,6 +927,9 @@ struct testcase_t hs_tests[] = { TT_FORK, &passthrough_setup, (void*)(CREATE_HS_DIR2) }, { "single_onion_poisoning_create_dir_both", test_single_onion_poisoning, TT_FORK, &passthrough_setup, (void*)(CREATE_HS_DIR1 | CREATE_HS_DIR2) }, + { "prune_services_on_reload", test_prune_services_on_reload, TT_FORK, + NULL, NULL }, + END_OF_TESTCASES };