From 955a050bd20eedbcdc2b96aa257ba30b0cfd3888 Mon Sep 17 00:00:00 2001 From: cypherpunks Date: Sun, 9 Feb 2020 01:10:58 +0000 Subject: [PATCH 1/9] rust/protover: make test match test name It was just a copypaste of the test above it. Fix on commit 15e59a1fedf47e7e689e06e5649849552d8b8c0d --- src/rust/protover/tests/protover.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rust/protover/tests/protover.rs b/src/rust/protover/tests/protover.rs index 942fe3c6ab..cf0fb5bd3a 100644 --- a/src/rust/protover/tests/protover.rs +++ b/src/rust/protover/tests/protover.rs @@ -78,7 +78,7 @@ fn parse_protocol_unvalidated_with_empty() { #[test] #[should_panic] fn parse_protocol_validated_with_empty() { - let _: UnvalidatedProtoEntry = "".parse().unwrap(); + let _: ProtoEntry = "".parse().unwrap(); } #[test] From 419ea2f813c83a2065396ac085c9525dd2e1f137 Mon Sep 17 00:00:00 2001 From: cypherpunks Date: Sun, 9 Feb 2020 01:01:25 +0000 Subject: [PATCH 2/9] rust/protover: remove redundant test commit 15e59a1fedf47e7e689e06e5649849552d8b8c0d added a test for parsing an empty string twice. --- src/rust/protover/protover.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs index 2661d811c4..383a781fdc 100644 --- a/src/rust/protover/protover.rs +++ b/src/rust/protover/protover.rs @@ -874,11 +874,6 @@ mod test { assert_protoentry_is_unparseable!("Desc=1-4294967295"); } - #[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(); From 44facb83d5b8801ec53135fab8a1f610d468fcd1 Mon Sep 17 00:00:00 2001 From: cypherpunks Date: Sun, 9 Feb 2020 01:15:18 +0000 Subject: [PATCH 3/9] rust/protover: parse empty string into empty *ProtoEntry Contrary to what 15e59a1fedf47e7e689e06e5649849552d8b8c0d said, the C implementation has always accepted an empty string without complaint. Meanwhile the rust implementation has always given an error. Make the rust implementation match C. Also eliminate some more redundant tests. Fix on 0.3.3.1-alpha. --- src/rust/protover/protover.rs | 12 +++++++++++- src/rust/protover/tests/protover.rs | 13 ------------- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs index 383a781fdc..14a70cd42b 100644 --- a/src/rust/protover/protover.rs +++ b/src/rust/protover/protover.rs @@ -245,6 +245,11 @@ impl FromStr for ProtoEntry { /// Otherwise, the `Err` value of this `Result` is a `ProtoverError`. fn from_str(protocol_entry: &str) -> Result { 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 { @@ -493,6 +498,10 @@ impl UnvalidatedProtoEntry { ) -> Result, 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, '='); @@ -851,7 +860,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] diff --git a/src/rust/protover/tests/protover.rs b/src/rust/protover/tests/protover.rs index cf0fb5bd3a..c97810a6f2 100644 --- a/src/rust/protover/tests/protover.rs +++ b/src/rust/protover/tests/protover.rs @@ -69,18 +69,6 @@ fn protocol_all_supported_with_one_value() { assert_eq!(true, unsupported.is_none()); } -#[test] -#[should_panic] -fn parse_protocol_unvalidated_with_empty() { - let _: UnvalidatedProtoEntry = "".parse().unwrap(); -} - -#[test] -#[should_panic] -fn parse_protocol_validated_with_empty() { - let _: ProtoEntry = "".parse().unwrap(); -} - #[test] fn protocol_all_supported_with_three_values() { let protocols: UnvalidatedProtoEntry = "LinkAuth=1 Microdesc=1-2 Relay=2".parse().unwrap(); @@ -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); From 5d330997f690845fadbd71f71f26a2ea464cd1ab Mon Sep 17 00:00:00 2001 From: cypherpunks Date: Sat, 8 Feb 2020 20:10:40 +0000 Subject: [PATCH 4/9] dirparse: reject votes with malformed routerstatus entries --- src/feature/dirparse/ns_parse.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/feature/dirparse/ns_parse.c b/src/feature/dirparse/ns_parse.c index 109eebeb66..7cb92ebe59 100644 --- a/src/feature/dirparse/ns_parse.c +++ b/src/feature/dirparse/ns_parse.c @@ -1432,6 +1432,7 @@ networkstatus_parse_vote_from_string(const char *s, const char **eos_out, smartlist_add(ns->routerstatus_list, rs); } else { vote_routerstatus_free(rs); + goto err; // Malformed routerstatus, reject this vote. } } else { routerstatus_t *rs; @@ -1441,6 +1442,8 @@ networkstatus_parse_vote_from_string(const char *s, const char **eos_out, flav))) { /* Use exponential-backoff scheduling when downloading microdescs */ smartlist_add(ns->routerstatus_list, rs); + } else { + goto err; // Malformed routerstatus, reject this vote. } } } From b3affc23ef263a978b89e783f55ad3e37c873b9a Mon Sep 17 00:00:00 2001 From: cypherpunks Date: Sat, 8 Feb 2020 20:20:54 +0000 Subject: [PATCH 5/9] dirparse: reject routerstatus entries with malformed protover Contrary to its name, protover_contains_long_protocol_names() detects all parse errors, not just long names. --- src/feature/dirparse/ns_parse.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/feature/dirparse/ns_parse.c b/src/feature/dirparse/ns_parse.c index 7cb92ebe59..2354ebf440 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" @@ -451,6 +452,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); } From ad1b378e87c4f2e000909abed41b80a1ec166113 Mon Sep 17 00:00:00 2001 From: cypherpunks Date: Sat, 8 Feb 2020 20:45:24 +0000 Subject: [PATCH 6/9] dirparse: curlybraceify if statements --- src/feature/dirparse/ns_parse.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/feature/dirparse/ns_parse.c b/src/feature/dirparse/ns_parse.c index 2354ebf440..ce548b221e 100644 --- a/src/feature/dirparse/ns_parse.c +++ b/src/feature/dirparse/ns_parse.c @@ -1168,14 +1168,18 @@ networkstatus_parse_vote_from_string(const char *s, const char **eos_out, } } - if ((tok = find_opt_by_keyword(tokens, K_RECOMMENDED_CLIENT_PROTOCOLS))) + 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))) + } + 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))) + } + 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))) + } + if ((tok = find_opt_by_keyword(tokens, K_REQUIRED_RELAY_PROTOCOLS))) { ns->required_relay_protocols = tor_strdup(tok->args[0]); + } tok = find_by_keyword(tokens, K_VALID_AFTER); if (parse_iso_time(tok->args[0], &ns->valid_after)) From 8d89aa5eeaef2c009c7fc8250eacc0ed32ebde64 Mon Sep 17 00:00:00 2001 From: cypherpunks Date: Sat, 8 Feb 2020 21:11:44 +0000 Subject: [PATCH 7/9] dirparse: reject votes with invalid recommended/required protocols --- src/feature/dirparse/ns_parse.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/feature/dirparse/ns_parse.c b/src/feature/dirparse/ns_parse.c index ce548b221e..6172a57bbb 100644 --- a/src/feature/dirparse/ns_parse.c +++ b/src/feature/dirparse/ns_parse.c @@ -1168,16 +1168,25 @@ networkstatus_parse_vote_from_string(const char *s, const char **eos_out, } } + // Reject the vote if any of the protocols lines are malformed. if ((tok = find_opt_by_keyword(tokens, K_RECOMMENDED_CLIENT_PROTOCOLS))) { + if (protover_contains_long_protocol_names(tok->args[0])) + goto err; ns->recommended_client_protocols = tor_strdup(tok->args[0]); } if ((tok = find_opt_by_keyword(tokens, K_RECOMMENDED_RELAY_PROTOCOLS))) { + if (protover_contains_long_protocol_names(tok->args[0])) + goto err; ns->recommended_relay_protocols = tor_strdup(tok->args[0]); } if ((tok = find_opt_by_keyword(tokens, K_REQUIRED_CLIENT_PROTOCOLS))) { + if (protover_contains_long_protocol_names(tok->args[0])) + goto err; ns->required_client_protocols = tor_strdup(tok->args[0]); } if ((tok = find_opt_by_keyword(tokens, K_REQUIRED_RELAY_PROTOCOLS))) { + if (protover_contains_long_protocol_names(tok->args[0])) + goto err; ns->required_relay_protocols = tor_strdup(tok->args[0]); } From c2b95da4d49504281f7c30c8f523cd492be932a7 Mon Sep 17 00:00:00 2001 From: cypherpunks Date: Sat, 8 Feb 2020 21:16:26 +0000 Subject: [PATCH 8/9] dirparse: add helper for recommended/required protocols --- src/feature/dirparse/ns_parse.c | 44 ++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 20 deletions(-) diff --git a/src/feature/dirparse/ns_parse.c b/src/feature/dirparse/ns_parse.c index 6172a57bbb..5ba87176aa 100644 --- a/src/feature/dirparse/ns_parse.c +++ b/src/feature/dirparse/ns_parse.c @@ -1053,6 +1053,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 s, and return the result. Return NULL on failure. */ networkstatus_t * @@ -1169,26 +1182,17 @@ networkstatus_parse_vote_from_string(const char *s, const char **eos_out, } // Reject the vote if any of the protocols lines are malformed. - if ((tok = find_opt_by_keyword(tokens, K_RECOMMENDED_CLIENT_PROTOCOLS))) { - if (protover_contains_long_protocol_names(tok->args[0])) - goto err; - ns->recommended_client_protocols = tor_strdup(tok->args[0]); - } - if ((tok = find_opt_by_keyword(tokens, K_RECOMMENDED_RELAY_PROTOCOLS))) { - if (protover_contains_long_protocol_names(tok->args[0])) - goto err; - ns->recommended_relay_protocols = tor_strdup(tok->args[0]); - } - if ((tok = find_opt_by_keyword(tokens, K_REQUIRED_CLIENT_PROTOCOLS))) { - if (protover_contains_long_protocol_names(tok->args[0])) - goto err; - ns->required_client_protocols = tor_strdup(tok->args[0]); - } - if ((tok = find_opt_by_keyword(tokens, K_REQUIRED_RELAY_PROTOCOLS))) { - if (protover_contains_long_protocol_names(tok->args[0])) - goto err; - ns->required_relay_protocols = tor_strdup(tok->args[0]); - } + 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)) From 32b33c0d21ce471d735abbedc26d26998238c380 Mon Sep 17 00:00:00 2001 From: cypherpunks Date: Fri, 17 Aug 2018 22:45:33 +0000 Subject: [PATCH 9/9] protover: reject unexpected commas Consistently reject extra commas, instead of only rejecting leading commas. Fix on b2b2e1c7f24d9b65059e3d089768d6c49ba4f58f. Fixes #27194; bugfix on 0.2.9.4-alpha. --- changes/bug27194 | 3 +++ src/core/or/protover.c | 3 ++- src/test/test_protover.c | 4 ++++ 3 files changed, 9 insertions(+), 1 deletion(-) create mode 100644 changes/bug27194 diff --git a/changes/bug27194 b/changes/bug27194 new file mode 100644 index 0000000000..a1919c6c49 --- /dev/null +++ b/changes/bug27194 @@ -0,0 +1,3 @@ + o Minor bugfixes (protover): + - Consistently reject extra commas, instead of only rejecting leading commas. + Fixes bug 27194; bugfix on 0.2.9.4-alpha. diff --git a/src/core/or/protover.c b/src/core/or/protover.c index 17979d04ea..763eadadf9 100644 --- a/src/core/or/protover.c +++ b/src/core/or/protover.c @@ -245,7 +245,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/test/test_protover.c b/src/test/test_protover.c index 63c508bd13..664576f625 100644 --- a/src/test/test_protover.c +++ b/src/test/test_protover.c @@ -572,6 +572,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 */