From ed7c0170c48fe4c0d86734969cf24c94d442cfcb Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 24 Apr 2017 14:39:54 -0400 Subject: [PATCH 1/2] hs: Add rend_validate_service() function This new function validates a service object and is used everytime a service is successfully loaded from the configuration file. It is currently copying the validation that rend_add_service() also does which means both functions validate. It will be decoupled in the next commit. Signed-off-by: David Goulet --- src/or/rendservice.c | 85 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index c19c85f6a3..9012fffdcd 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -245,6 +245,81 @@ rend_service_free_all(void) rend_service_list = NULL; } +/* Validate a service. Use the service_list to make sure there + * is no duplicate entry for the given service object. Return 0 if valid else + * -1 if not.*/ +static int +rend_validate_service(const smartlist_t *service_list, + const rend_service_t *service) +{ + int dupe = 0; + + tor_assert(service_list); + tor_assert(service); + + if (service->max_streams_per_circuit < 0) { + log_warn(LD_CONFIG, "Hidden service (%s) configured with negative max " + "streams per circuit.", + rend_service_escaped_dir(service)); + goto invalid; + } + + if (service->max_streams_close_circuit < 0 || + service->max_streams_close_circuit > 1) { + log_warn(LD_CONFIG, "Hidden service (%s) configured with invalid " + "max streams handling.", + rend_service_escaped_dir(service)); + goto invalid; + } + + if (service->auth_type != REND_NO_AUTH && + (!service->clients || smartlist_len(service->clients) == 0)) { + log_warn(LD_CONFIG, "Hidden service (%s) with client authorization but " + "no clients.", + rend_service_escaped_dir(service)); + goto invalid; + } + + if (!service->ports || !smartlist_len(service->ports)) { + log_warn(LD_CONFIG, "Hidden service (%s) with no ports configured.", + rend_service_escaped_dir(service)); + goto invalid; + } + + /* XXX This duplicate check has two problems: + * + * a) It's O(n^2), but the same comment from the bottom of + * rend_config_services() should apply. + * + * b) We only compare directory paths as strings, so we can't + * detect two distinct paths that specify the same directory + * (which can arise from symlinks, case-insensitivity, bind + * mounts, etc.). + * + * It also can't detect that two separate Tor instances are trying + * to use the same HiddenServiceDir; for that, we would need a + * lock file. But this is enough to detect a simple mistake that + * at least one person has actually made. + */ + if (!rend_service_is_ephemeral(service)) { + /* Skip dupe for ephemeral services. */ + SMARTLIST_FOREACH(service_list, rend_service_t *, ptr, + dupe = dupe || + !strcmp(ptr->directory, service->directory)); + if (dupe) { + log_warn(LD_REND, "Another hidden service is already configured for " + "directory %s.", + rend_service_escaped_dir(service)); + goto invalid; + } + } + + /* Valid. */ + return 0; + invalid: + return -1; +} + /** 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. @@ -671,6 +746,11 @@ 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) { + if (rend_validate_service(rend_service_staging_list, service) < 0) { + goto free_and_return; + } + } /* register the service we just finished parsing * this code registers every service except the last one parsed, * which is registered below the loop */ @@ -872,6 +952,11 @@ rend_config_services(const or_options_t *options, int validate_only) } } } + /* Validate the last service that we just parsed. */ + if (service && + rend_validate_service(rend_service_staging_list, service) < 0) { + goto free_and_return; + } /* 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. It is ok for this service to be NULL, it is ignored. */ From 6f27843d578a830e9dbf44dfe9aa4d84cdcc6d15 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 24 Apr 2017 14:51:34 -0400 Subject: [PATCH 2/2] hs: Refactor rend_add_service() Remove duplicate code that validates a service object which is now in rend_validate_service(). Add some comments on why we nullify a service in the code path of rend_config_services(). Signed-off-by: David Goulet --- src/or/rendservice.c | 142 ++++++++++++------------------------------- 1 file changed, 40 insertions(+), 102 deletions(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 9012fffdcd..53901fca0b 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -320,11 +320,9 @@ rend_validate_service(const smartlist_t *service_list, return -1; } -/** 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. - */ +/** 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(smartlist_t *service_list, rend_service_t *service) { @@ -343,100 +341,35 @@ rend_add_service(smartlist_t *service_list, rend_service_t *service) service->intro_nodes = smartlist_new(); service->expiring_nodes = smartlist_new(); - if (service->max_streams_per_circuit < 0) { - log_warn(LD_CONFIG, "Hidden service (%s) configured with negative max " - "streams per circuit.", - rend_service_escaped_dir(service)); - rend_service_free(service); - return -1; - } - - if (service->max_streams_close_circuit < 0 || - service->max_streams_close_circuit > 1) { - log_warn(LD_CONFIG, "Hidden service (%s) configured with invalid " - "max streams handling.", - rend_service_escaped_dir(service)); - rend_service_free(service); - return -1; - } - - if (service->auth_type != REND_NO_AUTH && - (!service->clients || - smartlist_len(service->clients) == 0)) { - log_warn(LD_CONFIG, "Hidden service (%s) with client authorization but no " - "clients.", - rend_service_escaped_dir(service)); - rend_service_free(service); - return -1; - } - - if (!service->ports || !smartlist_len(service->ports)) { - log_warn(LD_CONFIG, "Hidden service (%s) with no ports configured.", - rend_service_escaped_dir(service)); - rend_service_free(service); - return -1; - } else { - int dupe = 0; - /* XXX This duplicate check has two problems: - * - * a) It's O(n^2), but the same comment from the bottom of - * rend_config_services() should apply. - * - * b) We only compare directory paths as strings, so we can't - * detect two distinct paths that specify the same directory - * (which can arise from symlinks, case-insensitivity, bind - * mounts, etc.). - * - * It also can't detect that two separate Tor instances are trying - * to use the same HiddenServiceDir; for that, we would need a - * lock file. But this is enough to detect a simple mistake that - * at least one person has actually made. - */ - tor_assert(s_list); - if (!rend_service_is_ephemeral(service)) { - /* Skip dupe for ephemeral services. */ - SMARTLIST_FOREACH(s_list, rend_service_t*, ptr, - dupe = dupe || - !strcmp(ptr->directory, service->directory)); - if (dupe) { - log_warn(LD_REND, "Another hidden service is already configured for " - "directory %s.", - rend_service_escaped_dir(service)); - rend_service_free(service); - return -1; - } - } - log_debug(LD_REND,"Configuring service with directory %s", - rend_service_escaped_dir(service)); - for (i = 0; i < smartlist_len(service->ports); ++i) { - p = smartlist_get(service->ports, i); - if (!(p->is_unix_addr)) { - log_debug(LD_REND, - "Service maps port %d to %s", - p->virtual_port, - fmt_addrport(&p->real_addr, p->real_port)); - } else { + log_debug(LD_REND,"Configuring service with directory %s", + rend_service_escaped_dir(service)); + for (i = 0; i < smartlist_len(service->ports); ++i) { + p = smartlist_get(service->ports, i); + if (!(p->is_unix_addr)) { + log_debug(LD_REND, + "Service maps port %d to %s", + p->virtual_port, + fmt_addrport(&p->real_addr, p->real_port)); + } else { #ifdef HAVE_SYS_UN_H - log_debug(LD_REND, - "Service maps port %d to socket at \"%s\"", - p->virtual_port, p->unix_addr); + log_debug(LD_REND, + "Service maps port %d to socket at \"%s\"", + p->virtual_port, p->unix_addr); #else - log_warn(LD_BUG, - "Service maps port %d to an AF_UNIX socket, but we " - "have no AF_UNIX support on this platform. This is " - "probably a bug.", - p->virtual_port); - rend_service_free(service); - return -1; + log_warn(LD_BUG, + "Service maps port %d to an AF_UNIX socket, but we " + "have no AF_UNIX support on this platform. This is " + "probably a bug.", + p->virtual_port); + rend_service_free(service); + return -1; #endif /* defined(HAVE_SYS_UN_H) */ - } } - /* The service passed all the checks */ - tor_assert(s_list); - smartlist_add(s_list, service); - return 0; } - /* NOTREACHED */ + /* The service passed all the checks */ + tor_assert(s_list); + smartlist_add(s_list, service); + return 0; } /** Return a new rend_service_port_config_t with its path set to @@ -747,17 +680,18 @@ 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) { + /* Validate and register the service we just finished parsing this + * code registers every service except the last one parsed, which is + * validated and registered below the loop. */ if (rend_validate_service(rend_service_staging_list, service) < 0) { goto free_and_return; } - } - /* 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(rend_service_staging_list, options, - service, validate_only) < 0) { - service = NULL; - goto free_and_return; + if (rend_service_check_dir_and_add(rend_service_staging_list, options, + service, validate_only) < 0) { + /* The above frees the service on error so nullify the pointer. */ + service = NULL; + goto free_and_return; + } } service = tor_malloc_zero(sizeof(rend_service_t)); service->directory = tor_strdup(line->value); @@ -962,9 +896,13 @@ rend_config_services(const or_options_t *options, int validate_only) * within the loop. It is ok for this service to be NULL, it is ignored. */ if (rend_service_check_dir_and_add(rend_service_staging_list, options, service, validate_only) < 0) { + /* Service object is freed on error so nullify pointer. */ service = NULL; goto free_and_return; } + /* The service is in the staging list so nullify pointer to avoid double + * free of this object in case of error because we lost ownership of it at + * this point. */ service = NULL; /* Free the newly added services if validating */