diff --git a/changes/bug25249 b/changes/bug25249 new file mode 100644 index 000000000..b4153eeae --- /dev/null +++ b/changes/bug25249 @@ -0,0 +1,3 @@ + o Minor bugfixes (spec conformance): + - Forbid "-0" as a protocol version. Fixes part of bug 25249; bugfix on + 0.2.9.4-alpha. diff --git a/changes/bug25249.2 b/changes/bug25249.2 new file mode 100644 index 000000000..9058c1107 --- /dev/null +++ b/changes/bug25249.2 @@ -0,0 +1,3 @@ + o Minor bugfixes (spec conformance): + - Forbid UINT32_MAX as a protocol version. Fixes part of bug 25249; + bugfix on 0.2.9.4-alpha. diff --git a/changes/trove-2018-001.1 b/changes/trove-2018-001.1 new file mode 100644 index 000000000..f0ee92f40 --- /dev/null +++ b/changes/trove-2018-001.1 @@ -0,0 +1,6 @@ + o Major bugfixes (denial-of-service, directory authority): + - Fix a protocol-list handling bug that could be used to remotely crash + directory authorities with a null-pointer exception. Fixes bug 25074; + bugfix on 0.2.9.4-alpha. Also tracked as TROVE-2018-001. + + diff --git a/changes/trove-2018-004 b/changes/trove-2018-004 new file mode 100644 index 000000000..37e0a89b0 --- /dev/null +++ b/changes/trove-2018-004 @@ -0,0 +1,8 @@ + o Minor bugfixes (denial-of-service): + - Fix a possible crash on malformed consensus. If a consensus had + contained an unparseable protocol line, it could have made clients + and relays crash with a null-pointer exception. To exploit this + issue, however, an attacker would need to be able to subvert the + directory-authority system. Fixes bug 25251; bugfix on + 0.2.9.4-alpha. Also tracked as TROVE-2018-004. + diff --git a/src/or/protover.c b/src/or/protover.c index 033e9063a..45f0377d6 100644 --- a/src/or/protover.c +++ b/src/or/protover.c @@ -103,6 +103,9 @@ proto_entry_free(proto_entry_t *entry) tor_free(entry); } +/** The largest possible protocol version. */ +#define MAX_PROTOCOL_VERSION (UINT32_MAX-1) + /** * Given a string s and optional end-of-string pointer * end_of_range, parse the protocol range and store it in @@ -123,9 +126,14 @@ parse_version_range(const char *s, const char *end_of_range, if (BUG(!end_of_range)) end_of_range = s + strlen(s); // LCOV_EXCL_LINE + /* A range must start with a digit. */ + if (!TOR_ISDIGIT(*s)) { + goto error; + } + /* Note that this wouldn't be safe if we didn't know that eventually, * we'd hit a NUL */ - low = (uint32_t) tor_parse_ulong(s, 10, 0, UINT32_MAX, &ok, &next); + low = (uint32_t) tor_parse_ulong(s, 10, 0, MAX_PROTOCOL_VERSION, &ok, &next); if (!ok) goto error; if (next > end_of_range) @@ -138,13 +146,21 @@ parse_version_range(const char *s, const char *end_of_range, if (*next != '-') goto error; s = next+1; + /* ibid */ - high = (uint32_t) tor_parse_ulong(s, 10, 0, UINT32_MAX, &ok, &next); + if (!TOR_ISDIGIT(*s)) { + goto error; + } + high = (uint32_t) tor_parse_ulong(s, 10, 0, + MAX_PROTOCOL_VERSION, &ok, &next); if (!ok) goto error; if (next != end_of_range) goto error; + if (low > high) + goto error; + done: *high_out = high; *low_out = low; @@ -195,10 +211,6 @@ parse_single_entry(const char *s, const char *end_of_entry) goto error; } - if (range->low > range->high) { - goto error; - } - s = comma; while (*s == ',' && s < end_of_entry) ++s; @@ -554,6 +566,12 @@ protover_compute_vote(const smartlist_t *list_of_proto_strings, // First, parse the inputs and break them into singleton entries. SMARTLIST_FOREACH_BEGIN(list_of_proto_strings, const char *, vote) { smartlist_t *unexpanded = parse_protocol_list(vote); + if (! unexpanded) { + log_warn(LD_NET, "I failed with parsing a protocol list from " + "an authority. The offending string was: %s", + escaped(vote)); + continue; + } smartlist_t *this_vote = expand_protocol_list(unexpanded); if (this_vote == NULL) { log_warn(LD_NET, "When expanding a protocol list from an authority, I " @@ -618,6 +636,11 @@ protover_all_supported(const char *s, char **missing_out) } smartlist_t *entries = parse_protocol_list(s); + if (BUG(entries == NULL)) { + log_warn(LD_NET, "Received an unparseable protocol list %s" + " from the consensus", escaped(s)); + return 1; + } missing = smartlist_new(); diff --git a/src/test/test_protover.c b/src/test/test_protover.c index 562681602..c093e6996 100644 --- a/src/test/test_protover.c +++ b/src/test/test_protover.c @@ -139,6 +139,70 @@ test_protover_vote(void *arg) tt_str_op(result, OP_EQ, "Bar=3-6,8 Foo=9"); tor_free(result); + /* High threshold */ + result = protover_compute_vote(lst, 3); + tt_str_op(result, OP_EQ, ""); + tor_free(result); + + /* Bad votes: the result must be empty */ + smartlist_clear(lst); + smartlist_add(lst, (void*) "Faux=10-5"); + result = protover_compute_vote(lst, 1); + tt_str_op(result, OP_EQ, ""); + tor_free(result); + + /* This fails, since "-0" is not valid. */ + smartlist_clear(lst); + smartlist_add(lst, (void*) "Faux=-0"); + result = protover_compute_vote(lst, 1); + tt_str_op(result, OP_EQ, ""); + tor_free(result); + + /* Vote large protover lists that are just below the threshold */ + + /* Just below the threshold: Rust */ + smartlist_clear(lst); + smartlist_add(lst, (void*) "Sleen=1-500"); + result = protover_compute_vote(lst, 1); + tt_str_op(result, OP_EQ, "Sleen=1-500"); + tor_free(result); + + /* Just below the threshold: C */ + smartlist_clear(lst); + smartlist_add(lst, (void*) "Sleen=1-65536"); + result = protover_compute_vote(lst, 1); + tt_str_op(result, OP_EQ, "Sleen=1-65536"); + tor_free(result); + + /* Large protover lists that exceed the threshold */ + + /* By adding two votes, C allows us to exceed the limit */ + smartlist_add(lst, (void*) "Sleen=1-65536"); + smartlist_add(lst, (void*) "Sleen=100000"); + result = protover_compute_vote(lst, 1); + tt_str_op(result, OP_EQ, "Sleen=1-65536,100000"); + tor_free(result); + + /* Large integers */ + smartlist_clear(lst); + smartlist_add(lst, (void*) "Sleen=4294967294"); + result = protover_compute_vote(lst, 1); + tt_str_op(result, OP_EQ, "Sleen=4294967294"); + tor_free(result); + + /* This parses, but fails at the vote stage */ + smartlist_clear(lst); + smartlist_add(lst, (void*) "Sleen=4294967295"); + result = protover_compute_vote(lst, 1); + tt_str_op(result, OP_EQ, ""); + tor_free(result); + + smartlist_clear(lst); + smartlist_add(lst, (void*) "Sleen=4294967296"); + result = protover_compute_vote(lst, 1); + tt_str_op(result, OP_EQ, ""); + tor_free(result); + done: tor_free(result); smartlist_free(lst); @@ -178,8 +242,114 @@ test_protover_all_supported(void *arg) tt_str_op(msg, OP_EQ, "Link=3-999"); tor_free(msg); - done: + /* CPU/RAM DoS loop: Rust only */ + tt_assert(! protover_all_supported("Sleen=0-2147483648", &msg)); + tt_str_op(msg, OP_EQ, "Sleen=0-2147483648"); tor_free(msg); + + /* This case is allowed. */ + tt_assert(! protover_all_supported("Sleen=0-4294967294", &msg)); + 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! + 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. */ + 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_(); + tor_free(msg); +} + +static void +test_protover_vote_roundtrip(void *args) +{ + (void) args; + static const struct { + const char *input; + const char *expected_output; + } examples[] = { + { "Fkrkljdsf", NULL }, + { "Zn=4294967295", NULL }, + { "Zn=4294967295-1", NULL }, + { "Zn=4294967293-4294967295", NULL }, + /* Will fail because of 4294967295. */ + { "Foo=1,3 Bar=3 Baz= Quux=9-12,14,15-16,900 Zn=0,4294967295", + NULL }, + { "Foo=1,3 Bar=3 Baz= Quux=9-12,14,15-16,900 Zn=0,4294967294", + "Bar=3 Foo=1,3 Quux=9-12,14-16,900 Zn=0,4294967294" }, + { "Zu16=0,65536", "Zu16=0,65536" }, + { "N-1=1,2", "N-1=1-2" }, + { "-1=4294967295", NULL }, + { "-1=3", "-1=3" }, + /* junk. */ + { "!!3@*", NULL }, + /* Missing equals sign */ + { "Link=4 Haprauxymatyve Desc=9", NULL }, + { "Link=4 Haprauxymatyve=7 Desc=9", + "Desc=9 Haprauxymatyve=7 Link=4" }, + { "=10-11", NULL }, + { "X=10-11", "X=10-11" }, + { "Link=4 =3 Desc=9", NULL }, + { "Link=4 Z=3 Desc=9", "Desc=9 Link=4 Z=3" }, + { "Link=fred", NULL }, + { "Link=1,fred", NULL }, + { "Link=1,fred,3", NULL }, + { "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 }, + /* Large integers */ + { "Link=4294967296", NULL }, + /* Large range */ + { "Sleen=1-501", "Sleen=1-501" }, + { "Sleen=1-65537", NULL }, + /* CPU/RAM DoS Loop: Rust only. */ + { "Sleen=0-2147483648", NULL }, + /* Rust seems to experience an internal error here. */ + { "Sleen=0-4294967295", NULL }, + }; + unsigned u; + smartlist_t *votes = smartlist_new(); + char *result = NULL; + + for (u = 0; u < ARRAY_LENGTH(examples); ++u) { + const char *input = examples[u].input; + const char *expected_output = examples[u].expected_output; + + smartlist_add(votes, (void*)input); + result = protover_compute_vote(votes, 1); + if (expected_output != NULL) { + tt_str_op(result, OP_EQ, expected_output); + } else { + tt_str_op(result, OP_EQ, ""); + } + + smartlist_clear(votes); + tor_free(result); + } + + done: + smartlist_free(votes); + tor_free(result); } #define PV_TEST(name, flags) \ @@ -190,6 +360,7 @@ struct testcase_t protover_tests[] = { PV_TEST(parse_fail, 0), PV_TEST(vote, 0), PV_TEST(all_supported, 0), + PV_TEST(vote_roundtrip, 0), END_OF_TESTCASES };