commit dd63b972883f6c0b23ee2f7661b7897b229dd28f
Author: Nick Mathewson <nickm(a)torproject.org>
Date: Wed Oct 14 11:28:37 2020 -0400
Implement proposal 318: Limit protovers to 0..63
In brief: we go through a lot of gymnastics to handle huge protover
numbers, but after years of development we're not even close to 10
for any of our current versions. We also have a convenient
workaround available in case we ever run out of protocols: if (for
example) we someday need Link=64, we can just add Link2=0 or
something.
This patch is a minimal patch to change tor's behavior; it doesn't
take advantage of the new restrictions.
Implements #40133 and proposal 318.
---
changes/ticket40133 | 5 ++
src/core/or/protover.c | 4 +-
src/rust/protover/errors.rs | 2 +-
src/rust/protover/protoset.rs | 20 +++++---
src/rust/protover/protover.rs | 10 ++--
src/rust/protover/tests/protover.rs | 60 +++++++-----------------
src/test/test_protover.c | 91 +++++++++----------------------------
7 files changed, 66 insertions(+), 126 deletions(-)
diff --git a/changes/ticket40133 b/changes/ticket40133
new file mode 100644
index 0000000000..8bbe00b6b2
--- /dev/null
+++ b/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.
diff --git a/src/core/or/protover.c b/src/core/or/protover.c
index 17979d04ea..dfb0e9e303 100644
--- a/src/core/or/protover.c
+++ b/src/core/or/protover.c
@@ -113,13 +113,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,
diff --git a/src/rust/protover/errors.rs b/src/rust/protover/errors.rs
index dc0d8735f4..04397ac4fe 100644
--- a/src/rust/protover/errors.rs
+++ b/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,
diff --git a/src/rust/protover/protoset.rs b/src/rust/protover/protoset.rs
index 3b283983c8..0ab94457c5 100644
--- a/src/rust/protover/protoset.rs
+++ b/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(&4294967294));
+ assert!(v.contains(&42));
}
}
diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs
index 06fdf56c69..536667f61b 100644
--- a/src/rust/protover/protover.rs
+++ b/src/rust/protover/protover.rs
@@ -866,12 +866,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]
@@ -910,10 +910,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]
@@ -962,7 +962,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()
diff --git a/src/rust/protover/tests/protover.rs b/src/rust/protover/tests/protover.rs
index 942fe3c6ab..d563202d87 100644
--- a/src/rust/protover/tests/protover.rs
+++ b/src/rust/protover/tests/protover.rs
@@ -98,10 +98,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]
@@ -114,10 +114,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]
@@ -195,27 +195,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(),
- "Link=123-456,78 Cons=2-6,8 Desc=9".parse().unwrap(),
+ "Desc=1-10,50 Cons=1,3-7,8".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(),
- "Link=123-456,78 Cons=2-6,8 Desc=9".parse().unwrap(),
+ "Desc=1-10,50 Cons=1,3-7,8".parse().unwrap(),
+ "Link=8,12-45 Cons=2-6,8 Desc=9".parse().unwrap(),
];
let listed = ProtoverVote::compute(protocols, &2);
@@ -320,30 +320,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]
@@ -356,26 +346,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 result: String = proto.all_supported().unwrap().to_string();
-
- assert_eq!(result, "Link=6-12 Quokka=9000-9001".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 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-4294967294".to_string());
+ assert_eq!(result, "Link=6-12 Quokka=50-51".to_string());
}
#[test]
diff --git a/src/test/test_protover.c b/src/test/test_protover.c
index 63c508bd13..b4689045cf 100644
--- a/src/test/test_protover.c
+++ b/src/test/test_protover.c
@@ -25,7 +25,7 @@ test_protover_parse(void *arg)
#else
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);
@@ -61,7 +61,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);
@@ -74,10 +74,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);
@@ -149,14 +145,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);
@@ -194,45 +190,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");
- 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");
+ smartlist_add(lst, (void*) "Sleen=1-63");
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, "");
+ tt_str_op(result, OP_EQ, "Sleen=1-63");
tor_free(result);
/* Protocol name too long */
@@ -272,8 +239,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_str_op(msg, OP_EQ, "Link=999");
+ tt_assert(! protover_all_supported("Link=60", &msg));
+ tt_str_op(msg, OP_EQ, "Link=60");
tor_free(msg);
// Mix of things we support and things we don't
@@ -283,11 +250,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_str_op(msg, OP_EQ, "Link=6-999");
+ tt_assert(! protover_all_supported("Link=3-60", &msg));
+ tt_str_op(msg, OP_EQ, "Link=6-60");
tor_free(msg);
- tt_assert(! protover_all_supported("Link=1-3,345-666", &msg));
- tt_str_op(msg, OP_EQ, "Link=345-666");
+ tt_assert(! protover_all_supported("Link=1-3,50-63", &msg));
+ 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");
@@ -295,18 +262,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_str_op(msg, OP_EQ, "Link=6-12 Quokka=9000-9001");
- 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");
+ tt_assert(! protover_all_supported("Link=1-3,5-12 Quokka=40-41", &msg));
+ tt_str_op(msg, OP_EQ, "Link=6-12 Quokka=40-41");
tor_free(msg);
/* If we get a (barely) valid (but unsupported list, we say "yes, that's
@@ -566,9 +523,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",
- "Bar=3 Foo=1,3 Quux=9-12,14-16,900 Zn=1,4294967294" },
- { "Zu16=1,65536", "Zu16=1,65536" },
+ { "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,50 Zn=1,42" },
+ { "Zu16=1,63", "Zu16=1,63" },
{ "N-1=1,2", "N-1=1-2" },
{ "-1=4294967295", NULL },
{ "-1=3", "-1=3" },
@@ -602,12 +559,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();
