Merge branch 'maint-0.3.3' into release-0.3.3

7 files changed, 164 insertions, 17 deletions

diff --git a/changes/TROVE-2018-005 b/changes/TROVE-2018-005
new file mode 100644
index 0000000000..769c653f43
--- /dev/null
+++ b/changes/TROVE-2018-005
@@ -0,0 +1,6 @@
+  o Major bugfixes (security, directory authority, denial-of-service):
+    - Fix a bug that could have allowed an attacker to force a
+      directory authority to use up all its RAM by passing it a
+      maliciously crafted protocol versions string. Fixes bug 25517;
+      bugfix on 0.2.9.4-alpha.  This issue is also tracked as
+      TROVE-2018-005.
diff --git a/src/or/dirserv.c b/src/or/dirserv.c
index 1c1610ff93..2a8da6a10a 100644
--- a/src/or/dirserv.c
+++ b/src/or/dirserv.c
@@ -2963,6 +2963,12 @@ dirserv_generate_networkstatus_vote_obj(crypto_pk_t *private_key,
   microdescriptors = smartlist_new();
 
   SMARTLIST_FOREACH_BEGIN(routers, routerinfo_t *, ri) {
+   /* If it has a protover list and contains a protocol name greater than
+    * MAX_PROTOCOL_NAME_LENGTH, skip it. */
+    if (ri->protocol_list &&
+        protover_contains_long_protocol_names(ri->protocol_list)) {
+      continue;
+    }
     if (ri->cache_info.published_on >= cutoff) {
       routerstatus_t *rs;
       vote_routerstatus_t *vrs;
diff --git a/src/or/protover.c b/src/or/protover.c
index 6532f09c2f..811f91410f 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);
@@ -263,6 +277,18 @@ parse_protocol_list(const char *s)
 }
 
 /**
+ * Return true if the unparsed protover in <b>s</b> would contain a protocol
+ * name longer than MAX_PROTOCOL_NAME_LENGTH, and false otherwise.
+ */
+bool
+protover_contains_long_protocol_names(const char *s)
+{
+  if (!parse_protocol_list(s))
+    return true;
+  return false;
+}
+
+/**
  * Given a protocol type and version number, return true iff we know
  * how to speak that protocol.
  */
@@ -439,6 +465,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/or/protover.h b/src/or/protover.h
index 477274e293..b94ebab15b 100644
--- a/src/or/protover.h
+++ b/src/or/protover.h
@@ -10,7 +10,7 @@
 #define TOR_PROTOVER_H
 
 #include "container.h"
-
+#include <stdbool.h>
 /** The first version of Tor that included "proto" entries in its
  * descriptors.  Authorities should use this to decide whether to
  * guess proto lines. */
@@ -42,6 +42,7 @@ typedef enum protocol_type_t {
   PRT_CONS,
 } protocol_type_t;
 
+bool protover_contains_long_protocol_names(const char *s);
 int protover_all_supported(const char *s, char **missing);
 int protover_is_supported_here(protocol_type_t pr, uint32_t ver);
 const char *protover_get_supported_protocols(void);
diff --git a/src/rust/protover/ffi.rs b/src/rust/protover/ffi.rs
index a40353eb13..ed078654f7 100644
--- a/src/rust/protover/ffi.rs
+++ b/src/rust/protover/ffi.rs
@@ -59,7 +59,8 @@ pub extern "C" fn protover_all_supported(
         Err(_) => return 1,
     };
 
-    let relay_proto_entry: UnvalidatedProtoEntry = match relay_version.parse() {
+    let relay_proto_entry: UnvalidatedProtoEntry =
+        match UnvalidatedProtoEntry::from_str_any_len(relay_version) {
         Ok(n)  => n,
         Err(_) => return 1,
     };
@@ -181,6 +182,7 @@ pub extern "C" fn protover_get_supported_protocols() -> *const c_char {
 pub extern "C" fn protover_compute_vote(
     list: *const Stringlist,
     threshold: c_int,
+    allow_long_proto_names: bool,
 ) -> *mut c_char {
 
     if list.is_null() {
@@ -195,9 +197,13 @@ pub extern "C" fn protover_compute_vote(
     let mut proto_entries: Vec<UnvalidatedProtoEntry> = Vec::new();
 
     for datum in data {
-        let entry: UnvalidatedProtoEntry = match datum.parse() {
-            Ok(x)  => x,
-            Err(_) => continue,
+        let entry: UnvalidatedProtoEntry = match allow_long_proto_names {
+            true => match UnvalidatedProtoEntry::from_str_any_len(datum.as_str()) {
+                Ok(n)  => n,
+                Err(_) => continue},
+            false => match datum.parse() {
+                Ok(n)  => n,
+                Err(_) => continue},
         };
         proto_entries.push(entry);
     }
diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs
index 5e5a31cd33..17a8d60ec6 100644
--- a/src/rust/protover/protover.rs
+++ b/src/rust/protover/protover.rs
@@ -28,6 +28,9 @@ const FIRST_TOR_VERSION_TO_ADVERTISE_PROTOCOLS: &'static str = "0.2.9.3-alpha";
 /// C_RUST_COUPLED: src/or/protover.c `MAX_PROTOCOLS_TO_EXPAND`
 const MAX_PROTOCOLS_TO_EXPAND: usize = (1<<16);
 
+/// The maximum size an `UnknownProtocol`'s name may be.
+pub(crate) const MAX_PROTOCOL_NAME_LENGTH: usize = 100;
+
 /// Currently supported protocols and their versions, as a byte-slice.
 ///
 /// # Warning
@@ -114,6 +117,18 @@ impl FromStr for UnknownProtocol {
     type Err = ProtoverError;
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
+        if s.len() <= MAX_PROTOCOL_NAME_LENGTH {
+            Ok(UnknownProtocol(s.to_string()))
+        } else {
+            Err(ProtoverError::ExceedsNameLimit)
+        }
+    }
+}
+
+impl UnknownProtocol {
+    /// Create an `UnknownProtocol`, ignoring whether or not it
+    /// exceeds MAX_PROTOCOL_NAME_LENGTH.
+    fn from_str_any_len(s: &str) -> Result<Self, ProtoverError> {
         Ok(UnknownProtocol(s.to_string()))
     }
 }
@@ -428,6 +443,49 @@ impl UnvalidatedProtoEntry {
         };
         supported_versions.iter().any(|v| v.1 >= *vers)
     }
+
+    /// Split a string containing (potentially) several protocols and their
+    /// versions into a `Vec` of tuples of string in `(protocol, versions)`
+    /// form.
+    ///
+    /// # Inputs
+    ///
+    /// A &str in the form `"Link=3-4 Cons=5"`.
+    ///
+    /// # Returns
+    ///
+    /// A `Result` whose `Ok` variant is a `Vec<(&str, &str)>` of `(protocol,
+    /// versions)`, or whose `Err` variant is a `ProtoverError`.
+    ///
+    /// # Errors
+    ///
+    /// This will error with a `ProtoverError::Unparseable` if any of the
+    /// following are true:
+    ///
+    /// * If a protocol name is an empty string, e.g. `"Cons=1,3 =3-5"`.
+    /// * If a protocol name cannot be parsed as utf-8.
+    /// * If the version numbers are an empty string, e.g. `"Cons="`.
+    fn parse_protocol_and_version_str<'a>(protocol_string: &'a str)
+        -> Result<Vec<(&'a str, &'a str)>, ProtoverError>
+    {
+        let mut protovers: Vec<(&str, &str)> = Vec::new();
+
+        for subproto in protocol_string.split(' ') {
+            let mut parts = subproto.splitn(2, '=');
+
+            let name = match parts.next() {
+                Some("") => return Err(ProtoverError::Unparseable),
+                Some(n) => n,
+                None => return Err(ProtoverError::Unparseable),
+            };
+            let vers = match parts.next() {
+                Some(n) => n,
+                None => return Err(ProtoverError::Unparseable),
+            };
+            protovers.push((name, vers));
+        }
+        Ok(protovers)
+    }
 }
 
 impl FromStr for UnvalidatedProtoEntry {
@@ -460,19 +518,10 @@ impl FromStr for UnvalidatedProtoEntry {
     /// * If the version string is malformed. See `impl FromStr for ProtoSet`.
     fn from_str(protocol_string: &str) -> Result<UnvalidatedProtoEntry, ProtoverError> {
         let mut parsed: UnvalidatedProtoEntry = UnvalidatedProtoEntry::default();
+        let parts: Vec<(&str, &str)> =
+            UnvalidatedProtoEntry::parse_protocol_and_version_str(protocol_string)?;
 
-        for subproto in protocol_string.split(' ') {
-            let mut parts = subproto.splitn(2, '=');
-
-            let name = match parts.next() {
-                Some("") => return Err(ProtoverError::Unparseable),
-                Some(n) => n,
-                None => return Err(ProtoverError::Unparseable),
-            };
-            let vers = match parts.next() {
-                Some(n) => n,
-                None => return Err(ProtoverError::Unparseable),
-            };
+        for &(name, vers) in parts.iter() {
             let versions = ProtoSet::from_str(vers)?;
             let protocol = UnknownProtocol::from_str(name)?;
 
@@ -482,6 +531,26 @@ impl FromStr for UnvalidatedProtoEntry {
     }
 }
 
+impl UnvalidatedProtoEntry {
+    /// Create an `UnknownProtocol`, ignoring whether or not it
+    /// exceeds MAX_PROTOCOL_NAME_LENGTH.
+    pub(crate) fn from_str_any_len(protocol_string: &str)
+        -> Result<UnvalidatedProtoEntry, ProtoverError>
+    {
+        let mut parsed: UnvalidatedProtoEntry = UnvalidatedProtoEntry::default();
+        let parts: Vec<(&str, &str)> =
+            UnvalidatedProtoEntry::parse_protocol_and_version_str(protocol_string)?;
+
+        for &(name, vers) in parts.iter() {
+            let versions = ProtoSet::from_str(vers)?;
+            let protocol = UnknownProtocol::from_str_any_len(name)?;
+
+            parsed.insert(protocol, versions);
+        }
+        Ok(parsed)
+    }
+}
+
 /// Pretend a `ProtoEntry` is actually an `UnvalidatedProtoEntry`.
 impl From<ProtoEntry> for UnvalidatedProtoEntry {
     fn from(proto_entry: ProtoEntry) -> UnvalidatedProtoEntry {
diff --git a/src/test/test_protover.c b/src/test/test_protover.c
index 7bf1471ebd..a7d4667dfc 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,15 @@ 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);