[tor-commits] [tor/master] protover: TROVE-2018-005 Fix potential DoS in protover protocol parsing.

[nickm at torproject.org]

commit 056be68b1b5a727bb6cb26d98f37bfa131f76701
Author: Isis Lovecruft <isis at torproject.org>
Date:   Thu Mar 29 01:54:05 2018 +0000

    protover: TROVE-2018-005 Fix potential DoS in protover protocol parsing.
    
    In protover.c, the `expand_protocol_list()` function expands a `smartlist_t` of
    `proto_entry_t`s to their protocol name concatenated with each version number.
    For example, given a `proto_entry_t` like so:
    
        proto_entry_t *proto = tor_malloc(sizeof(proto_entry_t));
        proto_range_t *range = tor_malloc_zero(sizeof(proto_range_t));
    
        proto->name = tor_strdup("DoSaaaaaaaaaaaaaaaaaaaaaa[19KB]aaa");
        proto->ranges = smartlist_new();
    
        range->low = 1;
        range->high = 65536;
    
        smartlist_add(proto->ranges, range);
    
    (Where `[19KB]` is roughly 19KB of `"a"` bytes.)  This would expand in
    `expand_protocol_list()` to a `smartlist_t` containing 65536 copies of the
    string, e.g.:
    
        "DoSaaaaaaaaaaaaaaaaaaaaaa[19KB]aaa=1"
        "DoSaaaaaaaaaaaaaaaaaaaaaa[19KB]aaa=2"
        […]
        "DoSaaaaaaaaaaaaaaaaaaaaaa[19KB]aaa=65535"
    
    Thus constituting a potential resource exhaustion attack.
    
    The Rust implementation is not subject to this attack, because it instead
    expands the above string into a `HashMap<String, HashSet<u32>` prior to #24031,
    and a `HashMap<UnvalidatedProtocol, ProtoSet>` after).  Neither Rust version is
    subject to this attack, because it only stores the `String` once per protocol.
    (Although a related, but apparently of too minor impact to be usable, DoS bug
    has been fixed in #24031. [0])
    
    [0]: https://bugs.torproject.org/24031
    
     * ADDS hard limit on protocol name lengths in protover.c and checks in
       parse_single_entry() and expand_protocol_list().
     * ADDS tests to ensure the bug is caught.
     * FIXES #25517: https://bugs.torproject.org/25517
---
 src/or/protover.c        | 22 ++++++++++++++++++++++
 src/test/test_protover.c | 24 ++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/src/or/protover.c b/src/or/protover.c
index 18382ba7c..97d436dd1 100644
--- a/src/or/protover.c
+++ b/src/or/protover.c
@@ -53,6 +53,11 @@ static const struct {
 
 #define N_PROTOCOL_NAMES ARRAY_LENGTH(PROTOCOL_NAMES)
 
+/* Maximum allowed length of any single subprotocol name. */
+// C_RUST_COUPLED: src/rust/protover/protover.rs
+//                 `MAX_PROTOCOL_NAME_LENGTH`
+static const uint MAX_PROTOCOL_NAME_LENGTH = 100;
+
 /**
  * Given a protocol_type_t, return the corresponding string used in
  * descriptors.
@@ -198,6 +203,15 @@ parse_single_entry(const char *s, const char *end_of_entry)
   if (equals == s)
     goto error;
 
+  /* The name must not be longer than MAX_PROTOCOL_NAME_LENGTH. */
+  if (equals - s > MAX_PROTOCOL_NAME_LENGTH) {
+    log_warn(LD_NET, "When parsing a protocol entry, I got a very large "
+             "protocol name. This is possibly an attack or a bug, unless "
+             "the Tor network truly supports protocol names larger than "
+             "%ud characters. The offending string was: %s",
+             MAX_PROTOCOL_NAME_LENGTH, escaped(out->name));
+    goto error;
+  }
   out->name = tor_strndup(s, equals-s);
 
   tor_assert(equals < end_of_entry);
@@ -439,6 +453,14 @@ expand_protocol_list(const smartlist_t *protos)
 
   SMARTLIST_FOREACH_BEGIN(protos, const proto_entry_t *, ent) {
     const char *name = ent->name;
+    if (strlen(name) > MAX_PROTOCOL_NAME_LENGTH) {
+      log_warn(LD_NET, "When expanding a protocol entry, I got a very large "
+               "protocol name. This is possibly an attack or a bug, unless "
+               "the Tor network truly supports protocol names larger than "
+               "%ud characters. The offending string was: %s",
+               MAX_PROTOCOL_NAME_LENGTH, escaped(name));
+      continue;
+    }
     SMARTLIST_FOREACH_BEGIN(ent->ranges, const proto_range_t *, range) {
       uint32_t u;
       for (u = range->low; u <= range->high; ++u) {
diff --git a/src/test/test_protover.c b/src/test/test_protover.c
index 7bf1471eb..7a4fffad8 100644
--- a/src/test/test_protover.c
+++ b/src/test/test_protover.c
@@ -125,6 +125,13 @@ test_protover_parse_fail(void *arg)
   /* Broken range */
   elts = parse_protocol_list("Link=1,9-8,3");
   tt_ptr_op(elts, OP_EQ, NULL);
+
+  /* Protocol name too long */
+  elts = parse_protocol_list("DoSaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+                             "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+                             "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa");
+  tt_ptr_op(elts, OP_EQ, NULL);
+
 #endif
  done:
   ;
@@ -219,6 +226,15 @@ test_protover_vote(void *arg)
   tt_str_op(result, OP_EQ, "");
   tor_free(result);
 
+  /* Protocol name too long */
+  smartlist_clear(lst);
+  smartlist_add(lst, (void*) "DoSaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+                             "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+                             "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa");
+  result = protover_compute_vote(lst, 1);
+  tt_str_op(result, OP_EQ, "");
+  tor_free(result);
+
  done:
   tor_free(result);
   smartlist_free(lst);
@@ -300,6 +316,14 @@ test_protover_all_supported(void *arg)
   tt_assert(protover_all_supported("Sleen=0-4294967295", &msg));
   tor_end_capture_bugs_();
 
+  /* Protocol name too long */
+  tor_capture_bugs_(1);
+  tt_assert(protover_all_supported("DoSaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+                                   "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+                                   "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+                                   "aaaaaaaaaaaa=1-65536", &msg));
+  tor_end_capture_bugs_();
+
  done:
   tor_end_capture_bugs_();
   tor_free(msg);