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

[tor/release-0.3.3] protover: Change protover_all_supported() to return only unsupported.
by nickm＠torproject.org 04 Apr '18

04 Apr '18

commit ad369313f87cba286a4f3347553e7322608dbd9c Author: Isis Lovecruft <isis(a)torproject.org> Date: Tue Mar 27 16:59:49 2018 +0000 protover: Change protover_all_supported() to return only unsupported. Previously, if "Link=1-5" was supported, and you asked protover_all_supported() (or protover::all_supported() in Rust) if it supported "Link=3-999", the C version would return "Link=3-999" and the Rust would return "Link=6-999". These both behave the same now, i.e. both return "Link=6-999". --- src/or/protover.c | 77 +++++++++++++++++++++++++++++++++++++++++++----- src/test/test_protover.c | 17 ++++++++++- 2 files changed, 86 insertions(+), 8 deletions(-) diff --git a/src/or/protover.c b/src/or/protover.c index cb168085c..6532f09c2 100644 --- a/src/or/protover.c +++ b/src/or/protover.c @@ -671,7 +671,9 @@ int protover_all_supported(const char *s, char **missing_out) { int all_supported = 1; - smartlist_t *missing; + smartlist_t *missing_some; + smartlist_t *missing_completely; + smartlist_t *missing_all; if (!s) { return 1; @@ -684,7 +686,8 @@ protover_all_supported(const char *s, char **missing_out) return 1; } - missing = smartlist_new(); + missing_some = smartlist_new(); + missing_completely = smartlist_new(); SMARTLIST_FOREACH_BEGIN(entries, const proto_entry_t *, ent) { protocol_type_t tp; @@ -696,26 +699,86 @@ protover_all_supported(const char *s, char **missing_out) } SMARTLIST_FOREACH_BEGIN(ent->ranges, const proto_range_t *, range) { + proto_entry_t *unsupported = tor_malloc_zero(sizeof(proto_entry_t)); + proto_range_t *versions = tor_malloc_zero(sizeof(proto_range_t)); uint32_t i; + + unsupported->name = tor_strdup(ent->name); + unsupported->ranges = smartlist_new(); + for (i = range->low; i <= range->high; ++i) { if (!protover_is_supported_here(tp, i)) { - goto unsupported; + if (versions->low == 0 && versions->high == 0) { + versions->low = i; + /* Pre-emptively add the high now, just in case we're in a single + * version range (e.g. "Link=999"). */ + versions->high = i; + } + /* If the last one to be unsupported is one less than the current + * one, we're in a continous range, so set the high field. */ + if ((versions->high && versions->high == i - 1) || + /* Similarly, if the last high wasn't set and we're currently + * one higher than the low, add current index as the highest + * known high. */ + (!versions->high && versions->low == i - 1)) { + versions->high = i; + continue; + } + } else { + /* If we hit a supported version, and we previously had a range, + * we've hit a non-continuity. Copy the previous range and add it to + * the unsupported->ranges list and zero-out the previous range for + * the next iteration. */ + if (versions->low != 0 && versions->high != 0) { + proto_range_t *versions_to_add = tor_malloc(sizeof(proto_range_t)); + + versions_to_add->low = versions->low; + versions_to_add->high = versions->high; + smartlist_add(unsupported->ranges, versions_to_add); + + versions->low = 0; + versions->high = 0; + } } } + /* Once we've run out of versions to check, see if we had any unsupported + * ones and, if so, add them to unsupported->ranges. */ + if (versions->low != 0 && versions->high != 0) { + smartlist_add(unsupported->ranges, versions); + } + /* Finally, if we had something unsupported, add it to the list of + * missing_some things and mark that there was something missing. */ + if (smartlist_len(unsupported->ranges) != 0) { + smartlist_add(missing_some, (void*) unsupported); + all_supported = 0; + } else { + proto_entry_free(unsupported); + tor_free(versions); + } } SMARTLIST_FOREACH_END(range); continue; unsupported: all_supported = 0; - smartlist_add(missing, (void*) ent); + smartlist_add(missing_completely, (void*) ent); } SMARTLIST_FOREACH_END(ent); + /* We keep the two smartlists separate so that we can free the proto_entry_t + * we created and put in missing_some, so here we add them together to build + * the string. */ + missing_all = smartlist_new(); + smartlist_add_all(missing_all, missing_some); + smartlist_add_all(missing_all, missing_completely); + if (missing_out && !all_supported) { - tor_assert(0 != smartlist_len(missing)); - *missing_out = encode_protocol_list(missing); + tor_assert(smartlist_len(missing_all) != 0); + *missing_out = encode_protocol_list(missing_all); } - smartlist_free(missing); + SMARTLIST_FOREACH(missing_some, proto_entry_t *, ent, proto_entry_free(ent)); + smartlist_free(missing_some); + smartlist_free(missing_completely); + smartlist_free(missing_all); SMARTLIST_FOREACH(entries, proto_entry_t *, ent, proto_entry_free(ent)); smartlist_free(entries); diff --git a/src/test/test_protover.c b/src/test/test_protover.c index c343e9957..95cc5f083 100644 --- a/src/test/test_protover.c +++ b/src/test/test_protover.c @@ -254,8 +254,23 @@ test_protover_all_supported(void *arg) tt_assert(! protover_all_supported("Link=3-4 Wombat=9", &msg)); tt_str_op(msg, OP_EQ, "Wombat=9"); tor_free(msg); + + /* Mix of things we support and don't support within a single protocol + * which we do support */ tt_assert(! protover_all_supported("Link=3-999", &msg)); - tt_str_op(msg, OP_EQ, "Link=3-999"); + tt_str_op(msg, OP_EQ, "Link=6-999"); + tor_free(msg); + tt_assert(! protover_all_supported("Link=1-3,345-666", &msg)); + tt_str_op(msg, OP_EQ, "Link=345-666"); + tor_free(msg); + tt_assert(! protover_all_supported("Link=1-3,5-12", &msg)); + tt_str_op(msg, OP_EQ, "Link=6-12"); + tor_free(msg); + + /* Mix of protocols we do support and some we don't, where the protocols + * we do support have some versions we don't support. */ + tt_assert(! protover_all_supported("Link=1-3,5-12 Quokka=9000-9001", &msg)); + tt_str_op(msg, OP_EQ, "Link=6-12 Quokka=9000-9001"); tor_free(msg); /* CPU/RAM DoS loop: Rust only */

