From 98057d274c8928ae14ed6adc2903fce4a9f87214 Mon Sep 17 00:00:00 2001 From: teor Date: Fri, 4 Nov 2016 15:46:24 +1100 Subject: [PATCH 1/5] Create HS directories in rend_config_services, then check before use (We only create HS directories if we are acting on the config.) Log a BUG warning if the directories aren't present immediately before they are used, then fail. --- src/or/rendservice.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 56dbacdaf..15b7feb13 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -466,7 +466,8 @@ rend_config_services(const or_options_t *options, int validate_only) for (line = options->RendConfigLines; line; line = line->next) { if (!strcasecmp(line->key, "HiddenServiceDir")) { if (service) { /* register the one we just finished parsing */ - if (rend_service_check_private_dir(options, service, 0) < 0) { + if (rend_service_check_private_dir(options, service, !validate_only) + < 0) { rend_service_free(service); return -1; } @@ -681,7 +682,7 @@ rend_config_services(const or_options_t *options, int validate_only) } } if (service) { - if (rend_service_check_private_dir(options, service, 0) < 0) { + if (rend_service_check_private_dir(options, service, !validate_only) < 0) { rend_service_free(service); return -1; } @@ -1098,8 +1099,8 @@ poison_new_single_onion_hidden_service_dir(const rend_service_t *service) return -1; } - /* Make sure the directory exists */ - if (rend_service_check_private_dir(get_options(), service, 1) < 0) + /* Make sure the directory was created in options_act */ + if (BUG(rend_service_check_private_dir(get_options(), service, 0) < 0)) return -1; poison_fname = rend_service_sos_poison_path(service); @@ -1297,7 +1298,8 @@ rend_service_load_keys(rend_service_t *s) char *fname = NULL; char buf[128]; - if (rend_service_check_private_dir(get_options(), s, 1) < 0) + /* Make sure the directory was created in options_act */ + if (BUG(rend_service_check_private_dir(get_options(), s, 0) < 0)) goto err; /* Load key */ From 8bdedab8da25382aa5f33297bedddfa4b8715c43 Mon Sep 17 00:00:00 2001 From: teor Date: Fri, 4 Nov 2016 16:04:05 +1100 Subject: [PATCH 2/5] Refactor duplicate code out of rend_config_services Put that code in rend_service_check_dir_and_add. No behaviour change. This is a defence in depth measure against similar bugs to 20529. --- src/or/rendservice.c | 61 +++++++++++++++++++++++++++++--------------- 1 file changed, 40 insertions(+), 21 deletions(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 15b7feb13..cf71db59b 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -218,6 +218,7 @@ rend_service_free_all(void) /** Validate service and add it to rend_service_list if possible. * Return 0 on success. On failure, free service and return -1. + * Takes ownership of service. */ static int rend_add_service(rend_service_t *service) @@ -444,6 +445,34 @@ rend_service_port_config_free(rend_service_port_config_t *p) tor_free(p); } +/* Check the directory for service, and add the service to the global + * list if validate_only is false. + * If validate_only is true, free the service. + * If service is NULL, ignore it, and return 0. + * Returns 0 on success, and -1 on failure. + * Takes ownership of service. + */ +static int +rend_service_check_dir_and_add(const or_options_t *options, + rend_service_t *service, + int validate_only) +{ + if (service) { /* register the one we just finished parsing */ + if (rend_service_check_private_dir(options, service, !validate_only) + < 0) { + rend_service_free(service); + return -1; + } + + if (validate_only) + rend_service_free(service); + else + rend_add_service(service); + } + + return 0; +} + /** 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 @@ -465,17 +494,12 @@ rend_config_services(const or_options_t *options, int validate_only) for (line = options->RendConfigLines; line; line = line->next) { if (!strcasecmp(line->key, "HiddenServiceDir")) { - if (service) { /* register the one we just finished parsing */ - if (rend_service_check_private_dir(options, service, !validate_only) - < 0) { - rend_service_free(service); + /* register the service we just finished parsing + * this code registers every service except the last one parsed, + * which is registered below the loop */ + if (rend_service_check_dir_and_add(options, service, !validate_only) + < 0) { return -1; - } - - if (validate_only) - rend_service_free(service); - else - rend_add_service(service); } service = tor_malloc_zero(sizeof(rend_service_t)); service->directory = tor_strdup(line->value); @@ -681,17 +705,12 @@ rend_config_services(const or_options_t *options, int validate_only) } } } - if (service) { - if (rend_service_check_private_dir(options, service, !validate_only) < 0) { - rend_service_free(service); - return -1; - } - - if (validate_only) { - rend_service_free(service); - } else { - rend_add_service(service); - } + /* register the final service after we have finished parsing all services + * this code only registers the last service, other services are registered + * within the loop */ + if (rend_service_check_dir_and_add(options, service, !validate_only) + < 0) { + return -1; } /* If this is a reload and there were hidden services configured before, From 1d1d37bbc672f28ac481511df3a5dc4c9c732ed0 Mon Sep 17 00:00:00 2001 From: teor Date: Fri, 4 Nov 2016 16:28:33 +1100 Subject: [PATCH 3/5] Refactor rend_service_check_dir_and_add Make the function flatter, and prepare for #20559. No behaviour change. --- src/or/rendservice.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index cf71db59b..d4d2405cc 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -450,27 +450,34 @@ rend_service_port_config_free(rend_service_port_config_t *p) * If validate_only is true, free the service. * If service is NULL, ignore it, and return 0. * Returns 0 on success, and -1 on failure. - * Takes ownership of service. + * Takes ownership of service, either freeing it, or adding it to the + * global service list. */ static int rend_service_check_dir_and_add(const or_options_t *options, rend_service_t *service, int validate_only) { - if (service) { /* register the one we just finished parsing */ - if (rend_service_check_private_dir(options, service, !validate_only) - < 0) { - rend_service_free(service); - return -1; - } - - if (validate_only) - rend_service_free(service); - else - rend_add_service(service); + if (!service) { + /* It is ok for a service to be NULL, this means there are no services */ + return 0; } - return 0; + if (rend_service_check_private_dir(options, service, !validate_only) + < 0) { + rend_service_free(service); + return -1; + } + + if (validate_only) { + rend_service_free(service); + return 0; + } else { + /* rend_add_service takes ownership, either adding or freeing the service + */ + rend_add_service(service); + return 0; + } } /** Set up rend_service_list, based on the values of HiddenServiceDir and From 91abd60cad2fa3ca9f85fe20956f5f6a336c9c67 Mon Sep 17 00:00:00 2001 From: teor Date: Fri, 18 Nov 2016 14:32:13 +1100 Subject: [PATCH 4/5] Update unit tests for 20484, 20529 Add extra logging and extra validity checks for hidden services. --- src/or/rendservice.c | 73 +++++++++++++++++++++++++++++++++----------- src/or/rendservice.h | 4 +++ src/test/test_hs.c | 28 ++++++++++++++--- 3 files changed, 83 insertions(+), 22 deletions(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index d4d2405cc..91844e854 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -216,16 +216,30 @@ rend_service_free_all(void) rend_service_list = NULL; } -/** Validate service and add it to rend_service_list if possible. +/** Validate service and add it to service_list, or to + * the global rend_service_list if service_list is NULL. * Return 0 on success. On failure, free service and return -1. * Takes ownership of service. */ static int -rend_add_service(rend_service_t *service) +rend_add_service(smartlist_t *service_list, rend_service_t *service) { int i; rend_service_port_config_t *p; + smartlist_t *s_list; + /* If no special service list is provided, then just use the global one. */ + if (!service_list) { + if (BUG(!rend_service_list)) { + /* No global HS list, which is a failure. */ + return -1; + } + + s_list = rend_service_list; + } else { + s_list = service_list; + } + service->intro_nodes = smartlist_new(); service->expiring_nodes = smartlist_new(); @@ -247,7 +261,8 @@ rend_add_service(rend_service_t *service) } if (service->auth_type != REND_NO_AUTH && - smartlist_len(service->clients) == 0) { + (!service->clients || + smartlist_len(service->clients) == 0)) { log_warn(LD_CONFIG, "Hidden service (%s) with client authorization but no " "clients; ignoring.", rend_service_escaped_dir(service)); @@ -255,7 +270,7 @@ rend_add_service(rend_service_t *service) return -1; } - if (!smartlist_len(service->ports)) { + if (!service->ports || !smartlist_len(service->ports)) { log_warn(LD_CONFIG, "Hidden service (%s) with no ports configured; " "ignoring.", rend_service_escaped_dir(service)); @@ -278,8 +293,9 @@ rend_add_service(rend_service_t *service) * lock file. But this is enough to detect a simple mistake that * at least one person has actually made. */ - if (service->directory != NULL) { /* Skip dupe for ephemeral services. */ - SMARTLIST_FOREACH(rend_service_list, rend_service_t*, ptr, + if (service->directory != NULL) { + /* Skip dupe for ephemeral services. */ + SMARTLIST_FOREACH(s_list, rend_service_t*, ptr, dupe = dupe || !strcmp(ptr->directory, service->directory)); if (dupe) { @@ -290,7 +306,7 @@ rend_add_service(rend_service_t *service) return -1; } } - smartlist_add(rend_service_list, service); + smartlist_add(s_list, service); log_debug(LD_REND,"Configuring service with directory \"%s\"", service->directory); for (i = 0; i < smartlist_len(service->ports); ++i) { @@ -445,16 +461,18 @@ rend_service_port_config_free(rend_service_port_config_t *p) tor_free(p); } -/* Check the directory for service, and add the service to the global - * list if validate_only is false. +/* Check the directory for service, and add the service to + * service_list, or to the global list if service_list is NULL. + * Only add the service to the list if validate_only is false. * If validate_only is true, free the service. * If service is NULL, ignore it, and return 0. * Returns 0 on success, and -1 on failure. * Takes ownership of service, either freeing it, or adding it to the * global service list. */ -static int -rend_service_check_dir_and_add(const or_options_t *options, +STATIC int +rend_service_check_dir_and_add(smartlist_t *service_list, + const or_options_t *options, rend_service_t *service, int validate_only) { @@ -463,6 +481,22 @@ rend_service_check_dir_and_add(const or_options_t *options, return 0; } + smartlist_t *s_list = NULL; + /* If no special service list is provided, then just use the global one. */ + if (!service_list) { + if (!rend_service_list) { + /* No global HS list, which is a failure if we plan on adding to it */ + if (BUG(!validate_only)) { + return -1; + } + /* Otherwise, we validate, */ + } + + s_list = rend_service_list; + } else { + s_list = service_list; + } + if (rend_service_check_private_dir(options, service, !validate_only) < 0) { rend_service_free(service); @@ -474,8 +508,12 @@ rend_service_check_dir_and_add(const or_options_t *options, return 0; } else { /* rend_add_service takes ownership, either adding or freeing the service + * s_list can not be NULL here - if both service_list and rend_service_list + * are NULL, and validate_only is false, we exit earlier in the function */ - rend_add_service(service); + tor_assert(s_list); + /* Ignore service failures until 030 */ + rend_add_service(s_list, service); return 0; } } @@ -504,8 +542,8 @@ rend_config_services(const or_options_t *options, int validate_only) /* register the service we just finished parsing * this code registers every service except the last one parsed, * which is registered below the loop */ - if (rend_service_check_dir_and_add(options, service, !validate_only) - < 0) { + if (rend_service_check_dir_and_add(NULL, options, service, + validate_only) < 0) { return -1; } service = tor_malloc_zero(sizeof(rend_service_t)); @@ -715,8 +753,8 @@ rend_config_services(const or_options_t *options, int validate_only) /* register the final service after we have finished parsing all services * this code only registers the last service, other services are registered * within the loop */ - if (rend_service_check_dir_and_add(options, service, !validate_only) - < 0) { + if (rend_service_check_dir_and_add(NULL, options, service, + validate_only) < 0) { return -1; } @@ -874,7 +912,7 @@ rend_service_add_ephemeral(crypto_pk_t *pk, } /* Initialize the service. */ - if (rend_add_service(s)) { + if (rend_add_service(NULL, s)) { return RSAE_INTERNAL; } *service_id_out = tor_strdup(s->service_id); @@ -1310,6 +1348,7 @@ rend_service_check_private_dir(const or_options_t *options, } /* Check/create directory */ if (check_private_dir(s->directory, check_opts, options->User) < 0) { + log_warn(LD_REND, "Checking service directory %s failed.", s->directory); return -1; } return 0; diff --git a/src/or/rendservice.h b/src/or/rendservice.h index 630191e8b..bd3fb1fda 100644 --- a/src/or/rendservice.h +++ b/src/or/rendservice.h @@ -119,6 +119,10 @@ typedef struct rend_service_t { STATIC void rend_service_free(rend_service_t *service); STATIC char *rend_service_sos_poison_path(const rend_service_t *service); +STATIC int rend_service_check_dir_and_add(smartlist_t *service_list, + const or_options_t *options, + rend_service_t *service, + int validate_only); #endif diff --git a/src/test/test_hs.c b/src/test/test_hs.c index b42bc590c..e1f39b1f7 100644 --- a/src/test/test_hs.c +++ b/src/test/test_hs.c @@ -571,8 +571,28 @@ test_single_onion_poisoning(void *arg) service_1->directory = dir1; service_2->directory = dir2; + /* The services own the directory pointers now */ dir1 = dir2 = NULL; - smartlist_add(services, service_1); + /* Add port to service 1 */ + service_1->ports = smartlist_new(); + service_2->ports = smartlist_new(); + char *err_msg = NULL; + rend_service_port_config_t *port1 = rend_service_parse_port_config("80", " ", + &err_msg); + tt_assert(port1); + tt_assert(!err_msg); + smartlist_add(service_1->ports, port1); + + rend_service_port_config_t *port2 = rend_service_parse_port_config("90", " ", + &err_msg); + /* Add port to service 2 */ + tt_assert(port2); + tt_assert(!err_msg); + smartlist_add(service_2->ports, port2); + + /* Add the first service */ + ret = rend_service_check_dir_and_add(services, mock_options, service_1, 0); + tt_assert(ret == 0); /* But don't add the second service yet. */ /* Service directories, but no previous keys, no problem! */ @@ -636,7 +656,7 @@ test_single_onion_poisoning(void *arg) tt_assert(ret == 0); /* Now add the second service: it has no key and no poison file */ - smartlist_add(services, service_2); + ret = rend_service_check_dir_and_add(services, mock_options, service_2, 0); /* A new service, and an existing poisoned service. Not ok. */ mock_options->HiddenServiceSingleHopMode = 0; @@ -698,13 +718,11 @@ test_single_onion_poisoning(void *arg) done: /* The test harness deletes the directories at exit */ + smartlist_free(services); rend_service_free(service_1); rend_service_free(service_2); - smartlist_free(services); UNMOCK(get_options); tor_free(mock_options->DataDirectory); - tor_free(dir1); - tor_free(dir2); } struct testcase_t hs_tests[] = { From f80a43d16f5f7a5e63d0949df74077c875ee5d94 Mon Sep 17 00:00:00 2001 From: teor Date: Fri, 18 Nov 2016 11:46:01 +1100 Subject: [PATCH 5/5] Stop ignoring hidden service key anonymity when first starting tor Instead, refuse to start tor if any hidden service key has been used in a different hidden service anonymity mode. Fixes bug 20638; bugfix on 17178 in 0.2.9.3-alpha; reported by ahf. The original single onion service poisoning code checked poisoning state in options_validate, and poisoned in options_act. This was problematic, because the global array of hidden services had not been populated in options_validate (and there were ordrering issues with hidden service directory creation). This patch fixes this issue in rend_service_check_dir_and_add, which: * creates the directory, or checks permissions on an existing directory, then * checks the poisoning state of the directory, then * poisons the directory. When validating, only the permissions checks and the poisoning state checks are perfomed (the directory is not modified). --- changes/bug20638 | 5 + src/or/config.c | 34 ------ src/or/rendservice.c | 277 ++++++++++++++++++++++++++----------------- src/or/rendservice.h | 12 +- src/test/test_hs.c | 92 ++++++++++---- 5 files changed, 251 insertions(+), 169 deletions(-) create mode 100644 changes/bug20638 diff --git a/changes/bug20638 b/changes/bug20638 new file mode 100644 index 000000000..260d7d0a7 --- /dev/null +++ b/changes/bug20638 @@ -0,0 +1,5 @@ + o Minor bugfixes (hidden services): + - Stop ignoring hidden service key anonymity when first starting tor. + Instead, refuse to start tor if any hidden service key has been used in + a different hidden service anonymity mode. + Fixes bug 20638; bugfix on 17178 in 0.2.9.3-alpha; reported by ahf. diff --git a/src/or/config.c b/src/or/config.c index 9ad5d63f8..8568ea9d6 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -1729,25 +1729,6 @@ options_act(const or_options_t *old_options) monitor_owning_controller_process(options->OwningControllerProcess); - /* We must create new keys after we poison the directories, because our - * poisoning code checks for existing keys, and refuses to modify their - * directories. */ - - /* If we use non-anonymous single onion services, make sure we poison any - new hidden service directories, so that we never accidentally launch the - non-anonymous hidden services thinking they are anonymous. */ - if (running_tor && rend_service_non_anonymous_mode_enabled(options)) { - if (options->RendConfigLines && !num_rend_services()) { - log_warn(LD_BUG,"Error: hidden services configured, but not parsed."); - return -1; - } - if (rend_service_poison_new_single_onion_dirs(NULL) < 0) { - log_warn(LD_GENERAL,"Failed to mark new hidden services as non-anonymous" - "."); - return -1; - } - } - /* reload keys as needed for rendezvous services. */ if (rend_service_load_all_keys(NULL)<0) { log_warn(LD_GENERAL,"Error loading rendezvous service keys"); @@ -2891,21 +2872,6 @@ options_validate_single_onion(or_options_t *options, char **msg) options->UseEntryGuards = 0; } - /* Check if existing hidden service keys were created in a different - * single onion service mode, and refuse to launch if they - * have. We'll poison new keys in options_act() just before we create them. - */ - if (rend_service_list_verify_single_onion_poison(NULL, options) < 0) { - log_warn(LD_GENERAL, "We are configured with " - "HiddenServiceNonAnonymousMode %d, but one or more hidden " - "service keys were created in %s mode. This is not allowed.", - rend_service_non_anonymous_mode_enabled(options) ? 1 : 0, - rend_service_non_anonymous_mode_enabled(options) ? - "an anonymous" : "a non-anonymous" - ); - return -1; - } - return 0; } diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 91844e854..4d04da02a 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -75,6 +75,9 @@ static ssize_t rend_service_parse_intro_for_v3( static int rend_service_check_private_dir(const or_options_t *options, const rend_service_t *s, int create); +static int rend_service_check_private_dir_impl(const or_options_t *options, + const rend_service_t *s, + int create); /** Represents the mapping from a virtual port of a rendezvous service to * a real port on some IP. @@ -481,22 +484,6 @@ rend_service_check_dir_and_add(smartlist_t *service_list, return 0; } - smartlist_t *s_list = NULL; - /* If no special service list is provided, then just use the global one. */ - if (!service_list) { - if (!rend_service_list) { - /* No global HS list, which is a failure if we plan on adding to it */ - if (BUG(!validate_only)) { - return -1; - } - /* Otherwise, we validate, */ - } - - s_list = rend_service_list; - } else { - s_list = service_list; - } - if (rend_service_check_private_dir(options, service, !validate_only) < 0) { rend_service_free(service); @@ -507,11 +494,25 @@ rend_service_check_dir_and_add(smartlist_t *service_list, rend_service_free(service); return 0; } else { - /* rend_add_service takes ownership, either adding or freeing the service - * s_list can not be NULL here - if both service_list and rend_service_list + /* Use service_list for unit tests */ + smartlist_t *s_list = NULL; + /* If no special service list is provided, then just use the global one. */ + if (!service_list) { + if (BUG(!rend_service_list)) { + /* No global HS list, which is a failure, because we plan on adding to + * it */ + return -1; + } + s_list = rend_service_list; + } else { + s_list = service_list; + } + /* s_list can not be NULL here - if both service_list and rend_service_list * are NULL, and validate_only is false, we exit earlier in the function */ - tor_assert(s_list); + if (BUG(!s_list)) { + return -1; + } /* Ignore service failures until 030 */ rend_add_service(s_list, service); return 0; @@ -752,7 +753,7 @@ rend_config_services(const or_options_t *options, int validate_only) } /* register the final service after we have finished parsing all services * this code only registers the last service, other services are registered - * within the loop */ + * within the loop. It is ok for this service to be NULL, it is ignored. */ if (rend_service_check_dir_and_add(NULL, options, service, validate_only) < 0) { return -1; @@ -1061,6 +1062,11 @@ service_is_single_onion_poisoned(const rend_service_t *service) char *poison_fname = NULL; file_status_t fstatus; + /* Passing a NULL service is a bug */ + if (BUG(!service)) { + return 0; + } + if (!service->directory) { return 0; } @@ -1094,58 +1100,64 @@ rend_service_private_key_exists(const rend_service_t *service) return private_key_status == FN_FILE; } -/** Check the single onion service poison state of all existing hidden service - * directories: - * - If each service is poisoned, and we are in Single Onion Mode, +/** Check the single onion service poison state of the directory for s: + * - If the service is poisoned, and we are in Single Onion Mode, * return 0, - * - If each service is not poisoned, and we are not in Single Onion Mode, + * - If the service is not poisoned, and we are not in Single Onion Mode, * return 0, - * - Otherwise, the poison state is invalid, and a service that was created in - * one mode is being used in the other, return -1. - * Hidden service directories without keys are not checked for consistency. - * When their keys are created, they will be poisoned (if needed). - * If a service_list is provided, treat it - * as the list of hidden services (used in unittests). */ -int -rend_service_list_verify_single_onion_poison(const smartlist_t *service_list, - const or_options_t *options) + * - Otherwise, the poison state is invalid: the service was created in one + * mode, and is being used in the other, return -1. + * Hidden service directories without keys are always considered consistent. + * They will be poisoned after their directory is created (if needed). */ +STATIC int +rend_service_verify_single_onion_poison(const rend_service_t* s, + const or_options_t* options) { - const smartlist_t *s_list; - /* If no special service list is provided, then just use the global one. */ - if (!service_list) { - if (!rend_service_list) { /* No global HS list. Nothing to see here. */ - return 0; - } - - s_list = rend_service_list; - } else { - s_list = service_list; + /* Passing a NULL service is a bug */ + if (BUG(!s)) { + return -1; } - int consistent = 1; - SMARTLIST_FOREACH_BEGIN(s_list, const rend_service_t *, s) { - if (service_is_single_onion_poisoned(s) != - rend_service_non_anonymous_mode_enabled(options) && - rend_service_private_key_exists(s)) { - consistent = 0; - } - } SMARTLIST_FOREACH_END(s); + /* Ephemeral services are checked at ADD_ONION time */ + if (!s->directory) { + return 0; + } - return consistent ? 0 : -1; + /* Services without keys are always ok - their keys will only ever be used + * in the current mode */ + if (!rend_service_private_key_exists(s)) { + return 0; + } + + /* The key has been used before in a different mode */ + if (service_is_single_onion_poisoned(s) != + rend_service_non_anonymous_mode_enabled(options)) { + return -1; + } + + /* The key exists and is consistent with the current mode */ + return 0; } -/*** Helper for rend_service_poison_new_single_onion_dirs(). Add a file to - * this hidden service directory that marks it as a single onion service. - * Tor must be in single onion mode before calling this function. +/*** Helper for rend_service_poison_new_single_onion_dir(). Add a file to + * the hidden service directory for s that marks it as a single onion service. + * Tor must be in single onion mode before calling this function, and the + * service directory must already have been created. * Returns 0 when a directory is successfully poisoned, or if it is already * poisoned. Returns -1 on a failure to read the directory or write the poison * file, or if there is an existing private key file in the directory. (The * service should have been poisoned when the key was created.) */ static int -poison_new_single_onion_hidden_service_dir(const rend_service_t *service) +poison_new_single_onion_hidden_service_dir_impl(const rend_service_t *service, + const or_options_t* options) { + /* Passing a NULL service is a bug */ + if (BUG(!service)) { + return -1; + } + /* We must only poison directories if we're in Single Onion mode */ - tor_assert(rend_service_non_anonymous_mode_enabled(get_options())); + tor_assert(rend_service_non_anonymous_mode_enabled(options)); int fd; int retval = -1; @@ -1163,8 +1175,8 @@ poison_new_single_onion_hidden_service_dir(const rend_service_t *service) return -1; } - /* Make sure the directory was created in options_act */ - if (BUG(rend_service_check_private_dir(get_options(), service, 0) < 0)) + /* Make sure the directory was created before calling this function. */ + if (BUG(rend_service_check_private_dir_impl(options, service, 0) < 0)) return -1; poison_fname = rend_service_sos_poison_path(service); @@ -1201,44 +1213,29 @@ poison_new_single_onion_hidden_service_dir(const rend_service_t *service) return retval; } -/** We just got launched in Single Onion Mode. That's a non-anoymous - * mode for hidden services; hence we should mark all new hidden service - * directories appropriately so that they are never launched as - * location-private hidden services again. (New directories don't have private - * key files.) - * If a service_list is provided, treat it as the list of hidden - * services (used in unittests). +/** We just got launched in Single Onion Mode. That's a non-anoymous mode for + * hidden services. If s is new, we should mark its hidden service + * directory appropriately so that it is never launched as a location-private + * hidden service. (New directories don't have private key files.) * Return 0 on success, -1 on fail. */ -int -rend_service_poison_new_single_onion_dirs(const smartlist_t *service_list) +STATIC int +rend_service_poison_new_single_onion_dir(const rend_service_t *s, + const or_options_t* options) { - /* We must only poison directories if we're in Single Onion mode */ - tor_assert(rend_service_non_anonymous_mode_enabled(get_options())); - - const smartlist_t *s_list; - /* If no special service list is provided, then just use the global one. */ - if (!service_list) { - if (!rend_service_list) { /* No global HS list. Nothing to see here. */ - return 0; - } - - s_list = rend_service_list; - } else { - s_list = service_list; + /* Passing a NULL service is a bug */ + if (BUG(!s)) { + return -1; } - SMARTLIST_FOREACH_BEGIN(s_list, const rend_service_t *, s) { - if (!rend_service_private_key_exists(s)) { - if (poison_new_single_onion_hidden_service_dir(s) < 0) { - return -1; - } - } - } SMARTLIST_FOREACH_END(s); + /* We must only poison directories if we're in Single Onion mode */ + tor_assert(rend_service_non_anonymous_mode_enabled(options)); - /* The keys for these services are linked to the server IP address */ - log_notice(LD_REND, "The configured onion service directories have been " - "used in single onion mode. They can not be used for anonymous " - "hidden services."); + if (!rend_service_private_key_exists(s)) { + if (poison_new_single_onion_hidden_service_dir_impl(s, options) + < 0) { + return -1; + } + } return 0; } @@ -1252,10 +1249,12 @@ rend_service_poison_new_single_onion_dirs(const smartlist_t *service_list) int rend_service_load_all_keys(const smartlist_t *service_list) { - const smartlist_t *s_list; + const smartlist_t *s_list = NULL; /* If no special service list is provided, then just use the global one. */ if (!service_list) { - tor_assert(rend_service_list); + if (BUG(!rend_service_list)) { + return -1; + } s_list = rend_service_list; } else { s_list = service_list; @@ -1322,19 +1321,12 @@ rend_service_derive_key_digests(struct rend_service_t *s) return 0; } -/** Make sure that the directory for s is private, using the config in - * options. - * If create is true: - * - if the directory exists, change permissions if needed, - * - if the directory does not exist, create it with the correct permissions. - * If create is false: - * - if the directory exists, check permissions, - * - if the directory does not exist, check if we think we can create it. - * Return 0 on success, -1 on failure. */ +/* Implements the directory check from rend_service_check_private_dir, + * without doing the single onion poison checks. */ static int -rend_service_check_private_dir(const or_options_t *options, - const rend_service_t *s, - int create) +rend_service_check_private_dir_impl(const or_options_t *options, + const rend_service_t *s, + int create) { cpd_check_t check_opts = CPD_NONE; if (create) { @@ -1351,6 +1343,76 @@ rend_service_check_private_dir(const or_options_t *options, log_warn(LD_REND, "Checking service directory %s failed.", s->directory); return -1; } + + return 0; +} + +/** Make sure that the directory for s is private, using the config in + * options. + * If create is true: + * - if the directory exists, change permissions if needed, + * - if the directory does not exist, create it with the correct permissions. + * If create is false: + * - if the directory exists, check permissions, + * - if the directory does not exist, check if we think we can create it. + * Return 0 on success, -1 on failure. */ +static int +rend_service_check_private_dir(const or_options_t *options, + const rend_service_t *s, + int create) +{ + /* Passing a NULL service is a bug */ + if (BUG(!s)) { + return -1; + } + + /* Check/create directory */ + if (rend_service_check_private_dir_impl(options, s, create) < 0) { + return -1; + } + + /* Check if the hidden service key exists, and was created in a different + * single onion service mode, and refuse to launch if it has. + * This is safe to call even when create is false, as it ignores missing + * keys and directories: they are always valid. + */ + if (rend_service_verify_single_onion_poison(s, options) < 0) { + /* We can't use s->service_id here, as the key may not have been loaded */ + log_warn(LD_GENERAL, "We are configured with " + "HiddenServiceNonAnonymousMode %d, but the hidden " + "service key in directory %s was created in %s mode. " + "This is not allowed.", + rend_service_non_anonymous_mode_enabled(options) ? 1 : 0, + rend_service_escaped_dir(s), + rend_service_non_anonymous_mode_enabled(options) ? + "an anonymous" : "a non-anonymous" + ); + return -1; + } + + /* Poison new single onion directories immediately after they are created, + * so that we never accidentally launch non-anonymous hidden services + * thinking they are anonymous. Any keys created later will end up with the + * correct poisoning state. + */ + if (create && rend_service_non_anonymous_mode_enabled(options)) { + static int logged_warning = 0; + + if (rend_service_poison_new_single_onion_dir(s, options) < 0) { + log_warn(LD_GENERAL,"Failed to mark new hidden services as non-anonymous" + "."); + return -1; + } + + if (!logged_warning) { + /* The keys for these services are linked to the server IP address */ + log_notice(LD_REND, "The configured onion service directories have been " + "used in single onion mode. They can not be used for " + "anonymous hidden services."); + logged_warning = 1; + } + } + return 0; } @@ -1363,7 +1425,8 @@ rend_service_load_keys(rend_service_t *s) char *fname = NULL; char buf[128]; - /* Make sure the directory was created in options_act */ + /* Make sure the directory was created and single onion poisoning was + * checked before calling this function */ if (BUG(rend_service_check_private_dir(get_options(), s, 0) < 0)) goto err; diff --git a/src/or/rendservice.h b/src/or/rendservice.h index bd3fb1fda..3b185672f 100644 --- a/src/or/rendservice.h +++ b/src/or/rendservice.h @@ -123,7 +123,12 @@ STATIC int rend_service_check_dir_and_add(smartlist_t *service_list, const or_options_t *options, rend_service_t *service, int validate_only); - +STATIC int rend_service_verify_single_onion_poison( + const rend_service_t *s, + const or_options_t *options); +STATIC int rend_service_poison_new_single_onion_dir( + const rend_service_t *s, + const or_options_t* options); #endif int num_rend_services(void); @@ -169,11 +174,6 @@ void rend_service_port_config_free(rend_service_port_config_t *p); void rend_authorized_client_free(rend_authorized_client_t *client); -int rend_service_list_verify_single_onion_poison( - const smartlist_t *service_list, - const or_options_t *options); -int rend_service_poison_new_single_onion_dirs(const smartlist_t *service_list); - /** Return value from rend_service_add_ephemeral. */ typedef enum { RSAE_BADAUTH = -5, /**< Invalid auth_type/auth_clients */ diff --git a/src/test/test_hs.c b/src/test/test_hs.c index e1f39b1f7..fc8ce9785 100644 --- a/src/test/test_hs.c +++ b/src/test/test_hs.c @@ -542,16 +542,16 @@ test_single_onion_poisoning(void *arg) char *dir2 = tor_strdup(get_fname_rnd("test_hs_dir2")); smartlist_t *services = smartlist_new(); - /* No services, no problem! */ + /* No services, no service to verify, no problem! */ mock_options->HiddenServiceSingleHopMode = 0; mock_options->HiddenServiceNonAnonymousMode = 0; - ret = rend_service_list_verify_single_onion_poison(services, mock_options); + ret = rend_config_services(mock_options, 1); tt_assert(ret == 0); /* Either way, no problem. */ mock_options->HiddenServiceSingleHopMode = 1; mock_options->HiddenServiceNonAnonymousMode = 1; - ret = rend_service_list_verify_single_onion_poison(services, mock_options); + ret = rend_config_services(mock_options, 1); tt_assert(ret == 0); /* Create the data directory, and, if the correct bit in arg is set, @@ -590,6 +590,22 @@ test_single_onion_poisoning(void *arg) tt_assert(!err_msg); smartlist_add(service_2->ports, port2); + /* No services, a service to verify, no problem! */ + mock_options->HiddenServiceSingleHopMode = 0; + mock_options->HiddenServiceNonAnonymousMode = 0; + ret = rend_service_verify_single_onion_poison(service_1, mock_options); + tt_assert(ret == 0); + ret = rend_service_verify_single_onion_poison(service_2, mock_options); + tt_assert(ret == 0); + + /* Either way, no problem. */ + mock_options->HiddenServiceSingleHopMode = 1; + mock_options->HiddenServiceNonAnonymousMode = 1; + ret = rend_service_verify_single_onion_poison(service_1, mock_options); + tt_assert(ret == 0); + ret = rend_service_verify_single_onion_poison(service_2, mock_options); + tt_assert(ret == 0); + /* Add the first service */ ret = rend_service_check_dir_and_add(services, mock_options, service_1, 0); tt_assert(ret == 0); @@ -598,35 +614,43 @@ test_single_onion_poisoning(void *arg) /* Service directories, but no previous keys, no problem! */ mock_options->HiddenServiceSingleHopMode = 0; mock_options->HiddenServiceNonAnonymousMode = 0; - ret = rend_service_list_verify_single_onion_poison(services, mock_options); + ret = rend_service_verify_single_onion_poison(service_1, mock_options); + tt_assert(ret == 0); + ret = rend_service_verify_single_onion_poison(service_2, mock_options); tt_assert(ret == 0); /* Either way, no problem. */ mock_options->HiddenServiceSingleHopMode = 1; mock_options->HiddenServiceNonAnonymousMode = 1; - ret = rend_service_list_verify_single_onion_poison(services, mock_options); + ret = rend_service_verify_single_onion_poison(service_1, mock_options); + tt_assert(ret == 0); + ret = rend_service_verify_single_onion_poison(service_2, mock_options); tt_assert(ret == 0); /* Poison! Poison! Poison! * This can only be done in HiddenServiceSingleHopMode. */ mock_options->HiddenServiceSingleHopMode = 1; mock_options->HiddenServiceNonAnonymousMode = 1; - ret = rend_service_poison_new_single_onion_dirs(services); + ret = rend_service_poison_new_single_onion_dir(service_1, mock_options); tt_assert(ret == 0); /* Poisoning twice is a no-op. */ - ret = rend_service_poison_new_single_onion_dirs(services); + ret = rend_service_poison_new_single_onion_dir(service_1, mock_options); tt_assert(ret == 0); /* Poisoned service directories, but no previous keys, no problem! */ mock_options->HiddenServiceSingleHopMode = 0; mock_options->HiddenServiceNonAnonymousMode = 0; - ret = rend_service_list_verify_single_onion_poison(services, mock_options); + ret = rend_service_verify_single_onion_poison(service_1, mock_options); + tt_assert(ret == 0); + ret = rend_service_verify_single_onion_poison(service_2, mock_options); tt_assert(ret == 0); /* Either way, no problem. */ mock_options->HiddenServiceSingleHopMode = 1; mock_options->HiddenServiceNonAnonymousMode = 1; - ret = rend_service_list_verify_single_onion_poison(services, mock_options); + ret = rend_service_verify_single_onion_poison(service_1, mock_options); + tt_assert(ret == 0); + ret = rend_service_verify_single_onion_poison(service_2, mock_options); tt_assert(ret == 0); /* Now add some keys, and we'll have a problem. */ @@ -636,23 +660,29 @@ test_single_onion_poisoning(void *arg) /* Poisoned service directories with previous keys are not allowed. */ mock_options->HiddenServiceSingleHopMode = 0; mock_options->HiddenServiceNonAnonymousMode = 0; - ret = rend_service_list_verify_single_onion_poison(services, mock_options); + ret = rend_service_verify_single_onion_poison(service_1, mock_options); tt_assert(ret < 0); + ret = rend_service_verify_single_onion_poison(service_2, mock_options); + tt_assert(ret == 0); /* But they are allowed if we're in non-anonymous mode. */ mock_options->HiddenServiceSingleHopMode = 1; mock_options->HiddenServiceNonAnonymousMode = 1; - ret = rend_service_list_verify_single_onion_poison(services, mock_options); + ret = rend_service_verify_single_onion_poison(service_1, mock_options); + tt_assert(ret == 0); + ret = rend_service_verify_single_onion_poison(service_2, mock_options); tt_assert(ret == 0); /* Re-poisoning directories with existing keys is a no-op, because * directories with existing keys are ignored. */ mock_options->HiddenServiceSingleHopMode = 1; mock_options->HiddenServiceNonAnonymousMode = 1; - ret = rend_service_poison_new_single_onion_dirs(services); + ret = rend_service_poison_new_single_onion_dir(service_1, mock_options); tt_assert(ret == 0); /* And it keeps the poison. */ - ret = rend_service_list_verify_single_onion_poison(services, mock_options); + ret = rend_service_verify_single_onion_poison(service_1, mock_options); + tt_assert(ret == 0); + ret = rend_service_verify_single_onion_poison(service_2, mock_options); tt_assert(ret == 0); /* Now add the second service: it has no key and no poison file */ @@ -661,13 +691,17 @@ test_single_onion_poisoning(void *arg) /* A new service, and an existing poisoned service. Not ok. */ mock_options->HiddenServiceSingleHopMode = 0; mock_options->HiddenServiceNonAnonymousMode = 0; - ret = rend_service_list_verify_single_onion_poison(services, mock_options); + ret = rend_service_verify_single_onion_poison(service_1, mock_options); tt_assert(ret < 0); + ret = rend_service_verify_single_onion_poison(service_2, mock_options); + tt_assert(ret == 0); /* But ok to add in non-anonymous mode. */ mock_options->HiddenServiceSingleHopMode = 1; mock_options->HiddenServiceNonAnonymousMode = 1; - ret = rend_service_list_verify_single_onion_poison(services, mock_options); + ret = rend_service_verify_single_onion_poison(service_1, mock_options); + tt_assert(ret == 0); + ret = rend_service_verify_single_onion_poison(service_2, mock_options); tt_assert(ret == 0); /* Now remove the poisoning from the first service, and we have the opposite @@ -681,40 +715,54 @@ test_single_onion_poisoning(void *arg) * directories. */ mock_options->HiddenServiceSingleHopMode = 0; mock_options->HiddenServiceNonAnonymousMode = 0; - ret = rend_service_list_verify_single_onion_poison(services, mock_options); + ret = rend_service_verify_single_onion_poison(service_1, mock_options); + tt_assert(ret == 0); + ret = rend_service_verify_single_onion_poison(service_2, mock_options); tt_assert(ret == 0); /* But the existing unpoisoned key is not ok in non-anonymous mode, even if * there is an empty service. */ mock_options->HiddenServiceSingleHopMode = 1; mock_options->HiddenServiceNonAnonymousMode = 1; - ret = rend_service_list_verify_single_onion_poison(services, mock_options); + ret = rend_service_verify_single_onion_poison(service_1, mock_options); tt_assert(ret < 0); + ret = rend_service_verify_single_onion_poison(service_2, mock_options); + tt_assert(ret == 0); /* Poisoning directories with existing keys is a no-op, because directories * with existing keys are ignored. But the new directory should poison. */ mock_options->HiddenServiceSingleHopMode = 1; mock_options->HiddenServiceNonAnonymousMode = 1; - ret = rend_service_poison_new_single_onion_dirs(services); + ret = rend_service_poison_new_single_onion_dir(service_1, mock_options); + tt_assert(ret == 0); + ret = rend_service_poison_new_single_onion_dir(service_2, mock_options); tt_assert(ret == 0); /* And the old directory remains unpoisoned. */ - ret = rend_service_list_verify_single_onion_poison(services, mock_options); + ret = rend_service_verify_single_onion_poison(service_1, mock_options); tt_assert(ret < 0); + ret = rend_service_verify_single_onion_poison(service_2, mock_options); + tt_assert(ret == 0); /* And the new directory should be ignored, because it has no key. */ mock_options->HiddenServiceSingleHopMode = 0; mock_options->HiddenServiceNonAnonymousMode = 0; - ret = rend_service_list_verify_single_onion_poison(services, mock_options); + ret = rend_service_verify_single_onion_poison(service_1, mock_options); + tt_assert(ret == 0); + ret = rend_service_verify_single_onion_poison(service_2, mock_options); tt_assert(ret == 0); /* Re-poisoning directories without existing keys is a no-op. */ mock_options->HiddenServiceSingleHopMode = 1; mock_options->HiddenServiceNonAnonymousMode = 1; - ret = rend_service_poison_new_single_onion_dirs(services); + ret = rend_service_poison_new_single_onion_dir(service_1, mock_options); + tt_assert(ret == 0); + ret = rend_service_poison_new_single_onion_dir(service_2, mock_options); tt_assert(ret == 0); /* And the old directory remains unpoisoned. */ - ret = rend_service_list_verify_single_onion_poison(services, mock_options); + ret = rend_service_verify_single_onion_poison(service_1, mock_options); tt_assert(ret < 0); + ret = rend_service_verify_single_onion_poison(service_2, mock_options); + tt_assert(ret == 0); done: /* The test harness deletes the directories at exit */