Merge remote-tracking branch 'tor-gitlab/mr/94'

5 files changed, 50 insertions, 28 deletions

diff --git a/src/core/or/protover.c b/src/core/or/protover.c
index 1ac264925c..22128dff18 100644
--- a/src/core/or/protover.c
+++ b/src/core/or/protover.c
@@ -250,7 +250,8 @@ parse_single_entry(const char *s, const char *end_of_entry)
     }
 
     s = comma;
-    while (*s == ',' && s < end_of_entry)
+    // Skip the comma separator between ranges. Don't ignore a trailing comma.
+    if (s < (end_of_entry - 1))
       ++s;
   }
 
diff --git a/src/feature/dirparse/ns_parse.c b/src/feature/dirparse/ns_parse.c
index d4bcdae973..726da8a64c 100644
--- a/src/feature/dirparse/ns_parse.c
+++ b/src/feature/dirparse/ns_parse.c
@@ -13,6 +13,7 @@
 
 #include "core/or/or.h"
 #include "app/config/config.h"
+#include "core/or/protover.h"
 #include "core/or/versions.h"
 #include "feature/client/entrynodes.h"
 #include "feature/dirauth/dirvote.h"
@@ -466,6 +467,10 @@ routerstatus_parse_entry_from_string(memarea_t *area,
       }
     }
 
+    // If the protover line is malformed, reject this routerstatus.
+    if (protocols && protover_contains_long_protocol_names(protocols)) {
+      goto err;
+    }
     summarize_protover_flags(&rs->pv, protocols, version);
   }
 
@@ -1063,6 +1068,19 @@ extract_shared_random_srvs(networkstatus_t *ns, smartlist_t *tokens)
   }
 }
 
+/** Allocate a copy of a protover line, if present. If present but malformed,
+ * set *error to true. */
+static char *
+dup_protocols_string(smartlist_t *tokens, bool *error, directory_keyword kw)
+{
+  directory_token_t *tok = find_opt_by_keyword(tokens, kw);
+  if (!tok)
+    return NULL;
+  if (protover_contains_long_protocol_names(tok->args[0]))
+    *error = true;
+  return tor_strdup(tok->args[0]);
+}
+
 /** Parse a v3 networkstatus vote, opinion, or consensus (depending on
  * ns_type), from <b>s</b>, and return the result.  Return NULL on failure. */
 networkstatus_t *
@@ -1184,14 +1202,18 @@ networkstatus_parse_vote_from_string(const char *s,
     }
   }
 
-  if ((tok = find_opt_by_keyword(tokens, K_RECOMMENDED_CLIENT_PROTOCOLS)))
-    ns->recommended_client_protocols = tor_strdup(tok->args[0]);
-  if ((tok = find_opt_by_keyword(tokens, K_RECOMMENDED_RELAY_PROTOCOLS)))
-    ns->recommended_relay_protocols = tor_strdup(tok->args[0]);
-  if ((tok = find_opt_by_keyword(tokens, K_REQUIRED_CLIENT_PROTOCOLS)))
-    ns->required_client_protocols = tor_strdup(tok->args[0]);
-  if ((tok = find_opt_by_keyword(tokens, K_REQUIRED_RELAY_PROTOCOLS)))
-    ns->required_relay_protocols = tor_strdup(tok->args[0]);
+  // Reject the vote if any of the protocols lines are malformed.
+  bool unparseable = false;
+  ns->recommended_client_protocols = dup_protocols_string(tokens, &unparseable,
+                                         K_RECOMMENDED_CLIENT_PROTOCOLS);
+  ns->recommended_relay_protocols = dup_protocols_string(tokens, &unparseable,
+                                         K_RECOMMENDED_RELAY_PROTOCOLS);
+  ns->required_client_protocols = dup_protocols_string(tokens, &unparseable,
+                                         K_REQUIRED_CLIENT_PROTOCOLS);
+  ns->required_relay_protocols = dup_protocols_string(tokens, &unparseable,
+                                         K_REQUIRED_RELAY_PROTOCOLS);
+  if (unparseable)
+    goto err;
 
   tok = find_by_keyword(tokens, K_VALID_AFTER);
   if (parse_iso_time(tok->args[0], &ns->valid_after))
