Merge branch 'maint-0.3.1' into maint-0.3.2

6 files changed, 220 insertions, 6 deletions

diff --git a/changes/bug25249 b/changes/bug25249
new file mode 100644
index 0000000000..b4153eeaef
--- /dev/null
+++ b/changes/bug25249
@@ -0,0 +1,3 @@
+  o Minor bugfixes (spec conformance):
+    - Forbid "-0" as a protocol version. Fixes part of bug 25249; bugfix on
+      0.2.9.4-alpha.
diff --git a/changes/bug25249.2 b/changes/bug25249.2
new file mode 100644
index 0000000000..9058c11071
--- /dev/null
+++ b/changes/bug25249.2
@@ -0,0 +1,3 @@
+  o Minor bugfixes (spec conformance):
+    - Forbid UINT32_MAX as a protocol version.  Fixes part of bug 25249;
+      bugfix on 0.2.9.4-alpha.
diff --git a/changes/trove-2018-001.1 b/changes/trove-2018-001.1
new file mode 100644
index 0000000000..f0ee92f409
--- /dev/null
+++ b/changes/trove-2018-001.1
@@ -0,0 +1,6 @@
+  o Major bugfixes (denial-of-service, directory authority):
+    - Fix a protocol-list handling bug that could be used to remotely crash
+      directory authorities with a null-pointer exception. Fixes bug 25074;
+      bugfix on 0.2.9.4-alpha. Also tracked as TROVE-2018-001.
+
+
diff --git a/changes/trove-2018-004 b/changes/trove-2018-004
new file mode 100644
index 0000000000..37e0a89b0d
--- /dev/null
+++ b/changes/trove-2018-004
@@ -0,0 +1,8 @@
+  o Minor bugfixes (denial-of-service):
+    - Fix a possible crash on malformed consensus. If a consensus had
+      contained an unparseable protocol line, it could have made clients
+      and relays crash with a null-pointer exception. To exploit this
+      issue, however, an attacker would need to be able to subvert the
+      directory-authority system. Fixes bug 25251; bugfix on
+      0.2.9.4-alpha. Also tracked as TROVE-2018-004.
+
diff --git a/src/or/protover.c b/src/or/protover.c
index 033e9063ac..45f0377d61 100644
--- a/src/or/protover.c
+++ b/src/or/protover.c
@@ -103,6 +103,9 @@ proto_entry_free(proto_entry_t *entry)
   tor_free(entry);
 }
 
+/** The largest possible protocol version. */
+#define MAX_PROTOCOL_VERSION (UINT32_MAX-1)
+
 /**
  * Given a string <b>s</b> and optional end-of-string pointer
  * <b>end_of_range</b>, parse the protocol range and store it in
@@ -123,9 +126,14 @@ parse_version_range(const char *s, const char *end_of_range,
   if (BUG(!end_of_range))
     end_of_range = s + strlen(s); // LCOV_EXCL_LINE
 
+  /* A range must start with a digit. */
+  if (!TOR_ISDIGIT(*s)) {
+    goto error;
+  }
+
   /* Note that this wouldn't be safe if we didn't know that eventually,
    * we'd hit a NUL */
-  low = (uint32_t) tor_parse_ulong(s, 10, 0, UINT32_MAX, &ok, &next);
+  low = (uint32_t) tor_parse_ulong(s, 10, 0, MAX_PROTOCOL_VERSION, &ok, &next);
   if (!ok)
     goto error;
   if (next > end_of_range)
@@ -138,13 +146,21 @@ parse_version_range(const char *s, const char *end_of_range,
   if (*next != '-')
     goto error;
   s = next+1;
+
   /* ibid */
-  high = (uint32_t) tor_parse_ulong(s, 10, 0, UINT32_MAX, &ok, &next);
+  if (!TOR_ISDIGIT(*s)) {
+    goto error;
+  }
+  high = (uint32_t) tor_parse_ulong(s, 10, 0,
+                                    MAX_PROTOCOL_VERSION, &ok, &next);
   if (!ok)
     goto error;
   if (next != end_of_range)
     goto error;
 
+  if (low > high)
+    goto error;
+
  done:
   *high_out = high;
   *low_out = low;
@@ -195,10 +211,6 @@ parse_single_entry(const char *s, const char *end_of_entry)
       goto error;
     }
 
-    if (range->low > range->high) {
-      goto error;
-    }
-
     s = comma;
     while (*s == ',' && s < end_of_entry)
       ++s;
