From 65f2eec694f18a64291cc85317b9f22dacc1d8e4 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 1 Feb 2018 16:33:52 -0500 Subject: [PATCH 1/8] Correctly handle NULL returns from parse_protocol_list when voting. In some cases we had checked for it, but in others we had not. One of these cases could have been used to remotely cause denial-of-service against directory authorities while they attempted to vote. Fixes TROVE-2018-001. --- changes/trove-2018-001.1 | 6 ++++++ src/or/protover.c | 6 ++++++ 2 files changed, 12 insertions(+) create mode 100644 changes/trove-2018-001.1 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/src/or/protover.c b/src/or/protover.c index 98957cabd..a75077462 100644 --- a/src/or/protover.c +++ b/src/or/protover.c @@ -554,6 +554,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 " From a83650852d3cd00c9916cae74d755ae55a6b506d Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 14 Feb 2018 10:45:57 -0500 Subject: [PATCH 2/8] Add another NULL-pointer fix for protover.c. This one can only be exploited if you can generate a correctly signed consensus, so it's not as bad as 25074. Fixes bug 25251; also tracked as TROVE-2018-004. --- changes/trove-2018-004 | 8 ++++++++ src/or/protover.c | 5 +++++ 2 files changed, 13 insertions(+) create mode 100644 changes/trove-2018-004 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 a75077462..e63036f78 100644 --- a/src/or/protover.c +++ b/src/or/protover.c @@ -624,6 +624,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(); From d3a1bdbf56c7e41b54fdd1d3169287ca761698a6 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 14 Feb 2018 11:47:05 -0500 Subject: [PATCH 3/8] Add some protover vote round-trip tests from Teor. I've refactored these to be a separate function, to avoid tricky merge conflicts. Some of these are disabled with "XXXX" comments; they should get fixed moving forward. --- src/test/test_protover.c | 76 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/src/test/test_protover.c b/src/test/test_protover.c index f00955d1b..5e65c019e 100644 --- a/src/test/test_protover.c +++ b/src/test/test_protover.c @@ -182,6 +182,81 @@ test_protover_all_supported(void *arg) 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 }, + // "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) \ { #name, test_protover_ ##name, (flags), NULL, NULL } @@ -190,6 +265,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 }; From 0953c43c955c4bdb82f0aa86f23f9c0cdcc2c9a1 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 14 Feb 2018 11:56:03 -0500 Subject: [PATCH 4/8] Add more of Teor's protover tests. These are as Teor wrote them; I've disabled the ones that don't pass yet, with XXXX comments. --- src/test/test_protover.c | 84 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/src/test/test_protover.c b/src/test/test_protover.c index 5e65c019e..609003a83 100644 --- a/src/test/test_protover.c +++ b/src/test/test_protover.c @@ -139,6 +139,80 @@ 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 in Rust, but not in C */ + smartlist_clear(lst); + smartlist_add(lst, (void*) "Faux=-0"); + result = protover_compute_vote(lst, 1); + tt_str_op(result, OP_EQ, "Faux=0"); + 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: Rust */ +#if 0 // XXXXX + smartlist_clear(lst); + smartlist_add(lst, (void*) "Sleen=1-500"); + smartlist_add(lst, (void*) "Sleen=1000"); + result = protover_compute_vote(lst, 1); + tt_str_op(result, OP_EQ, ""); + tor_free(result); +#endif + + /* 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, ""); // XXXX "Sleen=4294967295"); + 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,6 +252,16 @@ test_protover_all_supported(void *arg) tt_str_op(msg, OP_EQ, "Link=3-999"); tor_free(msg); + /* 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); + + /* Rust seems to experience an internal error here */ + tt_assert(! protover_all_supported("Sleen=0-4294967295", &msg)); + tt_str_op(msg, OP_EQ, "Sleen=0-4294967295"); + tor_free(msg); + done: tor_free(msg); } From 8b405c609e82fbfb5470967fc4c45165c708e72b Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 15 Feb 2018 08:46:13 -0500 Subject: [PATCH 5/8] Forbid "-0" as a protocol version. Fixes part of 24249; bugfix on 0.2.9.4-alpha. --- changes/bug25249 | 3 +++ src/or/protover.c | 9 +++++++++ src/test/test_protover.c | 6 ++++-- 3 files changed, 16 insertions(+), 2 deletions(-) create mode 100644 changes/bug25249 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/src/or/protover.c b/src/or/protover.c index e63036f78..f32316f8e 100644 --- a/src/or/protover.c +++ b/src/or/protover.c @@ -123,6 +123,11 @@ 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); @@ -138,7 +143,11 @@ parse_version_range(const char *s, const char *end_of_range, if (*next != '-') goto error; s = next+1; + /* ibid */ + if (!TOR_ISDIGIT(*s)) { + goto error; + } high = (uint32_t) tor_parse_ulong(s, 10, 0, UINT32_MAX, &ok, &next); if (!ok) goto error; diff --git a/src/test/test_protover.c b/src/test/test_protover.c index 609003a83..4c41b6db6 100644 --- a/src/test/test_protover.c +++ b/src/test/test_protover.c @@ -151,11 +151,11 @@ test_protover_vote(void *arg) tt_str_op(result, OP_EQ, ""); tor_free(result); - /* This fails in Rust, but not in C */ + /* 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, "Faux=0"); + tt_str_op(result, OP_EQ, ""); tor_free(result); /* Vote large protover lists that are just below the threshold */ @@ -301,6 +301,8 @@ test_protover_vote_roundtrip(void *args) { "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 }, From 1fe0bae508120bbf4954de6b590dd0c722a883bc Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 15 Feb 2018 09:05:55 -0500 Subject: [PATCH 6/8] Forbid UINT32_MAX as a protocol version The C code and the rust code had different separate integer overflow bugs here. That suggests that we're better off just forbidding this pathological case. Also, add tests for expected behavior on receiving a bad protocol list in a consensus. Fixes another part of 25249. --- changes/bug25249.2 | 3 +++ src/or/protover.c | 8 ++++++-- src/test/test_protover.c | 21 ++++++++++++++++++--- 3 files changed, 27 insertions(+), 5 deletions(-) create mode 100644 changes/bug25249.2 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/src/or/protover.c b/src/or/protover.c index f32316f8e..a035b5c83 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 @@ -130,7 +133,7 @@ parse_version_range(const char *s, const char *end_of_range, /* 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) @@ -148,7 +151,8 @@ parse_version_range(const char *s, const char *end_of_range, if (!TOR_ISDIGIT(*s)) { goto error; } - high = (uint32_t) tor_parse_ulong(s, 10, 0, UINT32_MAX, &ok, &next); + high = (uint32_t) tor_parse_ulong(s, 10, 0, + MAX_PROTOCOL_VERSION, &ok, &next); if (!ok) goto error; if (next != end_of_range) diff --git a/src/test/test_protover.c b/src/test/test_protover.c index 4c41b6db6..8d061c69c 100644 --- a/src/test/test_protover.c +++ b/src/test/test_protover.c @@ -257,12 +257,27 @@ test_protover_all_supported(void *arg) tt_str_op(msg, OP_EQ, "Sleen=0-2147483648"); tor_free(msg); - /* Rust seems to experience an internal error here */ - tt_assert(! protover_all_supported("Sleen=0-4294967295", &msg)); - tt_str_op(msg, OP_EQ, "Sleen=0-4294967295"); + /* 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." */ + 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_(); + done: + tor_end_capture_bugs_(); tor_free(msg); } From c5295cc1beb1c13743e11d3a7134e9e75b6da470 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 15 Feb 2018 10:49:47 -0500 Subject: [PATCH 7/8] Spec conformance on protover: always reject ranges where lo>hi --- src/or/protover.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/or/protover.c b/src/or/protover.c index a035b5c83..0c79037f6 100644 --- a/src/or/protover.c +++ b/src/or/protover.c @@ -158,6 +158,9 @@ parse_version_range(const char *s, const char *end_of_range, if (next != end_of_range) goto error; + if (low > high) + goto error; + done: *high_out = high; *low_out = low; @@ -208,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; From c1bb8836ff6b3abe32a17ffb3ae25cfdc770ceb4 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 15 Feb 2018 18:56:29 -0500 Subject: [PATCH 8/8] Protover tests: disable some obsoleted tests These were meant to demonstrate old behavior, or old rust behavior. One of them _should_ work in Rust, but won't because of implementation details. We'll fix that up later. --- src/test/test_protover.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/test/test_protover.c b/src/test/test_protover.c index 8d061c69c..92ead3ca3 100644 --- a/src/test/test_protover.c +++ b/src/test/test_protover.c @@ -176,16 +176,6 @@ test_protover_vote(void *arg) /* Large protover lists that exceed the threshold */ - /* By adding two votes: Rust */ -#if 0 // XXXXX - smartlist_clear(lst); - smartlist_add(lst, (void*) "Sleen=1-500"); - smartlist_add(lst, (void*) "Sleen=1000"); - result = protover_compute_vote(lst, 1); - tt_str_op(result, OP_EQ, ""); - tor_free(result); -#endif - /* By adding two votes, C allows us to exceed the limit */ smartlist_add(lst, (void*) "Sleen=1-65536"); smartlist_add(lst, (void*) "Sleen=100000"); @@ -204,7 +194,7 @@ test_protover_vote(void *arg) smartlist_clear(lst); smartlist_add(lst, (void*) "Sleen=4294967295"); result = protover_compute_vote(lst, 1); - tt_str_op(result, OP_EQ, ""); // XXXX "Sleen=4294967295"); + tt_str_op(result, OP_EQ, ""); tor_free(result); smartlist_clear(lst); @@ -263,6 +253,9 @@ test_protover_all_supported(void *arg) 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); @@ -275,6 +268,7 @@ test_protover_all_supported(void *arg) tt_ptr_op(msg, OP_EQ, NULL); tor_free(msg); tor_end_capture_bugs_(); +#endif done: tor_end_capture_bugs_();