Merge branch 'maint-0.2.9'

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.

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.
+      

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.

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")) {

src/or/dirvote.c

@@ -454,16 +454,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;
 }
 

src/or/policies.c

@@ -1248,48 +1248,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. */
@@ -1305,7 +1305,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 <b>ent</b> */
@@ -1356,7 +1356,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;
 }

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,

src/or/routerlist.c

@@ -5423,7 +5423,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;

src/or/routerparse.c

@@ -4785,6 +4785,55 @@ microdescs_parse_from_string(const char *s, const char *eos,
   return result;
 }
 
+/** Extract a Tor version from a <b>platform</b> line from a router
+ * descriptor, and place the result in <b>router_version</b>.
+ *
+ * Return 1 on success, -1 on parsing failure, and 0 if the
+ * platform line does not indicate some version of Tor.
+ *
+ * If <b>strict</b> 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 <b>platform</b>,
  * and compare it to the version in <b>cutoff</b>. Return 1 if
  * the router is at least as new as the cutoff, else return 0.
@@ -4793,32 +4842,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:
@@ -4945,26 +4983,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 <b>a</b> and <b>b</b> belong to the same series.

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);

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));