1 0

[tor/release-0.3.3] changes: Add changes file for #24031.
by nickm＠torproject.org 04 Apr '18

04 Apr '18

commit b503df2775d22cff0b74740357121ba5195e4a12 Author: Isis Lovecruft <isis(a)torproject.org> Date: Tue Apr 3 19:19:40 2018 +0000 changes: Add changes file for #24031. (cherry picked from commit 5a8cdec3f8617920f19e3ab7707233ad3f02424f) --- changes/bug24031 | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/changes/bug24031 b/changes/bug24031 new file mode 100644 index 000000000..adffa46d8 --- /dev/null +++ b/changes/bug24031 @@ -0,0 +1,13 @@ + o Major bugfixes (protover, voting): + - Revise Rust implementation of protover to use a more memory-efficient + voting algorithm and corresponding data structures, thus avoiding a + potential (but small impact) DoS attack where specially crafted protocol + strings would expand to several potential megabytes in memory. In the + process, several portions of code were revised to be methods on new, + custom types, rather than functions taking interchangeable types, thus + increasing type safety of the module. Custom error types and handling + were added as well, in order to facilitate better error dismissal/handling + in outside crates and avoid mistakenly passing an internal error string to + C over the FFI boundary. Many tests were added, and some previous + differences between the C and Rust implementations have been + remedied. Fixes 24031; bugfix on 0.3.3.1-alpha.

1 0

[tor/release-0.3.3] Merge remote-tracking branch 'isis-github/bug24031_r5_squashed_033' into maint-0.3.3
by nickm＠torproject.org 04 Apr '18

04 Apr '18