@@ -554,6 +566,12 @@ protover_compute_vote(const smartlist_t *list_of_proto_strings,
   // First, parse the inputs and break them into singleton entries.
   SMARTLIST_FOREACH_BEGIN(list_of_proto_strings, const char *, vote) {
     smartlist_t *unexpanded = parse_protocol_list(vote);
+    if (! unexpanded) {
+      log_warn(LD_NET, "I failed with parsing a protocol list from "
+               "an authority. The offending string was: %s",
+               escaped(vote));
+      continue;
+    }
     smartlist_t *this_vote = expand_protocol_list(unexpanded);
     if (this_vote == NULL) {
       log_warn(LD_NET, "When expanding a protocol list from an authority, I "
@@ -618,6 +636,11 @@ protover_all_supported(const char *s, char **missing_out)
   }
 
   smartlist_t *entries = parse_protocol_list(s);
+  if (BUG(entries == NULL)) {
+    log_warn(LD_NET, "Received an unparseable protocol list %s"
+             " from the consensus", escaped(s));
+    return 1;
+  }
 
   missing = smartlist_new();
 
diff --git a/src/test/test_protover.c b/src/test/test_protover.c
index 6ce54890d6..9b94044b91 100644
--- a/src/test/test_protover.c
+++ b/src/test/test_protover.c
@@ -139,6 +139,70 @@ test_protover_vote(void *arg)
   tt_str_op(result, OP_EQ, "Bar=3-6,8 Foo=9");
   tor_free(result);
 
+  /* High threshold */
+  result = protover_compute_vote(lst, 3);
+  tt_str_op(result, OP_EQ, "");
+  tor_free(result);
+
+  /* Bad votes: the result must be empty */
+  smartlist_clear(lst);
+  smartlist_add(lst, (void*) "Faux=10-5");
+  result = protover_compute_vote(lst, 1);
+  tt_str_op(result, OP_EQ, "");
+  tor_free(result);
+
+  /* This fails, since "-0" is not valid. */
+  smartlist_clear(lst);
+  smartlist_add(lst, (void*) "Faux=-0");
+  result = protover_compute_vote(lst, 1);
+  tt_str_op(result, OP_EQ, "");
+  tor_free(result);
+
+  /* Vote large protover lists that are just below the threshold */
+
+  /* Just below the threshold: Rust */
+  smartlist_clear(lst);
+  smartlist_add(lst, (void*) "Sleen=1-500");
+  result = protover_compute_vote(lst, 1);
+  tt_str_op(result, OP_EQ, "Sleen=1-500");
+  tor_free(result);
+
+  /* Just below the threshold: C */
+  smartlist_clear(lst);
+  smartlist_add(lst, (void*) "Sleen=1-65536");
+  result = protover_compute_vote(lst, 1);
+  tt_str_op(result, OP_EQ, "Sleen=1-65536");
+  tor_free(result);
+
+  /* Large protover lists that exceed the threshold */
+
+  /* By adding two votes, C allows us to exceed the limit */
+  smartlist_add(lst, (void*) "Sleen=1-65536");
+  smartlist_add(lst, (void*) "Sleen=100000");
+  result = protover_compute_vote(lst, 1);
+  tt_str_op(result, OP_EQ, "Sleen=1-65536,100000");
+  tor_free(result);
+
+  /* Large integers */
+  smartlist_clear(lst);
+  smartlist_add(lst, (void*) "Sleen=4294967294");
+  result = protover_compute_vote(lst, 1);
+  tt_str_op(result, OP_EQ, "Sleen=4294967294");
+  tor_free(result);
+
+  /* This parses, but fails at the vote stage */
+  smartlist_clear(lst);
+  smartlist_add(lst, (void*) "Sleen=4294967295");
+  result = protover_compute_vote(lst, 1);
+  tt_str_op(result, OP_EQ, "");
+  tor_free(result);
+
+  smartlist_clear(lst);
+  smartlist_add(lst, (void*) "Sleen=4294967296");
+  result = protover_compute_vote(lst, 1);
+  tt_str_op(result, OP_EQ, "");
+  tor_free(result);
+
  done:
   tor_free(result);
   smartlist_free(lst);
@@ -178,10 +242,116 @@ test_protover_all_supported(void *arg)
   tt_str_op(msg, OP_EQ, "Link=3-999");
   tor_free(msg);
 
+  /* CPU/RAM DoS loop: Rust only */
+  tt_assert(! protover_all_supported("Sleen=0-2147483648", &msg));
+  tt_str_op(msg, OP_EQ, "Sleen=0-2147483648");
+  tor_free(msg);
+
+  /* This case is allowed. */
+  tt_assert(! protover_all_supported("Sleen=0-4294967294", &msg));
+  tt_str_op(msg, OP_EQ, "Sleen=0-4294967294");
+  tor_free(msg);
+
+  /* If we get an unparseable list, we say "yes, that's supported." */
+#ifndef HAVE_RUST
+  // XXXX let's make this section unconditional: rust should behave the
+  // XXXX same as C here!
+  tor_capture_bugs_(1);
+  tt_assert(protover_all_supported("Fribble", &msg));
+  tt_ptr_op(msg, OP_EQ, NULL);
+  tor_end_capture_bugs_();
+
+  /* This case is forbidden. Since it came from a protover_all_supported,
+   * it can trigger a bug message.  */
+  tor_capture_bugs_(1);
+  tt_assert(protover_all_supported("Sleen=0-4294967295", &msg));
+  tt_ptr_op(msg, OP_EQ, NULL);
+  tor_free(msg);
+  tor_end_capture_bugs_();
+#endif
+
  done:
+  tor_end_capture_bugs_();
   tor_free(msg);
 }
 
+static void
+test_protover_vote_roundtrip(void *args)
+{
+  (void) args;
+  static const struct {
+    const char *input;
+    const char *expected_output;
+  } examples[] = {
+    { "Fkrkljdsf", NULL },
+    { "Zn=4294967295", NULL },
+    { "Zn=4294967295-1", NULL },
+    { "Zn=4294967293-4294967295", NULL },
+    /* Will fail because of 4294967295. */
+    { "Foo=1,3 Bar=3 Baz= Quux=9-12,14,15-16,900 Zn=0,4294967295",
+       NULL },
+    { "Foo=1,3 Bar=3 Baz= Quux=9-12,14,15-16,900 Zn=0,4294967294",
+      "Bar=3 Foo=1,3 Quux=9-12,14-16,900 Zn=0,4294967294" },
+    { "Zu16=0,65536", "Zu16=0,65536" },
+    { "N-1=1,2", "N-1=1-2" },
+    { "-1=4294967295", NULL },
+    { "-1=3", "-1=3" },
+    /* junk. */
+    { "!!3@*", NULL },
+    /* Missing equals sign */
+    { "Link=4 Haprauxymatyve Desc=9", NULL },
+    { "Link=4 Haprauxymatyve=7 Desc=9",
+      "Desc=9 Haprauxymatyve=7 Link=4" },
+    { "=10-11", NULL },
+    { "X=10-11", "X=10-11" },
+    { "Link=4 =3 Desc=9", NULL },
+    { "Link=4 Z=3 Desc=9", "Desc=9 Link=4 Z=3" },
+    { "Link=fred", NULL },
+    { "Link=1,fred", NULL },
+    { "Link=1,fred,3", NULL },
+    { "Link=1,9-8,3", NULL },
+    { "Faux=-0", NULL },
+    { "Faux=0--0", NULL },
+    // "These fail at the splitting stage in Rust, but the number parsing
+    // stage in C."
+    { "Faux=-1", NULL },
+    { "Faux=-1-3", NULL },
+    { "Faux=1--1", NULL },
+    /* Large integers */
+    { "Link=4294967296", NULL },
+    /* Large range */
+    { "Sleen=1-501", "Sleen=1-501" },
+    { "Sleen=1-65537", NULL },
+    /* CPU/RAM DoS Loop: Rust only. */
+    { "Sleen=0-2147483648", NULL },
+    /* Rust seems to experience an internal error here. */
+    { "Sleen=0-4294967295", NULL },
+  };
+  unsigned u;
+  smartlist_t *votes = smartlist_new();
+  char *result = NULL;
+
+  for (u = 0; u < ARRAY_LENGTH(examples); ++u) {
+    const char *input = examples[u].input;
+    const char *expected_output = examples[u].expected_output;
+
+    smartlist_add(votes, (void*)input);
+    result = protover_compute_vote(votes, 1);
+    if (expected_output != NULL) {
+      tt_str_op(result, OP_EQ, expected_output);
+    } else {
+      tt_str_op(result, OP_EQ, "");
+    }
+
+    smartlist_clear(votes);
+    tor_free(result);
+  }
+
+ done:
+  smartlist_free(votes);
+  tor_free(result);
+}
+
 #define PV_TEST(name, flags)                       \
   { #name, test_protover_ ##name, (flags), NULL, NULL }
 
@@ -190,6 +360,7 @@ struct testcase_t protover_tests[] = {
   PV_TEST(parse_fail, 0),
   PV_TEST(vote, 0),
   PV_TEST(all_supported, 0),
+  PV_TEST(vote_roundtrip, 0),
   END_OF_TESTCASES
 };