May 2018 - tor-commits - lists.torproject.org

[tor/master] protover: TROVE-2018-005 Fix potential DoS in protover protocol parsing.
by nickm＠torproject.org 22 May '18

22 May '18

commit eb966928428a80c105d33bd60bcae5503a1adeb7 Author: Isis Lovecruft <isis(a)torproject.org> Date: Thu Mar 29 01:54:05 2018 +0000 protover: TROVE-2018-005 Fix potential DoS in protover protocol parsing. In protover.c, the `expand_protocol_list()` function expands a `smartlist_t` of `proto_entry_t`s to their protocol name concatenated with each version number. For example, given a `proto_entry_t` like so: proto_entry_t *proto = tor_malloc(sizeof(proto_entry_t)); proto_range_t *range = tor_malloc_zero(sizeof(proto_range_t)); proto->name = tor_strdup("DoSaaaaaaaaaaaaaaaaaaaaaa[19KB]aaa"); proto->ranges = smartlist_new(); range->low = 1; range->high = 65536; smartlist_add(proto->ranges, range); (Where `[19KB]` is roughly 19KB of `"a"` bytes.) This would expand in `expand_protocol_list()` to a `smartlist_t` containing 65536 copies of the string, e.g.: "DoSaaaaaaaaaaaaaaaaaaaaaa[19KB]aaa=1" "DoSaaaaaaaaaaaaaaaaaaaaaa[19KB]aaa=2" […] "DoSaaaaaaaaaaaaaaaaaaaaaa[19KB]aaa=65535" Thus constituting a potential resource exhaustion attack. The Rust implementation is not subject to this attack, because it instead expands the above string into a `HashMap<String, HashSet<u32>` prior to #24031, and a `HashMap<UnvalidatedProtocol, ProtoSet>` after). Neither Rust version is subject to this attack, because it only stores the `String` once per protocol. (Although a related, but apparently of too minor impact to be usable, DoS bug has been fixed in #24031. [0]) [0]: https://bugs.torproject.org/24031 * ADDS hard limit on protocol name lengths in protover.c and checks in parse_single_entry() and expand_protocol_list(). * ADDS tests to ensure the bug is caught. * FIXES #25517: https://bugs.torproject.org/25517 --- src/or/protover.c | 22 ++++++++++++++++++++++ src/test/test_protover.c | 24 ++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/src/or/protover.c b/src/or/protover.c index 45f0377d6..24b381361 100644 --- a/src/or/protover.c +++ b/src/or/protover.c @@ -50,6 +50,11 @@ static const struct { #define N_PROTOCOL_NAMES ARRAY_LENGTH(PROTOCOL_NAMES) +/* Maximum allowed length of any single subprotocol name. */ +// C_RUST_COUPLED: src/rust/protover/protover.rs +// `MAX_PROTOCOL_NAME_LENGTH` +static const uint MAX_PROTOCOL_NAME_LENGTH = 100; + /** * Given a protocol_type_t, return the corresponding string used in * descriptors. @@ -195,6 +200,15 @@ parse_single_entry(const char *s, const char *end_of_entry) if (equals == s) goto error; + /* The name must not be longer than MAX_PROTOCOL_NAME_LENGTH. */ + if (equals - s > MAX_PROTOCOL_NAME_LENGTH) { + log_warn(LD_NET, "When parsing a protocol entry, I got a very large " + "protocol name. This is possibly an attack or a bug, unless " + "the Tor network truly supports protocol names larger than " + "%ud characters. The offending string was: %s", + MAX_PROTOCOL_NAME_LENGTH, escaped(out->name)); + goto error; + } out->name = tor_strndup(s, equals-s); tor_assert(equals < end_of_entry); @@ -397,6 +411,14 @@ expand_protocol_list(const smartlist_t *protos) SMARTLIST_FOREACH_BEGIN(protos, const proto_entry_t *, ent) { const char *name = ent->name; + if (strlen(name) > MAX_PROTOCOL_NAME_LENGTH) { + log_warn(LD_NET, "When expanding a protocol entry, I got a very large " + "protocol name. This is possibly an attack or a bug, unless " + "the Tor network truly supports protocol names larger than " + "%ud characters. The offending string was: %s", + MAX_PROTOCOL_NAME_LENGTH, escaped(name)); + continue; + } SMARTLIST_FOREACH_BEGIN(ent->ranges, const proto_range_t *, range) { uint32_t u; for (u = range->low; u <= range->high; ++u) { diff --git a/src/test/test_protover.c b/src/test/test_protover.c index 9b94044b9..66deb7551 100644 --- a/src/test/test_protover.c +++ b/src/test/test_protover.c @@ -110,6 +110,12 @@ test_protover_parse_fail(void *arg) elts = parse_protocol_list("Link=1,9-8,3"); tt_ptr_op(elts, OP_EQ, NULL); + /* Protocol name too long */ + elts = parse_protocol_list("DoSaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); + tt_ptr_op(elts, OP_EQ, NULL); + done: ; } @@ -203,6 +209,15 @@ test_protover_vote(void *arg) tt_str_op(result, OP_EQ, ""); tor_free(result); + /* Protocol name too long */ + smartlist_clear(lst); + smartlist_add(lst, (void*) "DoSaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); + result = protover_compute_vote(lst, 1); + tt_str_op(result, OP_EQ, ""); + tor_free(result); + done: tor_free(result); smartlist_free(lst); @@ -270,6 +285,15 @@ test_protover_all_supported(void *arg) tor_end_capture_bugs_(); #endif + /* Protocol name too long */ + tor_capture_bugs_(1); + tt_assert(protover_all_supported( + "DoSaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaa=1-65536", &msg)); + tor_end_capture_bugs_(); + done: tor_end_capture_bugs_(); tor_free(msg);

