August 2012 - tor-commits - lists.torproject.org

[tor/maint-0.2.2] Try to clarify impact of bug 6537
by nickm＠torproject.org 03 Aug '12

03 Aug '12

commit d48cebc5e498b0ae673635f40fc57cdddab45d5b Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Aug 3 11:54:11 2012 -0400 Try to clarify impact of bug 6537 I don't personally agree that this is likely to be easy to exploit, and some initial experimention I've done suggests that cache-miss times are just plain too fast to get useful info out of when they're mixed up with the rest of Tor's timing noise. Nevertheless, I'm leaving Robert's initial changelog entry in the git history so that he can be the voice of reason if I'm wrong. :) --- changes/pathsel-BUGGY-a | 16 +++++++++------- 1 files changed, 9 insertions(+), 7 deletions(-) diff --git a/changes/pathsel-BUGGY-a b/changes/pathsel-BUGGY-a index cad2af5..2e642c7 100644 --- a/changes/pathsel-BUGGY-a +++ b/changes/pathsel-BUGGY-a @@ -3,10 +3,12 @@ - Try to leak less information about what relays a client is choosing to a side-channel attacker. Previously, a Tor client would stop iterating through the list of available relays as - soon as it had chosen one, thus leaking information about which - relays it picked for a circuit to a timing attack. (Tor is - likely to still leak information about which relays it has - chosen for a circuit to other processes on the same computer, - through e.g. which cache lines it loads while building the - circuit.) - + soon as it had chosen one, thus finishing a little earlier + when it picked a router earlier in the list. If an attacker + can recover this timing information (nontrivial but not + proven to be impossible), they could learn some coarse- + grained information about which relays a client was picking + (middle nodes in particular are likelier to be affected than + exits). The timing attack might be mitigated by other factors + (see bug #6537 for some discussion), but it's best not to + take chances. Fixes bug 6537; bugfix on 0.0.8rc1.

1 0

[tor/maint-0.2.2] Mitigate a side-channel leak of which relays Tor chooses for a circuit
by nickm＠torproject.org 03 Aug '12

03 Aug '12

