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

[tor/master] rust: Refactor Rust impl of protover_compute_vote().
by nickm＠torproject.org 03 Apr '18

03 Apr '18

commit 32638ed4a6c47a03b899cd09582888674ae3bf08 Author: Isis Lovecruft <isis(a)torproject.org> Date: Wed Mar 21 03:05:56 2018 +0000 rust: Refactor Rust impl of protover_compute_vote(). This includes a subtle difference in behaviour to the previous Rust implementation, where, for each vote that we're computing over, if a single one fails to parse, we skip it. This now matches the current behaviour in the C implementation. * REFACTOR `protover::ffi::protover_compute_vote()` to use new types and methods. --- src/rust/protover/ffi.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/rust/protover/ffi.rs b/src/rust/protover/ffi.rs index e7c821116..13d64821f 100644 --- a/src/rust/protover/ffi.rs +++ b/src/rust/protover/ffi.rs @@ -175,6 +175,8 @@ pub extern "C" fn protover_get_supported_protocols() -> *const c_char { /// Provide an interface for C to translate arguments and return types for /// protover::compute_vote +// +// Why is the threshold a signed integer? —isis #[no_mangle] pub extern "C" fn protover_compute_vote( list: *const Stringlist, @@ -189,10 +191,19 @@ pub extern "C" fn protover_compute_vote( // Dereference of raw pointer requires an unsafe block. The pointer is // checked above to ensure it is not null. let data: Vec<String> = unsafe { (*list).get_list() }; + let hold: usize = threshold as usize; + let mut proto_entries: Vec<UnvalidatedProtoEntry> = Vec::new(); - let vote = compute_vote(data, threshold); + for datum in data { + let entry: UnvalidatedProtoEntry = match datum.parse() { + Ok(x) => x, + Err(_) => continue, + }; + proto_entries.push(entry); + } + let vote: UnvalidatedProtoEntry = ProtoverVote::compute(&proto_entries, &hold); - allocate_and_copy_string(&vote) + allocate_and_copy_string(&vote.to_string()) } /// Provide an interface for C to translate arguments and return types for

1 0

[tor/master] rust: Refactor Rust impl of protover_all_supported().
by nickm＠torproject.org 03 Apr '18

03 Apr '18

