commit b4aa8fc3d918cc3aea375985c44abd086f91ae7a Author: teor teor2345@gmail.com Date: Thu Oct 26 14:47:54 2017 +1100

Remove buggy IPv6 support from pick_intro_point() and service_intro_point_new()

The previous version of these functions had the following issues: * they can't supply both the IPv4 and IPv6 addresses in link specifiers, * they try to fall back to a 3-hop path when the address for a direct connection is unreachable, but this isn't supported by launch_rendezvous_point_circuit(), so it fails. But we can't fix these things in a bugfix release.

Instead, always put IPv4 addresses in link specifiers. And if a v3 single onion service can't reach any intro points, fail.

This supports v3 hidden services on IPv4, dual-stack, and IPv6, and v3 single onion services on IPv4 only.

Part of 23820, bugfix on 0.3.2.1-alpha. --- src/or/hs_service.c | 50 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 30 insertions(+), 20 deletions(-)

diff --git a/src/or/hs_service.c b/src/or/hs_service.c index 3d1945aa9..f334de643 100644 --- a/src/or/hs_service.c +++ b/src/or/hs_service.c @@ -375,7 +375,14 @@ service_intro_point_free_(void *obj)

/* Return a newly allocated service intro point and fully initialized from the * given extend_info_t ei if non NULL. If is_legacy is true, we also generate - * the legacy key. On error, NULL is returned. */ + * the legacy key. On error, NULL is returned. + * + * If ei is NULL, returns a hs_service_intro_point_t with an empty link + * specifier list and no onion key. (This is used for testing.) + * + * ei must be an extend_info_t containing an IPv4 address. (We will add supoort + * for IPv6 in a later release.) When calling extend_info_from_node(), pass + * 0 in for_direct_connection to make sure ei always has an IPv4 address. */ STATIC hs_service_intro_point_t * service_intro_point_new(const extend_info_t *ei, unsigned int is_legacy) { @@ -427,8 +434,8 @@ service_intro_point_new(const extend_info_t *ei, unsigned int is_legacy) goto done; }

- /* We'll try to add all link specifier. Legacy, IPv4 and ed25519 are - * mandatory. */ + /* We'll try to add all link specifiers. Legacy is mandatory. + * IPv4 or IPv6 is required, and we always send IPv4. */ ls = hs_desc_link_specifier_new(ei, LS_IPV4); /* It is impossible to have an extend info object without a v4. */ if (BUG(!ls)) { @@ -450,11 +457,7 @@ service_intro_point_new(const extend_info_t *ei, unsigned int is_legacy) smartlist_add(ip->base.link_specifiers, ls); }

- /* IPv6 is optional. */ - ls = hs_desc_link_specifier_new(ei, LS_IPV6); - if (ls) { - smartlist_add(ip->base.link_specifiers, ls); - } + /* IPv6 is not supported in this release. */

/* Finally, copy onion key from the extend_info_t object. */ memcpy(&ip->onion_key, &ei->curve25519_onion_key, sizeof(ip->onion_key)); @@ -1520,10 +1523,17 @@ build_all_descriptors(time_t now) /* Randomly pick a node to become an introduction point but not present in the * given exclude_nodes list. The chosen node is put in the exclude list * regardless of success or not because in case of failure, the node is simply - * unsusable from that point on. If direct_conn is set, try to pick a node - * that our local firewall/policy allows to directly connect to and if not, - * fallback to a normal 3-hop node. Return a newly allocated service intro - * point ready to be used for encoding. NULL on error. */ + * unsusable from that point on. + * + * If direct_conn is set, try to pick a node that our local firewall/policy + * allows us to connect to directly. If we can't find any, return NULL. + * This function supports selecting dual-stack nodes for direct single onion + * service IPv6 connections. But it does not send IPv6 addresses in link + * specifiers. (Current clients don't use IPv6 addresses to extend, and + * direct client connections to intro points are not supported.) + * + * Return a newly allocated service intro point ready to be used for encoding. + * Return NULL on error. */ static hs_service_intro_point_t * pick_intro_point(unsigned int direct_conn, smartlist_t *exclude_nodes) { @@ -1537,12 +1547,9 @@ pick_intro_point(unsigned int direct_conn, smartlist_t *exclude_nodes)

node = router_choose_random_node(exclude_nodes, get_options()->ExcludeNodes, direct_conn ? direct_flags : flags); - if (node == NULL && direct_conn) { - /* Unable to find a node for direct connection, let's fall back to a - * normal 3-hop node. */ - node = router_choose_random_node(exclude_nodes, - get_options()->ExcludeNodes, 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 (!node) { goto err; } @@ -1553,8 +1560,11 @@ pick_intro_point(unsigned int direct_conn, smartlist_t *exclude_nodes) smartlist_add(exclude_nodes, (void *) node);

/* We do this to ease our life but also this call makes appropriate checks - * of the node object such as validating ntor support for instance. */ - info = extend_info_from_node(node, direct_conn); + * of the node object such as validating ntor support for instance. + * + * We must provide an extend_info for clients to connect over a 3-hop path, + * so we don't pass direct_conn here. */ + info = extend_info_from_node(node, 0); if (BUG(info == NULL)) { goto err; }