commit 8d6b1da2e6729c0cf8331e663bdfeee5d5660237 Merge: 961d2ad59 b503df277 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Apr 3 15:29:29 2018 -0400 Merge remote-tracking branch 'isis-github/bug24031_r5_squashed_033' into maint-0.3.3 changes/bug24031 | 13 + src/or/protover.c | 77 ++- src/rust/protover/errors.rs | 43 ++ src/rust/protover/ffi.rs | 94 ++- src/rust/protover/lib.rs | 4 + src/rust/protover/protoset.rs | 634 ++++++++++++++++++ src/rust/protover/protover.rs | 1265 ++++++++++++++++------------------- src/rust/protover/tests/protover.rs | 421 +++++++----- src/test/test_protover.c | 44 +- 9 files changed, 1690 insertions(+), 905 deletions(-)

1 0

[tor/release-0.3.3] rust: Refactor protover::compute_for_old_tor().
by nickm＠torproject.org 04 Apr '18

04 Apr '18

commit cd28b4c7f5cefd319d6ded635d25911b4323b50b Author: Isis Lovecruft <isis(a)torproject.org> Date: Tue Mar 27 02:41:25 2018 +0000 rust: Refactor protover::compute_for_old_tor(). During code review and discussion with Chelsea Komlo, she pointed out that protover::compute_for_old_tor() was a public function whose return type was `&'static CStr`. We both agree that C-like parts of APIs should: 1. not be exposed publicly (to other Rust crates), 2. only be called in the appropriate FFI code, 3. not expose types which are meant for FFI code (e.g. `*mut char`, `CString`, `*const c_int`, etc.) to the pure-Rust code of other crates. 4. FFI code (e.g. things in `ffi.rs` modules) should _never_ be called from pure-Rust, not even from other modules in its own crate (i.e. do not call `protover::ffi::*` from anywhere in `protover::protoset::*`, etc). With that in mind, this commit makes the following changes: * CHANGE `protover::compute_for_old_tor()` to be visible only at the `pub(crate)` level. * RENAME `protover::compute_for_old_tor()` to `protover::compute_for_old_tor_cstr()` to reflect the last change. * ADD a new `protover::compute_for_old_tor()` function wrapper which is public and intended for other Rust code to use, which returns a `&str`. --- src/rust/protover/ffi.rs | 2 +- src/rust/protover/protover.rs | 58 +++++++++++++++++++++++++++++++++++++------ 2 files changed, 51 insertions(+), 9 deletions(-) diff --git a/src/rust/protover/ffi.rs b/src/rust/protover/ffi.rs index 3632f5de8..a40353eb1 100644 --- a/src/rust/protover/ffi.rs +++ b/src/rust/protover/ffi.rs @@ -246,7 +246,7 @@ pub extern "C" fn protover_compute_for_old_tor(version: *const c_char) -> *const Err(_) => return empty.as_ptr(), }; - elder_protocols = compute_for_old_tor(&version); + elder_protocols = compute_for_old_tor_cstr(&version); // If we're going to pass it to C, there cannot be any intermediate NUL // bytes. An assert is okay here, since changing the const byte slice diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs index 96e9dd582..fc89f70d4 100644 --- a/src/rust/protover/protover.rs +++ b/src/rust/protover/protover.rs @@ -611,8 +611,8 @@ pub fn is_supported_here(proto: &Protocol, vers: &Version) -> bool { supported_versions.contains(vers) } -/// Older versions of Tor cannot infer their own subprotocols -/// Used to determine which subprotocols are supported by older Tor versions. +/// Since older versions of Tor cannot infer their own subprotocols, +/// determine which subprotocols are supported by older Tor versions. /// /// # Inputs /// @@ -626,24 +626,28 @@ pub fn is_supported_here(proto: &Protocol, vers: &Version) -> bool { /// "HSDir=1-1 LinkAuth=1" /// /// This function returns the protocols that are supported by the version input, -/// only for tor versions older than FIRST_TOR_VERSION_TO_ADVERTISE_PROTOCOLS. +/// only for tor versions older than `FIRST_TOR_VERSION_TO_ADVERTISE_PROTOCOLS` +/// (but not older than 0.2.4.19). For newer tors (or older than 0.2.4.19), it +/// returns an empty string. /// -/// C_RUST_COUPLED: src/rust/protover.c `compute_for_old_tor` -pub fn compute_for_old_tor(version: &str) -> &'static [u8] { +/// # Note +/// +/// This function is meant to be called for/within FFI code. If you'd +/// like to use this code in Rust, please see `compute_for_old_tor()`. +// +// C_RUST_COUPLED: src/rust/protover.c `compute_for_old_tor` +pub(crate) fn compute_for_old_tor_cstr(version: &str) -> &'static [u8] { if c_tor_version_as_new_as(version, FIRST_TOR_VERSION_TO_ADVERTISE_PROTOCOLS) { return NUL_BYTE; } - if c_tor_version_as_new_as(version, "0.2.9.1-alpha") { return b"Cons=1-2 Desc=1-2 DirCache=1 HSDir=1 HSIntro=3 HSRend=1-2 \ Link=1-4 LinkAuth=1 Microdesc=1-2 Relay=1-2\0"; } - if c_tor_version_as_new_as(version, "0.2.7.5") { return b"Cons=1-2 Desc=1-2 DirCache=1 HSDir=1 HSIntro=3 HSRend=1 \ Link=1-4 LinkAuth=1 Microdesc=1-2 Relay=1-2\0"; } - if c_tor_version_as_new_as(version, "0.2.4.19") { return b"Cons=1 Desc=1 DirCache=1 HSDir=1 HSIntro=3 HSRend=1 \ Link=1-4 LinkAuth=1 Microdesc=1 Relay=1-2\0"; @@ -652,6 +656,44 @@ pub fn compute_for_old_tor(version: &str) -> &'static [u8] { NUL_BYTE } +/// Since older versions of Tor cannot infer their own subprotocols, +/// determine which subprotocols are supported by older Tor versions. +/// +/// # Inputs +/// +/// * `version`, a string comprised of "[0-9a-z.-]" +/// +/// # Returns +/// +/// A `Result` whose `Ok` value is an `&'static str` encoding a list of protocol +/// names and supported versions. The string takes the following format: +/// +/// "HSDir=1-1 LinkAuth=1" +/// +/// This function returns the protocols that are supported by the version input, +/// only for tor versions older than `FIRST_TOR_VERSION_TO_ADVERTISE_PROTOCOLS`. +/// (but not older than 0.2.4.19). For newer tors (or older than 0.2.4.19), its +/// `Ok` `Result` contains an empty string. +/// +/// Otherwise, its `Err` contains a `ProtoverError::Unparseable` if the +/// `version` string was invalid utf-8. +/// +/// # Note +/// +/// This function is meant to be called for/within non-FFI Rust code. +// +// C_RUST_COUPLED: src/rust/protover.c `compute_for_old_tor` +pub fn compute_for_old_tor(version: &str) -> Result<&'static str, ProtoverError> { + let mut computed: &'static [u8] = compute_for_old_tor_cstr(version); + + // Remove the NULL byte at the end. + computed = &computed[..computed.len() - 1]; + + // .from_utf8() fails with a Utf8Error if it couldn't validate the + // utf-8, so convert that here into an Unparseable ProtoverError. + str::from_utf8(computed).or(Err(ProtoverError::Unparseable)) +} + #[cfg(test)] mod test { use std::str::FromStr;

