commit 056be68b1b5a727bb6cb26d98f37bfa131f76701 Author: Isis Lovecruft <isis@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);