1 0

[tor/maint-0.3.3] rust: Mirror TROVE-2018-005 fix in Rust protover implementation.
by nickm＠torproject.org 22 May '18

22 May '18

commit 569b4e57e23d728969a12751afc6b45f32d0f093 Author: Isis Lovecruft <isis(a)torproject.org> Date: Wed Mar 21 02:22:54 2018 +0000 rust: Mirror TROVE-2018-005 fix in Rust protover implementation. * REFACTORS `UnvalidatedProtoEntry::from_str` to place the bulk of the splitting/parsing logic in to a new `UnvalidatedProtoEntry::parse_protocol_and_version_str()` method (so that both `from_str()` and `from_str_any_len()` can call it.) * ADD a new `UnvalidatedProtoEntry::from_str_any_len()` method in order to maintain compatibility with consensus methods older than 29. * ADD a limit on the number of characters in a protocol name. * FIXES part of #25517: https://bugs.torproject.org/25517 --- src/rust/protover/ffi.rs | 14 +++++-- src/rust/protover/protover.rs | 93 +++++++++++++++++++++++++++++++++++++------ 2 files changed, 91 insertions(+), 16 deletions(-) diff --git a/src/rust/protover/ffi.rs b/src/rust/protover/ffi.rs index a40353eb1..ed078654f 100644 --- a/src/rust/protover/ffi.rs +++ b/src/rust/protover/ffi.rs @@ -59,7 +59,8 @@ pub extern "C" fn protover_all_supported( Err(_) => return 1, }; - let relay_proto_entry: UnvalidatedProtoEntry = match relay_version.parse() { + let relay_proto_entry: UnvalidatedProtoEntry = + match UnvalidatedProtoEntry::from_str_any_len(relay_version) { Ok(n) => n, Err(_) => return 1, }; @@ -181,6 +182,7 @@ pub extern "C" fn protover_get_supported_protocols() -> *const c_char { pub extern "C" fn protover_compute_vote( list: *const Stringlist, threshold: c_int, + allow_long_proto_names: bool, ) -> *mut c_char { if list.is_null() { @@ -195,9 +197,13 @@ pub extern "C" fn protover_compute_vote( let mut proto_entries: Vec<UnvalidatedProtoEntry> = Vec::new(); for datum in data { - let entry: UnvalidatedProtoEntry = match datum.parse() { - Ok(x) => x, - Err(_) => continue, + let entry: UnvalidatedProtoEntry = match allow_long_proto_names { + true => match UnvalidatedProtoEntry::from_str_any_len(datum.as_str()) { + Ok(n) => n, + Err(_) => continue}, + false => match datum.parse() { + Ok(n) => n, + Err(_) => continue}, }; proto_entries.push(entry); } diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs index 5e5a31cd3..17a8d60ec 100644 --- a/src/rust/protover/protover.rs +++ b/src/rust/protover/protover.rs @@ -28,6 +28,9 @@ const FIRST_TOR_VERSION_TO_ADVERTISE_PROTOCOLS: &'static str = "0.2.9.3-alpha"; /// C_RUST_COUPLED: src/or/protover.c `MAX_PROTOCOLS_TO_EXPAND` const MAX_PROTOCOLS_TO_EXPAND: usize = (1<<16); +/// The maximum size an `UnknownProtocol`'s name may be. +pub(crate) const MAX_PROTOCOL_NAME_LENGTH: usize = 100; + /// Currently supported protocols and their versions, as a byte-slice. /// /// # Warning @@ -114,6 +117,18 @@ impl FromStr for UnknownProtocol { type Err = ProtoverError; fn from_str(s: &str) -> Result<Self, Self::Err> { + if s.len() <= MAX_PROTOCOL_NAME_LENGTH { + Ok(UnknownProtocol(s.to_string())) + } else { + Err(ProtoverError::ExceedsNameLimit) + } + } +} + +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> { Ok(UnknownProtocol(s.to_string())) } } @@ -428,6 +443,49 @@ impl UnvalidatedProtoEntry { }; supported_versions.iter().any(|v| v.1 >= *vers) } + + /// Split a string containing (potentially) several protocols and their + /// versions into a `Vec` of tuples of string in `(protocol, versions)` + /// form. + /// + /// # Inputs + /// + /// A &str in the form `"Link=3-4 Cons=5"`. + /// + /// # Returns + /// + /// A `Result` whose `Ok` variant is a `Vec<(&str, &str)>` of `(protocol, + /// versions)`, or whose `Err` variant is a `ProtoverError`. + /// + /// # Errors + /// + /// This will error with a `ProtoverError::Unparseable` if any of the + /// following are true: + /// + /// * If a protocol name is an empty string, e.g. `"Cons=1,3 =3-5"`. + /// * If a protocol name cannot be parsed as utf-8. + /// * If the version numbers are an empty string, e.g. `"Cons="`. + fn parse_protocol_and_version_str<'a>(protocol_string: &'a str) + -> Result<Vec<(&'a str, &'a str)>, ProtoverError> + { + let mut protovers: Vec<(&str, &str)> = Vec::new(); + + for subproto in protocol_string.split(' ') { + let mut parts = subproto.splitn(2, '='); + + let name = match parts.next() { + Some("") => return Err(ProtoverError::Unparseable), + Some(n) => n, + None => return Err(ProtoverError::Unparseable), + }; + let vers = match parts.next() { + Some(n) => n, + None => return Err(ProtoverError::Unparseable), + }; + protovers.push((name, vers)); + } + Ok(protovers) + } } impl FromStr for UnvalidatedProtoEntry { @@ -460,19 +518,10 @@ impl FromStr for UnvalidatedProtoEntry { /// * If the version string is malformed. See `impl FromStr for ProtoSet`. fn from_str(protocol_string: &str) -> Result<UnvalidatedProtoEntry, ProtoverError> { let mut parsed: UnvalidatedProtoEntry = UnvalidatedProtoEntry::default(); + let parts: Vec<(&str, &str)> = + UnvalidatedProtoEntry::parse_protocol_and_version_str(protocol_string)?; - for subproto in protocol_string.split(' ') { - let mut parts = subproto.splitn(2, '='); - - let name = match parts.next() { - Some("") => return Err(ProtoverError::Unparseable), - Some(n) => n, - None => return Err(ProtoverError::Unparseable), - }; - let vers = match parts.next() { - Some(n) => n, - None => return Err(ProtoverError::Unparseable), - }; + for &(name, vers) in parts.iter() { let versions = ProtoSet::from_str(vers)?; let protocol = UnknownProtocol::from_str(name)?; @@ -482,6 +531,26 @@ impl FromStr for UnvalidatedProtoEntry { } } +impl UnvalidatedProtoEntry { + /// Create an `UnknownProtocol`, ignoring whether or not it + /// exceeds MAX_PROTOCOL_NAME_LENGTH. + pub(crate) fn from_str_any_len(protocol_string: &str) + -> Result<UnvalidatedProtoEntry, ProtoverError> + { + let mut parsed: UnvalidatedProtoEntry = UnvalidatedProtoEntry::default(); + let parts: Vec<(&str, &str)> = + UnvalidatedProtoEntry::parse_protocol_and_version_str(protocol_string)?; + + for &(name, vers) in parts.iter() { + let versions = ProtoSet::from_str(vers)?; + let protocol = UnknownProtocol::from_str_any_len(name)?; + + parsed.insert(protocol, versions); + } + Ok(parsed) + } +} + /// Pretend a `ProtoEntry` is actually an `UnvalidatedProtoEntry`. impl From<ProtoEntry> for UnvalidatedProtoEntry { fn from(proto_entry: ProtoEntry) -> UnvalidatedProtoEntry {

