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

[tor/release-0.2.9] Merge branch 'bug21278_extra_029' into maint-0.2.9
by nickm＠torproject.org 15 Feb '17

15 Feb '17

commit 5d88267bf47fb532b53691ab428cbfd9db3881cf Merge: ec6b5a0 9f71fde Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Feb 15 07:48:30 2017 -0500 Merge branch 'bug21278_extra_029' into maint-0.2.9 changes/bug21278_extras | 3 +++ src/or/dirvote.c | 30 ++++++++++++++++++------- src/or/policies.c | 58 ++++++++++++++++++++++++------------------------- src/or/policies.h | 2 +- src/or/routerlist.c | 2 +- src/test/test_policy.c | 8 +++---- 6 files changed, 60 insertions(+), 43 deletions(-)

1 0

[tor/release-0.2.9] Merge branch 'bug21278_redux_029_squashed' into maint-0.2.9
by nickm＠torproject.org 15 Feb '17

15 Feb '17

commit ec6b5a098dcd1a264e84405006cd6a645d415ac4 Merge: eeb7435 3c4da8a Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Feb 15 07:48:18 2017 -0500 Merge branch 'bug21278_redux_029_squashed' into maint-0.2.9 changes/bug21278_prevention | 4 +++ src/or/dirserv.c | 10 +++++++ src/or/routerparse.c | 72 ++++++++++++++++++++++++++++++++++----------- src/or/routerparse.h | 3 ++ 4 files changed, 72 insertions(+), 17 deletions(-)

1 0

[tor/release-0.2.9] Merge branch 'maint-0.2.9' into release-0.2.9
by nickm＠torproject.org 15 Feb '17

15 Feb '17

commit 563b8d9f221c935ab5e4095729ea1fe0c905b7b3 Merge: 1166174 5d88267 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Feb 15 07:51:01 2017 -0500 Merge branch 'maint-0.2.9' into release-0.2.9 changes/bug21278_extras | 3 ++ changes/bug21278_prevention | 4 ++ changes/trove-2017-001.2 | 8 +++ src/or/dirserv.c | 10 ++++ src/or/dirvote.c | 30 ++++++++--- src/or/policies.c | 58 ++++++++++----------- src/or/policies.h | 2 +- src/or/routerlist.c | 2 +- src/or/routerparse.c | 121 +++++++++++++++++++++++++++++++------------- src/or/routerparse.h | 3 ++ src/test/test_policy.c | 8 +-- 11 files changed, 170 insertions(+), 79 deletions(-)

1 0

[tor/maint-0.2.9] Merge branch 'maint-0.2.8' into maint-0.2.9
by nickm＠torproject.org 15 Feb '17

15 Feb '17

commit eeb743588af0ec09150bd7601d553bfbabba97ac Merge: 4a2afd5 1ebdae6 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Feb 15 07:48:10 2017 -0500 Merge branch 'maint-0.2.8' into maint-0.2.9 changes/trove-2017-001.2 | 8 ++++++++ src/or/routerparse.c | 49 +++++++++++++++++++++++++++++------------------- 2 files changed, 38 insertions(+), 19 deletions(-)

1 0

[tor/maint-0.2.9] changes file for removing compare-by-subtraction pattern
by nickm＠torproject.org 15 Feb '17

15 Feb '17

commit 9f71fde146712d4fafbf3e967d560b18aed64794 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Feb 14 16:32:59 2017 -0500 changes file for removing compare-by-subtraction pattern --- changes/bug21278_extras | 3 +++ 1 file changed, 3 insertions(+) diff --git a/changes/bug21278_extras b/changes/bug21278_extras new file mode 100644 index 0000000..ffdf4a0 --- /dev/null +++ b/changes/bug21278_extras @@ -0,0 +1,3 @@ + o Minor bugfixes (code correctness): + - Repair a couple of (unreachable or harmless) cases of the risky + comparison-by-subtraction pattern that caused bug 21278.

1 0

[tor/maint-0.2.9] give tor_version_parse_platform some function documentation
by nickm＠torproject.org 15 Feb '17

15 Feb '17

commit 3c4da8a130e41b9096eef360c437cef3cd22a62c Author: Roger Dingledine <arma(a)torproject.org> Date: Tue Feb 14 03:52:01 2017 -0500 give tor_version_parse_platform some function documentation --- src/or/routerparse.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/or/routerparse.c b/src/or/routerparse.c index 28b9c57..c4691b5 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -5512,9 +5512,14 @@ microdescs_parse_from_string(const char *s, const char *eos, return result; } -/** Parse the Tor version of the platform string platform, - * and compare it to the version in cutoff. Return 1 if - * the router is at least as new as the cutoff, else return 0. +/** Extract a Tor version from a platform line from a router + * descriptor, and place the result in router_version. + * + * Return 1 on success, -1 on parsing failure, and 0 if the + * platform line does not indicate some version of Tor. + * + * If strict is non-zero, finding any weird version components + * (like negative numbers) counts as a parsing failure. */ int tor_version_parse_platform(const char *platform, @@ -5552,6 +5557,7 @@ tor_version_parse_platform(const char *platform, return -1; } } + return 1; }

1 0

[tor/maint-0.2.9] Avoid integer underflow in tor_version_compare.
by nickm＠torproject.org 15 Feb '17

15 Feb '17

commit 194e31057fbf07d6bdf4b62d26e1a9db334e5f1c Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Feb 7 10:58:02 2017 -0500 Avoid integer underflow in tor_version_compare. Fix for TROVE-2017-001 and bug 21278. (Note: Instead of handling signed ints "correctly", we keep the old behavior, except for the part where we would crash with -ftrapv.) --- changes/trove-2017-001.2 | 8 ++++++++ src/or/routerparse.c | 49 +++++++++++++++++++++++++++++------------------- 2 files changed, 38 insertions(+), 19 deletions(-) diff --git a/changes/trove-2017-001.2 b/changes/trove-2017-001.2 new file mode 100644 index 0000000..3ef073c --- /dev/null +++ b/changes/trove-2017-001.2 @@ -0,0 +1,8 @@ + o Major bugfixes (parsing): + - Fix an integer underflow bug when comparing malformed Tor versions. + This bug is harmless, except when Tor has been built with + --enable-expensive-hardening, which would turn it into a crash; + or on Tor 0.2.9.1-alpha through Tor 0.2.9.8, which were built with + -ftrapv by default. + Part of TROVE-2017-001. Fixes bug 21278; bugfix on + 0.0.8pre1. Found by OSS-Fuzz. diff --git a/src/or/routerparse.c b/src/or/routerparse.c index 71373ce..1243035 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -4545,26 +4545,37 @@ tor_version_compare(tor_version_t *a, tor_version_t *b) int i; tor_assert(a); tor_assert(b); - if ((i = a->major - b->major)) - return i; - else if ((i = a->minor - b->minor)) - return i; - else if ((i = a->micro - b->micro)) - return i; - else if ((i = a->status - b->status)) - return i; - else if ((i = a->patchlevel - b->patchlevel)) - return i; - else if ((i = strcmp(a->status_tag, b->status_tag))) - return i; - else if ((i = a->svn_revision - b->svn_revision)) - return i; - else if ((i = a->git_tag_len - b->git_tag_len)) - return i; - else if (a->git_tag_len) - return fast_memcmp(a->git_tag, b->git_tag, a->git_tag_len); + + /* We take this approach to comparison to ensure the same (bogus!) behavior + * on all inputs as we would have seen before bug #21278 was fixed. The + * only important difference here is that this method doesn't cause + * a signed integer underflow. + */ +#define CMP(field) do { \ + unsigned aval = (unsigned) a->field; \ + unsigned bval = (unsigned) b->field; \ + int result = (int) (aval - bval); \ + if (result < 0) \ + return -1; \ + else if (result > 0) \ + return 1; \ + } while (0) + + CMP(major); + CMP(minor); + CMP(micro); + CMP(status); + CMP(patchlevel); + if ((i = strcmp(a->status_tag, b->status_tag))) + return i; + CMP(svn_revision); + CMP(git_tag_len); + if (a->git_tag_len) + return fast_memcmp(a->git_tag, b->git_tag, a->git_tag_len); else - return 0; + return 0; + +#undef CMP } /** Return true iff versions a and b belong to the same series.