@@ -1453,6 +1475,7 @@ networkstatus_parse_vote_from_string(const char *s,
         smartlist_add(ns->routerstatus_list, rs);
       } else {
         vote_routerstatus_free(rs);
+        goto err; // Malformed routerstatus, reject this vote.
       }
     } else {
       routerstatus_t *rs;
@@ -1463,6 +1486,8 @@ networkstatus_parse_vote_from_string(const char *s,
                                                      flav))) {
         /* Use exponential-backoff scheduling when downloading microdescs */
         smartlist_add(ns->routerstatus_list, rs);
+      } else {
+        goto err; // Malformed routerstatus, reject this vote.
       }
     }
   }
diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs
index 076cd5301e..550732734c 100644
--- a/src/rust/protover/protover.rs
+++ b/src/rust/protover/protover.rs
@@ -253,6 +253,11 @@ impl FromStr for ProtoEntry {
     /// Otherwise, the `Err` value of this `Result` is a `ProtoverError`.
     fn from_str(protocol_entry: &str) -> Result<ProtoEntry, ProtoverError> {
         let mut proto_entry: ProtoEntry = ProtoEntry::default();
+
+        if protocol_entry.is_empty() {
+            return Ok(proto_entry);
+        }
+
         let entries = protocol_entry.split(' ');
 
         for entry in entries {
@@ -501,6 +506,10 @@ impl UnvalidatedProtoEntry {
     ) -> Result<Vec<(&'a str, &'a str)>, ProtoverError> {
         let mut protovers: Vec<(&str, &str)> = Vec::new();
 
+        if protocol_string.is_empty() {
+            return Ok(protovers);
+        }
+
         for subproto in protocol_string.split(' ') {
             let mut parts = subproto.splitn(2, '=');
 
@@ -859,7 +868,8 @@ mod test {
 
     #[test]
     fn test_protoentry_from_str_empty() {
-        assert_protoentry_is_unparseable!("");
+        assert_protoentry_is_parseable!("");
+        assert!(UnvalidatedProtoEntry::from_str("").is_ok());
     }
 
     #[test]
@@ -883,11 +893,6 @@ mod test {
     }
 
     #[test]
-    fn test_protoentry_from_str_() {
-        assert_protoentry_is_unparseable!("");
-    }
-
-    #[test]
     fn test_protoentry_all_supported_single_protocol_single_version() {
         let protocol: UnvalidatedProtoEntry = "Cons=1".parse().unwrap();
         let unsupported: Option<UnvalidatedProtoEntry> = protocol.all_supported();
diff --git a/src/rust/protover/tests/protover.rs b/src/rust/protover/tests/protover.rs
index 942fe3c6ab..c97810a6f2 100644
--- a/src/rust/protover/tests/protover.rs
+++ b/src/rust/protover/tests/protover.rs
@@ -70,18 +70,6 @@ fn protocol_all_supported_with_one_value() {
 }
 
 #[test]
-#[should_panic]
-fn parse_protocol_unvalidated_with_empty() {
-    let _: UnvalidatedProtoEntry = "".parse().unwrap();
-}
-
-#[test]
-#[should_panic]
-fn parse_protocol_validated_with_empty() {
-    let _: UnvalidatedProtoEntry = "".parse().unwrap();
-}
-
-#[test]
 fn protocol_all_supported_with_three_values() {
     let protocols: UnvalidatedProtoEntry = "LinkAuth=1 Microdesc=1-2 Relay=2".parse().unwrap();
     let unsupported: Option<UnvalidatedProtoEntry> = protocols.all_supported();
@@ -156,7 +144,6 @@ fn parse_protocol_with_unexpected_characters() {
 }
 
 #[test]
-#[should_panic]
 fn protover_compute_vote_returns_empty_for_empty_string() {
     let protocols: &[UnvalidatedProtoEntry] = &["".parse().unwrap()];
     let listed = ProtoverVote::compute(protocols, &1);
diff --git a/src/test/test_protover.c b/src/test/test_protover.c
index 5e74265550..4ccec73699 100644
--- a/src/test/test_protover.c
+++ b/src/test/test_protover.c
@@ -612,6 +612,10 @@ test_protover_vote_roundtrip(void *args)
     { "N-1=1,2", "N-1=1-2" },
     { "-1=4294967295", NULL },
     { "-1=3", "-1=3" },
+    { "Foo=,", NULL },
+    { "Foo=,1", NULL },
+    { "Foo=1,,3", NULL },
+    { "Foo=1,3,", NULL },
     /* junk. */
     { "!!3@*", NULL },
     /* Missing equals sign */