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

[tor/maint-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/maint-0.2.9] Extract the part of tor_version_as_new_as that extracts platform
by nickm＠torproject.org 15 Feb '17

15 Feb '17

commit f63e06d3dc6757d08ecf26d418ba59bfe060de39 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Feb 13 11:17:16 2017 -0500 Extract the part of tor_version_as_new_as that extracts platform Also add a "strict" mode to reject negative inputs. --- src/or/routerparse.c | 60 ++++++++++++++++++++++++++++++++++++++++------------ src/or/routerparse.h | 3 +++ 2 files changed, 49 insertions(+), 14 deletions(-) diff --git a/src/or/routerparse.c b/src/or/routerparse.c index d7e4252..28b9c57 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -5517,35 +5517,67 @@ microdescs_parse_from_string(const char *s, const char *eos, * the router is at least as new as the cutoff, else return 0. */ int -tor_version_as_new_as(const char *platform, const char *cutoff) +tor_version_parse_platform(const char *platform, + tor_version_t *router_version, + int strict) { - tor_version_t cutoff_version, router_version; - char *s, *s2, *start; char tmp[128]; + char *s, *s2, *start; - tor_assert(platform); - - if (tor_version_parse(cutoff, &cutoff_version)<0) { - log_warn(LD_BUG,"cutoff version '%s' unparseable.",cutoff); + if (strcmpstart(platform,"Tor ")) /* nonstandard Tor; say 0. */ return 0; - } - if (strcmpstart(platform,"Tor ")) /* nonstandard Tor; be safe and say yes */ - return 1; start = (char *)eat_whitespace(platform+3); - if (!*start) return 0; + if (!*start) return -1; s = (char *)find_whitespace(start); /* also finds '\0', which is fine */ s2 = (char*)eat_whitespace(s); if (!strcmpstart(s2, "(r") || !strcmpstart(s2, "(git-")) s = (char*)find_whitespace(s2); if ((size_t)(s-start+1) >= sizeof(tmp)) /* too big, no */ - return 0; + return -1; strlcpy(tmp, start, s-start+1); - if (tor_version_parse(tmp, &router_version)<0) { + if (tor_version_parse(tmp, router_version)<0) { log_info(LD_DIR,"Router version '%s' unparseable.",tmp); - return 1; /* be safe and say yes */ + return -1; + } + + if (strict) { + if (router_version->major < 0 || + router_version->minor < 0 || + router_version->minor < 0 || + router_version->patchlevel < 0 || + router_version->svn_revision < 0) { + return -1; + } + } + return 1; +} + +/** 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. + */ +int +tor_version_as_new_as(const char *platform, const char *cutoff) +{ + tor_version_t cutoff_version, router_version; + int r; + tor_assert(platform); + + if (tor_version_parse(cutoff, &cutoff_version)<0) { + log_warn(LD_BUG,"cutoff version '%s' unparseable.",cutoff); + return 0; + } + + r = tor_version_parse_platform(platform, &router_version, 0); + if (r == 0) { + /* nonstandard Tor; be safe and say yes */ + return 1; + } else if (r < 0) { + /* unparseable version; be safe and say yes. */ + return 1; } /* Here's why we don't need to do any special handling for svn revisions: diff --git a/src/or/routerparse.h b/src/or/routerparse.h index 9a3fadc..01a5de8 100644 --- a/src/or/routerparse.h +++ b/src/or/routerparse.h @@ -45,6 +45,9 @@ MOCK_DECL(addr_policy_t *, router_parse_addr_policy_item_from_string, (const char *s, int assume_action, int *malformed_list)); version_status_t tor_version_is_obsolete(const char *myversion, const char *versionlist); +int tor_version_parse_platform(const char *platform, + tor_version_t *version_out, + int strict); int tor_version_as_new_as(const char *platform, const char *cutoff); int tor_version_parse(const char *s, tor_version_t *out); int tor_version_compare(tor_version_t *a, tor_version_t *b);

1 0

[tor/maint-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 3781f24b8014213c200f74973deb550382957a01 Merge: 43c18b1 a452b71 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Feb 15 07:47:12 2017 -0500 Merge branch 'maint-0.2.5' into maint-0.2.6 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] 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/maint-0.2.4] 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] 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/master] 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

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

15 Feb '17

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

1 0