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

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

15 Feb '17

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

1 0

[tor/master] 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/master] 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/master] 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/master] 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/master] 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