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
This commit is contained in:
Isis Lovecruft 2018-03-29 01:54:05 +00:00 committed by Nick Mathewson
parent add00045aa
commit 056be68b1b
2 changed files with 46 additions and 0 deletions

View File

@ -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) {

View File

@ -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);