From cc0342a2ae4264c5b8796e2731a51272342bb8f4 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 1 Feb 2017 11:07:09 -0500 Subject: [PATCH 1/3] hs: Fix possible integer underflow with IP nodes In rend_consider_services_intro_points(), we had a possible interger underflow which could lead to creating a very large number of intro points. We had a safe guard against that *except* if the expiring_nodes list was not empty which is realistic thing. This commit removes the check on the expiring nodes length being zero. It's not because we have an empty list of expiring nodes that we don't want to open new IPs. Prior to this check, we remove invalid IP nodes from the main list of a service so it should be the only thing to look at when deciding if we need to create new IP(s) or not. Partially fixes #21302. Signed-off-by: David Goulet --- src/or/rendservice.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 00f251588c..5514de6f06 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -4072,17 +4072,17 @@ rend_consider_services_intro_points(void) /* Avoid mismatched signed comparaison below. */ intro_nodes_len = (unsigned int) smartlist_len(service->intro_nodes); - /* Quiescent state, no node expiring and we have more or the amount of - * wanted node for this service. Proceed to the next service. Could be - * more because we launch two preemptive circuits if our intro nodes - * list is empty. */ - if (smartlist_len(service->expiring_nodes) == 0 && - intro_nodes_len >= service->n_intro_points_wanted) { + /* Quiescent state, we have more or the equal amount of wanted node for + * this service. Proceed to the next service. We can have more nodes + * because we launch extra preemptive circuits if our intro nodes list was + * originally empty for performance reasons. */ + if (intro_nodes_len >= service->n_intro_points_wanted) { continue; } - /* Number of intro points we want to open which is the wanted amount - * minus the current amount of valid nodes. */ + /* Number of intro points we want to open which is the wanted amount minus + * the current amount of valid nodes. We know that this won't underflow + * because of the check above. */ n_intro_points_to_open = service->n_intro_points_wanted - intro_nodes_len; if (intro_nodes_len == 0) { /* We want to end up with n_intro_points_wanted intro points, but if From 8573d994706c16628bb60e0aabdb9a5190beff13 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 1 Feb 2017 11:22:17 -0500 Subject: [PATCH 2/3] hs: Fix an underflow in rend_service_intro_has_opened() In rend_service_intro_has_opened(), this is subject to a possible underflow because of how the if() casts the results. In the case where the expiring nodes list length is bigger than the number of IP circuits, we end up in the following situation where the result will be cast to an unsigned int. For instance, "5 - 6" is actually a BIG number. Ultimately leading to closing IP circuits in a non stop loop. Partially fixes #21302. Signed-off-by: David Goulet --- src/or/rendservice.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 5514de6f06..0db81aed5f 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -3227,6 +3227,7 @@ rend_service_intro_has_opened(origin_circuit_t *circuit) rend_service_t *service; char buf[RELAY_PAYLOAD_SIZE]; char serviceid[REND_SERVICE_ID_LEN_BASE32+1]; + unsigned int expiring_nodes_len, num_ip_circuits, valid_ip_circuits = 0; int reason = END_CIRC_REASON_TORPROTOCOL; const char *rend_pk_digest; @@ -3248,13 +3249,22 @@ rend_service_intro_has_opened(origin_circuit_t *circuit) goto err; } + /* Take the current amount of expiring nodes and the current amount of IP + * circuits and compute how many valid IP circuits we have. */ + expiring_nodes_len = (unsigned int) smartlist_len(service->expiring_nodes); + num_ip_circuits = count_intro_point_circuits(service); + /* Let's avoid an underflow. The valid_ip_circuits is initialized to 0 in + * case this condition turns out false because it means that all circuits + * are expiring so we need to keep this circuit. */ + if (num_ip_circuits > expiring_nodes_len) { + valid_ip_circuits = num_ip_circuits - expiring_nodes_len; + } + /* If we already have enough introduction circuits for this service, * redefine this one as a general circuit or close it, depending. - * Substract the amount of expiring nodes here since the circuits are + * Substract the amount of expiring nodes here because the circuits are * still opened. */ - if ((count_intro_point_circuits(service) - - smartlist_len(service->expiring_nodes)) > - service->n_intro_points_wanted) { + if (valid_ip_circuits > service->n_intro_points_wanted) { const or_options_t *options = get_options(); /* Remove the intro point associated with this circuit, it's being * repurposed or closed thus cleanup memory. */ From eea763400f16541f990ea7f65eea389336fab7b8 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 1 Feb 2017 11:30:46 -0500 Subject: [PATCH 3/3] hs: Remove intro point expiring node if no circuit Once a second, we go over all services and consider the validity of the intro points. Now, also try to remove expiring nodes that have no more circuit associated to them. This is possible if we moved an intro point object previously to that list and the circuit actually timed out or was closed by the introduction point itself. Signed-off-by: David Goulet --- changes/bug21302 | 8 ++++++++ src/or/rendservice.c | 13 +++++++++++++ 2 files changed, 21 insertions(+) create mode 100644 changes/bug21302 diff --git a/changes/bug21302 b/changes/bug21302 new file mode 100644 index 0000000000..351b23ea2c --- /dev/null +++ b/changes/bug21302 @@ -0,0 +1,8 @@ + o Minor bugfixes (hidden service): + - Two possible underflow which would ultimately lead to creating a lot of + introduction points circuits and closing them in a non stop loop. Fixes + bug 21302; bugfix on tor-0.2.7.2-alpha. + - Cleanup expiring intro point nodes if no circuit is associated to it + anymore. It was causing, rarely, the service to not open enough + introduction points circuit in the case we had dead expiring nodes.; + bugfix on tor-0.2.7.2-alpha. diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 0db81aed5f..4c5372cc43 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -3907,6 +3907,19 @@ remove_invalid_intro_points(rend_service_t *service, { tor_assert(service); + /* Remove any expired nodes that doesn't have a circuit. */ + SMARTLIST_FOREACH_BEGIN(service->expiring_nodes, rend_intro_point_t *, + intro) { + origin_circuit_t *intro_circ = + find_intro_circuit(intro, service->pk_digest); + if (intro_circ) { + continue; + } + /* No more circuit, cleanup the into point object. */ + SMARTLIST_DEL_CURRENT(service->expiring_nodes, intro); + rend_intro_point_free(intro); + } SMARTLIST_FOREACH_END(intro); + SMARTLIST_FOREACH_BEGIN(service->intro_nodes, rend_intro_point_t *, intro) { /* Find the introduction point node object. */