1 0

[tor/release-0.3.3] tests: Make inline comments in test_protover.c more accurate.
by nickm＠torproject.org 04 Apr '18

04 Apr '18

commit f769edd148bfbb381a48217e9016902f036b9ed8 Author: Isis Lovecruft <isis(a)torproject.org> Date: Tue Mar 27 21:34:00 2018 +0000 tests: Make inline comments in test_protover.c more accurate. The DoS potential is slightly higher in C now due to some differences to the Rust code, see the C_RUST_DIFFERS tags in src/rust/protover/tests/protover.rs. Also, the comment about "failing at the splitting stage" in Rust wasn't true, since when we split, we ignore empty chunks (e.g. "1--1" parses into "(1,None),(None,1)" and "None" can't be parsed into an integer). Finally, the comment about "Rust seems to experience an internal error" is only true in debug mode, where u32s are bounds-checked at runtime. In release mode, code expressing the equivalent of this test will error with `Err(ProtoverError::Unparseable)` because 4294967295 is too large. --- src/test/test_protover.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/test/test_protover.c b/src/test/test_protover.c index 95cc5f083..e7e17efe3 100644 --- a/src/test/test_protover.c +++ b/src/test/test_protover.c @@ -273,7 +273,7 @@ test_protover_all_supported(void *arg) tt_str_op(msg, OP_EQ, "Link=6-12 Quokka=9000-9001"); tor_free(msg); - /* CPU/RAM DoS loop: Rust only */ + /* We shouldn't be able to DoS ourselves parsing a large range. */ tt_assert(! protover_all_supported("Sleen=0-2147483648", &msg)); tt_str_op(msg, OP_EQ, "Sleen=0-2147483648"); tor_free(msg); @@ -546,8 +546,6 @@ test_protover_vote_roundtrip(void *args) { "Link=1,9-8,3", NULL }, { "Faux=-0", NULL }, { "Faux=0--0", NULL }, - // "These fail at the splitting stage in Rust, but the number parsing - // stage in C." { "Faux=-1", NULL }, { "Faux=-1-3", NULL }, { "Faux=1--1", NULL }, @@ -556,9 +554,9 @@ test_protover_vote_roundtrip(void *args) /* Large range */ { "Sleen=1-501", "Sleen=1-501" }, { "Sleen=1-65537", NULL }, - /* CPU/RAM DoS Loop: Rust only. */ + /* Both C/Rust implementations should be able to handle this mild DoS. */ { "Sleen=0-2147483648", NULL }, - /* Rust seems to experience an internal error here. */ + /* Rust tests are built in debug mode, so ints are bounds-checked. */ { "Sleen=0-4294967295", NULL }, }; unsigned u;

