commit 26183476575abf0f8daccc2f4ca8be4ba0e2a5de Author: Neel Chauhan neel@neelc.org Date: Tue Jul 31 08:41:21 2018 -0400

Use fascist_firewall_choose_address_ls() in hs_get_extend_info_from_lspecs() --- changes/bug23588 | 7 +++++ src/core/or/policies.c | 22 ++++++++------ src/core/or/policies.h | 3 +- src/feature/hs/hs_common.c | 76 ++++++++++++++++++---------------------------- 4 files changed, 51 insertions(+), 57 deletions(-)

diff --git a/changes/bug23588 b/changes/bug23588 new file mode 100644 index 000000000..754d08361 --- /dev/null +++ b/changes/bug23588 @@ -0,0 +1,7 @@ + o Minor bugfixes (address selection): + - Introduce fascist_firewall_choose_address_ls() which chooses an + IPv6 or IPv4 address based on a smartlist of link specifiers of + what is available and what we prefer. We use this function in + hs_get_extend_info_from_lspecs(). Fixes bug 23588; bugfix on + 0.3.5.1-alpha. Patch by Neel Chauhan. + diff --git a/src/core/or/policies.c b/src/core/or/policies.c index 26a053821..b58a5c9cb 100644 --- a/src/core/or/policies.c +++ b/src/core/or/policies.c @@ -1017,17 +1017,26 @@ fascist_firewall_choose_address_rs(const routerstatus_t *rs, }

