commit 91abd60cad2fa3ca9f85fe20956f5f6a336c9c67 Author: teor teor2345@gmail.com Date: Fri Nov 18 14:32:13 2016 +1100

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 d4d2405..91844e8 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 <b>service</b> and add it to rend_service_list if possible. +/** Validate <b>service</b> and add it to <b>service_list</b>, or to + * the global rend_service_list if <b>service_list</b> is NULL. * Return 0 on success. On failure, free <b>service</b> and return -1. * Takes ownership of <b>service</b>. */ 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 <b>service</b>, and add the service to the global - * list if <b>validate_only</b> is false. +/* Check the directory for <b>service</b>, and add the service to + * <b>service_list</b>, or to the global list if <b>service_list</b> is NULL. + * Only add the service to the list if <b>validate_only</b> is false. * If <b>validate_only</b> is true, free the service. * If <b>service</b> is NULL, ignore it, and return 0. * Returns 0 on success, and -1 on failure. * Takes ownership of <b>service</b>, 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 630191e..bd3fb1f 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 b42bc59..e1f39b1 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[] = {