1 0

[tor/release-0.3.3] tests: Run all existing protover tests in both languages.
by nickm＠torproject.org 04 Apr '18

04 Apr '18

commit 6739a69c590a12af0f1cb2af62972f4305803670 Author: Isis Lovecruft <isis(a)torproject.org> Date: Tue Mar 27 21:36:10 2018 +0000 tests: Run all existing protover tests in both languages. There's now no difference in these tests w.r.t. the C or Rust: both fail miserably (well, Rust fails with nice descriptive errors, and C gives you a traceback, because, well, C). --- src/test/test_protover.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/test/test_protover.c b/src/test/test_protover.c index e7e17efe3..7bf1471eb 100644 --- a/src/test/test_protover.c +++ b/src/test/test_protover.c @@ -283,23 +283,22 @@ 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 done: tor_end_capture_bugs_();

1 0

[tor/release-0.3.3] Merge branch 'maint-0.3.3' into release-0.3.3
by nickm＠torproject.org 04 Apr '18

04 Apr '18

commit 753840a8a7c946dbf2188cb4f8b03b8ff7cfbe61 Merge: 5bdc18b2d 7ccb1c5a8 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Apr 3 19:03:38 2018 -0400 Merge branch 'maint-0.3.3' into release-0.3.3 changes/bug24031 | 13 + src/or/protover.c | 77 ++- src/rust/protover/errors.rs | 43 ++ src/rust/protover/ffi.rs | 94 ++- src/rust/protover/lib.rs | 4 + src/rust/protover/protoset.rs | 634 ++++++++++++++++++ src/rust/protover/protover.rs | 1265 ++++++++++++++++------------------- src/rust/protover/tests/protover.rs | 421 +++++++----- src/test/test_protover.c | 44 +- 9 files changed, 1690 insertions(+), 905 deletions(-)

1 0

[tor/release-0.3.3] rust: Fix ProtoSet and ProtoEntry to use the same DoS limits as C.
by nickm＠torproject.org 04 Apr '18

04 Apr '18

commit c65088cb1943748412e1a390de655e20bdb28692 Author: Isis Lovecruft <isis(a)torproject.org> Date: Tue Mar 27 22:46:14 2018 +0000 rust: Fix ProtoSet and ProtoEntry to use the same DoS limits as C. Previously, the limit for MAX_PROTOCOLS_TO_EXPAND was actually being applied in Rust to the maximum number of version (total, for all subprotocols). Whereas in C, it was being applied to the number of subprotocols that were allowed. This changes the Rust to match C's behaviour. --- src/rust/protover/protoset.rs | 14 +++++++------- src/rust/protover/protover.rs | 18 +++++++++++++++--- src/rust/protover/tests/protover.rs | 14 -------------- 3 files changed, 22 insertions(+), 24 deletions(-) diff --git a/src/rust/protover/protoset.rs b/src/rust/protover/protoset.rs index f94e6299c..4afc50edf 100644 --- a/src/rust/protover/protoset.rs +++ b/src/rust/protover/protoset.rs @@ -8,7 +8,6 @@ use std::slice; use std::str::FromStr; use std::u32; -use protover::MAX_PROTOCOLS_TO_EXPAND; use errors::ProtoverError; /// A single version number. @@ -183,10 +182,6 @@ impl ProtoSet { last_high = high; } - if self.len() > MAX_PROTOCOLS_TO_EXPAND { - return Err(ProtoverError::ExceedsMax); - } - Ok(self) } @@ -317,9 +312,15 @@ impl FromStr for ProtoSet { /// assert!(protoset.contains(&5)); /// assert!(!protoset.contains(&10)); /// - /// // We can also equivalently call `ProtoSet::from_str` by doing: + /// // We can also equivalently call `ProtoSet::from_str` by doing (all + /// // implementations of `FromStr` can be called this way, this one isn't + /// // special): /// let protoset: ProtoSet = "4-6,12".parse()?; /// + /// // Calling it (either way) can take really large ranges (up to `u32::MAX`): + /// let protoset: ProtoSet = "1-70000".parse()?; + /// let protoset: ProtoSet = "1-4294967296".parse()?; + /// /// // There are lots of ways to get an `Err` from this function. Here are /// // a few: /// assert_eq!(Err(ProtoverError::Unparseable), ProtoSet::from_str("=")); @@ -327,7 +328,6 @@ impl FromStr for ProtoSet { /// assert_eq!(Err(ProtoverError::Unparseable), ProtoSet::from_str("not_an_int")); /// assert_eq!(Err(ProtoverError::Unparseable), ProtoSet::from_str("3-")); /// assert_eq!(Err(ProtoverError::Unparseable), ProtoSet::from_str("1-,4")); - /// assert_eq!(Err(ProtoverError::ExceedsMax), ProtoSet::from_str("1-70000")); /// /// // Things which would get parsed into an _empty_ `ProtoSet` are, /// // however, legal, and result in an empty `ProtoSet`: diff --git a/src/rust/protover/protover.rs b/src/rust/protover/protover.rs index fc89f70d4..5e5a31cd3 100644 --- a/src/rust/protover/protover.rs +++ b/src/rust/protover/protover.rs @@ -26,7 +26,7 @@ const FIRST_TOR_VERSION_TO_ADVERTISE_PROTOCOLS: &'static str = "0.2.9.3-alpha"; /// before concluding that someone is trying to DoS us /// /// C_RUST_COUPLED: src/or/protover.c `MAX_PROTOCOLS_TO_EXPAND` -pub(crate) const MAX_PROTOCOLS_TO_EXPAND: usize = (1<<16); +const MAX_PROTOCOLS_TO_EXPAND: usize = (1<<16); /// Currently supported protocols and their versions, as a byte-slice. /// @@ -166,6 +166,10 @@ impl ProtoEntry { supported.parse() } + pub fn len(&self) -> usize { + self.0.len() + } + pub fn get(&self, protocol: &Protocol) -> Option<&ProtoSet> { self.0.get(protocol) } @@ -220,8 +224,11 @@ impl FromStr for ProtoEntry { let proto_name: Protocol = proto.parse()?; proto_entry.insert(proto_name, versions); - } + if proto_entry.len() > MAX_PROTOCOLS_TO_EXPAND { + return Err(ProtoverError::ExceedsMax); + } + } Ok(proto_entry) } } @@ -738,8 +745,13 @@ mod test { } #[test] + fn test_protoentry_from_str_allowed_number_of_versions() { + assert_protoentry_is_parseable!("Desc=1-4294967294"); + } + + #[test] fn test_protoentry_from_str_too_many_versions() { - assert_protoentry_is_unparseable!("Desc=1-65537"); + assert_protoentry_is_unparseable!("Desc=1-4294967295"); } #[test] diff --git a/src/rust/protover/tests/protover.rs b/src/rust/protover/tests/protover.rs index 11015f35b..2db01a163 100644 --- a/src/rust/protover/tests/protover.rs +++ b/src/rust/protover/tests/protover.rs @@ -353,13 +353,6 @@ fn protover_all_supported_should_exclude_some_versions_and_entire_protocols() { } #[test] -#[should_panic(expected = "called `Result::unwrap()` on an `Err` value: ExceedsMax")] -// C_RUST_DIFFERS: This test fails in Rust (but not in C) because an -// UnvalidatedProtoEntry is defined as a Hashmap<UnknownProtocol, ProtoSet>. -// Because it contains the ProtoSet part, there's still *some* validation -// happening, so in this case the DoS protections in the Rust code are kicking -// in because the range here is exceeds the maximum, so it returns an -// Err(ProtoverError::ExceedsMax). The C, however seems to parse it anyway. fn protover_all_supported_should_not_dos_anyones_computer() { let proto: UnvalidatedProtoEntry = "Sleen=0-2147483648".parse().unwrap(); let result: String = proto.all_supported().unwrap().to_string(); @@ -368,13 +361,6 @@ fn protover_all_supported_should_not_dos_anyones_computer() { } #[test] -#[should_panic(expected = "called `Result::unwrap()` on an `Err` value: ExceedsMax")] -// C_RUST_DIFFERS: This test fails in Rust (but not in C) because an -// UnvalidatedProtoEntry is defined as a Hashmap<UnknownProtocol, ProtoSet>. -// Because it contains the ProtoSet part, there's still *some* validation -// happening, so in this case the DoS protections in the Rust code are kicking -// in because the range here is exceeds the maximum, so it returns an -// Err(ProtoverError::ExceedsMax). The C, however seems to parse it anyway. fn protover_all_supported_should_not_dos_anyones_computer_max_versions() { let proto: UnvalidatedProtoEntry = "Sleen=0-4294967294".parse().unwrap(); let result: String = proto.all_supported().unwrap().to_string();

1 0

[tor/release-0.3.3] rust: Port all C protover_all_supported tests to Rust.
by nickm＠torproject.org 04 Apr '18

04 Apr '18

commit 4b4e36a413bb5d0e2ea499dd6bc34b3d24bd3375 Author: Isis Lovecruft <isis(a)torproject.org> Date: Tue Mar 27 21:38:29 2018 +0000 rust: Port all C protover_all_supported tests to Rust. The behaviours still do not match, unsurprisingly, but now we know where a primary difference is: the Rust is validating version ranges more than the C, so in the C it's possible to call protover_all_supported on a ridiculous version range like "Sleen=0-4294967294" because the C uses MAX_PROTOCOLS_TO_EXPAND to count the number of *subprotocols* whereas the Rust uses it to count the total number of *versions* of all subprotocols. --- src/rust/protover/tests/protover.rs | 86 +++++++++++++++++++++++++++++++++++-- 1 file changed, 83 insertions(+), 3 deletions(-) diff --git a/src/rust/protover/tests/protover.rs b/src/rust/protover/tests/protover.rs index 9f8b5a443..11015f35b 100644 --- a/src/rust/protover/tests/protover.rs +++ b/src/rust/protover/tests/protover.rs @@ -6,6 +6,7 @@ extern crate protover; use protover::ProtoEntry; use protover::ProtoverVote; use protover::UnvalidatedProtoEntry; +use protover::errors::ProtoverError; #[test] fn parse_protocol_with_single_proto_and_single_version() { @@ -320,9 +321,88 @@ fn protover_compute_vote_may_exceed_limit() { } #[test] -fn protover_all_supported_should_include_version_we_actually_do_support() { +fn protover_all_supported_should_exclude_versions_we_actually_do_support() { let proto: UnvalidatedProtoEntry = "Link=3-999".parse().unwrap(); - let _result: String = proto.all_supported().unwrap().to_string(); + let result: String = proto.all_supported().unwrap().to_string(); - assert_eq!(_result, "Link=3-999".to_string()); + assert_eq!(result, "Link=6-999".to_string()); +} + +#[test] +fn protover_all_supported_should_exclude_versions_we_actually_do_support_complex1() { + let proto: UnvalidatedProtoEntry = "Link=1-3,345-666".parse().unwrap(); + let result: String = proto.all_supported().unwrap().to_string(); + + assert_eq!(result, "Link=345-666".to_string()); +} + +#[test] +fn protover_all_supported_should_exclude_versions_we_actually_do_support_complex2() { + let proto: UnvalidatedProtoEntry = "Link=1-3,5-12".parse().unwrap(); + let result: String = proto.all_supported().unwrap().to_string(); + + assert_eq!(result, "Link=6-12".to_string()); +} + +#[test] +fn protover_all_supported_should_exclude_some_versions_and_entire_protocols() { + let proto: UnvalidatedProtoEntry = "Link=1-3,5-12 Quokka=9000-9001".parse().unwrap(); + let result: String = proto.all_supported().unwrap().to_string(); + + assert_eq!(result, "Link=6-12 Quokka=9000-9001".to_string()); +} + +#[test] +#[should_panic(expected = "called `Result::unwrap()` on an `Err` value: ExceedsMax")] +// C_RUST_DIFFERS: This test fails in Rust (but not in C) because an +// UnvalidatedProtoEntry is defined as a Hashmap<UnknownProtocol, ProtoSet>. +// Because it contains the ProtoSet part, there's still *some* validation +// happening, so in this case the DoS protections in the Rust code are kicking +// in because the range here is exceeds the maximum, so it returns an +// Err(ProtoverError::ExceedsMax). The C, however seems to parse it anyway. +fn protover_all_supported_should_not_dos_anyones_computer() { + let proto: UnvalidatedProtoEntry = "Sleen=0-2147483648".parse().unwrap(); + let result: String = proto.all_supported().unwrap().to_string(); + + assert_eq!(result, "Sleen=0-2147483648".to_string()); +} + +#[test] +#[should_panic(expected = "called `Result::unwrap()` on an `Err` value: ExceedsMax")] +// C_RUST_DIFFERS: This test fails in Rust (but not in C) because an +// UnvalidatedProtoEntry is defined as a Hashmap<UnknownProtocol, ProtoSet>. +// Because it contains the ProtoSet part, there's still *some* validation +// happening, so in this case the DoS protections in the Rust code are kicking +// in because the range here is exceeds the maximum, so it returns an +// Err(ProtoverError::ExceedsMax). The C, however seems to parse it anyway. +fn protover_all_supported_should_not_dos_anyones_computer_max_versions() { + let proto: UnvalidatedProtoEntry = "Sleen=0-4294967294".parse().unwrap(); + let result: String = proto.all_supported().unwrap().to_string(); + + assert_eq!(result, "Sleen=0-4294967294".to_string()); +} + +#[test] +// C_RUST_DIFFERS: The C will return true (e.g. saying "yes, that's supported") +// but set the msg to NULL (??? seems maybe potentially bad). The Rust will +// simply return a None. +fn protover_all_supported_should_return_empty_string_for_weird_thing() { + let proto: UnvalidatedProtoEntry = "Fribble=".parse().unwrap(); + let result: Option<UnvalidatedProtoEntry> = proto.all_supported(); + + assert!(result.is_none()); +} + +#[test] +fn protover_unvalidatedprotoentry_should_err_entirely_unparseable_things() { + let proto: Result<UnvalidatedProtoEntry, ProtoverError> = "Fribble".parse(); + + assert_eq!(Err(ProtoverError::Unparseable), proto); +} + +#[test] +fn protover_all_supported_over_maximum_limit() { + let proto: Result<UnvalidatedProtoEntry, ProtoverError> = "Sleen=0-4294967295".parse(); + + assert_eq!(Err(ProtoverError::ExceedsMax), proto); }

1 0

[tor/release-0.3.3] add a missing word
by nickm＠torproject.org 04 Apr '18

04 Apr '18

commit 7ccb1c5a859873656ab074c88935865bcf4b4c38 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Apr 3 15:31:30 2018 -0400 add a missing word --- changes/bug24031 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changes/bug24031 b/changes/bug24031 index adffa46d8..2bb0e8309 100644 --- a/changes/bug24031 +++ b/changes/bug24031 @@ -10,4 +10,4 @@ in outside crates and avoid mistakenly passing an internal error string to C over the FFI boundary. Many tests were added, and some previous differences between the C and Rust implementations have been - remedied. Fixes 24031; bugfix on 0.3.3.1-alpha. + remedied. Fixes bug 24031; bugfix on 0.3.3.1-alpha.

1 0