1 0

[tor/master] changes file for TROVE-2018-005
by nickm＠torproject.org 22 May '18

22 May '18

commit d2bc019053058b09b5552d327106d9fbe0acad56 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue May 22 12:21:00 2018 -0400 changes file for TROVE-2018-005 --- changes/TROVE-2018-005 | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/changes/TROVE-2018-005 b/changes/TROVE-2018-005 new file mode 100644 index 000000000..769c653f4 --- /dev/null +++ b/changes/TROVE-2018-005 @@ -0,0 +1,6 @@ + o Major bugfixes (security, directory authority, denial-of-service): + - Fix a bug that could have allowed an attacker to force a + directory authority to use up all its RAM by passing it a + maliciously crafted protocol versions string. Fixes bug 25517; + bugfix on 0.2.9.4-alpha. This issue is also tracked as + TROVE-2018-005.

1 0

[tor/master] rust: Mirror TROVE-2018-005 fix in Rust protover implementation.
by nickm＠torproject.org 22 May '18

22 May '18

commit 569b4e57e23d728969a12751afc6b45f32d0f093 Author: Isis Lovecruft <isis(a)torproject.org> Date: Wed Mar 21 02:22:54 2018 +0000 rust: Mirror TROVE-2018-005 fix in Rust protover implementation. * REFACTORS `UnvalidatedProtoEntry::from_str` to place the bulk of the splitting/parsing logic in to a new `UnvalidatedProtoEntry::parse_protocol_and_version_str()` method (so that both `from_str()` and `from_str_any_len()` can call it.) * ADD a new `UnvalidatedProtoEntry::from_str_any_len()` method in order to maintain compatibility with consensus methods older than 29. * ADD a limit on the number of characters in a protocol name. * FIXES part of #25517: https://bugs.torproject.org/25517 --- src/rust/protover/ffi.rs | 14 +++++-- src/rust/protover/protover.rs | 93 +++++++++++++++++++++++++++++++++++++------ 2 files changed, 91 insertions(+), 16 deletions(-) diff --git a/src/rust/protover/ffi.rs b/src/rust/protover/ffi.rs index a40353eb1..ed078654f 100644 --- a/src/rust/protover/ffi.rs +++ b/src/rust/protover/ffi.rs @@ -59,7 +59,8 @@ pub extern "C" fn protover_all_supported( Err(_) => return 1, }; - let relay_proto_entry: UnvalidatedProtoEntry = match relay_version.parse() { + let relay_proto_entry: UnvalidatedProtoEntry = + match UnvalidatedProtoEntry::from_str_any_len(relay_version) { Ok(n) => n, Err(_) => return 1, }; @@ -181,6 +182,7 @@ pub extern "C" fn protover_get_supported_protocols() -> *const c_char { pub extern "C" fn protover_compute_vote( list: *const Stringlist, threshold: c_int, + allow_long_proto_names: bool, ) -> *mut c_char { if list.is_null() { @@ -195,9 +197,13 @@ pub extern "C" fn protover_compute_vote( let mut proto_entries: Vec<UnvalidatedProtoEntry> = Vec::new(); for datum in data { - let entry: UnvalidatedProtoEntry = match datum.parse() { - Ok(x) => x, - Err(_) => continue, + let entry: UnvalidatedProtoEntry = match allow_long_proto_names { + true => match UnvalidatedProtoEntry::from_str_any_len(datum.as_str()) { + Ok(n) => n, + Err(_) => continue}, + false => match datum.parse() { + Ok(n) => n, + Err(_) => continue}, }; proto_entries.push(entry); } diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs index 5e5a31cd3..17a8d60ec 100644 --- a/src/rust/protover/protover.rs +++ b/src/rust/protover/protover.rs @@ -28,6 +28,9 @@ const FIRST_TOR_VERSION_TO_ADVERTISE_PROTOCOLS: &'static str = "0.2.9.3-alpha"; /// C_RUST_COUPLED: src/or/protover.c `MAX_PROTOCOLS_TO_EXPAND` const MAX_PROTOCOLS_TO_EXPAND: usize = (1<<16); +/// The maximum size an `UnknownProtocol`'s name may be. +pub(crate) const MAX_PROTOCOL_NAME_LENGTH: usize = 100; + /// Currently supported protocols and their versions, as a byte-slice. /// /// # Warning @@ -114,6 +117,18 @@ impl FromStr for UnknownProtocol { type Err = ProtoverError; fn from_str(s: &str) -> Result<Self, Self::Err> { + if s.len() <= MAX_PROTOCOL_NAME_LENGTH { + Ok(UnknownProtocol(s.to_string())) + } else { + Err(ProtoverError::ExceedsNameLimit) + } + } +} + +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> { Ok(UnknownProtocol(s.to_string())) } } @@ -428,6 +443,49 @@ impl UnvalidatedProtoEntry { }; supported_versions.iter().any(|v| v.1 >= *vers) } + + /// Split a string containing (potentially) several protocols and their + /// versions into a `Vec` of tuples of string in `(protocol, versions)` + /// form. + /// + /// # Inputs + /// + /// A &str in the form `"Link=3-4 Cons=5"`. + /// + /// # Returns + /// + /// A `Result` whose `Ok` variant is a `Vec<(&str, &str)>` of `(protocol, + /// versions)`, or whose `Err` variant is a `ProtoverError`. + /// + /// # Errors + /// + /// This will error with a `ProtoverError::Unparseable` if any of the + /// following are true: + /// + /// * If a protocol name is an empty string, e.g. `"Cons=1,3 =3-5"`. + /// * If a protocol name cannot be parsed as utf-8. + /// * If the version numbers are an empty string, e.g. `"Cons="`. + fn parse_protocol_and_version_str<'a>(protocol_string: &'a str) + -> Result<Vec<(&'a str, &'a str)>, ProtoverError> + { + let mut protovers: Vec<(&str, &str)> = Vec::new(); + + for subproto in protocol_string.split(' ') { + let mut parts = subproto.splitn(2, '='); + + let name = match parts.next() { + Some("") => return Err(ProtoverError::Unparseable), + Some(n) => n, + None => return Err(ProtoverError::Unparseable), + }; + let vers = match parts.next() { + Some(n) => n, + None => return Err(ProtoverError::Unparseable), + }; + protovers.push((name, vers)); + } + Ok(protovers) + } } impl FromStr for UnvalidatedProtoEntry { @@ -460,19 +518,10 @@ impl FromStr for UnvalidatedProtoEntry { /// * If the version string is malformed. See `impl FromStr for ProtoSet`. fn from_str(protocol_string: &str) -> Result<UnvalidatedProtoEntry, ProtoverError> { let mut parsed: UnvalidatedProtoEntry = UnvalidatedProtoEntry::default(); + let parts: Vec<(&str, &str)> = + UnvalidatedProtoEntry::parse_protocol_and_version_str(protocol_string)?; - for subproto in protocol_string.split(' ') { - let mut parts = subproto.splitn(2, '='); - - let name = match parts.next() { - Some("") => return Err(ProtoverError::Unparseable), - Some(n) => n, - None => return Err(ProtoverError::Unparseable), - }; - let vers = match parts.next() { - Some(n) => n, - None => return Err(ProtoverError::Unparseable), - }; + for &(name, vers) in parts.iter() { let versions = ProtoSet::from_str(vers)?; let protocol = UnknownProtocol::from_str(name)?; @@ -482,6 +531,26 @@ impl FromStr for UnvalidatedProtoEntry { } } +impl UnvalidatedProtoEntry { + /// Create an `UnknownProtocol`, ignoring whether or not it + /// exceeds MAX_PROTOCOL_NAME_LENGTH. + pub(crate) fn from_str_any_len(protocol_string: &str) + -> Result<UnvalidatedProtoEntry, ProtoverError> + { + let mut parsed: UnvalidatedProtoEntry = UnvalidatedProtoEntry::default(); + let parts: Vec<(&str, &str)> = + UnvalidatedProtoEntry::parse_protocol_and_version_str(protocol_string)?; + + for &(name, vers) in parts.iter() { + let versions = ProtoSet::from_str(vers)?; + let protocol = UnknownProtocol::from_str_any_len(name)?; + + parsed.insert(protocol, versions); + } + Ok(parsed) + } +} + /// Pretend a `ProtoEntry` is actually an `UnvalidatedProtoEntry`. impl From<ProtoEntry> for UnvalidatedProtoEntry { fn from(proto_entry: ProtoEntry) -> UnvalidatedProtoEntry {

