From a9fb97e91aa6232619d9e0b81c47e4eb3037d0f7 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Fri, 28 Jul 2017 11:47:32 -0400 Subject: [PATCH 1/5] prop224: Don't move intro points but rather descriptors Apart from the fact that a newly allocated service doesn't have descriptors thus the move condition can never be true, the service needs the descriptor signing key to cross-certify the authentication key of each intro point so we need to move the descriptors between services and not only the intro points. Fixes #23056 Signed-off-by: David Goulet --- src/or/hs_service.c | 37 ++++++++++++------------------------- 1 file changed, 12 insertions(+), 25 deletions(-) diff --git a/src/or/hs_service.c b/src/or/hs_service.c index 0d0db1cd6b..f2e07e78c7 100644 --- a/src/or/hs_service.c +++ b/src/or/hs_service.c @@ -709,34 +709,20 @@ close_service_circuits(hs_service_t *service) close_service_rp_circuits(service); } -/* Move introduction points from the src descriptor to the dst descriptor. The - * destination service intropoints are wiped out if any before moving. */ +/* Move descriptor(s) from the src service to the dst service. */ static void -move_descriptor_intro_points(hs_service_descriptor_t *src, - hs_service_descriptor_t *dst) +move_descriptors(hs_service_t *src, hs_service_t *dst) { tor_assert(src); tor_assert(dst); - digest256map_free(dst->intro_points.map, service_intro_point_free_); - dst->intro_points.map = src->intro_points.map; - /* Nullify the source. */ - src->intro_points.map = NULL; -} - -/* Move introduction points from the src service to the dst service. The - * destination service intropoints are wiped out if any before moving. */ -static void -move_intro_points(hs_service_t *src, hs_service_t *dst) -{ - tor_assert(src); - tor_assert(dst); - - if (src->desc_current && dst->desc_current) { - move_descriptor_intro_points(src->desc_current, dst->desc_current); + if (src->desc_current) { + dst->desc_current = src->desc_current; + src->desc_current = NULL; } - if (src->desc_next && dst->desc_next) { - move_descriptor_intro_points(src->desc_next, dst->desc_next); + if (src->desc_next) { + dst->desc_next = src->desc_next; + src->desc_next = NULL; } } @@ -812,13 +798,14 @@ register_all_services(void) * transfer the intro points to it. */ s = find_service(hs_service_map, &snew->keys.identity_pk); if (s) { - /* Pass ownership of intro points from s (the current service) to snew - * (the newly configured one). */ - move_intro_points(s, snew); + /* Pass ownership of the descriptors from s (the current service) to + * snew (the newly configured one). */ + move_descriptors(s, snew); /* Remove the service from the global map because after this, we need to * go over the remaining service in that map that aren't surviving the * reload to close their circuits. */ remove_service(hs_service_map, s); + hs_service_free(s); } /* Great, this service is now ready to be added to our new map. */ if (BUG(register_service(new_service_map, snew) < 0)) { From 17fd2c8a51900bd71eaeff397ec64b6949a51730 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Fri, 25 Aug 2017 16:06:17 +0300 Subject: [PATCH 2/5] prop224: Move function move_descriptors() around. We want to use some static functions so move it below them. --- src/or/hs_service.c | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/src/or/hs_service.c b/src/or/hs_service.c index f2e07e78c7..12410588b5 100644 --- a/src/or/hs_service.c +++ b/src/or/hs_service.c @@ -73,6 +73,7 @@ static const char address_tld[] = "onion"; static smartlist_t *hs_service_staging_list; static void set_descriptor_revision_counter(hs_descriptor_t *hs_desc); +static void move_descriptors(hs_service_t *src, hs_service_t *dst); /* Helper: Function to compare two objects in the service map. Return 1 if the * two service have the same master public identity key. */ @@ -709,23 +710,6 @@ close_service_circuits(hs_service_t *service) close_service_rp_circuits(service); } -/* Move descriptor(s) from the src service to the dst service. */ -static void -move_descriptors(hs_service_t *src, hs_service_t *dst) -{ - tor_assert(src); - tor_assert(dst); - - if (src->desc_current) { - dst->desc_current = src->desc_current; - src->desc_current = NULL; - } - if (src->desc_next) { - dst->desc_next = src->desc_next; - src->desc_next = NULL; - } -} - /* Move every ephemeral services from the src service map to the dst service * map. It is possible that a service can't be register to the dst map which * won't stop the process of moving them all but will trigger a log warn. */ @@ -980,6 +964,23 @@ service_descriptor_new(void) return sdesc; } +/* Move descriptor(s) from the src service to the dst service. */ +static void +move_descriptors(hs_service_t *src, hs_service_t *dst) +{ + tor_assert(src); + tor_assert(dst); + + if (src->desc_current) { + dst->desc_current = src->desc_current; + src->desc_current = NULL; + } + if (src->desc_next) { + dst->desc_next = src->desc_next; + src->desc_next = NULL; + } +} + /* From the given service, remove all expired failing intro points for each * descriptor. */ static void From 409ecbec521ea4b5eef8605f71f4f7b768087486 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Fri, 25 Aug 2017 16:13:19 +0300 Subject: [PATCH 3/5] prop224: Be more careful to not overwrite descriptors in HUP. --- src/or/hs_service.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/or/hs_service.c b/src/or/hs_service.c index 12410588b5..80078e203f 100644 --- a/src/or/hs_service.c +++ b/src/or/hs_service.c @@ -964,7 +964,8 @@ service_descriptor_new(void) return sdesc; } -/* Move descriptor(s) from the src service to the dst service. */ +/* Move descriptor(s) from the src service to the dst service. We do this + * during SIGHUP when we re-create our hidden services. */ static void move_descriptors(hs_service_t *src, hs_service_t *dst) { @@ -972,10 +973,19 @@ move_descriptors(hs_service_t *src, hs_service_t *dst) tor_assert(dst); if (src->desc_current) { + /* Nothing should be there, but clean it up just in case */ + if (BUG(dst->desc_current)) { + service_descriptor_free(dst->desc_current); + } dst->desc_current = src->desc_current; src->desc_current = NULL; } + if (src->desc_next) { + /* Nothing should be there, but clean it up just in case */ + if (BUG(dst->desc_next)) { + service_descriptor_free(dst->desc_next); + } dst->desc_next = src->desc_next; src->desc_next = NULL; } From ea5af8f44202c3272c616d319cdf0fa048eec9da Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Fri, 25 Aug 2017 17:09:17 +0300 Subject: [PATCH 4/5] prop224: When HUPing, move HS state from old to new service. We used to not copy the state which means that after HUP we would forget if we are in overlap mode or not. That caused bugs where the service would enter overlap mode twice, and rotate its descs twice, causing all sorts of bugs. --- src/or/hs_service.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/or/hs_service.c b/src/or/hs_service.c index 80078e203f..4171a8e0ac 100644 --- a/src/or/hs_service.c +++ b/src/or/hs_service.c @@ -750,6 +750,26 @@ service_escaped_dir(const hs_service_t *s) escaped(s->config.directory_path); } +/** Move the hidden service state from src to dst. We do this + * when we receive a SIGHUP: dst is the post-HUP service */ +static void +move_hs_state(hs_service_t *src_service, hs_service_t *dst_service) +{ + tor_assert(src_service); + tor_assert(dst_service); + + hs_service_state_t *src = &src_service->state; + hs_service_state_t *dst = &dst_service->state; + + /* Let's do a shallow copy */ + dst->intro_circ_retry_started_time = src->intro_circ_retry_started_time; + dst->num_intro_circ_launched = src->num_intro_circ_launched; + dst->in_overlap_period = src->in_overlap_period; + dst->replay_cache_rend_cookie = src->replay_cache_rend_cookie; + + src->replay_cache_rend_cookie = NULL; /* steal pointer reference */ +} + /* Register services that are in the staging list. Once this function returns, * the global service map will be set with the right content and all non * surviving services will be cleaned up. */ @@ -785,6 +805,7 @@ register_all_services(void) /* Pass ownership of the descriptors from s (the current service) to * snew (the newly configured one). */ move_descriptors(s, snew); + move_hs_state(s, snew); /* Remove the service from the global map because after this, we need to * go over the remaining service in that map that aren't surviving the * reload to close their circuits. */ From 6d48e756855b79becf275c062b4e2fd48ccee23b Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Fri, 25 Aug 2017 17:16:53 +0300 Subject: [PATCH 5/5] prop224: Better missing hsdir index logs. Seems like hsdir index bugs are around to haunt us. Let's improve the log messages to make debugging easier. --- src/or/hs_common.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/or/hs_common.c b/src/or/hs_common.c index 03dd07f6ca..a5ad269007 100644 --- a/src/or/hs_common.c +++ b/src/or/hs_common.c @@ -1206,12 +1206,16 @@ node_has_hsdir_index(const node_t *node, int is_for_next_period) if (BUG(node->hsdir_index == NULL) || BUG(tor_mem_is_zero((const char*)node->hsdir_index->current, DIGEST256_LEN))) { + log_warn(LD_BUG, "Zero current index (ri: %p, rs: %p, md: %p)", + node->ri, node->rs, node->md); return 0; } if (is_for_next_period && BUG(tor_mem_is_zero((const char*)node->hsdir_index->next, DIGEST256_LEN))) { + log_warn(LD_BUG, "Zero next index (ri: %p, rs: %p, md: %p)", + node->ri, node->rs, node->md); return 0; }