commit 75772ea096e030ecc79f67b1444cac42aaed7449 Author: Nick Mathewson <nickm@torproject.org> Date: Thu Aug 6 11:21:00 2020 -0400 Validate address more carefully when checking self-reachability Previously, we would treat *any* incoming circuit on a non-local channel as meaning that our ORPort was reachable. With this patch, we make sure that the address that the peer _says_ we have is the same as the one we're trying to advertise right now. Closes 20165. Bugfix on 4f5192b2803c706 in 0.1.0.1-rc, when reachability self-tests were first introduced. --- changes/bug20165 | 6 ++++++ src/core/or/channel.c | 2 ++ src/core/or/channel.h | 3 +++ src/core/or/channeltls.c | 7 +++++++ src/feature/relay/circuitbuild_relay.c | 24 +++++++++++++++++------- src/feature/relay/router.c | 25 +++++++++++++++++++++++++ src/feature/relay/router.h | 1 + 7 files changed, 61 insertions(+), 7 deletions(-) diff --git a/changes/bug20165 b/changes/bug20165 new file mode 100644 index 0000000000..bbe9f00032 --- /dev/null +++ b/changes/bug20165 @@ -0,0 +1,6 @@ + o Minor bugfixes (self-testing): + - When receiving an incoming circuit, only accept it as evidence that we + are reachable if the declared address of its channel is the same + address we think that we have. Otherwise, it could be evidence that + we're reachable on some other address. Fixes bug 20165; bugfix on + 0.1.0.1-rc. diff --git a/src/core/or/channel.c b/src/core/or/channel.c index 5a42d452f2..0f74950b4f 100644 --- a/src/core/or/channel.c +++ b/src/core/or/channel.c @@ -872,6 +872,8 @@ channel_init(channel_t *chan) /* Channel is not in the scheduler heap. */ chan->sched_heap_idx = -1; + + tor_addr_make_unspec(&chan->addr_according_to_peer); } /** diff --git a/src/core/or/channel.h b/src/core/or/channel.h index d52ebdf619..fc0dd3aaac 100644 --- a/src/core/or/channel.h +++ b/src/core/or/channel.h @@ -236,6 +236,9 @@ struct channel_t { /** The handle to this channel (to free on canceled timers) */ struct channel_handle_t *timer_handle; + /** If not UNSPEC, the address that the peer says we have. */ + tor_addr_t addr_according_to_peer; + /** * These two fields specify the minimum and maximum negotiated timeout * values for inactivity (send or receive) before we decide to pad a diff --git a/src/core/or/channeltls.c b/src/core/or/channeltls.c index ae60038c34..a9012fd317 100644 --- a/src/core/or/channeltls.c +++ b/src/core/or/channeltls.c @@ -1868,6 +1868,13 @@ channel_tls_process_netinfo_cell(cell_t *cell, channel_tls_t *chan) } } + if (me) { + /* We have a descriptor, so we are a relay: record the address that the + * other side said we had. */ + tor_addr_copy(&TLS_CHAN_TO_BASE(chan)->addr_according_to_peer, + &my_apparent_addr); + } + n_other_addrs = netinfo_cell_get_n_my_addrs(netinfo_cell); for (uint8_t i = 0; i < n_other_addrs; i++) { /* Consider all the other addresses; if any matches, this connection is diff --git a/src/feature/relay/circuitbuild_relay.c b/src/feature/relay/circuitbuild_relay.c index ad20e143be..64f3c341ae 100644 --- a/src/feature/relay/circuitbuild_relay.c +++ b/src/feature/relay/circuitbuild_relay.c @@ -588,13 +588,23 @@ onionskin_answer(struct or_circuit_t *circ, if ((!channel_is_local(circ->p_chan) || get_options()->ExtendAllowPrivateAddresses) && !channel_is_outgoing(circ->p_chan)) { - /* record that we could process create cells from a non-local conn - * that we didn't initiate; presumably this means that create cells - * can reach us too. */ - tor_addr_t remote_addr; - if (channel_get_addr_if_possible(circ->p_chan, &remote_addr)) { - int family = tor_addr_family(&remote_addr); - router_orport_found_reachable(family); + /* Okay, it's a create cell from a non-local connection + * that we didn't initiate. Presumably this means that create cells + * can reach us too. But what address can they reach us on? */ + const tor_addr_t *my_supposed_addr = &circ->p_chan->addr_according_to_peer; + if (router_addr_is_my_published_addr(my_supposed_addr)) { + /* Great, this create cell came on connection where the peer says + * that the our address is an address we're actually advertising! + * That should mean that we're reachable. But before we finally + * declare ourselves reachable, make sure that the address listed + * by the peer is the same family as the peer is actually using. + */ + tor_addr_t remote_addr; + int family = tor_addr_family(my_supposed_addr); + if (channel_get_addr_if_possible(circ->p_chan, &remote_addr) && + tor_addr_family(&remote_addr) == family) { + router_orport_found_reachable(family); + } } } diff --git a/src/feature/relay/router.c b/src/feature/relay/router.c index 675b977ade..3fcf0d616b 100644 --- a/src/feature/relay/router.c +++ b/src/feature/relay/router.c @@ -1729,6 +1729,31 @@ router_is_me(const routerinfo_t *router) return router_digest_is_me(router->cache_info.identity_digest); } +/** + * Return true if we are a server, and if @a addr is an address we are + * currently publishing (or trying to publish) in our descriptor. + * Return false otherwise. + **/ +bool +router_addr_is_my_published_addr(const tor_addr_t *addr) +{ + IF_BUG_ONCE(!addr) + return false; + + const routerinfo_t *me = router_get_my_routerinfo(); + if (!me) + return false; + + switch (tor_addr_family(addr)) { + case AF_INET: + return tor_addr_eq(addr, &me->ipv4_addr); + case AF_INET6: + return tor_addr_eq(addr, &me->ipv6_addr); + default: + return false; + } +} + /** Return a routerinfo for this OR, rebuilding a fresh one if * necessary. Return NULL on error, or if called on an OP. */ MOCK_IMPL(const routerinfo_t *, diff --git a/src/feature/relay/router.h b/src/feature/relay/router.h index 89b4a479a4..f71ada8eb7 100644 --- a/src/feature/relay/router.h +++ b/src/feature/relay/router.h @@ -100,6 +100,7 @@ int router_digest_is_me(const char *digest); const uint8_t *router_get_my_id_digest(void); int router_extrainfo_digest_is_me(const char *digest); int router_is_me(const routerinfo_t *router); +bool router_addr_is_my_published_addr(const tor_addr_t *addr); int router_build_fresh_descriptor(routerinfo_t **r, extrainfo_t **e); int router_rebuild_descriptor(int force); char *router_dump_router_to_string(routerinfo_t *router,