June 2017 - tor-commits - lists.torproject.org

[tor/master] Merge branch 'trove-2017-006' into maint-0.3.0
by nickm＠torproject.org 29 Jun '17

29 Jun '17

commit 52c4440c4895cb90dd155bb094b49a97128baedf Merge: 4c21d4e 88666d0 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jun 29 15:57:42 2017 -0400 Merge branch 'trove-2017-006' into maint-0.3.0 changes/bug22753 | 7 +++++++ src/or/entrynodes.c | 39 ++++++++++++++++++++++++++++++++++++++- src/or/entrynodes.h | 9 +++++---- src/or/nodelist.c | 2 +- src/or/nodelist.h | 2 ++ src/test/test_entrynodes.c | 19 +++++++++++++++++-- 6 files changed, 70 insertions(+), 8 deletions(-)

1 0

[tor/master] Merge branch 'maint-0.3.1'
by nickm＠torproject.org 29 Jun '17

29 Jun '17

commit 71b9f4f0bb8fe62bbc5c8b92d5172a6d5ba0f068 Merge: 3781678 1712dc9 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jun 29 15:57:49 2017 -0400 Merge branch 'maint-0.3.1' changes/bug22753 | 7 +++++++ src/or/entrynodes.c | 39 ++++++++++++++++++++++++++++++++++++++- src/or/entrynodes.h | 9 +++++---- src/or/nodelist.c | 2 +- src/or/nodelist.h | 2 ++ src/test/test_entrynodes.c | 19 +++++++++++++++++-- 6 files changed, 70 insertions(+), 8 deletions(-)

1 0

[tor/release-0.3.0] Merge branch 'trove-2017-006' into maint-0.3.0
by nickm＠torproject.org 29 Jun '17

29 Jun '17

commit 52c4440c4895cb90dd155bb094b49a97128baedf Merge: 4c21d4e 88666d0 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jun 29 15:57:42 2017 -0400 Merge branch 'trove-2017-006' into maint-0.3.0 changes/bug22753 | 7 +++++++ src/or/entrynodes.c | 39 ++++++++++++++++++++++++++++++++++++++- src/or/entrynodes.h | 9 +++++---- src/or/nodelist.c | 2 +- src/or/nodelist.h | 2 ++ src/test/test_entrynodes.c | 19 +++++++++++++++++-- 6 files changed, 70 insertions(+), 8 deletions(-)

1 0

[tor/release-0.3.1] Adjust unit tests to account for fix to bug 22753.
by nickm＠torproject.org 29 Jun '17

29 Jun '17

commit 88666d0482c235f407be1edf6e89da8c9142b92d Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jun 29 10:09:06 2017 -0400 Adjust unit tests to account for fix to bug 22753. Our mock network put all the guards on the same IPv4 address, which doesn't fly when we start applying EnforceDistinctSubnets. So in this commit, I disable EnforceDistinctSubnets when running the old guard_restriction_t test. This commit also adds a regression test for #22753. --- src/test/test_entrynodes.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/test/test_entrynodes.c b/src/test/test_entrynodes.c index 95c8f6c..e215c60 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); }

1 0

[tor/release-0.3.1] Merge branch 'maint-0.3.0' into maint-0.3.1
by nickm＠torproject.org 29 Jun '17

29 Jun '17

commit 1712dc98b04ef7b84df67e02da21ea1aa70bc74c Merge: 31a08ba 52c4440 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jun 29 15:57:48 2017 -0400 Merge branch 'maint-0.3.0' into maint-0.3.1 changes/bug22753 | 7 +++++++ src/or/entrynodes.c | 39 ++++++++++++++++++++++++++++++++++++++- src/or/entrynodes.h | 9 +++++---- src/or/nodelist.c | 2 +- src/or/nodelist.h | 2 ++ src/test/test_entrynodes.c | 19 +++++++++++++++++-- 6 files changed, 70 insertions(+), 8 deletions(-)

1 0

[tor/release-0.3.1] Merge branch 'maint-0.3.1' into release-0.3.1
by nickm＠torproject.org 29 Jun '17

29 Jun '17

commit b2ae5fc96bf82501f100c882e433100439346d63 Merge: 62c87f8 1712dc9 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jun 29 15:57:48 2017 -0400 Merge branch 'maint-0.3.1' into release-0.3.1 changes/bug22753 | 7 +++++++ src/or/entrynodes.c | 39 ++++++++++++++++++++++++++++++++++++++- src/or/entrynodes.h | 9 +++++---- src/or/nodelist.c | 2 +- src/or/nodelist.h | 2 ++ src/test/test_entrynodes.c | 19 +++++++++++++++++-- 6 files changed, 70 insertions(+), 8 deletions(-)

1 0

[tor/release-0.3.0] Merge branch 'maint-0.3.0' into release-0.3.0
by nickm＠torproject.org 29 Jun '17

29 Jun '17

commit 7444f2daf97dc91ebb74eed90053bdddc99ec1c1 Merge: 50cc49d 52c4440 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jun 29 15:57:48 2017 -0400 Merge branch 'maint-0.3.0' into release-0.3.0 changes/bug22753 | 7 +++++++ src/or/entrynodes.c | 39 ++++++++++++++++++++++++++++++++++++++- src/or/entrynodes.h | 9 +++++---- src/or/nodelist.c | 2 +- src/or/nodelist.h | 2 ++ src/test/test_entrynodes.c | 19 +++++++++++++++++-- 6 files changed, 70 insertions(+), 8 deletions(-)

1 0

[tor/release-0.3.1] Merge branch 'trove-2017-006' into maint-0.3.0
by nickm＠torproject.org 29 Jun '17

29 Jun '17

commit 52c4440c4895cb90dd155bb094b49a97128baedf Merge: 4c21d4e 88666d0 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jun 29 15:57:42 2017 -0400 Merge branch 'trove-2017-006' into maint-0.3.0 changes/bug22753 | 7 +++++++ src/or/entrynodes.c | 39 ++++++++++++++++++++++++++++++++++++++- src/or/entrynodes.h | 9 +++++---- src/or/nodelist.c | 2 +- src/or/nodelist.h | 2 ++ src/test/test_entrynodes.c | 19 +++++++++++++++++-- 6 files changed, 70 insertions(+), 8 deletions(-)

1 0

[tor/release-0.3.0] Adjust unit tests to account for fix to bug 22753.
by nickm＠torproject.org 29 Jun '17

29 Jun '17

commit 88666d0482c235f407be1edf6e89da8c9142b92d Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jun 29 10:09:06 2017 -0400 Adjust unit tests to account for fix to bug 22753. Our mock network put all the guards on the same IPv4 address, which doesn't fly when we start applying EnforceDistinctSubnets. So in this commit, I disable EnforceDistinctSubnets when running the old guard_restriction_t test. This commit also adds a regression test for #22753. --- src/test/test_entrynodes.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/test/test_entrynodes.c b/src/test/test_entrynodes.c index 95c8f6c..e215c60 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); }

1 0

[tor/release-0.3.1] Consider the exit family when applying guard restrictions.
by nickm＠torproject.org 29 Jun '17

29 Jun '17

commit 665baf5ed5c6186d973c46cdea165c0548027350 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Jun 28 11:41:50 2017 -0400 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. --- changes/bug22753 | 7 +++++++ src/or/entrynodes.c | 39 ++++++++++++++++++++++++++++++++++++++- src/or/entrynodes.h | 9 +++++---- src/or/nodelist.c | 2 +- src/or/nodelist.h | 2 ++ 5 files changed, 53 insertions(+), 6 deletions(-) diff --git a/changes/bug22753 b/changes/bug22753 new file mode 100644 index 0000000..32a6dfa --- /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 aba35e6..ccb0808 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 f02901f..6ccc48f 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 96e95ba..2ca52e7 100644 --- a/src/or/nodelist.c +++ b/src/or/nodelist.c @@ -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) { diff --git a/src/or/nodelist.h b/src/or/nodelist.h index 8456d21..4e5301d 100644 --- a/src/or/nodelist.h +++ b/src/or/nodelist.h @@ -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.

1 0