Merge branch 'prop318_limit_protover_035' into prop318_limit_protover_045

changes/ticket40133

@@ -0,0 +1,5 @@
+  o Minor features (protocol simplification):
+    - Tor no longer allows subprotocol versions larger than 63.  Previously
+      versions up to UINT32_MAX were allowed, which significantly complicated
+      our code.
+      Implements proposal 318; closes ticket 40133.

src/core/or/protover.c

@@ -118,13 +118,13 @@ proto_entry_free_(proto_entry_t *entry)
 }
 
 /** The largest possible protocol version. */
-#define MAX_PROTOCOL_VERSION (UINT32_MAX-1)
+#define MAX_PROTOCOL_VERSION (63)
 
 /**
  * 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
  * <b>low_out</b> and <b>high_out</b>.  A protocol range has the format U, or
- * U-U, where U is an unsigned 32-bit integer.
+ * U-U, where U is an unsigned integer between 0 and 63 inclusive.
  */
 static int
 parse_version_range(const char *s, const char *end_of_range,

src/rust/protover/errors.rs

@@ -36,7 +36,7 @@ impl Display for ProtoverError {
             ProtoverError::Unparseable => write!(f, "The protover string was unparseable."),
             ProtoverError::ExceedsMax => write!(
                 f,
-                "The high in a (low, high) protover range exceeds u32::MAX."
+                "The high in a (low, high) protover range exceeds 63."
             ),
             ProtoverError::ExceedsExpansionLimit => write!(
                 f,

src/rust/protover/protoset.rs

@@ -294,6 +294,10 @@ impl ProtoSet {
     }
 }
 
+/// Largest allowed protocol version.
+/// C_RUST_COUPLED: protover.c `MAX_PROTOCOL_VERSION`
+const MAX_PROTOCOL_VERSION: Version = 63;
+
 impl FromStr for ProtoSet {
     type Err = ProtoverError;
 
@@ -370,7 +374,7 @@ impl FromStr for ProtoSet {
         let pieces: ::std::str::Split<char> = version_string.split(',');
 
         for p in pieces {
-            if p.contains('-') {
+            let (lo,hi) = if p.contains('-') {
                 let mut pair = p.splitn(2, '-');
 
                 let low = pair.next().ok_or(ProtoverError::Unparseable)?;
@@ -379,12 +383,17 @@ impl FromStr for ProtoSet {
                 let lo: Version = low.parse().or(Err(ProtoverError::Unparseable))?;
                 let hi: Version = high.parse().or(Err(ProtoverError::Unparseable))?;
 
-                pairs.push((lo, hi));
+                (lo,hi)
             } else {
                 let v: u32 = p.parse().or(Err(ProtoverError::Unparseable))?;
 
-                pairs.push((v, v));
+                (v, v)
+            };
+
+            if lo > MAX_PROTOCOL_VERSION || hi > MAX_PROTOCOL_VERSION {
+                return Err(ProtoverError::ExceedsMax);
             }
+            pairs.push((lo, hi));
         }
 
         ProtoSet::from_slice(&pairs[..])
@@ -674,12 +683,11 @@ mod test {
 
     #[test]
     fn test_protoset_into_vec() {
-        let ps: ProtoSet = "1-13,42,9001,4294967294".parse().unwrap();
+        let ps: ProtoSet = "1-13,42".parse().unwrap();
         let v: Vec<Version> = ps.into();
 
         assert!(v.contains(&7));
-        assert!(v.contains(&9001));
+        assert!(v.contains(&42));
-        assert!(v.contains(&4294967294));
     }
 }
 

src/rust/protover/protover.rs

@@ -884,12 +884,12 @@ mod test {
 
     #[test]
     fn test_protoentry_from_str_allowed_number_of_versions() {
-        assert_protoentry_is_parseable!("Desc=1-4294967294");
+        assert_protoentry_is_parseable!("Desc=1-63");
     }
 
     #[test]
     fn test_protoentry_from_str_too_many_versions() {
-        assert_protoentry_is_unparseable!("Desc=1-4294967295");
+        assert_protoentry_is_unparseable!("Desc=1-64");
     }
 
     #[test]
@@ -923,10 +923,10 @@ mod test {
 
     #[test]
     fn test_protoentry_all_supported_unsupported_high_version() {
-        let protocols: UnvalidatedProtoEntry = "HSDir=12-100".parse().unwrap();
+        let protocols: UnvalidatedProtoEntry = "HSDir=12-60".parse().unwrap();
         let unsupported: Option<UnvalidatedProtoEntry> = protocols.all_supported();
         assert_eq!(true, unsupported.is_some());
-        assert_eq!("HSDir=12-100", &unsupported.unwrap().to_string());
+        assert_eq!("HSDir=12-60", &unsupported.unwrap().to_string());
     }
 
     #[test]
@@ -975,7 +975,7 @@ mod test {
             ProtoSet::from_str(&versions).unwrap().to_string()
         );
 
-        versions = "1-3,500";
+        versions = "1-3,50";
         assert_eq!(
             String::from(versions),
             ProtoSet::from_str(&versions).unwrap().to_string()

src/rust/protover/tests/protover.rs

@@ -86,10 +86,10 @@ fn protocol_all_supported_with_unsupported_protocol() {
 
 #[test]
 fn protocol_all_supported_with_unsupported_versions() {
-    let protocols: UnvalidatedProtoEntry = "Link=3-999".parse().unwrap();
+    let protocols: UnvalidatedProtoEntry = "Link=3-63".parse().unwrap();
     let unsupported: Option<UnvalidatedProtoEntry> = protocols.all_supported();
     assert_eq!(true, unsupported.is_some());
-    assert_eq!("Link=6-999", &unsupported.unwrap().to_string());
+    assert_eq!("Link=6-63", &unsupported.unwrap().to_string());
 }
 
 #[test]
@@ -102,10 +102,10 @@ fn protocol_all_supported_with_unsupported_low_version() {
 
 #[test]
 fn protocol_all_supported_with_unsupported_high_version() {
-    let protocols: UnvalidatedProtoEntry = "Cons=1-2,999".parse().unwrap();
+    let protocols: UnvalidatedProtoEntry = "Cons=1-2,60".parse().unwrap();
     let unsupported: Option<UnvalidatedProtoEntry> = protocols.all_supported();
     assert_eq!(true, unsupported.is_some());
-    assert_eq!("Cons=999", &unsupported.unwrap().to_string());
+    assert_eq!("Cons=60", &unsupported.unwrap().to_string());
 }
 
 #[test]
@@ -182,27 +182,27 @@ fn protover_compute_vote_returns_protocols_that_it_doesnt_currently_support() {
 
 #[test]
 fn protover_compute_vote_returns_matching_for_mix() {
-    let protocols: &[UnvalidatedProtoEntry] = &["Link=1-10,500 Cons=1,3-7,8".parse().unwrap()];
+    let protocols: &[UnvalidatedProtoEntry] = &["Link=1-10,50 Cons=1,3-7,8".parse().unwrap()];
     let listed = ProtoverVote::compute(protocols, &1);
-    assert_eq!("Cons=1,3-8 Link=1-10,500", listed.to_string());
+    assert_eq!("Cons=1,3-8 Link=1-10,50", listed.to_string());
 }
 
 #[test]
 fn protover_compute_vote_returns_matching_for_longer_mix() {
     let protocols: &[UnvalidatedProtoEntry] = &[
-        "Desc=1-10,500 Cons=1,3-7,8".parse().unwrap(),
+        "Desc=1-10,50 Cons=1,3-7,8".parse().unwrap(),
-        "Link=123-456,78 Cons=2-6,8 Desc=9".parse().unwrap(),
+        "Link=12-45,8 Cons=2-6,8 Desc=9".parse().unwrap(),
     ];
 
     let listed = ProtoverVote::compute(protocols, &1);
-    assert_eq!("Cons=1-8 Desc=1-10,500 Link=78,123-456", listed.to_string());
+    assert_eq!("Cons=1-8 Desc=1-10,50 Link=8,12-45", listed.to_string());
 }
 
 #[test]
 fn protover_compute_vote_returns_matching_for_longer_mix_with_threshold_two() {
     let protocols: &[UnvalidatedProtoEntry] = &[
-        "Desc=1-10,500 Cons=1,3-7,8".parse().unwrap(),
+        "Desc=1-10,50 Cons=1,3-7,8".parse().unwrap(),
-        "Link=123-456,78 Cons=2-6,8 Desc=9".parse().unwrap(),
+        "Link=8,12-45 Cons=2-6,8 Desc=9".parse().unwrap(),
     ];
 
     let listed = ProtoverVote::compute(protocols, &2);
@@ -307,30 +307,20 @@ fn protocol_all_supported_with_single_protocol_and_protocol_range() {
     assert_eq!(true, unsupported.is_none());
 }
 
-// By allowing us to add to votes, the C implementation allows us to
-// exceed the limit.
-#[test]
-fn protover_compute_vote_may_exceed_limit() {
-    let proto1: UnvalidatedProtoEntry = "Sleen=1-65535".parse().unwrap();
-    let proto2: UnvalidatedProtoEntry = "Sleen=100000".parse().unwrap();
-
-    let _result: UnvalidatedProtoEntry = ProtoverVote::compute(&[proto1, proto2], &1);
-}
-
 #[test]
 fn protover_all_supported_should_exclude_versions_we_actually_do_support() {
-    let proto: UnvalidatedProtoEntry = "Link=3-999".parse().unwrap();
+    let proto: UnvalidatedProtoEntry = "Link=3-63".parse().unwrap();
     let result: String = proto.all_supported().unwrap().to_string();
 
-    assert_eq!(result, "Link=6-999".to_string());
+    assert_eq!(result, "Link=6-63".to_string());
 }
 
 #[test]
 fn protover_all_supported_should_exclude_versions_we_actually_do_support_complex1() {
-    let proto: UnvalidatedProtoEntry = "Link=1-3,345-666".parse().unwrap();
+    let proto: UnvalidatedProtoEntry = "Link=1-3,30-63".parse().unwrap();
     let result: String = proto.all_supported().unwrap().to_string();
 
-    assert_eq!(result, "Link=345-666".to_string());
+    assert_eq!(result, "Link=30-63".to_string());
 }
 
 #[test]
@@ -343,26 +333,10 @@ fn protover_all_supported_should_exclude_versions_we_actually_do_support_complex
 
 #[test]
 fn protover_all_supported_should_exclude_some_versions_and_entire_protocols() {
-    let proto: UnvalidatedProtoEntry = "Link=1-3,5-12 Quokka=9000-9001".parse().unwrap();
+    let proto: UnvalidatedProtoEntry = "Link=1-3,5-12 Quokka=50-51".parse().unwrap();
     let result: String = proto.all_supported().unwrap().to_string();
 
-    assert_eq!(result, "Link=6-12 Quokka=9000-9001".to_string());
+    assert_eq!(result, "Link=6-12 Quokka=50-51".to_string());
-}
-
-#[test]
-fn protover_all_supported_should_not_dos_anyones_computer() {
-    let proto: UnvalidatedProtoEntry = "Link=1-2147483648".parse().unwrap();
-    let result: String = proto.all_supported().unwrap().to_string();
-
-    assert_eq!(result, "Link=6-2147483648".to_string());
-}
-
-#[test]
-fn protover_all_supported_should_not_dos_anyones_computer_max_versions() {
-    let proto: UnvalidatedProtoEntry = "Link=1-4294967294".parse().unwrap();
-    let result: String = proto.all_supported().unwrap().to_string();
-
-    assert_eq!(result, "Link=6-4294967294".to_string());
 }
 
 #[test]

src/test/test_protover.c

@@ -32,7 +32,7 @@ test_protover_parse(void *arg)
 #else /* !defined(HAVE_RUST) */
   char *re_encoded = NULL;
 
-  const char *orig = "Foo=1,3 Bar=3 Baz= Quux=9-12,14,15-16,900";
+  const char *orig = "Foo=1,3 Bar=3 Baz= Quux=9-12,14,15-16";
   smartlist_t *elts = parse_protocol_list(orig);
 
   tt_assert(elts);
@@ -68,7 +68,7 @@ test_protover_parse(void *arg)
 
   e = smartlist_get(elts, 3);
   tt_str_op(e->name, OP_EQ, "Quux");
-  tt_int_op(smartlist_len(e->ranges), OP_EQ, 4);
+  tt_int_op(smartlist_len(e->ranges), OP_EQ, 3);
   {
     r = smartlist_get(e->ranges, 0);
     tt_int_op(r->low, OP_EQ, 9);
@@ -81,10 +81,6 @@ test_protover_parse(void *arg)
     r = smartlist_get(e->ranges, 2);
     tt_int_op(r->low, OP_EQ, 15);
     tt_int_op(r->high, OP_EQ, 16);
-
-    r = smartlist_get(e->ranges, 3);
-    tt_int_op(r->low, OP_EQ, 900);
-    tt_int_op(r->high, OP_EQ, 900);
   }
 
   re_encoded = encode_protocol_list(elts);
@@ -156,14 +152,14 @@ test_protover_vote(void *arg)
   tt_str_op(result, OP_EQ, "");
   tor_free(result);
 
-  smartlist_add(lst, (void*) "Foo=1-10,500 Bar=1,3-7,8");
+  smartlist_add(lst, (void*) "Foo=1-10,63 Bar=1,3-7,8");
   result = protover_compute_vote(lst, 1);
-  tt_str_op(result, OP_EQ, "Bar=1,3-8 Foo=1-10,500");
+  tt_str_op(result, OP_EQ, "Bar=1,3-8 Foo=1-10,63");
   tor_free(result);
 
-  smartlist_add(lst, (void*) "Quux=123-456,78 Bar=2-6,8 Foo=9");
+  smartlist_add(lst, (void*) "Quux=12-45 Bar=2-6,8 Foo=9");
   result = protover_compute_vote(lst, 1);
-  tt_str_op(result, OP_EQ, "Bar=1-8 Foo=1-10,500 Quux=78,123-456");
+  tt_str_op(result, OP_EQ, "Bar=1-8 Foo=1-10,63 Quux=12-45");
   tor_free(result);
 
   result = protover_compute_vote(lst, 2);
@@ -201,45 +197,16 @@ test_protover_vote(void *arg)
 
   /* Just below the threshold: Rust */
   smartlist_clear(lst);
-  smartlist_add(lst, (void*) "Sleen=1-500");
+  smartlist_add(lst, (void*) "Sleen=1-50");
   result = protover_compute_vote(lst, 1);
-  tt_str_op(result, OP_EQ, "Sleen=1-500");
+  tt_str_op(result, OP_EQ, "Sleen=1-50");
   tor_free(result);
 
   /* Just below the threshold: C */
   smartlist_clear(lst);
-  smartlist_add(lst, (void*) "Sleen=1-65536");
+  smartlist_add(lst, (void*) "Sleen=1-63");
   result = protover_compute_vote(lst, 1);
-  tt_str_op(result, OP_EQ, "Sleen=1-65536");
+  tt_str_op(result, OP_EQ, "Sleen=1-63");
-  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);
 
   /* Protocol name too long */
@@ -279,8 +246,8 @@ test_protover_all_supported(void *arg)
   tt_assert(! protover_all_supported("Wombat=9", &msg));
   tt_str_op(msg, OP_EQ, "Wombat=9");
   tor_free(msg);
-  tt_assert(! protover_all_supported("Link=999", &msg));
+  tt_assert(! protover_all_supported("Link=60", &msg));
-  tt_str_op(msg, OP_EQ, "Link=999");
+  tt_str_op(msg, OP_EQ, "Link=60");
   tor_free(msg);
 
   // Mix of things we support and things we don't
@@ -290,11 +257,11 @@ test_protover_all_supported(void *arg)
 
   /* Mix of things we support and don't support within a single protocol
    * which we do support */
-  tt_assert(! protover_all_supported("Link=3-999", &msg));
+  tt_assert(! protover_all_supported("Link=3-60", &msg));
-  tt_str_op(msg, OP_EQ, "Link=6-999");
+  tt_str_op(msg, OP_EQ, "Link=6-60");
   tor_free(msg);
-  tt_assert(! protover_all_supported("Link=1-3,345-666", &msg));
+  tt_assert(! protover_all_supported("Link=1-3,50-63", &msg));
-  tt_str_op(msg, OP_EQ, "Link=345-666");
+  tt_str_op(msg, OP_EQ, "Link=50-63");
   tor_free(msg);
   tt_assert(! protover_all_supported("Link=1-3,5-12", &msg));
   tt_str_op(msg, OP_EQ, "Link=6-12");
@@ -302,18 +269,8 @@ test_protover_all_supported(void *arg)
 
   /* Mix of protocols we do support and some we don't, where the protocols
    * we do support have some versions we don't support. */
-  tt_assert(! protover_all_supported("Link=1-3,5-12 Quokka=9000-9001", &msg));
+  tt_assert(! protover_all_supported("Link=1-3,5-12 Quokka=40-41", &msg));
-  tt_str_op(msg, OP_EQ, "Link=6-12 Quokka=9000-9001");
+  tt_str_op(msg, OP_EQ, "Link=6-12 Quokka=40-41");
-  tor_free(msg);
-
-  /* We shouldn't be able to DoS ourselves parsing a large range. */
-  tt_assert(! protover_all_supported("Sleen=1-2147483648", &msg));
-  tt_str_op(msg, OP_EQ, "Sleen=1-2147483648");
-  tor_free(msg);
-
-  /* This case is allowed. */
-  tt_assert(! protover_all_supported("Sleen=1-4294967294", &msg));
-  tt_str_op(msg, OP_EQ, "Sleen=1-4294967294");
   tor_free(msg);
 
   /* If we get a (barely) valid (but unsupported list, we say "yes, that's
@@ -606,9 +563,9 @@ test_protover_vote_roundtrip(void *args)
     /* Will fail because of 4294967295. */
     { "Foo=1,3 Bar=3 Baz= Quux=9-12,14,15-16,900 Zn=1,4294967295",
        NULL },
-    { "Foo=1,3 Bar=3 Baz= Quux=9-12,14,15-16,900 Zn=1,4294967294",
+    { "Foo=1,3 Bar=3 Baz= Quux=9-12,14,15-16,50 Zn=1,42",
-      "Bar=3 Foo=1,3 Quux=9-12,14-16,900 Zn=1,4294967294" },
+      "Bar=3 Foo=1,3 Quux=9-12,14-16,50 Zn=1,42" },
-    { "Zu16=1,65536", "Zu16=1,65536" },
+    { "Zu16=1,63", "Zu16=1,63" },
     { "N-1=1,2", "N-1=1-2" },
     { "-1=4294967295", NULL },
     { "-1=3", "-1=3" },
@@ -646,12 +603,8 @@ test_protover_vote_roundtrip(void *args)
     /* Large integers */
     { "Link=4294967296", NULL },
     /* Large range */
-    { "Sleen=1-501", "Sleen=1-501" },
+    { "Sleen=1-63", "Sleen=1-63" },
     { "Sleen=1-65537", NULL },
-    /* Both C/Rust implementations should be able to handle this mild DoS. */
-    { "Sleen=1-2147483648", NULL },
-    /* Rust tests are built in debug mode, so ints are bounds-checked. */
-    { "Sleen=1-4294967295", NULL },
   };
   unsigned u;
   smartlist_t *votes = smartlist_new();