From ba54aca3a47df0e8c7afde968bd5f7479372a404 Mon Sep 17 00:00:00 2001 From: Dirk Engling Date: Sat, 24 Oct 2026 03:43:34 +0200 Subject: factor out accesslist parser to fix a potential out of bound read and add tests to the accesslist unit --- Makefile | 9 ++-- ot_accesslist.c | 63 +++++++++++++------------- ot_accesslist.h | 4 ++ tests/runner.c | 1 + tests/stubs.c | 20 +++++++++ tests/t_accesslist.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++++++ tests/test.h | 1 + 7 files changed, 187 insertions(+), 33 deletions(-) create mode 100644 tests/t_accesslist.c diff --git a/Makefile b/Makefile index a6b0b9b..e96269f 100644 --- a/Makefile +++ b/Makefile @@ -105,10 +105,13 @@ proxy.debug: $(OBJECTS_proxy_debug) $(HEADERS) # has to. tests/stubs.c supplies the few symbols those modules reference but # the tests never reach. TESTBIN=tests/runner -TEST_SOURCES=tests/runner.c tests/test.c tests/stubs.c tests/t_scan_urlencoded_query.c tests/t_iovec.c tests/t_vector.c tests/t_random.c tests/t_hex.c -TEST_UNITS=scan_urlencoded_query.c ot_iovec.c ot_vector.c ot_random.c ot_hex.c +TEST_SOURCES=tests/runner.c tests/test.c tests/stubs.c tests/t_scan_urlencoded_query.c tests/t_iovec.c tests/t_vector.c tests/t_random.c tests/t_hex.c tests/t_accesslist.c +TEST_UNITS=scan_urlencoded_query.c ot_iovec.c ot_vector.c ot_random.c ot_hex.c ot_accesslist.c TEST_HEADERS=tests/test.h $(HEADERS) -CFLAGS_test = $(CFLAGS) -I. -g -O1 $(FEATURES) +# Tests reach code that sits behind feature flags, so the units under test are +# built with those features enabled. A test binary is not a production binary. +FEATURES_test=$(FEATURES) -DWANT_ACCESSLIST_BLACK +CFLAGS_test = $(CFLAGS) -I. -g -O1 $(FEATURES_test) $(TESTBIN): $(TEST_SOURCES) $(TEST_UNITS) $(TEST_HEADERS) $(CC) -o $@ $(CFLAGS_test) $(TEST_SOURCES) $(TEST_UNITS) diff --git a/ot_accesslist.c b/ot_accesslist.c index ee6adb2..9b0024c 100644 --- a/ot_accesslist.c +++ b/ot_accesslist.c @@ -98,12 +98,33 @@ static void accesslist_clean(ot_accesslist *accesslist) { } } -/* Read initial access list */ +/* A hash needs forty bytes, and all but a final one are followed by at least + one separator, so n hashes need at least 41n - 1 bytes. (len + 1) / 41 is + therefore an exact upper bound on what a buffer of len bytes can hold. */ +size_t accesslist_parse_hashes(ot_hash *dest, size_t capacity, const char *buf, size_t len) { + const char *end = buf + len; + const char *offs = buf; + size_t count = 0; + + while (count < capacity && end - offs >= 40) { + if (hex_tohash(dest[count], offs)) { + offs += 40; + + if (offs == end || !hex_isdigit((unsigned char)*offs)) + ++count; + } + + /* Find start of next line */ + while (offs < end && *(offs++) != '\n') {} + } + + return count; +} + static void accesslist_readfile(void) { ot_accesslist *accesslist_new; - ot_hash *info_hash; - const char *map, *map_end, *read_offs; - size_t maplen; + const char *map; + size_t maplen, capacity, found; if ((map = mmap_read(g_accesslist_filename, &maplen)) == NULL) { char *wd = getcwd(NULL, 0); @@ -112,42 +133,24 @@ static void accesslist_readfile(void) { return; } - /* You need at least 41 bytes to pass an info_hash, make enough room - for the maximum amount of them */ - accesslist_new = accesslist_make(g_accesslist, maplen / 41); + /* Make room for the most info_hashes this file could possibly hold */ + capacity = (maplen + 1) / 41; + accesslist_new = accesslist_make(g_accesslist, capacity); if (!accesslist_new) { - fprintf(stderr, "Warning: Not enough memory to allocate %zd bytes for accesslist buffer. May succeed later.\n", (maplen / 41) * 20); + fprintf(stderr, "Warning: Not enough memory to allocate %zd bytes for accesslist buffer. May succeed later.\n", capacity * sizeof(ot_hash)); mmap_unmap(map, maplen); return; } - info_hash = accesslist_new->list; - /* No use to scan if there's not enough room for another full info_hash */ - map_end = map + maplen - 40; - read_offs = map; - - /* We do ignore anything that is not of the form "^[:xdigit:]{40}[^:xdigit:].*" */ - while (read_offs <= map_end) { - if (hex_tohash(*info_hash, read_offs)) { - read_offs += 40; - - /* Append accesslist to accesslist vector */ - if (read_offs == map_end || !hex_isdigit((unsigned char)*read_offs)) - ++info_hash; - } - - /* Find start of next line */ - while (read_offs <= map_end && *(read_offs++) != '\n') - ; - } + found = accesslist_parse_hashes(accesslist_new->list, capacity, map, maplen); #ifdef _DEBUG - fprintf(stderr, "Added %zd info_hashes to accesslist\n", (size_t)(info_hash - accesslist_new->list)); + fprintf(stderr, "Added %zd info_hashes to accesslist\n", found); #endif mmap_unmap(map, maplen); - qsort(accesslist_new->list, info_hash - accesslist_new->list, sizeof(*info_hash), vector_compare_hash); - accesslist_new->size = info_hash - accesslist_new->list; + qsort(accesslist_new->list, found, sizeof(ot_hash), vector_compare_hash); + accesslist_new->size = found; /* Now exchange the accesslist vector in the least race condition prone way */ pthread_mutex_lock(&g_accesslist_mutex); diff --git a/ot_accesslist.h b/ot_accesslist.h index d05e1f7..b3e979d 100644 --- a/ot_accesslist.h +++ b/ot_accesslist.h @@ -19,6 +19,10 @@ void accesslist_deinit(void); int accesslist_hashisvalid(ot_hash hash); void accesslist_cleanup(void); +/* Scan buf for up to capacity info_hashes in hexadecimal notation, one per + line, and return how many were written to dest */ +size_t accesslist_parse_hashes(ot_hash *dest, size_t capacity, const char *buf, size_t len); + extern char *g_accesslist_filename; #ifdef WANT_DYNAMIC_ACCESSLIST extern char *g_accesslist_pipe_add; diff --git a/tests/runner.c b/tests/runner.c index c9385f0..bf4fbde 100644 --- a/tests/runner.c +++ b/tests/runner.c @@ -19,6 +19,7 @@ static const struct module modules[] = {{"scan_urlencoded_query", test_scan_urle {"ot_vector", test_vector}, {"ot_random", test_random}, {"ot_hex", test_hex}, + {"ot_accesslist", test_accesslist}, {NULL, NULL}}; /* Run every module, or only those whose name contains argv[1] */ diff --git a/tests/stubs.c b/tests/stubs.c index ae40306..febdd7b 100644 --- a/tests/stubs.c +++ b/tests/stubs.c @@ -12,3 +12,23 @@ /* ot_vector.c calls this from vector_clean_list, which no test drives yet. */ void free_peerlist(ot_peerlist *peer_list) { (void)peer_list; } + +/* ot_accesslist.c is built with WANT_ACCESSLIST_BLACK so that the hash parser + is reachable. These four belong to accesslist_readfile and the reload + worker, neither of which any test drives. */ +time_t g_now_seconds; +volatile int g_opentracker_running; + +const char *mmap_read(const char *filename, size_t *filesize); +const char *mmap_read(const char *filename, size_t *filesize) { + (void)filename; + *filesize = 0; + return NULL; +} + +int mmap_unmap(const char *mapped, size_t maplen); +int mmap_unmap(const char *mapped, size_t maplen) { + (void)mapped; + (void)maplen; + return 0; +} diff --git a/tests/t_accesslist.c b/tests/t_accesslist.c new file mode 100644 index 0000000..9925acd --- /dev/null +++ b/tests/t_accesslist.c @@ -0,0 +1,122 @@ +/* This software was written by Dirk Engling + It is considered beerware. Prost. Skol. Cheers or whatever. + + $id$ */ + +/* System */ +#include +#include + +/* Opentracker */ +#include "ot_accesslist.h" +#include "test.h" + +#define A "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" +#define B "BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB" +#define C "CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC" + +static ot_hash dest[8]; + +/* The parser is handed a mapped file, so it must never look at a byte beyond + the length it was given. These cases place a hex digit immediately after + that length: a parser that reads one byte too far sees it, decides the hash + runs on, and silently drops the last entry. */ + +static void case_empty(void) { + test_case("an empty buffer yields nothing"); + CHECK_INT(accesslist_parse_hashes(dest, 8, "", 0), 0); +} + +static void case_single_with_newline(void) { + const char buf[] = A "\n"; + + test_case("one hash followed by a newline"); + CHECK_INT(accesslist_parse_hashes(dest, 8, buf, 41), 1); +} + +static void case_single_without_newline(void) { + const char buf[] = A "A"; + + test_case("one hash ending the buffer, with a hex digit just past the end"); + CHECK_INT(accesslist_parse_hashes(dest, 8, buf, 40), 1); +} + +static void case_several_last_without_newline(void) { + const char buf[] = A "\n" B "\n" C "C"; + + test_case("a final hash without a trailing newline still counts"); + CHECK_INT(accesslist_parse_hashes(dest, 8, buf, 3 * 40 + 2), 3); +} + +static void case_run_on_hash_is_rejected(void) { + const char buf[] = A "A\n"; + + test_case("forty one hex digits are not a hash"); + CHECK_INT(accesslist_parse_hashes(dest, 8, buf, 42), 0); +} + +static void case_junk_lines_skipped(void) { + const char buf[] = "# a comment\n" A "\nnot a hash\n" B "\n"; + + test_case("lines that are not hashes are ignored"); + CHECK_INT(accesslist_parse_hashes(dest, 8, buf, sizeof(buf) - 1), 2); +} + +static void case_trailing_junk_on_line(void) { + const char buf[] = A " some comment\n" B "\n"; + + test_case("a hash followed by other text on the same line still counts"); + CHECK_INT(accesslist_parse_hashes(dest, 8, buf, sizeof(buf) - 1), 2); +} + +static void case_capacity_is_respected(void) { + const char buf[] = A "\n" B "\n" C "\n"; + + test_case("no more than capacity hashes are written"); + CHECK_INT(accesslist_parse_hashes(dest, 2, buf, sizeof(buf) - 1), 2); +} + +static void case_zero_capacity_never_writes(void) { + const char buf[] = A "\n"; + + test_case("a zero capacity tolerates a null destination"); + CHECK_INT(accesslist_parse_hashes(NULL, 0, buf, 41), 0); +} + +static void case_decoded_value(void) { + const char buf[] = "000102030405060708090A0B0C0D0E0F10111213\n"; + const uint8_t want[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19}; + + test_case("the hash is decoded, not merely counted"); + CHECK_INT(accesslist_parse_hashes(dest, 8, buf, 41), 1); + CHECK_MEM(dest[0], want, sizeof(want)); +} + +static void case_capacity_bound_holds(void) { + /* (len + 1) / 41 is what accesslist_readfile allocates. Pack a buffer as + densely as the parser allows and check the bound is not exceeded. */ + char buf[41 * 4]; + size_t len, capacity, found; + + test_case("the capacity formula bounds what the parser can produce"); + memcpy(buf, A "\n" B "\n" C "\n" A, 4 * 40 + 3); + len = 4 * 40 + 3; + capacity = (len + 1) / 41; + found = accesslist_parse_hashes(dest, 8, buf, len); + CHECK_INT(found, 4); + CHECK(found <= capacity); +} + +void test_accesslist(void) { + case_empty(); + case_single_with_newline(); + case_single_without_newline(); + case_several_last_without_newline(); + case_run_on_hash_is_rejected(); + case_junk_lines_skipped(); + case_trailing_junk_on_line(); + case_capacity_is_respected(); + case_zero_capacity_never_writes(); + case_decoded_value(); + case_capacity_bound_holds(); +} diff --git a/tests/test.h b/tests/test.h index 6af0083..8be624b 100644 --- a/tests/test.h +++ b/tests/test.h @@ -81,5 +81,6 @@ void test_iovec(void); void test_vector(void); void test_random(void); void test_hex(void); +void test_accesslist(void); #endif -- cgit v1.3