From 0d4a689d3ae8f7e05b3baf8ad71d983a767ef55b Mon Sep 17 00:00:00 2001 From: Tobias Stoeckmann Date: Mon, 24 Jun 2019 22:08:49 +0200 Subject: [PATCH 01/11] Prevent UB on signed overflow. Overflowing a signed integer in C is an undefined behaviour. It is possible to trigger this undefined behaviour in tor_asprintf on Windows or systems lacking vasprintf. On these systems, eiter _vscprintf or vsnprintf is called to retrieve the required amount of bytes to hold the string. These functions can return INT_MAX. The easiest way to recreate this is the use of a specially crafted configuration file, e.g. containing the line: FirewallPorts AAAAA This line triggers the needed tor_asprintf call which eventually leads to an INT_MAX return value from _vscprintf or vsnprintf. The needed byte for \0 is added to the result, triggering the overflow and therefore the undefined behaviour. Casting the value to size_t before addition fixes the behaviour. Signed-off-by: Tobias Stoeckmann --- src/common/compat.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/common/compat.c b/src/common/compat.c index 9758751122..6f7ac7bd7d 100644 --- a/src/common/compat.c +++ b/src/common/compat.c @@ -540,8 +540,8 @@ tor_vasprintf(char **strp, const char *fmt, va_list args) *strp = NULL; return -1; } - strp_tmp = tor_malloc(len + 1); - r = _vsnprintf(strp_tmp, len+1, fmt, args); + strp_tmp = tor_malloc((size_t)len + 1); + r = _vsnprintf(strp_tmp, (size_t)len+1, fmt, args); if (r != len) { tor_free(strp_tmp); *strp = NULL; @@ -566,9 +566,9 @@ tor_vasprintf(char **strp, const char *fmt, va_list args) *strp = tor_strdup(buf); return len; } - strp_tmp = tor_malloc(len+1); + strp_tmp = tor_malloc((size_t)len+1); /* use of tor_vsnprintf() will ensure string is null terminated */ - r = tor_vsnprintf(strp_tmp, len+1, fmt, args); + r = tor_vsnprintf(strp_tmp, (size_t)len+1, fmt, args); if (r != len) { tor_free(strp_tmp); *strp = NULL; @@ -3543,4 +3543,3 @@ tor_get_avail_disk_space(const char *path) return -1; #endif } - From 97d73db7c36ec3fac2974726012f76bff63f9dfc Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 19 Jul 2019 09:21:08 -0400 Subject: [PATCH 02/11] Changes file for bug 31001 --- changes/ticket31001 | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changes/ticket31001 diff --git a/changes/ticket31001 b/changes/ticket31001 new file mode 100644 index 0000000000..2ce1cbdf34 --- /dev/null +++ b/changes/ticket31001 @@ -0,0 +1,6 @@ + o Minor bugfixes (compatibility, standards compliance): + - Fix a bug that would invoke undefined behavior on certain operating + systems when trying to asprintf() a string exactly INT_MAX bytes + long. We don't believe this is exploitable, but it's better + to fix it anyway. Fixes bug 31001; bugfix on 0.2.2.11-alpha. + Found and fixed by Tobias Stoeckmann. From dc08f8ac70e95b9a76f86a5e15dfcabd7ada25b9 Mon Sep 17 00:00:00 2001 From: teor Date: Fri, 23 Aug 2019 14:38:54 +1000 Subject: [PATCH 03/11] rendservice: Always use a 3-hop path when a v2 single onion rend fails Previously, we used a 1-hop path when a single onion rend failed immediately, and a 3-hop path when it failed after trying to build a circuit. Fixes bug 23818; bugfix on 0.2.9.3-alpha. --- src/or/rendservice.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 32b856452d..f145e9d694 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -2073,8 +2073,12 @@ rend_service_receive_introduction(origin_circuit_t *circuit, int flags = CIRCLAUNCH_NEED_CAPACITY | CIRCLAUNCH_IS_INTERNAL; if (circ_needs_uptime) flags |= CIRCLAUNCH_NEED_UPTIME; /* A Single Onion Service only uses a direct connection if its - * firewall rules permit direct connections to the address. */ - if (rend_service_use_direct_connection(options, rp)) { + * firewall rules permit direct connections to the address. + * + * We only use a one-hop path on the first attempt. If the first attempt + * fails, we use a 3-hop path for reachability / reliability. + * See the comment in rend_service_relauch_rendezvous() for details. */ + if (rend_service_use_direct_connection(options, rp) && i == 0) { flags = flags | CIRCLAUNCH_ONEHOP_TUNNEL; } launched = circuit_launch_by_extend_info( From 144084d6facc3d688c7d423a8b5b8da5e26de84c Mon Sep 17 00:00:00 2001 From: teor Date: Fri, 23 Aug 2019 14:54:15 +1000 Subject: [PATCH 04/11] rendservice: Always use a 3-hop path when a v2 single onion intro fails Previously, we always used a 1-hop path, no matter how many times a v2 single onion intro failed. Fixes bug 23818; bugfix on 0.2.9.3-alpha. --- src/or/rendservice.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index f145e9d694..ca1be5e5de 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -3041,8 +3041,15 @@ rend_service_launch_establish_intro(rend_service_t *service, extend_info_t *launch_ei = intro->extend_info; extend_info_t *direct_ei = NULL; - /* Are we in single onion mode? */ - if (rend_service_allow_non_anonymous_connection(options)) { + /* Are we in single onion mode? + * + * We only use a one-hop path on the first attempt. If the first attempt + * fails, we use a 3-hop path for reachability / reliability. + * (Unlike v3, retries is incremented by the caller after it calls this + * function.) + */ + if (rend_service_allow_non_anonymous_connection(options) && + intro->circuit_retries == 0) { /* Do we have a descriptor for the node? * We've either just chosen it from the consensus, or we've just reviewed * our intro points to see which ones are still valid, and deleted the ones From 231a74363fa9ccd1769b61229dc70a72e1e5807b Mon Sep 17 00:00:00 2001 From: teor Date: Fri, 23 Aug 2019 15:08:43 +1000 Subject: [PATCH 05/11] changes: file for 23818, v2 onion service fix --- changes/bug23818_v2 | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changes/bug23818_v2 diff --git a/changes/bug23818_v2 b/changes/bug23818_v2 new file mode 100644 index 0000000000..0219a20f49 --- /dev/null +++ b/changes/bug23818_v2 @@ -0,0 +1,6 @@ + o Minor bugfixes (v2 single onion services): + - Always retry v2 single onion service intro and rend circuits with a + 3-hop path. Previously, v2 single onion services used a 3-hop path + when rend circuits were retried after a remote or delayed failure, + but a 1-hop path for immediate retries. Fixes bug 23818; + bugfix on 0.2.9.3-alpha. From c94904b35982a73a652d9bed8b195ce625bbb962 Mon Sep 17 00:00:00 2001 From: teor Date: Fri, 23 Aug 2019 14:41:24 +1000 Subject: [PATCH 06/11] hs: Always use a 3-hop path when a v3 single onion rend fails Previously, we used a 1-hop path when a single onion rend failed immediately, and a 3-hop path when it failed after trying to build a circuit. Fixes bug 23818; bugfix on 0.3.2.1-alpha. --- src/feature/hs/hs_circuit.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/feature/hs/hs_circuit.c b/src/feature/hs/hs_circuit.c index e3873d2f18..f7996a24f5 100644 --- a/src/feature/hs/hs_circuit.c +++ b/src/feature/hs/hs_circuit.c @@ -405,8 +405,12 @@ launch_rendezvous_point_circuit(const hs_service_t *service, if (circ_needs_uptime) { circ_flags |= CIRCLAUNCH_NEED_UPTIME; } - /* Firewall and policies are checked when getting the extend info. */ - if (service->config.is_single_onion) { + /* Firewall and policies are checked when getting the extend info. + * + * We only use a one-hop path on the first attempt. If the first attempt + * fails, we use a 3-hop path for reachability / reliability. + * See the comment in retry_service_rendezvous_point() for details. */ + if (service->config.is_single_onion && i == 0) { circ_flags |= CIRCLAUNCH_ONEHOP_TUNNEL; } From 229a982405e0fd2f9980d3c1a41c34e0cf26ae26 Mon Sep 17 00:00:00 2001 From: teor Date: Fri, 23 Aug 2019 14:56:01 +1000 Subject: [PATCH 07/11] hs: Always use a 3-hop path when a v3 single onion intro fails Previously, we always used a 1-hop path, no matter how many times a v3 single onion intro failed. Fixes bug 23818; bugfix on 0.3.2.1-alpha. --- src/feature/hs/hs_circuit.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/feature/hs/hs_circuit.c b/src/feature/hs/hs_circuit.c index f7996a24f5..d74b088f07 100644 --- a/src/feature/hs/hs_circuit.c +++ b/src/feature/hs/hs_circuit.c @@ -775,8 +775,15 @@ hs_circ_launch_intro_point(hs_service_t *service, tor_assert(ei); /* Update circuit flags in case of a single onion service that requires a - * direct connection. */ - if (service->config.is_single_onion) { + * direct connection. + * + * We only use a one-hop path on the first attempt. If the first attempt + * fails, we use a 3-hop path for reachability / reliability. + * (Unlike v2, retries is incremented by the caller before it calls this + * function.) + */ + tor_assert_nonfatal(ip->circuit_retries > 0); + if (service->config.is_single_onion && ip->circuit_retries == 1) { circ_flags |= CIRCLAUNCH_ONEHOP_TUNNEL; } From 084245134b022aa983c45aa3fc0ace9fd7ae21a9 Mon Sep 17 00:00:00 2001 From: teor Date: Fri, 23 Aug 2019 15:10:45 +1000 Subject: [PATCH 08/11] changes: file for 23818, v3 onion service fix --- changes/bug23818_v3 | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changes/bug23818_v3 diff --git a/changes/bug23818_v3 b/changes/bug23818_v3 new file mode 100644 index 0000000000..c430144d81 --- /dev/null +++ b/changes/bug23818_v3 @@ -0,0 +1,6 @@ + o Minor bugfixes (v3 single onion services): + - Always retry v3 single onion service intro and rend circuits with a + 3-hop path. Previously, v3 single onion services used a 3-hop path + when rend circuits were retried after a remote or delayed failure, + but a 1-hop path for immediate retries. Fixes bug 23818; + bugfix on 0.3.2.1-alpha. From e2e1c07fd25c96a83b38f5d84d2c063b98ed8f46 Mon Sep 17 00:00:00 2001 From: teor Date: Fri, 23 Aug 2019 16:17:47 +1000 Subject: [PATCH 09/11] hs: v3 single onion services fall back to 3-hop intro for unreachable nodes Previously, v3 single onion services failed when all intro nodes were unreachable via a 1-hop path. Now, we select intros that are only available via a 3-hop path, and use a 3-hop path to connect to them. Fixes bug 23507; bugfix on 0.3.2.1-alpha. --- src/feature/hs/hs_circuit.c | 25 +++++++++++++++---------- src/feature/hs/hs_circuit.h | 3 ++- src/feature/hs/hs_service.c | 30 +++++++++++++++++++++++------- 3 files changed, 40 insertions(+), 18 deletions(-) diff --git a/src/feature/hs/hs_circuit.c b/src/feature/hs/hs_circuit.c index d74b088f07..8acfcbd65b 100644 --- a/src/feature/hs/hs_circuit.c +++ b/src/feature/hs/hs_circuit.c @@ -758,13 +758,16 @@ hs_circ_retry_service_rendezvous_point(origin_circuit_t *circ) } /* For a given service and a service intro point, launch a circuit to the - * extend info ei. If the service is a single onion, a one-hop circuit will be - * requested. Return 0 if the circuit was successfully launched and tagged + * extend info ei. If the service is a single onion, and direct_conn is true, + * a one-hop circuit will be requested. + * + * Return 0 if the circuit was successfully launched and tagged * with the correct identifier. On error, a negative value is returned. */ int hs_circ_launch_intro_point(hs_service_t *service, const hs_service_intro_point_t *ip, - extend_info_t *ei) + extend_info_t *ei, + bool direct_conn) { /* Standard flags for introduction circuit. */ int ret = -1, circ_flags = CIRCLAUNCH_NEED_UPTIME | CIRCLAUNCH_IS_INTERNAL; @@ -775,15 +778,17 @@ hs_circ_launch_intro_point(hs_service_t *service, tor_assert(ei); /* Update circuit flags in case of a single onion service that requires a - * direct connection. - * - * We only use a one-hop path on the first attempt. If the first attempt + * direct connection. */ + tor_assert_nonfatal(ip->circuit_retries > 0); + /* Only single onion services can make direct conns */ + if (BUG(!service->config.is_single_onion && direct_conn)) { + goto end; + } + /* We only use a one-hop path on the first attempt. If the first attempt * fails, we use a 3-hop path for reachability / reliability. * (Unlike v2, retries is incremented by the caller before it calls this - * function.) - */ - tor_assert_nonfatal(ip->circuit_retries > 0); - if (service->config.is_single_onion && ip->circuit_retries == 1) { + * function.) */ + if (direct_conn && ip->circuit_retries == 1) { circ_flags |= CIRCLAUNCH_ONEHOP_TUNNEL; } diff --git a/src/feature/hs/hs_circuit.h b/src/feature/hs/hs_circuit.h index b8d8b25add..e168b301f1 100644 --- a/src/feature/hs/hs_circuit.h +++ b/src/feature/hs/hs_circuit.h @@ -26,7 +26,8 @@ void hs_circ_service_rp_has_opened(const hs_service_t *service, origin_circuit_t *circ); int hs_circ_launch_intro_point(hs_service_t *service, const hs_service_intro_point_t *ip, - extend_info_t *ei); + extend_info_t *ei, + bool direct_conn); int hs_circ_launch_rendezvous_point(const hs_service_t *service, const curve25519_public_key_t *onion_key, const uint8_t *rendezvous_cookie); diff --git a/src/feature/hs/hs_service.c b/src/feature/hs/hs_service.c index 4029290364..b05f20366f 100644 --- a/src/feature/hs/hs_service.c +++ b/src/feature/hs/hs_service.c @@ -2105,6 +2105,7 @@ build_all_descriptors(time_t now) static hs_service_intro_point_t * pick_intro_point(unsigned int direct_conn, smartlist_t *exclude_nodes) { + const or_options_t *options = get_options(); const node_t *node; extend_info_t *info = NULL; hs_service_intro_point_t *ip = NULL; @@ -2113,11 +2114,19 @@ pick_intro_point(unsigned int direct_conn, smartlist_t *exclude_nodes) /* Single onion flags. */ router_crn_flags_t direct_flags = flags | CRN_PREF_ADDR | CRN_DIRECT_CONN; - node = router_choose_random_node(exclude_nodes, get_options()->ExcludeNodes, + node = router_choose_random_node(exclude_nodes, options->ExcludeNodes, direct_conn ? direct_flags : flags); - /* Unable to find a node. When looking for a node for a direct connection, - * we could try a 3-hop path instead. We'll add support for this in a later - * release. */ + + /* If we are in single onion mode, retry node selection for a 3-hop + * path */ + if (direct_conn && !node) { + log_info(LD_REND, + "Unable to find an intro point that we can connect to " + "directly, falling back to a 3-hop path."); + node = router_choose_random_node(exclude_nodes, options->ExcludeNodes, + flags); + } + if (!node) { goto err; } @@ -2644,7 +2653,7 @@ launch_intro_point_circuits(hs_service_t *service) * circuits using the current map. */ FOR_EACH_DESCRIPTOR_BEGIN(service, desc) { /* Keep a ref on if we need a direct connection. We use this often. */ - unsigned int direct_conn = service->config.is_single_onion; + bool direct_conn = service->config.is_single_onion; DIGEST256MAP_FOREACH_MODIFY(desc->intro_points.map, key, hs_service_intro_point_t *, ip) { @@ -2655,8 +2664,15 @@ launch_intro_point_circuits(hs_service_t *service) if (hs_circ_service_get_intro_circ(ip)) { continue; } - ei = get_extend_info_from_intro_point(ip, direct_conn); + + /* If we can't connect directly to the intro point, get an extend_info + * for a multi-hop path instead. */ + if (ei == NULL && direct_conn) { + direct_conn = false; + ei = get_extend_info_from_intro_point(ip, 0); + } + if (ei == NULL) { /* This is possible if we can get a node_t but not the extend info out * of it. In this case, we remove the intro point and a new one will @@ -2668,7 +2684,7 @@ launch_intro_point_circuits(hs_service_t *service) /* Launch a circuit to the intro point. */ ip->circuit_retries++; - if (hs_circ_launch_intro_point(service, ip, ei) < 0) { + if (hs_circ_launch_intro_point(service, ip, ei, direct_conn) < 0) { log_info(LD_REND, "Unable to launch intro circuit to node %s " "for service %s.", safe_str_client(extend_info_describe(ei)), From 41bc1fac8e77e399572e020c5ba59462afd75f4b Mon Sep 17 00:00:00 2001 From: teor Date: Fri, 23 Aug 2019 16:25:33 +1000 Subject: [PATCH 10/11] changes: file for 23507, v3 onion service fix --- changes/bug23507 | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changes/bug23507 diff --git a/changes/bug23507 b/changes/bug23507 new file mode 100644 index 0000000000..de18273fdb --- /dev/null +++ b/changes/bug23507 @@ -0,0 +1,5 @@ + o Minor bugfixes (v3 single onion services): + - Make v3 single onion services fall back to a 3-hop intro, when there + all intro points are unreachable via a 1-hop path. Previously, v3 + single onion services failed when all intro nodes were unreachable + via a 1-hop path. Fixes bug 23507; bugfix on 0.3.2.1-alpha. From 05fa1689eb96a1249030e1d51828a308d19d728c Mon Sep 17 00:00:00 2001 From: rl1987 Date: Sat, 20 Jul 2019 18:28:07 +0300 Subject: [PATCH 11/11] Change loglevel of message 'Hash of session info was not as expected' --- changes/bug12399 | 3 +++ src/feature/rend/rendmid.c | 3 ++- 2 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 changes/bug12399 diff --git a/changes/bug12399 b/changes/bug12399 new file mode 100644 index 0000000000..922c08c5e3 --- /dev/null +++ b/changes/bug12399 @@ -0,0 +1,3 @@ + o Minor bugfixes (logging): + - Change log level of message "Hash of session info was not as expected" + to LOG_PROTOCOL_WARN. Fixes bug 12399; bugfix on 0.1.1.10-alpha. diff --git a/src/feature/rend/rendmid.c b/src/feature/rend/rendmid.c index fcfa5052cb..3ba48f8858 100644 --- a/src/feature/rend/rendmid.c +++ b/src/feature/rend/rendmid.c @@ -70,7 +70,8 @@ rend_mid_establish_intro_legacy(or_circuit_t *circ, const uint8_t *request, goto err; } if (tor_memneq(expected_digest, request+2+asn1len, DIGEST_LEN)) { - log_warn(LD_PROTOCOL, "Hash of session info was not as expected."); + log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, + "Hash of session info was not as expected."); reason = END_CIRC_REASON_TORPROTOCOL; goto err; }