/** Like fascist_firewall_choose_address_base(), but takes in a smartlist - * <b>lspecs</b> consisting of one or more link specifiers. + * <b>lspecs</b> consisting of one or more link specifiers. We assume + * fw_connection is FIREWALL_OR_CONNECTION as link specifiers cannot + * contain DirPorts. */ void fascist_firewall_choose_address_ls(const smartlist_t *lspecs, - int pref_only, tor_addr_port_t* ap, - int direct_conn) + int pref_only, tor_addr_port_t* ap) { int have_v4 = 0, have_v6 = 0; uint16_t port_v4 = 0, port_v6 = 0; tor_addr_t addr_v4, addr_v6;

+ tor_assert(ap); + + tor_addr_make_null(&ap->addr, AF_UNSPEC); + ap->port = 0; + + tor_addr_make_null(&addr_v4, AF_INET); + tor_addr_make_null(&addr_v6, AF_INET6); + SMARTLIST_FOREACH_BEGIN(lspecs, const link_specifier_t *, ls) { switch (link_specifier_get_ls_type(ls)) { case LS_IPV4: @@ -1041,7 +1050,7 @@ fascist_firewall_choose_address_ls(const smartlist_t *lspecs, case LS_IPV6: /* Skip if we already seen a v6, or deliberately skip it if we're not a * direct connection. */ - if (have_v6 || !direct_conn) continue; + if (have_v6) continue; tor_addr_from_ipv6_bytes(&addr_v6, (const char *) link_specifier_getconstarray_un_ipv6_addr(ls)); port_v6 = link_specifier_get_un_ipv6_port(ls); @@ -1053,11 +1062,6 @@ fascist_firewall_choose_address_ls(const smartlist_t *lspecs, } } SMARTLIST_FOREACH_END(ls);

- tor_assert(ap); - - tor_addr_make_null(&ap->addr, AF_UNSPEC); - ap->port = 0; - /* Here, don't check for DirPorts as link specifiers are only used for * ORPorts. */ const or_options_t *options = get_options(); diff --git a/src/core/or/policies.h b/src/core/or/policies.h index f4c68fd95..3c46363c0 100644 --- a/src/core/or/policies.h +++ b/src/core/or/policies.h @@ -93,8 +93,7 @@ void fascist_firewall_choose_address_rs(const routerstatus_t *rs, firewall_connection_t fw_connection, int pref_only, tor_addr_port_t* ap); void fascist_firewall_choose_address_ls(const smartlist_t *lspecs, - int pref_only, tor_addr_port_t* ap, - int direct_conn); + int pref_only, tor_addr_port_t* ap); void fascist_firewall_choose_address_node(const node_t *node, firewall_connection_t fw_connection, int pref_only, tor_addr_port_t* ap); diff --git a/src/feature/hs/hs_common.c b/src/feature/hs/hs_common.c index d4736c286..ca7e991b7 100644 --- a/src/feature/hs/hs_common.c +++ b/src/feature/hs/hs_common.c @@ -1672,14 +1672,16 @@ hs_pick_hsdir(smartlist_t *responsible_dirs, const char *req_key_str, return hs_dir; }

-/* From a list of link specifier, an onion key and if we are requesting a - * direct connection (ex: single onion service), return a newly allocated - * extend_info_t object. This function always returns an extend info with - * an IPv4 address, or NULL. +/* Given a list of link specifiers lspecs, a curve 25519 onion_key, and + * a direct connection boolean direct_conn (true for single onion services), + * return a newly allocated extend_info_t object. + * + * This function always returns an extend info with a valid IP address and + * ORPort, or NULL. If direct_conn is false, the IP address is always IPv4. * * It performs the following checks: - * if either IPv4 or legacy ID is missing, return NULL. - * if direct_conn, and we can't reach the IPv4 address, return NULL. + * if there is no usable IP address, or legacy ID is missing, return NULL. + * if direct_conn, and we can't reach any IP address, return NULL. */ extend_info_t * hs_get_extend_info_from_lspecs(const smartlist_t *lspecs, @@ -1688,10 +1690,12 @@ hs_get_extend_info_from_lspecs(const smartlist_t *lspecs, { int have_v4 = 0, have_legacy_id = 0, have_ed25519_id = 0; char legacy_id[DIGEST_LEN] = {0}; - uint16_t port_v4 = 0; - tor_addr_t addr_v4; ed25519_public_key_t ed25519_pk; extend_info_t *info = NULL; + tor_addr_port_t ap; + + tor_addr_make_null(&ap.addr, AF_UNSPEC); + ap.port = 0;

tor_assert(lspecs);

@@ -1704,11 +1708,14 @@ hs_get_extend_info_from_lspecs(const smartlist_t *lspecs, SMARTLIST_FOREACH_BEGIN(lspecs, const link_specifier_t *, ls) { switch (link_specifier_get_ls_type(ls)) { case LS_IPV4: - /* Skip if we already seen a v4. */ - if (have_v4) continue; - tor_addr_from_ipv4h(&addr_v4, + /* Skip if we already seen a v4. If direct_conn is true, we skip this + * block because fascist_firewall_choose_address_ls() will set ap. If + * direct_conn is false, set ap to the first IPv4 address and port in + * the link specifiers.*/ + if (have_v4 || direct_conn) continue; + tor_addr_from_ipv4h(&ap.addr, link_specifier_get_un_ipv4_addr(ls)); - port_v4 = link_specifier_get_un_ipv4_port(ls); + ap.port = link_specifier_get_un_ipv4_port(ls); have_v4 = 1; break; case LS_LEGACY_ID: @@ -1732,55 +1739,32 @@ hs_get_extend_info_from_lspecs(const smartlist_t *lspecs, } } SMARTLIST_FOREACH_END(ls);

- /* Legacy ID is mandatory, and we require IPv4. */ - if (!have_v4 || !have_legacy_id) { - bool both = !have_v4 && !have_legacy_id; - log_fn(LOG_PROTOCOL_WARN, LD_REND, "Missing %s%s%s link specifier%s.", - !have_v4 ? "IPv4" : "", - both ? " and " : "", - !have_legacy_id ? "legacy ID" : "", - both ? "s" : ""); - goto done; - } + /* Choose a preferred address first, but fall back to an allowed address. */ + if (direct_conn) + fascist_firewall_choose_address_ls(lspecs, 0, &ap);

- /* We know we have IPv4, because we just checked. */ - if (!direct_conn) { - /* All clients can extend to any IPv4 via a 3-hop path. */ - goto validate; - } else if (direct_conn && - fascist_firewall_allows_address_addr(&addr_v4, port_v4, - FIREWALL_OR_CONNECTION, - 0, 0)) { - /* Direct connection and we can reach it in IPv4 so go for it. */ - goto validate; - - /* We will add support for falling back to a 3-hop path in a later - * release. */ - } else { - /* If we can't reach IPv4, return NULL. */ - log_fn(LOG_PROTOCOL_WARN, LD_REND, - "Received an IPv4 link specifier, " - "but the address is not reachable: %s:%u", - fmt_addr(&addr_v4), port_v4); + /* Legacy ID is mandatory, and we require an IP address. */ + if (!tor_addr_port_is_valid_ap(&ap, 0) || !have_legacy_id) { + /* If we're missing the legacy ID or the IP address, return NULL. */ goto done; }

- /* We will add support for IPv6 in a later release. */ + /* We will add support for falling back to a 3-hop path in a later + * release. */

- validate: /* We'll validate now that the address we've picked isn't a private one. If * it is, are we allowed to extend to private addresses? */ - if (!extend_info_addr_is_allowed(&addr_v4)) { + if (!extend_info_addr_is_allowed(&ap.addr)) { log_fn(LOG_PROTOCOL_WARN, LD_REND, "Requested address is private and we are not allowed to extend to " - "it: %s:%u", fmt_addr(&addr_v4), port_v4); + "it: %s:%u", fmt_addr(&ap.addr), ap.port); goto done; }

/* We do have everything for which we think we can connect successfully. */ info = extend_info_new(NULL, legacy_id, (have_ed25519_id) ? &ed25519_pk : NULL, NULL, - onion_key, &addr_v4, port_v4); + onion_key, &ap.addr, ap.port); done: return info; }