[tor-commits] [tor/master] rust/protover: validate unknown protocol names use only allowed characters

[nickm at torproject.org]

commit 7c26f88fd7bde6844f36f4810675688542c313bf
Author: cypherpunks <cypherpunks at 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();