Merge branch 'trove-2017-006' into maint-0.3.0

This commit is contained in:
Nick Mathewson 2017-06-29 15:57:42 -04:00
commit 52c4440c48
6 changed files with 70 additions and 8 deletions

7
changes/bug22753 Normal file
View File

@ -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.

View File

@ -1428,6 +1428,38 @@ entry_guard_passes_filter(const or_options_t *options, guard_selection_t *gs,
}
}
/** Return true iff <b>guard</b> is in the same family as <b>node</b>.
*/
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 <b>guard</b> obeys the restrictions defined in <b>rst</b>.
* (If <b>rst</b> 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);
}

View File

@ -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 <em>one particular circuit</em>.
*/
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];
};

View File

@ -1343,7 +1343,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)
{

View File

@ -94,6 +94,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.

View File

@ -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);
}