From 419c0c07881c71050546c1049173a7eadf936799 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 22 Dec 2016 16:40:21 -0500 Subject: [PATCH 1/4] hs: Move service check private dir to hs_common.c Another building blocks for prop224 service work. This also makes the function takes specific argument instead of the or_option_t object. Signed-off-by: David Goulet --- src/or/hs_common.c | 34 ++++++++++++++++++++++++++++++++++ src/or/hs_common.h | 4 ++++ src/or/rendservice.c | 35 ++++------------------------------- 3 files changed, 42 insertions(+), 31 deletions(-) diff --git a/src/or/hs_common.c b/src/or/hs_common.c index 7e0b6ca1bc..4af3081502 100644 --- a/src/or/hs_common.c +++ b/src/or/hs_common.c @@ -16,6 +16,40 @@ #include "hs_common.h" #include "rendcommon.h" +/* Make sure that the directory for service is private, using the config + * username. + * 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. */ +int +hs_check_service_private_dir(const char *username, const char *path, + unsigned int dir_group_readable, + unsigned int create) +{ + cpd_check_t check_opts = CPD_NONE; + + tor_assert(path); + + if (create) { + check_opts |= CPD_CREATE; + } else { + check_opts |= CPD_CHECK_MODE_ONLY; + check_opts |= CPD_CHECK; + } + if (dir_group_readable) { + check_opts |= CPD_GROUP_READ; + } + /* Check/create directory */ + if (check_private_dir(path, check_opts, username) < 0) { + return -1; + } + return 0; +} + /* Create a new rend_data_t for a specific given version. * Return a pointer to the newly allocated data structure. */ static rend_data_t * diff --git a/src/or/hs_common.h b/src/or/hs_common.h index 7ac2a15ea1..890797c565 100644 --- a/src/or/hs_common.h +++ b/src/or/hs_common.h @@ -23,6 +23,10 @@ /* String prefix for the signature of ESTABLISH_INTRO */ #define ESTABLISH_INTRO_SIG_PREFIX "Tor establish-intro cell v1" +int hs_check_service_private_dir(const char *username, const char *path, + unsigned int dir_group_readable, + unsigned int create); + void rend_data_free(rend_data_t *data); rend_data_t *rend_data_dup(const rend_data_t *data); rend_data_t *rend_data_client_create(const char *onion_address, diff --git a/src/or/rendservice.c b/src/or/rendservice.c index ac231c2d1f..6b40ed980a 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -76,9 +76,6 @@ 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); static const smartlist_t* rend_get_service_list( const smartlist_t* substitute_service_list); static smartlist_t* rend_get_service_list_mutable( @@ -1294,7 +1291,8 @@ poison_new_single_onion_hidden_service_dir_impl(const rend_service_t *service, } /* Make sure the directory was created before calling this function. */ - if (BUG(rend_service_check_private_dir_impl(options, service, 0) < 0)) + if (BUG(hs_check_service_private_dir(options->User, service->directory, + service->dir_group_readable, 0) < 0)) return -1; poison_fname = rend_service_sos_poison_path(service); @@ -1444,32 +1442,6 @@ rend_service_derive_key_digests(struct rend_service_t *s) return 0; } -/* Implements the directory check from rend_service_check_private_dir, - * without doing the single onion poison checks. */ -static int -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) { - check_opts |= CPD_CREATE; - } else { - check_opts |= CPD_CHECK_MODE_ONLY; - check_opts |= CPD_CHECK; - } - if (s->dir_group_readable) { - check_opts |= CPD_GROUP_READ; - } - /* 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; -} - /** Make sure that the directory for s is private, using the config in * options. * If create is true: @@ -1490,7 +1462,8 @@ rend_service_check_private_dir(const or_options_t *options, } /* Check/create directory */ - if (rend_service_check_private_dir_impl(options, s, create) < 0) { + if (hs_check_service_private_dir(options->User, s->directory, + s->dir_group_readable, create) < 0) { return -1; } From c71670262506cfe75bb439928ce2da2b1ebd30c4 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 22 Dec 2016 16:59:18 -0500 Subject: [PATCH 2/4] hs: Remove redundant define of ed25519 auth key type Signed-off-by: David Goulet --- src/or/hs_common.h | 3 --- src/or/hs_intropoint.c | 4 ++-- src/or/hs_service.c | 4 +++- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/or/hs_common.h b/src/or/hs_common.h index 890797c565..400345c01f 100644 --- a/src/or/hs_common.h +++ b/src/or/hs_common.h @@ -17,9 +17,6 @@ /* Version 3 of the protocol (prop224). */ #define HS_VERSION_THREE 3 -/* Denotes ed25519 authentication key on ESTABLISH_INTRO cell. */ -#define AUTH_KEY_ED25519 0x02 - /* String prefix for the signature of ESTABLISH_INTRO */ #define ESTABLISH_INTRO_SIG_PREFIX "Tor establish-intro cell v1" diff --git a/src/or/hs_intropoint.c b/src/or/hs_intropoint.c index a7282aba8f..168a89bc96 100644 --- a/src/or/hs_intropoint.c +++ b/src/or/hs_intropoint.c @@ -73,9 +73,9 @@ verify_establish_intro_cell(const hs_cell_establish_intro_t *cell, size_t circuit_key_material_len) { /* We only reach this function if the first byte of the cell is 0x02 which - * means that auth_key_type is AUTH_KEY_ED25519, hence this check should + * means that auth_key_type is of ed25519 type, hence this check should * always pass. See hs_intro_received_establish_intro(). */ - if (BUG(cell->auth_key_type != AUTH_KEY_ED25519)) { + if (BUG(cell->auth_key_type != HS_INTRO_AUTH_KEY_TYPE_ED25519)) { return -1; } diff --git a/src/or/hs_service.c b/src/or/hs_service.c index 52de2bfa9d..5d69a1dfff 100644 --- a/src/or/hs_service.c +++ b/src/or/hs_service.c @@ -12,6 +12,7 @@ #include "circuitlist.h" #include "circpathbias.h" +#include "hs_intropoint.h" #include "hs_service.h" #include "hs_common.h" @@ -75,7 +76,8 @@ generate_establish_intro_cell(const uint8_t *circuit_key_material, cell = hs_cell_establish_intro_new(); /* Set AUTH_KEY_TYPE: 2 means ed25519 */ - hs_cell_establish_intro_set_auth_key_type(cell, AUTH_KEY_ED25519); + hs_cell_establish_intro_set_auth_key_type(cell, + HS_INTRO_AUTH_KEY_TYPE_ED25519); /* Set AUTH_KEY_LEN field */ /* Must also set byte-length of AUTH_KEY to match */ From e7b7e99cc72e1db14bad471fef69729bdc5eea94 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 22 Dec 2016 17:01:07 -0500 Subject: [PATCH 3/4] hs: Move common defines to hs_common.h Some of those defines will be used by the v3 HS protocol so move them to a common header out of rendservice.c. This is also ground work for prop224 service implementation. Signed-off-by: David Goulet --- src/or/hs_common.h | 20 ++++++++++++++++++++ src/or/rendservice.c | 17 ----------------- 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/src/or/hs_common.h b/src/or/hs_common.h index 400345c01f..d7d4228da2 100644 --- a/src/or/hs_common.h +++ b/src/or/hs_common.h @@ -17,6 +17,26 @@ /* Version 3 of the protocol (prop224). */ #define HS_VERSION_THREE 3 +/** Try to maintain this many intro points per service by default. */ +#define NUM_INTRO_POINTS_DEFAULT 3 +/** Maximum number of intro points per service. */ +#define NUM_INTRO_POINTS_MAX 10 +/** Number of extra intro points we launch if our set of intro nodes is empty. + * See proposal 155, section 4. */ +#define NUM_INTRO_POINTS_EXTRA 2 + +/** If we can't build our intro circuits, don't retry for this long. */ +#define INTRO_CIRC_RETRY_PERIOD (60*5) +/** Don't try to build more than this many circuits before giving up for a + * while.*/ +#define MAX_INTRO_CIRCS_PER_PERIOD 10 +/** How many times will a hidden service operator attempt to connect to a + * requested rendezvous point before giving up? */ +#define MAX_REND_FAILURES 1 +/** How many seconds should we spend trying to connect to a requested + * rendezvous point before giving up? */ +#define MAX_REND_TIMEOUT 30 + /* String prefix for the signature of ESTABLISH_INTRO */ #define ESTABLISH_INTRO_SIG_PREFIX "Tor establish-intro cell v1" diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 6b40ed980a..9d4dc5bbdc 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -98,23 +98,6 @@ struct rend_service_port_config_s { char unix_addr[FLEXIBLE_ARRAY_MEMBER]; }; -/** Try to maintain this many intro points per service by default. */ -#define NUM_INTRO_POINTS_DEFAULT 3 -/** Maximum number of intro points per service. */ -#define NUM_INTRO_POINTS_MAX 10 -/** Number of extra intro points we launch if our set of intro nodes is - * empty. See proposal 155, section 4. */ -#define NUM_INTRO_POINTS_EXTRA 2 - -/** If we can't build our intro circuits, don't retry for this long. */ -#define INTRO_CIRC_RETRY_PERIOD (60*5) -/** How many times will a hidden service operator attempt to connect to - * a requested rendezvous point before giving up? */ -#define MAX_REND_FAILURES 1 -/** How many seconds should we spend trying to connect to a requested - * rendezvous point before giving up? */ -#define MAX_REND_TIMEOUT 30 - /* Hidden service directory file names: * new file names should be added to rend_service_add_filenames_to_list() * for sandboxing purposes. */ From 0565f5a3bb19cf1027de2093ba2fbaf7b937e2fb Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 12 Jan 2017 10:46:15 -0500 Subject: [PATCH 4/4] hs: Make the service list pruning function public The reason for making the temporary list public is to keep it encapsulated in the rendservice subsystem so the prop224 code does not have direct access to it and can only affect it through the rendservice pruning function. It also has been modified to not take list as arguments but rather use the global lists (main and temporary ones) because prop224 code will call it to actually prune the rendservice's lists. The function does the needed rotation of pointers between those lists and then prune if needed. In order to make the unit test work and not completely horrible, there is a "impl_" version of the function that doesn't free memory, it simply moves pointers around. It is directly used in the unit test and two setter functions for those lists' pointer have been added only for unit test. Signed-off-by: David Goulet --- src/or/rendservice.c | 122 +++++++++++++++++++++++++++++-------------- src/or/rendservice.h | 12 +++-- src/test/test_hs.c | 20 +++++-- 3 files changed, 107 insertions(+), 47 deletions(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 9d4dc5bbdc..3c4b6775c0 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -106,9 +106,12 @@ static const char *hostname_fname = "hostname"; static const char *client_keys_fname = "client_keys"; static const char *sos_poison_fname = "onion_service_non_anonymous"; -/** A list of rend_service_t's for services run on this OP. - */ +/** A list of rend_service_t's for services run on this OP. */ static smartlist_t *rend_service_list = NULL; +/** A list of rend_service_t's for services run on this OP which is used as a + * staging area before they are put in the main list in order to prune dying + * service on config reload. */ +static smartlist_t *rend_service_staging_list = NULL; /* Like rend_get_service_list_mutable, but returns a read-only list. */ static const smartlist_t* @@ -520,18 +523,34 @@ 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. */ +/* Helper: Actual implementation of the pruning on reload which we've + * decoupled in order to make the unit test workeable without ugly hacks. + * Furthermore, this function does NOT free any memory but will nullify the + * temporary list pointer whatever happens. */ STATIC void -prune_services_on_reload(smartlist_t *old_service_list, - smartlist_t *new_service_list) +rend_service_prune_list_impl_(void) { origin_circuit_t *ocirc = NULL; - smartlist_t *surviving_services = NULL; + smartlist_t *surviving_services, *old_service_list, *new_service_list; - tor_assert(old_service_list); - tor_assert(new_service_list); + /* When pruning our current service list, we must have a staging list that + * contains what we want to check else it's a code flow error. */ + tor_assert(rend_service_staging_list); + + /* We are about to prune the current list of its dead service so set the + * semantic for that list to be the "old" one. */ + old_service_list = rend_service_list; + /* The staging list is now the "new" list so set this semantic. */ + new_service_list = rend_service_staging_list; + /* After this, whatever happens, we'll use our new list. */ + rend_service_list = new_service_list; + /* Finally, nullify the staging list pointer as we don't need it anymore + * and it needs to be NULL before the next reload. */ + rend_service_staging_list = NULL; + /* Nothing to prune if we have no service list so stop right away. */ + if (!old_service_list) { + return; + } /* This contains all _existing_ services that survives the relaod that is * that haven't been removed from the configuration. The difference between @@ -609,6 +628,27 @@ prune_services_on_reload(smartlist_t *old_service_list, smartlist_free(surviving_services); } +/* Try to prune our main service list using the temporary one that we just + * loaded and parsed successfully. The pruning process decides which onion + * services to keep and which to discard after a reload. */ +void +rend_service_prune_list(void) +{ + smartlist_t *old_service_list = rend_service_list; + /* Don't try to prune anything if we have no staging list. */ + if (!rend_service_staging_list) { + return; + } + rend_service_prune_list_impl_(); + if (old_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); + } +} + /** 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 @@ -620,22 +660,22 @@ rend_config_services(const or_options_t *options, int validate_only) config_line_t *line; rend_service_t *service = NULL; rend_service_port_config_t *portcfg; - smartlist_t *old_service_list = NULL; - smartlist_t *temp_service_list = NULL; int ok = 0; int rv = -1; - /* Use a temporary service list, so that we can check the new services' - * consistency with each other */ - temp_service_list = smartlist_new(); + /* Use the staging service list so that we can check then do the pruning + * process using the main list at the end. */ + if (rend_service_staging_list == NULL) { + rend_service_staging_list = smartlist_new(); + } for (line = options->RendConfigLines; line; line = line->next) { if (!strcasecmp(line->key, "HiddenServiceDir")) { /* 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(temp_service_list, options, service, - validate_only) < 0) { + if (rend_service_check_dir_and_add(rend_service_staging_list, options, + service, validate_only) < 0) { service = NULL; goto free_and_return; } @@ -835,8 +875,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. It is ok for this service to be NULL, it is ignored. */ - if (rend_service_check_dir_and_add(temp_service_list, options, service, - validate_only) < 0) { + if (rend_service_check_dir_and_add(rend_service_staging_list, options, + service, validate_only) < 0) { service = NULL; goto free_and_return; } @@ -848,31 +888,19 @@ rend_config_services(const or_options_t *options, int validate_only) goto free_and_return; } - /* Otherwise, use the newly added services as the new service list - * Since we have now replaced the global service list, from this point on we - * must succeed, or die trying. */ - old_service_list = rend_service_list; - rend_service_list = temp_service_list; - temp_service_list = NULL; - - /* 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. */ - if (old_service_list && !validate_only) { - 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); - } + /* This could be a reload of configuration so try to prune the main list + * using the staging one. And we know we are not in validate mode here. + * After this, the main and staging list will point to the right place and + * be in a quiescent usable state. */ + rend_service_prune_list(); return 0; free_and_return: rend_service_free(service); - SMARTLIST_FOREACH(temp_service_list, rend_service_t *, ptr, + SMARTLIST_FOREACH(rend_service_staging_list, rend_service_t *, ptr, rend_service_free(ptr)); - smartlist_free(temp_service_list); + smartlist_free(rend_service_staging_list); + rend_service_staging_list = NULL; return rv; } @@ -4551,3 +4579,19 @@ rend_service_non_anonymous_mode_enabled(const or_options_t *options) return options->HiddenServiceNonAnonymousMode ? 1 : 0; } +#ifdef TOR_UNIT_TESTS + +STATIC void +set_rend_service_list(smartlist_t *new_list) +{ + rend_service_list = new_list; +} + +STATIC void +set_rend_rend_service_staging_list(smartlist_t *new_list) +{ + rend_service_staging_list = new_list; +} + +#endif /* TOR_UNIT_TESTS */ + diff --git a/src/or/rendservice.h b/src/or/rendservice.h index 6f45f473a5..1583a6010b 100644 --- a/src/or/rendservice.h +++ b/src/or/rendservice.h @@ -133,13 +133,19 @@ STATIC ssize_t encode_establish_intro_cell_legacy(char *cell_body_out, size_t cell_body_out_len, 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); +#ifdef TOR_UNIT_TESTS -#endif +STATIC void set_rend_service_list(smartlist_t *new_list); +STATIC void set_rend_rend_service_staging_list(smartlist_t *new_list); +STATIC void rend_service_prune_list_impl_(void); + +#endif /* TOR_UNIT_TESTS */ + +#endif /* RENDSERVICE_PRIVATE */ int num_rend_services(void); int rend_config_services(const or_options_t *options, int validate_only); +void rend_service_prune_list(void); int rend_service_load_all_keys(const smartlist_t *service_list); void rend_services_add_filenames_to_lists(smartlist_t *open_lst, smartlist_t *stat_lst); diff --git a/src/test/test_hs.c b/src/test/test_hs.c index 3aa2eea7cb..c3457c43da 100644 --- a/src/test/test_hs.c +++ b/src/test/test_hs.c @@ -821,7 +821,9 @@ test_prune_services_on_reload(void *arg) smartlist_add(old, e1); /* Only put the non ephemeral in the new list. */ smartlist_add(new, s1); - prune_services_on_reload(old, new); + set_rend_service_list(old); + set_rend_rend_service_staging_list(new); + rend_service_prune_list_impl_(); /* 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); @@ -840,7 +842,9 @@ test_prune_services_on_reload(void *arg) * one. */ smartlist_add(old, s1); smartlist_add(old, e1); - prune_services_on_reload(old, new); + set_rend_service_list(old); + set_rend_rend_service_staging_list(new); + rend_service_prune_list_impl_(); 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); @@ -855,7 +859,9 @@ test_prune_services_on_reload(void *arg) * list being completely different. */ smartlist_add(new, s1); smartlist_add(new, e1); - prune_services_on_reload(old, new); + set_rend_service_list(old); + set_rend_rend_service_staging_list(new); + rend_service_prune_list_impl_(); 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); @@ -871,7 +877,9 @@ test_prune_services_on_reload(void *arg) /* Setup our list. */ smartlist_add(old, s1); smartlist_add(new, s2); - prune_services_on_reload(old, new); + set_rend_service_list(old); + set_rend_rend_service_staging_list(new); + rend_service_prune_list_impl_(); 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); @@ -892,7 +900,9 @@ test_prune_services_on_reload(void *arg) /* Test two ephemeral services. */ smartlist_add(old, e1); smartlist_add(old, e2); - prune_services_on_reload(old, new); + set_rend_service_list(old); + set_rend_rend_service_staging_list(new); + rend_service_prune_list_impl_(); /* 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);