Forbid UINT32_MAX as a protocol version

The C code and the rust code had different separate integer overflow
bugs here.  That suggests that we're better off just forbidding this
pathological case.

Also, add tests for expected behavior on receiving a bad protocol
list in a consensus.

Fixes another part of 25249.

diff --git a/changes/bug25249.2 b/changes/bug25249.2
new file mode 100644 (file)
index 0000000..9058c11
--- /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/src/or/protover.c b/src/or/protover.c
index f32316f8e72b76d3bf6d4877e0ad515ed2b62c81..a035b5c83bdda7b4065402e8ca97df88cb344845 100644 (file)
--- 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
@@ -130,7 +133,7 @@ parse_version_range(const char *s, const char *end_of_range,
 
   /* 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)
@@ -148,7 +151,8 @@ parse_version_range(const char *s, const char *end_of_range,
   if (!TOR_ISDIGIT(*s)) {
     goto error;
   }
-  high = (uint32_t) tor_parse_ulong(s, 10, 0, UINT32_MAX, &ok, &next);
+  high = (uint32_t) tor_parse_ulong(s, 10, 0,
+                                    MAX_PROTOCOL_VERSION, &ok, &next);
   if (!ok)
     goto error;
   if (next != end_of_range)

diff --git a/src/test/test_protover.c b/src/test/test_protover.c
index 4c41b6db6bf5c6a945e76b7d51d22b3305b2fa4f..8d061c69ca599a05fb247f6fee9ce0cdf86286be 100644 (file)
--- a/src/test/test_protover.c
+++ b/src/test/test_protover.c
@@ -257,12 +257,27 @@ test_protover_all_supported(void *arg)
   tt_str_op(msg, OP_EQ, "Sleen=0-2147483648");
   tor_free(msg);
 
-  /* Rust seems to experience an internal error here */
-  tt_assert(! protover_all_supported("Sleen=0-4294967295", &msg));
-  tt_str_op(msg, OP_EQ, "Sleen=0-4294967295");
+  /* 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." */
+  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_();
+
  done:
+  tor_end_capture_bugs_();
   tor_free(msg);
 }