From f69510ba4b196ed40fce64f24b5b7799b68d182b Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 15 Feb 2018 08:37:19 -0500 Subject: [PATCH 1/3] Rust protover compat: forbid more than MAX_VERSIONS_TO_EXPAND in a range Also correct MAX_VERSIONS_TO_EXPAND to match the C. NOTE that this patch leads to incorrect behavior: the C code allows huge ranges; it just doesn't allow votes on them (currently). For full compatibility, we'll need to make the rust code store ranges as ranges natively, possibly using something like the range_map crate. Still, this patch is smaller than a "proper" fix. Fixes TROVE-2018-003. --- src/rust/protover/protover.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs index 25f776aed4..cc9be67b6f 100644 --- a/src/rust/protover/protover.rs +++ b/src/rust/protover/protover.rs @@ -23,7 +23,7 @@ const FIRST_TOR_VERSION_TO_ADVERTISE_PROTOCOLS: &'static str = "0.2.9.3-alpha"; /// before concluding that someone is trying to DoS us /// /// C_RUST_COUPLED: src/or/protover.c `MAX_PROTOCOLS_TO_EXPAND` -const MAX_PROTOCOLS_TO_EXPAND: u32 = 500; +const MAX_PROTOCOLS_TO_EXPAND: usize = (1<<16); /// Currently supported protocols and their versions, as a byte-slice. /// @@ -209,7 +209,7 @@ impl Versions { )?); } - if versions.len() > MAX_PROTOCOLS_TO_EXPAND as usize { + if versions.len() > MAX_PROTOCOLS_TO_EXPAND { return Err("Too many versions to expand"); } } @@ -448,7 +448,13 @@ fn expand_version_range(range: &str) -> Result, &'static str> { ))?; // We can use inclusive range syntax when it becomes stable. - Ok(lower..higher + 1) + let result = lower..higher + 1; + + if result.len() > MAX_PROTOCOLS_TO_EXPAND { + Err("Too many protocols in expanded range") + } else { + Ok(result) + } } /// Checks to see if there is a continuous range of integers, starting at the @@ -862,6 +868,9 @@ mod test { Err("cannot parse protocol range upper bound"), expand_version_range("1-a") ); + assert_eq!(Ok(1000..66536), expand_version_range("1000-66535")); + assert_eq!(Err("Too many protocols in expanded range"), + expand_version_range("1000-66536")); } #[test] From b58a2febe32f96204a94fb547e14a84c8fd32651 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 15 Feb 2018 10:27:27 -0500 Subject: [PATCH 2/3] Forbid u32::MAX as a protover range element in rust Part of the 25249 fix to make rust match the C. --- src/rust/protover/protover.rs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs index cc9be67b6f..64f350c6b7 100644 --- a/src/rust/protover/protover.rs +++ b/src/rust/protover/protover.rs @@ -9,6 +9,7 @@ use std::fmt; use std::collections::{HashMap, HashSet}; use std::ops::Range; use std::string::String; +use std::u32; use tor_util::strings::NUL_BYTE; @@ -204,9 +205,13 @@ impl Versions { versions.insert(p); } } else { - versions.insert(u32::from_str(piece).or( + let v = u32::from_str(piece).or( Err("invalid protocol entry"), - )?); + )?; + if v == u32::MAX { + return Err("invalid protocol entry"); + } + versions.insert(v); } if versions.len() > MAX_PROTOCOLS_TO_EXPAND { @@ -447,6 +452,10 @@ fn expand_version_range(range: &str) -> Result, &'static str> { "cannot parse protocol range upper bound", ))?; + if lower == u32::MAX || higher == u32::MAX { + return Err("protocol range value out of range"); + } + // We can use inclusive range syntax when it becomes stable. let result = lower..higher + 1; From 5af03c1ef3c4718b79abb1638f5a8c275129530a Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 15 Feb 2018 10:47:09 -0500 Subject: [PATCH 3/3] rust protover: match the C implementation on empty-str cases Empty versions lists are permitted; empty keywords are not. --- src/rust/protover/protover.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs index 64f350c6b7..e5dc69b9a0 100644 --- a/src/rust/protover/protover.rs +++ b/src/rust/protover/protover.rs @@ -193,10 +193,6 @@ impl Versions { fn from_version_string( version_string: &str, ) -> Result { - if version_string.is_empty() { - return Err("version string is empty"); - } - let mut versions = HashSet::::new(); for piece in version_string.split(",") { @@ -204,6 +200,8 @@ impl Versions { for p in expand_version_range(piece)? { versions.insert(p); } + } else if piece == "" { + continue; } else { let v = u32::from_str(piece).or( Err("invalid protocol entry"), @@ -456,6 +454,10 @@ fn expand_version_range(range: &str) -> Result, &'static str> { return Err("protocol range value out of range"); } + if lower > higher { + return Err("protocol range is badly formed"); + } + // We can use inclusive range syntax when it becomes stable. let result = lower..higher + 1; @@ -583,6 +585,7 @@ fn parse_protocols_from_string_with_no_validation<'a>( let mut parts = subproto.splitn(2, "="); let name = match parts.next() { + Some("") => return Err("invalid protover entry"), Some(n) => n, None => return Err("invalid protover entry"), }; @@ -650,7 +653,6 @@ pub fn compute_vote( Ok(result) => result, Err(_) => continue, }; - for (protocol, versions) in this_vote { let supported_vers: &mut HashMap = all_count.entry(protocol).or_insert(HashMap::new()); @@ -794,7 +796,6 @@ mod test { use super::Versions; - assert_eq!(Err("version string is empty"), Versions::from_version_string("")); assert_eq!(Err("invalid protocol entry"), Versions::from_version_string("a,b")); assert_eq!(Err("invalid protocol entry"), Versions::from_version_string("1,!")); @@ -838,7 +839,7 @@ mod test { use super::contains_only_supported_protocols; assert_eq!(false, contains_only_supported_protocols("")); - assert_eq!(false, contains_only_supported_protocols("Cons=")); + assert_eq!(true, contains_only_supported_protocols("Cons=")); assert_eq!(true, contains_only_supported_protocols("Cons=1")); assert_eq!(false, contains_only_supported_protocols("Cons=0")); assert_eq!(false, contains_only_supported_protocols("Cons=0-1"));