1 0

[tor/maint-0.2.9] Merge branch 'maint-0.2.4' into maint-0.2.5
by nickm＠torproject.org 15 Feb '17

15 Feb '17

commit a452b713955422e217f7056cf50cb3591830dee9 Merge: 124062e 194e310 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Feb 15 07:47:04 2017 -0500 Merge branch 'maint-0.2.4' into maint-0.2.5 changes/bug20384 | 10 ++++++++++ changes/trove-2017-001.2 | 8 ++++++++ src/or/routerparse.c | 49 +++++++++++++++++++++++++++++------------------- 3 files changed, 48 insertions(+), 19 deletions(-)

1 0

[tor/maint-0.2.9] Merge branch 'maint-0.2.7' into maint-0.2.8
by nickm＠torproject.org 15 Feb '17

15 Feb '17

commit 1ebdae61711ec03ce49af2f356756b5db9587412 Merge: 9b90d51 ed80684 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Feb 15 07:47:28 2017 -0500 Merge branch 'maint-0.2.7' into maint-0.2.8 changes/bug20384 | 10 ++++++++++ changes/trove-2017-001.2 | 8 ++++++++ src/or/routerparse.c | 49 +++++++++++++++++++++++++++++------------------- 3 files changed, 48 insertions(+), 19 deletions(-)

1 0

[tor/maint-0.2.9] Fix policies.c instance of the "if (r=(a-b)) return r" pattern
by nickm＠torproject.org 15 Feb '17

15 Feb '17

