From 8905789170269d29ad87530642ecc2a6a891219b Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 23 Oct 2012 17:12:37 -0400 Subject: [PATCH 1/4] Fix binary search on lists of 0 or 1 element. The implementation we added has a tendency to crash with lists of 0 or one element. That can happen if we get a consensus vote, v2 consensus, consensus, or geoip file with 0 or 1 element. There's a DOS opportunity there that authorities could exploit against one another, and which an evil v2 authority could exploit against anything downloading v2 directory information.. This fix is minimalistic: It just adds a special-case for 0- and 1-element lists. For 0.2.4 (the current alpha series) we'll want a better patch. This is bug 7191; it's a fix on 0.2.0.10-alpha. --- src/common/container.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/common/container.c b/src/common/container.c index 5f53222374..c047562cd0 100644 --- a/src/common/container.c +++ b/src/common/container.c @@ -572,7 +572,28 @@ smartlist_bsearch_idx(const smartlist_t *sl, const void *key, int (*compare)(const void *key, const void **member), int *found_out) { - int hi = smartlist_len(sl) - 1, lo = 0, cmp, mid; + const int len = smartlist_len(sl); + int hi, lo, cmp, mid; + + if (len == 0) { + *found_out = 0; + return 0; + } else if (len == 1) { + cmp = compare(key, (const void **) &sl->list[0]); + if (cmp == 0) { + *found_out = 1; + return 0; + } else if (cmp < 0) { + *found_out = 0; + return 0; + } else { + *found_out = 0; + return 1; + } + } + + hi = smartlist_len(sl) - 1; + lo = 0; while (lo <= hi) { mid = (lo + hi) / 2; From 3365def68b91cb2bd9e741df679777155b746cf3 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 23 Oct 2012 21:31:42 -0400 Subject: [PATCH 2/4] Add a changes file for bug 7191. --- changes/bug7191 | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changes/bug7191 diff --git a/changes/bug7191 b/changes/bug7191 new file mode 100644 index 0000000000..a3bee6e5f7 --- /dev/null +++ b/changes/bug7191 @@ -0,0 +1,5 @@ + o Major bugfixes: + - Fix a denial of service attack by which any directory authority + could crash all the others, or by which a single v2 directory + authority could crash everybody downloading v2 directory + information. Fixes bug 7191; bugfix on 0.2.0.10-alpha. From cb693ef56e5fcef2a6d1844cb71c849022628bd8 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Tue, 23 Oct 2012 14:28:19 -0700 Subject: [PATCH 3/4] Add some unit tests for smartlist_bsearch_idx() on short lists Conflicts: src/test/test_containers.c --- src/test/test_containers.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/src/test/test_containers.c b/src/test/test_containers.c index af9fb1c5c9..46cc82d6c3 100644 --- a/src/test/test_containers.c +++ b/src/test/test_containers.c @@ -16,6 +16,15 @@ _compare_strs(const void **a, const void **b) return strcmp(s1, s2); } +/** Helper: return a tristate based on comparing the strings in a and + * *b. */ +static int +compare_strs_for_bsearch_(const void *a, const void **b) +{ + const char *s1 = a, *s2 = *b; + return strcmp(s1, s2); +} + /** Helper: return a tristate based on comparing the strings in *a and * *b, excluding a's first character, and ignoring case. */ static int @@ -204,6 +213,8 @@ test_container_smartlist_strings(void) /* Test bsearch_idx */ { int f; + smartlist_t *tmp = NULL; + test_eq(0, smartlist_bsearch_idx(sl," aaa",_compare_without_first_ch,&f)); test_eq(f, 0); test_eq(0, smartlist_bsearch_idx(sl," and",_compare_without_first_ch,&f)); @@ -216,6 +227,31 @@ test_container_smartlist_strings(void) test_eq(f, 0); test_eq(7, smartlist_bsearch_idx(sl," zzzz",_compare_without_first_ch,&f)); test_eq(f, 0); + + /* Test trivial cases for list of length 0 or 1 */ + tmp = smartlist_create(); + test_eq(0, smartlist_bsearch_idx(tmp, "foo", + compare_strs_for_bsearch_, &f)); + test_eq(f, 0); + smartlist_insert(tmp, 0, (void *)("bar")); + test_eq(1, smartlist_bsearch_idx(tmp, "foo", + compare_strs_for_bsearch_, &f)); + test_eq(f, 0); + test_eq(0, smartlist_bsearch_idx(tmp, "aaa", + compare_strs_for_bsearch_, &f)); + test_eq(f, 0); + test_eq(0, smartlist_bsearch_idx(tmp, "bar", + compare_strs_for_bsearch_, &f)); + test_eq(f, 1); + /* ... and one for length 2 */ + smartlist_insert(tmp, 1, (void *)("foo")); + test_eq(1, smartlist_bsearch_idx(tmp, "foo", + compare_strs_for_bsearch_, &f)); + test_eq(f, 1); + test_eq(2, smartlist_bsearch_idx(tmp, "goo", + compare_strs_for_bsearch_, &f)); + test_eq(f, 0); + smartlist_free(tmp); } /* Test reverse() and pop_last() */ From b99457d4295b2329e65f3e01b24b57d5c78ca017 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 23 Oct 2012 21:49:46 -0400 Subject: [PATCH 4/4] Make unit test for bug7191 work with new smartlist_new() name --- src/test/test_containers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/test_containers.c b/src/test/test_containers.c index 9a306ad797..45898df4eb 100644 --- a/src/test/test_containers.c +++ b/src/test/test_containers.c @@ -229,7 +229,7 @@ test_container_smartlist_strings(void) test_eq(f, 0); /* Test trivial cases for list of length 0 or 1 */ - tmp = smartlist_create(); + tmp = smartlist_new(); test_eq(0, smartlist_bsearch_idx(tmp, "foo", compare_strs_for_bsearch_, &f)); test_eq(f, 0);