1 0

[tor/master] Merge branch 'maint-0.3.3'
by nickm＠torproject.org 22 May '18

22 May '18

commit 1afdb5b6cc1d07d49d5c8f369e1493dd844acad9 Merge: add00045a 569b4e57e Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue May 22 12:33:09 2018 -0400 Merge branch 'maint-0.3.3' "Ours" merge to avoid the maint-0.3.3 version of the TROVE-2018-005 fix, which touches the files in their old locations.

1 0

[tor/master] Merge branch 'trove-2018-005_032' into trove-2018-005_033
by nickm＠torproject.org 22 May '18

22 May '18

commit a3a8d80bebdbb8988a2f33dea8b18a41e445c06f Merge: 3d1266324 d2bc01905 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue May 22 12:27:15 2018 -0400 Merge branch 'trove-2018-005_032' into trove-2018-005_033 changes/TROVE-2018-005 | 6 ++++++ src/or/dirserv.c | 6 ++++++ src/or/protover.c | 34 ++++++++++++++++++++++++++++++++++ src/or/protover.h | 3 ++- src/test/test_protover.c | 25 +++++++++++++++++++++++++ 5 files changed, 73 insertions(+), 1 deletion(-) diff --cc src/test/test_protover.c index 7bf1471eb,66deb7551..a7d4667df --- a/src/test/test_protover.c +++ b/src/test/test_protover.c @@@ -125,7 -109,13 +125,14 @@@ test_protover_parse_fail(void *arg /* Broken range */ elts = parse_protocol_list("Link=1,9-8,3"); tt_ptr_op(elts, OP_EQ, NULL); + + /* Protocol name too long */ + elts = parse_protocol_list("DoSaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); + tt_ptr_op(elts, OP_EQ, NULL); + +#endif done: ; } @@@ -283,23 -267,33 +299,32 @@@ test_protover_all_supported(void *arg tt_str_op(msg, OP_EQ, "Sleen=0-4294967294"); tor_free(msg); - /* If we get an unparseable list, we say "yes, that's supported." */ -#ifndef HAVE_RUST - // XXXX let's make this section unconditional: rust should behave the - // XXXX same as C here! + /* If we get a (barely) valid (but unsupported list, we say "yes, that's + * supported." */ + tt_assert(protover_all_supported("Fribble=", &msg)); + tt_ptr_op(msg, OP_EQ, NULL); + + /* If we get a completely unparseable list, protover_all_supported should + * hit a fatal assertion for BUG(entries == NULL). */ tor_capture_bugs_(1); tt_assert(protover_all_supported("Fribble", &msg)); - tt_ptr_op(msg, OP_EQ, NULL); tor_end_capture_bugs_(); - /* This case is forbidden. Since it came from a protover_all_supported, - * it can trigger a bug message. */ + /* If we get a completely unparseable list, protover_all_supported should + * hit a fatal assertion for BUG(entries == NULL). */ tor_capture_bugs_(1); tt_assert(protover_all_supported("Sleen=0-4294967295", &msg)); - tt_ptr_op(msg, OP_EQ, NULL); - tor_free(msg); tor_end_capture_bugs_(); -#endif + /* Protocol name too long */ + tor_capture_bugs_(1); + tt_assert(protover_all_supported( + "DoSaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaa=1-65536", &msg)); + tor_end_capture_bugs_(); + done: tor_end_capture_bugs_(); tor_free(msg);

1 0

[tor/master] protover: TROVE-2018-005 Fix potential DoS in protover protocol parsing.
by nickm＠torproject.org 22 May '18

22 May '18

commit 056be68b1b5a727bb6cb26d98f37bfa131f76701 Author: Isis Lovecruft <isis(a)torproject.org> Date: Thu Mar 29 01:54:05 2018 +0000 protover: TROVE-2018-005 Fix potential DoS in protover protocol parsing. In protover.c, the `expand_protocol_list()` function expands a `smartlist_t` of `proto_entry_t`s to their protocol name concatenated with each version number. For example, given a `proto_entry_t` like so: proto_entry_t *proto = tor_malloc(sizeof(proto_entry_t)); proto_range_t *range = tor_malloc_zero(sizeof(proto_range_t)); proto->name = tor_strdup("DoSaaaaaaaaaaaaaaaaaaaaaa[19KB]aaa"); proto->ranges = smartlist_new(); range->low = 1; range->high = 65536; smartlist_add(proto->ranges, range); (Where `[19KB]` is roughly 19KB of `"a"` bytes.) This would expand in `expand_protocol_list()` to a `smartlist_t` containing 65536 copies of the string, e.g.: "DoSaaaaaaaaaaaaaaaaaaaaaa[19KB]aaa=1" "DoSaaaaaaaaaaaaaaaaaaaaaa[19KB]aaa=2" […] "DoSaaaaaaaaaaaaaaaaaaaaaa[19KB]aaa=65535" Thus constituting a potential resource exhaustion attack. The Rust implementation is not subject to this attack, because it instead expands the above string into a `HashMap<String, HashSet<u32>` prior to #24031, and a `HashMap<UnvalidatedProtocol, ProtoSet>` after). Neither Rust version is subject to this attack, because it only stores the `String` once per protocol. (Although a related, but apparently of too minor impact to be usable, DoS bug has been fixed in #24031. [0]) [0]: https://bugs.torproject.org/24031 * ADDS hard limit on protocol name lengths in protover.c and checks in parse_single_entry() and expand_protocol_list(). * ADDS tests to ensure the bug is caught. * FIXES #25517: https://bugs.torproject.org/25517 --- src/or/protover.c | 22 ++++++++++++++++++++++ src/test/test_protover.c | 24 ++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/src/or/protover.c b/src/or/protover.c index 18382ba7c..97d436dd1 100644 --- a/src/or/protover.c +++ b/src/or/protover.c @@ -53,6 +53,11 @@ static const struct { #define N_PROTOCOL_NAMES ARRAY_LENGTH(PROTOCOL_NAMES) +/* Maximum allowed length of any single subprotocol name. */ +// C_RUST_COUPLED: src/rust/protover/protover.rs +// `MAX_PROTOCOL_NAME_LENGTH` +static const uint MAX_PROTOCOL_NAME_LENGTH = 100; + /** * Given a protocol_type_t, return the corresponding string used in * descriptors. @@ -198,6 +203,15 @@ parse_single_entry(const char *s, const char *end_of_entry) if (equals == s) goto error; + /* The name must not be longer than MAX_PROTOCOL_NAME_LENGTH. */ + if (equals - s > MAX_PROTOCOL_NAME_LENGTH) { + log_warn(LD_NET, "When parsing a protocol entry, I got a very large " + "protocol name. This is possibly an attack or a bug, unless " + "the Tor network truly supports protocol names larger than " + "%ud characters. The offending string was: %s", + MAX_PROTOCOL_NAME_LENGTH, escaped(out->name)); + goto error; + } out->name = tor_strndup(s, equals-s); tor_assert(equals < end_of_entry); @@ -439,6 +453,14 @@ expand_protocol_list(const smartlist_t *protos) SMARTLIST_FOREACH_BEGIN(protos, const proto_entry_t *, ent) { const char *name = ent->name; + if (strlen(name) > MAX_PROTOCOL_NAME_LENGTH) { + log_warn(LD_NET, "When expanding a protocol entry, I got a very large " + "protocol name. This is possibly an attack or a bug, unless " + "the Tor network truly supports protocol names larger than " + "%ud characters. The offending string was: %s", + MAX_PROTOCOL_NAME_LENGTH, escaped(name)); + continue; + } SMARTLIST_FOREACH_BEGIN(ent->ranges, const proto_range_t *, range) { uint32_t u; for (u = range->low; u <= range->high; ++u) { diff --git a/src/test/test_protover.c b/src/test/test_protover.c index 7bf1471eb..7a4fffad8 100644 --- a/src/test/test_protover.c +++ b/src/test/test_protover.c @@ -125,6 +125,13 @@ test_protover_parse_fail(void *arg) /* Broken range */ elts = parse_protocol_list("Link=1,9-8,3"); tt_ptr_op(elts, OP_EQ, NULL); + + /* Protocol name too long */ + elts = parse_protocol_list("DoSaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); + tt_ptr_op(elts, OP_EQ, NULL); + #endif done: ; @@ -219,6 +226,15 @@ test_protover_vote(void *arg) tt_str_op(result, OP_EQ, ""); tor_free(result); + /* Protocol name too long */ + smartlist_clear(lst); + smartlist_add(lst, (void*) "DoSaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); + result = protover_compute_vote(lst, 1); + tt_str_op(result, OP_EQ, ""); + tor_free(result); + done: tor_free(result); smartlist_free(lst); @@ -300,6 +316,14 @@ test_protover_all_supported(void *arg) tt_assert(protover_all_supported("Sleen=0-4294967295", &msg)); tor_end_capture_bugs_(); + /* Protocol name too long */ + tor_capture_bugs_(1); + tt_assert(protover_all_supported("DoSaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaa=1-65536", &msg)); + tor_end_capture_bugs_(); + done: tor_end_capture_bugs_(); tor_free(msg);

1 0

[tor/master] fix wide lines
by nickm＠torproject.org 22 May '18

22 May '18

commit 6442417fdea866c259faf4c364f00c988f9ace1f Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue May 22 12:32:00 2018 -0400 fix wide lines --- src/test/test_protover.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/test/test_protover.c b/src/test/test_protover.c index 7a4fffad8..1b0adb175 100644 --- a/src/test/test_protover.c +++ b/src/test/test_protover.c @@ -128,8 +128,8 @@ test_protover_parse_fail(void *arg) /* Protocol name too long */ elts = parse_protocol_list("DoSaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" - "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" - "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); tt_ptr_op(elts, OP_EQ, NULL); #endif @@ -229,8 +229,8 @@ test_protover_vote(void *arg) /* Protocol name too long */ smartlist_clear(lst); smartlist_add(lst, (void*) "DoSaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" - "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" - "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); result = protover_compute_vote(lst, 1); tt_str_op(result, OP_EQ, ""); tor_free(result); @@ -318,10 +318,11 @@ test_protover_all_supported(void *arg) /* Protocol name too long */ tor_capture_bugs_(1); - tt_assert(protover_all_supported("DoSaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" - "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" - "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" - "aaaaaaaaaaaa=1-65536", &msg)); + tt_assert(protover_all_supported( + "DoSaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaa=1-65536", &msg)); tor_end_capture_bugs_(); done:

1 0

[tor/master] Merge branch 'trove-2018-005_034'
by nickm＠torproject.org 22 May '18

22 May '18

commit a78957888957d0465a774ac938b71d680bec800f Merge: 1afdb5b6c 6442417fd Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue May 22 12:33:49 2018 -0400 Merge branch 'trove-2018-005_034' changes/TROVE-2018-005 | 6 +++ src/or/dirauth/dirvote.c | 6 +++ src/or/protover.c | 34 ++++++++++++++++ src/or/protover.h | 1 + src/rust/protover/ffi.rs | 14 +++++-- src/rust/protover/protover.rs | 93 +++++++++++++++++++++++++++++++++++++------ src/test/test_protover.c | 25 ++++++++++++ 7 files changed, 163 insertions(+), 16 deletions(-)

1 0

[tor/master] changes file for TROVE-2018-005
by nickm＠torproject.org 22 May '18

22 May '18

commit e5541996b7a63313820e4ac635a6beac0f09acc8 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue May 22 12:21:00 2018 -0400 changes file for TROVE-2018-005 --- changes/TROVE-2018-005 | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/changes/TROVE-2018-005 b/changes/TROVE-2018-005 new file mode 100644 index 000000000..769c653f4 --- /dev/null +++ b/changes/TROVE-2018-005 @@ -0,0 +1,6 @@ + o Major bugfixes (security, directory authority, denial-of-service): + - Fix a bug that could have allowed an attacker to force a + directory authority to use up all its RAM by passing it a + maliciously crafted protocol versions string. Fixes bug 25517; + bugfix on 0.2.9.4-alpha. This issue is also tracked as + TROVE-2018-005.

1 0