Skip to content
Permalink
Browse files Browse the repository at this point in the history
Consider the exit family when applying guard restrictions.
When the new path selection logic went into place, I accidentally
dropped the code that considered the _family_ of the exit node when
deciding if the guard was usable, and we didn't catch that during
code review.

This patch makes the guard_restriction_t code consider the exit
family as well, and adds some (hopefully redundant) checks for the
case where we lack a node_t for a guard but we have a bridge_info_t
for it.

Fixes bug 22753; bugfix on 0.3.0.1-alpha. Tracked as TROVE-2016-006
and CVE-2017-0377.
  • Loading branch information
nmathewson committed Jun 29, 2017
1 parent a242d19 commit 665baf5
Show file tree
Hide file tree
Showing 5 changed files with 53 additions and 6 deletions.
7 changes: 7 additions & 0 deletions 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.

39 changes: 38 additions & 1 deletion src/or/entrynodes.c
Expand Up @@ -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.)
Expand All @@ -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);
}

Expand Down
9 changes: 5 additions & 4 deletions src/or/entrynodes.h
Expand Up @@ -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];
};
Expand Down
2 changes: 1 addition & 1 deletion src/or/nodelist.c
Expand Up @@ -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)
{
Expand Down
2 changes: 2 additions & 0 deletions src/or/nodelist.h
Expand Up @@ -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.
Expand Down

0 comments on commit 665baf5

Please sign in to comment.