commit c7bcca0233d1d4c9805f78da5e7186be2c1bcdca Author: Isis Lovecruft <isis(a)torproject.org> Date: Wed Mar 21 02:45:41 2018 +0000 rust: Refactor Rust impl of protover_all_supported(). This includes differences in behaviour to before, which should now more closely match the C version: - If parsing a protover `char*` from C, and the string is not parseable, this function will return 1 early, which matches the C behaviour when protocols are unparseable. Previously, we would parse it and its version numbers simultaneously, i.e. there was no fail early option, causing us to spend more time unnecessarily parsing versions. * REFACTOR `protover::ffi::protover_all_supported()` to use new types and methods. --- src/rust/protover/ffi.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/rust/protover/ffi.rs b/src/rust/protover/ffi.rs index ce2837832..c17696803 100644 --- a/src/rust/protover/ffi.rs +++ b/src/rust/protover/ffi.rs @@ -59,19 +59,26 @@ pub extern "C" fn protover_all_supported( Err(_) => return 1, }; - let (is_supported, unsupported) = all_supported(relay_version); + let relay_proto_entry: UnvalidatedProtoEntry = match relay_version.parse() { + Ok(n) => n, + Err(_) => return 1, + }; + let maybe_unsupported: Option<UnvalidatedProtoEntry> = relay_proto_entry.all_supported(); - if unsupported.len() > 0 { - let c_unsupported = match CString::new(unsupported) { + if maybe_unsupported.is_some() { + let unsupported: UnvalidatedProtoEntry = maybe_unsupported.unwrap(); + let c_unsupported: CString = match CString::new(unsupported.to_string()) { Ok(n) => n, Err(_) => return 1, }; let ptr = c_unsupported.into_raw(); unsafe { *missing_out = ptr }; + + return 0; } - return if is_supported { 1 } else { 0 }; + 1 } /// Provide an interface for C to translate arguments and return types for

1 0

[tor/master] rust: Refactor Rust impl of protover_list_supports_protocol_or_later().
by nickm＠torproject.org 03 Apr '18

03 Apr '18

commit 269053a3801ebe925707db5a8e519836ad2242c9 Author: Isis Lovecruft <isis(a)torproject.org> Date: Wed Mar 21 02:59:25 2018 +0000 rust: Refactor Rust impl of protover_list_supports_protocol_or_later(). This includes a subtle difference in behaviour, as in 4258f1e18, where we return (matching the C impl's return behaviour) earlier than before if parsing failed, saving us computation in parsing the versions into a protover::protoset::ProtoSet. * REFACTOR `protover::ffi::protover_list_supports_protocol_or_later()` to use new types and methods. --- src/rust/protover/ffi.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/rust/protover/ffi.rs b/src/rust/protover/ffi.rs index d9365bdd7..e7c821116 100644 --- a/src/rust/protover/ffi.rs +++ b/src/rust/protover/ffi.rs @@ -141,11 +141,15 @@ pub extern "C" fn protocol_list_supports_protocol_or_later( Err(_) => return 0, }; - let is_supported = - protover_string_supports_protocol_or_later( - protocol_list, protocol, version); + let proto_entry: UnvalidatedProtoEntry = match protocol_list.parse() { + Ok(n) => n, + Err(_) => return 1, + }; - return if is_supported { 1 } else { 0 }; + if proto_entry.supports_protocol_or_later(&protocol.into(), &version) { + return 1; + } + 0 } /// Provide an interface for C to translate arguments and return types for

1 0

[tor/master] tests: Make inline comments in test_protover.c more accurate.
by nickm＠torproject.org 03 Apr '18

03 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/master] protover: Change protover_all_supported() to return only unsupported.
by nickm＠torproject.org 03 Apr '18

03 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/master] Merge remote-tracking branch 'isis-github/bug24031_r5_squashed'
by nickm＠torproject.org 03 Apr '18

03 Apr '18

commit 29b3b485b9722ceead4b931af521b999d3c2aeb3 Merge: c2c616eb1 5a8cdec3f Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Apr 3 15:33:50 2018 -0400 Merge remote-tracking branch 'isis-github/bug24031_r5_squashed' changes/bug24031 | 13 + src/or/protover.c | 77 ++- src/rust/protover/errors.rs | 43 ++ src/rust/protover/ffi.rs | 96 ++- src/rust/protover/lib.rs | 7 +- src/rust/protover/protoset.rs | 634 +++++++++++++++++ src/rust/protover/protover.rs | 1289 ++++++++++++++++------------------- src/rust/protover/tests/protover.rs | 421 +++++++----- src/test/test_protover.c | 44 +- 9 files changed, 1684 insertions(+), 940 deletions(-)

1 0

[tor/master] changes: Add changes file for #24031.
by nickm＠torproject.org 03 Apr '18

03 Apr '18

commit 5a8cdec3f8617920f19e3ab7707233ad3f02424f Author: Isis Lovecruft <isis(a)torproject.org> Date: Tue Apr 3 19:19:40 2018 +0000 changes: Add changes file for #24031. --- 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/master] add a missing word
by nickm＠torproject.org 03 Apr '18

03 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

[tor/master] changes: Add changes file for #24031.
by nickm＠torproject.org 03 Apr '18

03 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/master] Merge branch 'maint-0.3.3'
by nickm＠torproject.org 03 Apr '18

03 Apr '18

commit c2c616eb1958e32995577bdaa5f2c9507fd09c22 Merge: 3df954549 7ccb1c5a8 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Apr 3 15:33:29 2018 -0400 Merge branch 'maint-0.3.3'

1 0