From 635c5a8a92c8066412645b291817eadcc82d8f8f Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Mon, 13 Feb 2017 15:22:36 -0500 Subject: [PATCH 1/8] be sure to remember the changes file for #20384 --- changes/bug20384 | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 changes/bug20384 diff --git a/changes/bug20384 b/changes/bug20384 new file mode 100644 index 000000000..591015ad9 --- /dev/null +++ b/changes/bug20384 @@ -0,0 +1,10 @@ + o Major features (security fixes): + - Prevent a class of security bugs caused by treating the contents + of a buffer chunk as if they were a NUL-terminated string. At + least one such bug seems to be present in all currently used + versions of Tor, and would allow an attacker to remotely crash + most Tor instances, especially those compiled with extra compiler + hardening. With this defense in place, such bugs can't crash Tor, + though we should still fix them as they occur. Closes ticket + 20384 (TROVE-2016-10-001). + From 194e31057fbf07d6bdf4b62d26e1a9db334e5f1c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 7 Feb 2017 10:58:02 -0500 Subject: [PATCH 2/8] 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(-) create mode 100644 changes/trove-2017-001.2 diff --git a/changes/trove-2017-001.2 b/changes/trove-2017-001.2 new file mode 100644 index 000000000..3ef073cf9 --- /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 71373ce63..1243035f9 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. From 1afc2ed956a35b40dfd1d207652af5b50c295da7 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 13 Feb 2017 13:53:30 -0500 Subject: [PATCH 3/8] 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 6037ee311..28770bb38 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 850fa863d..f73f850c2 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 b87679544..2365f28fd 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 f2d42b956..1ffdc2cd5 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)); From a0ef3cf0880e3cd343977b3fcbd0a2e7572f0cb4 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 13 Feb 2017 14:07:35 -0500 Subject: [PATCH 4/8] 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 2c10e784b..738ab35bc 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; } From 9f71fde146712d4fafbf3e967d560b18aed64794 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 14 Feb 2017 16:32:59 -0500 Subject: [PATCH 5/8] changes file for removing compare-by-subtraction pattern --- changes/bug21278_extras | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changes/bug21278_extras diff --git a/changes/bug21278_extras b/changes/bug21278_extras new file mode 100644 index 000000000..ffdf4a047 --- /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. From f63e06d3dc6757d08ecf26d418ba59bfe060de39 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 13 Feb 2017 11:17:16 -0500 Subject: [PATCH 6/8] 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 d7e42529c..28b9c57e4 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 9a3fadca1..01a5de88e 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); From 02e05bd74dbec614397b696cfcda6525562a4675 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 13 Feb 2017 11:18:04 -0500 Subject: [PATCH 7/8] 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(+) create mode 100644 changes/bug21278_prevention diff --git a/changes/bug21278_prevention b/changes/bug21278_prevention new file mode 100644 index 000000000..e07f0a670 --- /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 1b614b949..fa3938b5e 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")) { From 3c4da8a130e41b9096eef360c437cef3cd22a62c Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Tue, 14 Feb 2017 03:52:01 -0500 Subject: [PATCH 8/8] 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 28b9c57e4..c4691b58c 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; }