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

[tor/release-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/release-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/release-0.2.9] Merge branch 'maint-0.2.6' into maint-0.2.7
by nickm＠torproject.org 15 Feb '17

15 Feb '17

commit ed806843dc8e7942a2a0abfbb2ae374e11963feb Merge: 75fe218 3781f24 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Feb 15 07:47:21 2017 -0500 Merge branch 'maint-0.2.6' into maint-0.2.7 changes/bug20384 | 10 ++++++++++ changes/trove-2017-001.2 | 8 ++++++++ src/or/routerparse.c | 49 +++++++++++++++++++++++++++++------------------- 3 files changed, 48 insertions(+), 19 deletions(-)

1 0

[tor/release-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 4a2afd5b33f02ed3e5eb591dd29537fa4f69399f Merge: a86f95d 9ff7618 Author: Roger Dingledine <arma(a)torproject.org> Date: Mon Feb 13 15:41:20 2017 -0500 Merge branch 'maint-0.2.8' into maint-0.2.9

1 0

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

15 Feb '17

commit e778a411b9e75ad2c1b46a6624d586a6f4a0c554 Merge: 43c18b1 144ec3d Author: Roger Dingledine <arma(a)torproject.org> Date: Mon Feb 13 15:27:57 2017 -0500 Merge branch 'maint-0.2.5' into maint-0.2.6 changes/bug20384 | 10 ++++++++++ 1 file changed, 10 insertions(+)

1 0

[tor/release-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/release-0.2.9] Prevent int underflow in dirvote.c compare_vote_rs_.
by nickm＠torproject.org 15 Feb '17

15 Feb '17

commit a0ef3cf0880e3cd343977b3fcbd0a2e7572f0cb4 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Feb 13 14:07:35 2017 -0500 Prevent int underflow in dirvote.c compare_vote_rs_. This should be "impossible" without making a SHA1 collision, but let's not keep the assumption that SHA1 collisions are super-hard. This prevents another case related to 21278. There should be no behavioral change unless -ftrapv is on. --- src/or/dirvote.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/src/or/dirvote.c b/src/or/dirvote.c index 2c10e78..738ab35 100644 --- a/src/or/dirvote.c +++ b/src/or/dirvote.c @@ -421,16 +421,30 @@ compare_vote_rs(const vote_routerstatus_t *a, const vote_routerstatus_t *b) b->status.descriptor_digest, DIGEST_LEN))) return r; - if ((r = (int)(b->status.published_on - a->status.published_on))) - return r; + /* If we actually reached this point, then the identities and + * the descriptor digests matched, so somebody is making SHA1 collisions. + */ +#define CMP_FIELD(utype, itype, field) do { \ + utype aval = (utype) (itype) a->status.field; \ + utype bval = (utype) (itype) b->status.field; \ + utype u = bval - aval; \ + itype r2 = (itype) u; \ + if (r2 < 0) { \ + return -1; \ + } else if (r2 > 0) { \ + return 1; \ + } \ + } while (0) + + CMP_FIELD(uint64_t, int64_t, published_on); + if ((r = strcmp(b->status.nickname, a->status.nickname))) return r; - if ((r = (((int)b->status.addr) - ((int)a->status.addr)))) - return r; - if ((r = (((int)b->status.or_port) - ((int)a->status.or_port)))) - return r; - if ((r = (((int)b->status.dir_port) - ((int)a->status.dir_port)))) - return r; + + CMP_FIELD(unsigned, int, addr); + CMP_FIELD(unsigned, int, or_port); + CMP_FIELD(unsigned, int, dir_port); + return 0; }

1 0

[tor/release-0.2.9] When examining descriptors as a dirserver, reject ones with bad versions
by nickm＠torproject.org 15 Feb '17

15 Feb '17

commit 02e05bd74dbec614397b696cfcda6525562a4675 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Feb 13 11:18:04 2017 -0500 When examining descriptors as a dirserver, reject ones with bad versions This is an extra fix for bug 21278: it ensures that these descriptors and platforms will never be listed in a legit consensus. --- changes/bug21278_prevention | 4 ++++ src/or/dirserv.c | 10 ++++++++++ 2 files changed, 14 insertions(+) diff --git a/changes/bug21278_prevention b/changes/bug21278_prevention new file mode 100644 index 0000000..e07f0a6 --- /dev/null +++ b/changes/bug21278_prevention @@ -0,0 +1,4 @@ + o Minor features (directory authority): + - Directory authorities now reject descriptors that claim to be + malformed versions of Tor. Helps prevent exploitation of bug 21278. + diff --git a/src/or/dirserv.c b/src/or/dirserv.c index 1b614b9..fa3938b 100644 --- a/src/or/dirserv.c +++ b/src/or/dirserv.c @@ -365,6 +365,16 @@ dirserv_get_status_impl(const char *id_digest, const char *nickname, strmap_size(fingerprint_list->fp_by_name), digestmap_size(fingerprint_list->status_by_digest)); + if (platform) { + tor_version_t ver_tmp; + if (tor_version_parse_platform(platform, &ver_tmp, 1) < 0) { + if (msg) { + *msg = "Malformed platform string."; + } + return FP_REJECT; + } + } + /* Versions before Tor 0.2.4.18-rc are too old to support, and are * missing some important security fixes too. Disable them. */ if (platform && !tor_version_as_new_as(platform,"0.2.4.18-rc")) {

1 0

[tor/release-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/release-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