GitLab is used only for code review, issue tracking and project management. Canonical locations for source code are still https://gitweb.torproject.org/ https://git.torproject.org/ and git-rw.torproject.org.

Commit dd63b972 authored by Nick Mathewson's avatar Nick Mathewson 🐛

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.
parent 741edf1b
Pipeline #1347 passed with stage
in 35 minutes and 17 seconds
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.
......@@ -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,
......
......@@ -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,
......
......@@ -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));
}
}
......
......@@ -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()
......
......@@ -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]
......
......@@ -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();
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment