commit 7c26f88fd7bde6844f36f4810675688542c313bf Author: cypherpunks cypherpunks@torproject.org Date: Thu Sep 13 16:33:58 2018 +0000

rust/protover: validate unknown protocol names use only allowed characters --- changes/bug27687 | 4 ++++ src/rust/protover/errors.rs | 3 +++ src/rust/protover/protover.rs | 34 +++++++++++++++++++++++++++++++++- 3 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/changes/bug27687 b/changes/bug27687 new file mode 100644 index 000000000..8b7903b63 --- /dev/null +++ b/changes/bug27687 @@ -0,0 +1,4 @@ + o Minor bugfixes (rust): + - protover parsed and accepted unknown protocol names containing invalid + characters outside the range [A-Za-z0-9-]. Fixes bug 27687; bugfix on + 0.3.3.1-alpha. diff --git a/src/rust/protover/errors.rs b/src/rust/protover/errors.rs index 56473d12e..d9dc73381 100644 --- a/src/rust/protover/errors.rs +++ b/src/rust/protover/errors.rs @@ -18,6 +18,7 @@ pub enum ProtoverError { ExceedsExpansionLimit, UnknownProtocol, ExceedsNameLimit, + InvalidProtocol, }

/// Descriptive error messages for `ProtoverError` variants. @@ -38,6 +39,8 @@ impl Display for ProtoverError { => write!(f, "A protocol in the protover string we attempted to parse is unknown."), ProtoverError::ExceedsNameLimit => write!(f, "An unrecognised protocol name was too long."), + ProtoverError::InvalidProtocol + => write!(f, "A protocol name includes invalid characters."), } } } diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs index 12ede53e5..b3563b063 100644 --- a/src/rust/protover/protover.rs +++ b/src/rust/protover/protover.rs @@ -113,11 +113,17 @@ impl fmt::Display for UnknownProtocol { } }

+fn is_valid_proto(s: &str) -> bool { + s.chars().all(|c| c.is_ascii_alphanumeric() || c == '-') +} + impl FromStr for UnknownProtocol { type Err = ProtoverError;

fn from_str(s: &str) -> Result<Self, Self::Err> { - if s.len() <= MAX_PROTOCOL_NAME_LENGTH { + if !is_valid_proto(s) { + Err(ProtoverError::InvalidProtocol) + } else if s.len() <= MAX_PROTOCOL_NAME_LENGTH { Ok(UnknownProtocol(s.to_string())) } else { Err(ProtoverError::ExceedsNameLimit) @@ -129,6 +135,9 @@ 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> { + if !is_valid_proto(s) { + return Err(ProtoverError::InvalidProtocol); + } Ok(UnknownProtocol(s.to_string())) } } @@ -777,6 +786,29 @@ mod test {

use super::*;

+ macro_rules! parse_proto { + ($e:expr) => {{ + let proto: Result<UnknownProtocol, _> = $e.parse(); + let proto2 = UnknownProtocol::from_str_any_len($e); + assert_eq!(proto, proto2); + proto + }}; + } + + #[test] + fn test_protocol_from_str() { + assert!(parse_proto!("Cons").is_ok()); + assert!(parse_proto!("123").is_ok()); + assert!(parse_proto!("1-2-3").is_ok()); + + let err = Err(ProtoverError::InvalidProtocol); + assert_eq!(err, parse_proto!("a_b_c")); + assert_eq!(err, parse_proto!("a b")); + assert_eq!(err, parse_proto!("a,")); + assert_eq!(err, parse_proto!("b.")); + assert_eq!(err, parse_proto!("é")); + } + macro_rules! assert_protoentry_is_parseable { ($e:expr) => ( let protoentry: Result<ProtoEntry, ProtoverError> = $e.parse();