commit 308f6dad20675c42b29862f4269ad1fbfb00dc9a Author: Robert Ransom <rransom.8774(a)gmail.com> Date: Thu Jun 14 17:15:54 2012 +0000 Mitigate a side-channel leak of which relays Tor chooses for a circuit Tor's and OpenSSL's current design guarantee that there are other leaks, but this one is likely to be more easily exploitable, and is easy to fix. --- changes/pathsel-BUGGY-a | 12 ++++++++++++ src/or/routerlist.c | 22 ++++++++++++++++++---- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/changes/pathsel-BUGGY-a b/changes/pathsel-BUGGY-a new file mode 100644 index 0000000..cad2af5 --- /dev/null +++ b/changes/pathsel-BUGGY-a @@ -0,0 +1,12 @@ + o Security fixes: + + - Try to leak less information about what relays a client is + choosing to a side-channel attacker. Previously, a Tor client + would stop iterating through the list of available relays as + soon as it had chosen one, thus leaking information about which + relays it picked for a circuit to a timing attack. (Tor is + likely to still leak information about which relays it has + chosen for a circuit to other processes on the same computer, + through e.g. which cache lines it loads while building the + circuit.) + diff --git a/src/or/routerlist.c b/src/or/routerlist.c index d21b40c..30c20bf 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -1674,6 +1674,8 @@ smartlist_choose_by_bandwidth_weights(smartlist_t *sl, double *bandwidths; double tmp = 0; unsigned int i; + unsigned int i_chosen; + unsigned int i_has_been_chosen; int have_unknown = 0; /* true iff sl contains element not in consensus. */ /* Can't choose exit and guard at same time */ @@ -1835,12 +1837,17 @@ smartlist_choose_by_bandwidth_weights(smartlist_t *sl, * from 1 below. See bug 1203 for details. */ /* Last, count through sl until we get to the element we picked */ + i_chosen = (unsigned)smartlist_len(sl); + i_has_been_chosen = 0; tmp = 0.0; for (i=0; i < (unsigned)smartlist_len(sl); i++) { tmp += bandwidths[i]; - if (tmp >= rand_bw) - break; + if (tmp >= rand_bw && !i_has_been_chosen) { + i_chosen = i; + i_has_been_chosen = 1; + } } + i = i_chosen; if (i == (unsigned)smartlist_len(sl)) { /* This was once possible due to round-off error, but shouldn't be able @@ -1877,6 +1884,8 @@ smartlist_choose_by_bandwidth(smartlist_t *sl, bandwidth_weight_rule_t rule, int statuses) { unsigned int i; + unsigned int i_chosen; + unsigned int i_has_been_chosen; routerinfo_t *router; routerstatus_t *status=NULL; int32_t *bandwidths; @@ -2092,6 +2101,8 @@ smartlist_choose_by_bandwidth(smartlist_t *sl, bandwidth_weight_rule_t rule, /* Last, count through sl until we get to the element we picked */ tmp = 0; + i_chosen = (unsigned)smartlist_len(sl); + i_has_been_chosen = 0; for (i=0; i < (unsigned)smartlist_len(sl); i++) { is_exit = bitarray_is_set(exit_bits, i); is_guard = bitarray_is_set(guard_bits, i); @@ -2106,9 +2117,12 @@ smartlist_choose_by_bandwidth(smartlist_t *sl, bandwidth_weight_rule_t rule, else tmp += bandwidths[i]; - if (tmp >= rand_bw) - break; + if (tmp >= rand_bw && !i_has_been_chosen) { + i_chosen = i; + i_has_been_chosen = 1; + } } + i = i_chosen; if (i == (unsigned)smartlist_len(sl)) { /* This was once possible due to round-off error, but shouldn't be able * to occur any longer. */

1 0

[tor/master] Remove bogus comment claiming that an assertion is triggerable by consensus
by nickm＠torproject.org 03 Aug '12

03 Aug '12

commit 82c5e385cbddec4fd80618d6e96111ad73d5a22e Author: Robert Ransom <rransom.8774(a)gmail.com> Date: Thu Jun 14 15:41:11 2012 +0000 Remove bogus comment claiming that an assertion is triggerable by consensus --- src/or/routerlist.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/or/routerlist.c b/src/or/routerlist.c index de1a66c..37de70f 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -1983,9 +1983,10 @@ smartlist_choose_node_by_bandwidth(smartlist_t *sl, if (is_guard) bitarray_set(guard_bits, i); if (is_known) { - bandwidths[i] = (int32_t) this_bw; // safe since MAX_BELIEVABLE<INT32_MAX - // XXX this is no longer true! We don't always cap the bw anymore. Can - // a consensus make us overflow?-sh + bandwidths[i] = (int32_t) this_bw; + /* Casting this_bw to int32_t is safe because both kb_to_bytes + and bridge_get_advertised_bandwidth_bounded limit it to below + INT32_MAX. */ tor_assert(bandwidths[i] >= 0); if (is_guard) total_guard_bw += this_bw;

1 0

[tor/master] Merge remote-tracking branch 'origin/maint-0.2.3'
by nickm＠torproject.org 03 Aug '12

03 Aug '12

commit 860c4fc81166568f2909708c455ca6a70d81a2f8 Merge: 6c64681 82c5e38 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Aug 3 11:46:03 2012 -0400 Merge remote-tracking branch 'origin/maint-0.2.3' src/or/routerlist.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)

1 0

[tor/maint-0.2.3] Remove bogus comment claiming that an assertion is triggerable by consensus
by nickm＠torproject.org 03 Aug '12

03 Aug '12

commit 82c5e385cbddec4fd80618d6e96111ad73d5a22e Author: Robert Ransom <rransom.8774(a)gmail.com> Date: Thu Jun 14 15:41:11 2012 +0000 Remove bogus comment claiming that an assertion is triggerable by consensus --- src/or/routerlist.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/or/routerlist.c b/src/or/routerlist.c index de1a66c..37de70f 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -1983,9 +1983,10 @@ smartlist_choose_node_by_bandwidth(smartlist_t *sl, if (is_guard) bitarray_set(guard_bits, i); if (is_known) { - bandwidths[i] = (int32_t) this_bw; // safe since MAX_BELIEVABLE<INT32_MAX - // XXX this is no longer true! We don't always cap the bw anymore. Can - // a consensus make us overflow?-sh + bandwidths[i] = (int32_t) this_bw; + /* Casting this_bw to int32_t is safe because both kb_to_bytes + and bridge_get_advertised_bandwidth_bounded limit it to below + INT32_MAX. */ tor_assert(bandwidths[i] >= 0); if (is_guard) total_guard_bw += this_bw;

1 0

[tor/master] Avoid possible segfault when handling networkstatus vote with bad flavor
by nickm＠torproject.org 03 Aug '12

03 Aug '12

commit 57e35ad3d91724882c345ac709666a551a977f0f Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Aug 3 10:53:00 2012 -0400 Avoid possible segfault when handling networkstatus vote with bad flavor Fix for 6530; fix on 0.2.2.6-alpha. --- changes/bug6530 | 4 ++++ src/or/routerparse.c | 2 +- 2 files changed, 5 insertions(+), 1 deletions(-) diff --git a/changes/bug6530 b/changes/bug6530 new file mode 100644 index 0000000..a6b7caa --- /dev/null +++ b/changes/bug6530 @@ -0,0 +1,4 @@ + o Major bugfixes: + - Avoid a read of uninitializd RAM when reading a vote or consensus + document with an unrecognized flavor name. Fixes bug 6530; bugfix on + 0.2.2.6-alpha. diff --git a/src/or/routerparse.c b/src/or/routerparse.c index 8c4f582..2ff546b 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -2821,7 +2821,7 @@ networkstatus_parse_vote_from_string(const char *s, const char **eos_out, int flavor = networkstatus_parse_flavor_name(tok->args[1]); if (flavor < 0) { log_warn(LD_DIR, "Can't parse document with unknown flavor %s", - escaped(tok->args[2])); + escaped(tok->args[1])); goto err; } ns->flavor = flav = flavor;

1 0

[tor/master] Clarify security impact of bug 6530
by nickm＠torproject.org 03 Aug '12

03 Aug '12

commit 55f635745afacefffdaafc72cc176ca7ab817546 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Aug 3 11:16:13 2012 -0400 Clarify security impact of bug 6530 --- changes/bug6530 | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-) diff --git a/changes/bug6530 b/changes/bug6530 index a6b7caa..825bbb7 100644 --- a/changes/bug6530 +++ b/changes/bug6530 @@ -1,4 +1,5 @@ - o Major bugfixes: + o Major security fixes: - Avoid a read of uninitializd RAM when reading a vote or consensus - document with an unrecognized flavor name. Fixes bug 6530; bugfix on - 0.2.2.6-alpha. + document with an unrecognized flavor name. This could lead to a + remote crash bug. Fixes bug 6530; bugfix on 0.2.2.6-alpha. +

1 0

[tor/master] Merge remote-tracking branch 'origin/maint-0.2.3'
by nickm＠torproject.org 03 Aug '12

03 Aug '12

commit babf8e2a859f56befa9a9f8da6697a954ce53f7c Merge: b50eb14 1040afb Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Aug 3 11:23:06 2012 -0400 Merge remote-tracking branch 'origin/maint-0.2.3' changes/bug6530 | 5 +++++ src/or/routerparse.c | 2 +- 2 files changed, 6 insertions(+), 1 deletions(-)

1 0

[tor/master] Merge remote-tracking branch 'origin/maint-0.2.2' into maint-0.2.3
by nickm＠torproject.org 03 Aug '12

03 Aug '12

commit 1040afb2425a8056dc41b35e6d825fc329663ee6 Merge: 122c8ef 55f6357 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Aug 3 11:18:40 2012 -0400 Merge remote-tracking branch 'origin/maint-0.2.2' into maint-0.2.3 changes/bug6530 | 5 +++++ src/or/routerparse.c | 2 +- 2 files changed, 6 insertions(+), 1 deletions(-)

1 0

[tor/master] Fix a bunch of "implicit 64->32" warnings from introduce refactoring
by nickm＠torproject.org 03 Aug '12

03 Aug '12

commit 6c64681879cca488c2cc017b99ed1e7104da5049 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Aug 3 11:31:04 2012 -0400 Fix a bunch of "implicit 64->32" warnings from introduce refactoring --- src/or/rendservice.c | 2 +- src/test/test_introduce.c | 2 +- src/test/test_replay.c | 22 +++++++++++----------- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 3c5d821..3d8d3e9 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -1206,7 +1206,7 @@ rend_service_introduce(origin_circuit_t *circuit, const uint8_t *request, /* check for replay of PK-encrypted portion. */ replay = replaycache_add_test_and_elapsed( intro_point->accepted_intro_rsa_parts, - parsed_req->ciphertext, parsed_req->ciphertext_len, + parsed_req->ciphertext, (int)parsed_req->ciphertext_len, &elapsed); if (replay) { diff --git a/src/test/test_introduce.c b/src/test/test_introduce.c index 4fbfd18..992d9cd 100644 --- a/src/test/test_introduce.c +++ b/src/test/test_introduce.c @@ -281,7 +281,7 @@ static void do_parse_test(uint8_t *plaintext, size_t plaintext_len, int phase) { crypto_pk_t *k = NULL; - int r; + ssize_t r; uint8_t *cell = NULL; size_t cell_len; rend_intro_cell_t *parsed_req = NULL; diff --git a/src/test/test_replay.c b/src/test/test_replay.c index 217b555..b08818f 100644 --- a/src/test/test_replay.c +++ b/src/test/test_replay.c @@ -44,7 +44,7 @@ test_replaycache_miss(void) result = replaycache_add_and_test_internal(1200, r, test_buffer, - strlen(test_buffer), NULL); + (int)strlen(test_buffer), NULL); test_eq(result, 0); done: @@ -65,12 +65,12 @@ test_replaycache_hit(void) result = replaycache_add_and_test_internal(1200, r, test_buffer, - strlen(test_buffer), NULL); + (int)strlen(test_buffer), NULL); test_eq(result, 0); result = replaycache_add_and_test_internal(1300, r, test_buffer, - strlen(test_buffer), NULL); + (int)strlen(test_buffer), NULL); test_eq(result, 1); done: @@ -91,17 +91,17 @@ test_replaycache_age(void) result = replaycache_add_and_test_internal(1200, r, test_buffer, - strlen(test_buffer), NULL); + (int)strlen(test_buffer), NULL); test_eq(result, 0); result = replaycache_add_and_test_internal(1300, r, test_buffer, - strlen(test_buffer), NULL); + (int)strlen(test_buffer), NULL); test_eq(result, 1); result = replaycache_add_and_test_internal(3000, r, test_buffer, - strlen(test_buffer), NULL); + (int)strlen(test_buffer), NULL); test_eq(result, 0); done: @@ -123,12 +123,12 @@ test_replaycache_elapsed(void) result = replaycache_add_and_test_internal(1200, r, test_buffer, - strlen(test_buffer), NULL); + (int)strlen(test_buffer), NULL); test_eq(result, 0); result = replaycache_add_and_test_internal(1300, r, test_buffer, - strlen(test_buffer), &elapsed); + (int)strlen(test_buffer), &elapsed); test_eq(result, 1); test_eq(elapsed, 100); @@ -150,17 +150,17 @@ test_replaycache_noexpire(void) result = replaycache_add_and_test_internal(1200, r, test_buffer, - strlen(test_buffer), NULL); + (int)strlen(test_buffer), NULL); test_eq(result, 0); result = replaycache_add_and_test_internal(1300, r, test_buffer, - strlen(test_buffer), NULL); + (int)strlen(test_buffer), NULL); test_eq(result, 1); result = replaycache_add_and_test_internal(3000, r, test_buffer, - strlen(test_buffer), NULL); + (int)strlen(test_buffer), NULL); test_eq(result, 1); done:

1 0