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.
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/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/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")) {
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;
}
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/or/routerparse.c b/src/or/routerparse.c
index d7e42529c..a896dde2b 100644
--- a/src/or/routerparse.c
+++ b/src/or/routerparse.c
@@ -5512,6 +5512,55 @@ microdescs_parse_from_string(const char *s, const char *eos,
return result;
}
+/** 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,
+ tor_version_t *router_version,
+ int strict)
+{
+ char tmp[128];
+ char *s, *s2, *start;
+
+ if (strcmpstart(platform,"Tor ")) /* nonstandard Tor; say 0. */
+ return 0;
+
+ start = (char *)eat_whitespace(platform+3);
+ 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 -1;
+ strlcpy(tmp, start, s-start+1);
+
+ if (tor_version_parse(tmp, router_version)<0) {
+ log_info(LD_DIR,"Router version '%s' unparseable.",tmp);
+ 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.
@@ -5520,32 +5569,21 @@ int
tor_version_as_new_as(const char *platform, const char *cutoff)
{
tor_version_t cutoff_version, router_version;
- char *s, *s2, *start;
- char tmp[128];
-
+ int r;
tor_assert(platform);
if (tor_version_parse(cutoff, &cutoff_version)<0) {
log_warn(LD_BUG,"cutoff version '%s' unparseable.",cutoff);
return 0;
}
- if (strcmpstart(platform,"Tor ")) /* nonstandard Tor; be safe and say yes */
+
+ 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;
-
- start = (char *)eat_whitespace(platform+3);
- if (!*start) return 0;
- 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;
- strlcpy(tmp, start, s-start+1);
-
- if (tor_version_parse(tmp, &router_version)<0) {
- log_info(LD_DIR,"Router version '%s' unparseable.",tmp);
- return 1; /* be safe and say yes */
}
/* Here's why we don't need to do any special handling for svn revisions:
@@ -5672,26 +5710,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.
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);
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));