diff --git a/changes/bug22753 b/changes/bug22753 new file mode 100644 index 0000000000..32a6dfa56c --- /dev/null +++ b/changes/bug22753 @@ -0,0 +1,7 @@ + o Major bugfixes (path selection, security): + - When choosing which guard to use for a circuit, avoid the + exit's family along with the exit itself. Previously, the new + guard selection logic avoided the exit, but did not consider + its family. Fixes bug 22753; bugfix on 0.3.0.1-alpha. Tracked + as TROVE-2016-006 and CVE-2017-0377. + diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c index be9f85a89f..fa768fc4a6 100644 --- a/src/or/entrynodes.c +++ b/src/or/entrynodes.c @@ -1428,6 +1428,38 @@ entry_guard_passes_filter(const or_options_t *options, guard_selection_t *gs, } } +/** Return true iff guard is in the same family as node. + */ +static int +guard_in_node_family(const entry_guard_t *guard, const node_t *node) +{ + const node_t *guard_node = node_get_by_id(guard->identity); + if (guard_node) { + return nodes_in_same_family(guard_node, node); + } else { + /* If we don't have a node_t for the guard node, we might have + * a bridge_info_t for it. So let's check to see whether the bridge + * address matches has any family issues. + * + * (Strictly speaking, I believe this check is unnecessary, since we only + * use it to avoid the exit's family when building circuits, and we don't + * build multihop circuits until we have a routerinfo_t for the + * bridge... at which point, we'll also have a node_t for the + * bridge. Nonetheless, it seems wise to include it, in case our + * assumptions change down the road. -nickm.) + */ + if (get_options()->EnforceDistinctSubnets && guard->bridge_addr) { + tor_addr_t node_addr; + node_get_addr(node, &node_addr); + if (addrs_in_same_network_family(&node_addr, + &guard->bridge_addr->addr)) { + return 1; + } + } + return 0; + } +} + /** * Return true iff guard obeys the restrictions defined in rst. * (If rst is NULL, there are no restrictions.) @@ -1440,7 +1472,12 @@ entry_guard_obeys_restriction(const entry_guard_t *guard, if (! rst) return 1; // No restriction? No problem. - // Only one kind of restriction exists right now + // Only one kind of restriction exists right now: excluding an exit + // ID and all of its family. + const node_t *node = node_get_by_id((const char*)rst->exclude_id); + if (node && guard_in_node_family(guard, node)) + return 0; + return tor_memneq(guard->identity, rst->exclude_id, DIGEST_LEN); } diff --git a/src/or/entrynodes.h b/src/or/entrynodes.h index 11b618bb50..735c7738ba 100644 --- a/src/or/entrynodes.h +++ b/src/or/entrynodes.h @@ -276,16 +276,17 @@ struct entry_guard_handle_t; * A restriction to remember which entry guards are off-limits for a given * circuit. * - * Right now, we only use restrictions to block a single guard from being - * selected; this mechanism is designed to be more extensible in the future, - * however. + * Right now, we only use restrictions to block a single guard and its family + * from being selected; this mechanism is designed to be more extensible in + * the future, however. * * Note: This mechanism is NOT for recording which guards are never to be * used: only which guards cannot be used on one particular circuit. */ struct entry_guard_restriction_t { /** - * The guard's RSA identity digest must not equal this. + * The guard's RSA identity digest must not equal this; and it must not + * be in the same family as any node with this digest. */ uint8_t exclude_id[DIGEST_LEN]; }; diff --git a/src/or/nodelist.c b/src/or/nodelist.c index 0ef9741243..3ac5c3e302 100644 --- a/src/or/nodelist.c +++ b/src/or/nodelist.c @@ -1332,7 +1332,7 @@ nodelist_refresh_countries(void) /** Return true iff router1 and router2 have similar enough network addresses * that we should treat them as being in the same family */ -static inline int +int addrs_in_same_network_family(const tor_addr_t *a1, const tor_addr_t *a2) { diff --git a/src/or/nodelist.h b/src/or/nodelist.h index 6c063de8a3..95ae778a5b 100644 --- a/src/or/nodelist.h +++ b/src/or/nodelist.h @@ -93,6 +93,8 @@ int node_is_unreliable(const node_t *router, int need_uptime, int router_exit_policy_all_nodes_reject(const tor_addr_t *addr, uint16_t port, int need_uptime); void router_set_status(const char *digest, int up); +int addrs_in_same_network_family(const tor_addr_t *a1, + const tor_addr_t *a2); /** router_have_minimum_dir_info tests to see if we have enough * descriptor information to create circuits. diff --git a/src/test/test_entrynodes.c b/src/test/test_entrynodes.c index 3db7e63ee3..1f008d93b3 100644 --- a/src/test/test_entrynodes.c +++ b/src/test/test_entrynodes.c @@ -121,6 +121,8 @@ big_fake_network_setup(const struct testcase_t *testcase) n->is_running = n->is_valid = n->is_fast = n->is_stable = 1; + /* Note: all these guards have the same address, so you'll need to + * disable EnforceDistinctSubnets when a restriction is applied. */ n->rs->addr = 0x04020202; n->rs->or_port = 1234; n->rs->is_v2_dir = 1; @@ -1846,14 +1848,17 @@ test_entry_guard_select_for_circuit_confirmed(void *arg) tt_uint_op(state, OP_EQ, GUARD_CIRC_STATE_USABLE_IF_NO_BETTER_GUARD); tt_i64_op(g2->last_tried_to_connect, OP_EQ, approx_time()); - // If we say that the next confirmed guard in order is excluded, we get - // The one AFTER that. + // If we say that the next confirmed guard in order is excluded, and + // we disable EnforceDistinctSubnets, we get the guard AFTER the + // one we excluded. + get_options_mutable()->EnforceDistinctSubnets = 0; g = smartlist_get(gs->confirmed_entry_guards, smartlist_len(gs->primary_entry_guards)+2); entry_guard_restriction_t rst; memset(&rst, 0, sizeof(rst)); memcpy(rst.exclude_id, g->identity, DIGEST_LEN); g2 = select_entry_guard_for_circuit(gs, GUARD_USAGE_TRAFFIC, &rst, &state); + tt_ptr_op(g2, OP_NE, NULL); tt_ptr_op(g2, OP_NE, g); tt_int_op(g2->confirmed_idx, OP_EQ, smartlist_len(gs->primary_entry_guards)+3); @@ -1873,6 +1878,16 @@ test_entry_guard_select_for_circuit_confirmed(void *arg) tt_assert(g->is_pending); tt_int_op(g->confirmed_idx, OP_EQ, -1); + // If we EnforceDistinctSubnets and apply a restriction, we get + // nothing, since we put all of the nodes in the same /16. + // Regression test for bug 22753/TROVE-2017-006. + get_options_mutable()->EnforceDistinctSubnets = 1; + g = smartlist_get(gs->confirmed_entry_guards, 0); + memset(&rst, 0, sizeof(rst)); + memcpy(rst.exclude_id, g->identity, DIGEST_LEN); + g2 = select_entry_guard_for_circuit(gs, GUARD_USAGE_TRAFFIC, &rst, &state); + tt_ptr_op(g2, OP_EQ, NULL); + done: guard_selection_free(gs); }