commit 1afc2ed956a35b40dfd1d207652af5b50c295da7 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Feb 13 13:53:30 2017 -0500 Fix policies.c instance of the "if (r=(a-b)) return r" pattern I think this one probably can't underflow, since the input ranges are small. But let's not tempt fate. This patch also replaces the "cmp" functions here with just "eq" functions, since nothing actually checked for anything besides 0 and nonzero. Related to 21278. --- src/or/policies.c | 58 +++++++++++++++++++++++++------------------------- src/or/policies.h | 2 +- src/or/routerlist.c | 2 +- src/test/test_policy.c | 8 +++---- 4 files changed, 35 insertions(+), 35 deletions(-) diff --git a/src/or/policies.c b/src/or/policies.c index 6037ee3..28770bb 100644 --- a/src/or/policies.c +++ b/src/or/policies.c @@ -1198,48 +1198,48 @@ policies_parse_from_options(const or_options_t *options) return ret; } -/** Compare two provided address policy items, and return -1, 0, or 1 +/** Compare two provided address policy items, and renturn -1, 0, or 1 * if the first is less than, equal to, or greater than the second. */ static int -cmp_single_addr_policy(addr_policy_t *a, addr_policy_t *b) +single_addr_policy_eq(const addr_policy_t *a, const addr_policy_t *b) { int r; - if ((r=((int)a->policy_type - (int)b->policy_type))) - return r; - if ((r=((int)a->is_private - (int)b->is_private))) - return r; +#define CMP_FIELD(field) do { \ + if (a->field != b->field) { \ + return 0; \ + } \ + } while (0) + CMP_FIELD(policy_type); + CMP_FIELD(is_private); /* refcnt and is_canonical are irrelevant to equality, * they are hash table implementation details */ if ((r=tor_addr_compare(&a->addr, &b->addr, CMP_EXACT))) - return r; - if ((r=((int)a->maskbits - (int)b->maskbits))) - return r; - if ((r=((int)a->prt_min - (int)b->prt_min))) - return r; - if ((r=((int)a->prt_max - (int)b->prt_max))) - return r; - return 0; + return 0; + CMP_FIELD(maskbits); + CMP_FIELD(prt_min); + CMP_FIELD(prt_max); +#undef CMP_FIELD + return 1; } -/** Like cmp_single_addr_policy() above, but looks at the - * whole set of policies in each case. */ +/** As single_addr_policy_eq, but compare every element of two policies. + */ int -cmp_addr_policies(smartlist_t *a, smartlist_t *b) +addr_policies_eq(const smartlist_t *a, const smartlist_t *b) { - int r, i; + int i; int len_a = a ? smartlist_len(a) : 0; int len_b = b ? smartlist_len(b) : 0; - for (i = 0; i < len_a && i < len_b; ++i) { - if ((r = cmp_single_addr_policy(smartlist_get(a, i), smartlist_get(b, i)))) - return r; - } - if (i == len_a && i == len_b) + if (len_a != len_b) return 0; - if (i < len_a) - return -1; - else - return 1; + + for (i = 0; i < len_a; ++i) { + if (! single_addr_policy_eq(smartlist_get(a, i), smartlist_get(b, i))) + return 0; + } + + return 1; } /** Node in hashtable used to store address policy entries. */ @@ -1255,7 +1255,7 @@ static HT_HEAD(policy_map, policy_map_ent_t) policy_root = HT_INITIALIZER(); static inline int policy_eq(policy_map_ent_t *a, policy_map_ent_t *b) { - return cmp_single_addr_policy(a->policy, b->policy) == 0; + return single_addr_policy_eq(a->policy, b->policy); } /** Return a hashcode for ent */ @@ -1306,7 +1306,7 @@ addr_policy_get_canonical_entry(addr_policy_t *e) HT_INSERT(policy_map, &policy_root, found); } - tor_assert(!cmp_single_addr_policy(found->policy, e)); + tor_assert(single_addr_policy_eq(found->policy, e)); ++found->policy->refcnt; return found->policy; } diff --git a/src/or/policies.h b/src/or/policies.h index 850fa86..f73f850 100644 --- a/src/or/policies.h +++ b/src/or/policies.h @@ -76,7 +76,7 @@ void policy_expand_unspec(smartlist_t **policy); int policies_parse_from_options(const or_options_t *options); addr_policy_t *addr_policy_get_canonical_entry(addr_policy_t *ent); -int cmp_addr_policies(smartlist_t *a, smartlist_t *b); +int addr_policies_eq(const smartlist_t *a, const smartlist_t *b); MOCK_DECL(addr_policy_result_t, compare_tor_addr_to_addr_policy, (const tor_addr_t *addr, uint16_t port, const smartlist_t *policy)); addr_policy_result_t compare_tor_addr_to_node_policy(const tor_addr_t *addr, diff --git a/src/or/routerlist.c b/src/or/routerlist.c index b876795..2365f28 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -5445,7 +5445,7 @@ router_differences_are_cosmetic(const routerinfo_t *r1, const routerinfo_t *r2) (r1->contact_info && r2->contact_info && strcasecmp(r1->contact_info, r2->contact_info)) || r1->is_hibernating != r2->is_hibernating || - cmp_addr_policies(r1->exit_policy, r2->exit_policy) || + ! addr_policies_eq(r1->exit_policy, r2->exit_policy) || (r1->supports_tunnelled_dir_requests != r2->supports_tunnelled_dir_requests)) return 0; diff --git a/src/test/test_policy.c b/src/test/test_policy.c index f2d42b9..1ffdc2c 100644 --- a/src/test/test_policy.c +++ b/src/test/test_policy.c @@ -304,10 +304,10 @@ test_policies_general(void *arg) tt_assert(!exit_policy_is_general_exit(policy10)); tt_assert(!exit_policy_is_general_exit(policy11)); - tt_assert(cmp_addr_policies(policy, policy2)); - tt_assert(cmp_addr_policies(policy, NULL)); - tt_assert(!cmp_addr_policies(policy2, policy2)); - tt_assert(!cmp_addr_policies(NULL, NULL)); + tt_assert(!addr_policies_eq(policy, policy2)); + tt_assert(!addr_policies_eq(policy, NULL)); + tt_assert(addr_policies_eq(policy2, policy2)); + tt_assert(addr_policies_eq(NULL, NULL)); tt_assert(!policy_is_reject_star(policy2, AF_INET, 1)); tt_assert(policy_is_reject_star(policy, AF_INET, 1));

1 0