diff --git a/Changes.md b/Changes.md index a93afe93..8f55ec5e 100644 --- a/Changes.md +++ b/Changes.md @@ -1,5 +1,59 @@ ## 1.13.0 +* `MMDB_get_entry_data_list()` now validates that the claimed array/map + size is plausible given the remaining bytes in the data section. A + crafted database could previously claim millions of array elements + while only having a few bytes of data, causing disproportionate memory + allocation (memory amplification DoS). +* On Windows, `GetFileSize()` was replaced with `GetFileSizeEx()` to + correctly handle files larger than 4GB. The previous code passed + `NULL` for the high DWORD, discarding the upper 32 bits of the file + size. +* Fixed integer overflow in `MMDB_read_node()` and `find_ipv4_start_node()` + pointer arithmetic. The `node_number * record_length` multiplication + was performed in `uint32_t`, which could overflow for very large + databases. Now cast to `uint64_t` before multiplying, matching the + pattern already used in `find_address_in_search_tree()`. +* Fixed printf format specifier mismatches in `mmdblookup`'s metadata + dump. `%i` was used for unsigned types and `%llu` for `uint64_t`, + which is technically undefined behavior. Now uses the portable + `PRIu32`, `PRIu16`, and `PRIu64` macros from ``. +* Fixed an integer overflow in the search tree bounds check in + `find_address_in_search_tree()`. The addition of `node_count` and + `data_section_size` was performed in `uint32_t` arithmetic, which + could wrap on very large databases, causing valid lookups to be + incorrectly rejected as corrupt. +* Fixed a NULL pointer dereference in `mmdblookup` when displaying + metadata for a database with an out-of-range `build_epoch`. The + `gmtime()` return value is now checked before passing to `strftime()`. +* `MMDB_close()` now NULLs the `file_content`, `data_section`, and + `metadata_section` pointers and zeroes `file_size`, `data_section_size`, + and `metadata_section_size` after unmapping. Previously, calling + `MMDB_close()` twice on the same struct (or calling it after a failed + `MMDB_open()` that succeeded at mapping) would double-munmap the file + content, which is undefined behavior. +* Fixed a stack buffer overflow in `print_indentation()` when + `MMDB_dump_entry_data_list()` was called with a negative `indent` + value. The negative integer was cast to `size_t`, producing a massive + value passed to `memset()`. Negative indent values are now clamped + to 0. +* `MMDB_lookup_string()` now sets `*mmdb_error` to `MMDB_SUCCESS` when + `getaddrinfo` fails (non-zero `*gai_error`). Previously, `*mmdb_error` + was left uninitialized in this case, which could cause callers to read + an indeterminate value. +* Fixed an off-by-one in `mmdblookup` on Windows where `alloca` allocated + one byte too few for the program name buffer, causing `_splitpath` to + write one byte past the end when appending the null terminator. +* Added a recursion depth limit to `skip_map_or_array()`, matching the + existing `MAXIMUM_DATA_STRUCTURE_DEPTH` (512) limit already used by + `get_entry_data_list()`. A crafted MMDB file with deeply nested maps + or arrays could previously cause a stack overflow via unbounded + recursion in the `MMDB_aget_value` / `MMDB_get_value` code path. +* Fixed an off-by-one error in `MMDB_read_node()` that allowed reading one + node past the end of the search tree when called with + `node_number == node_count`. This caused the function to read from the + data section separator and return an invalid record with an underflowed + data offset. The check now correctly rejects `node_number >= node_count`. * The handling of float and double types was rewritten to fix compiler errors and to eliminate the use of volatile. * Improved endian preprocessor check if `MMDB_LITTLE_ENDIAN` is not set. diff --git a/bin/mmdblookup.c b/bin/mmdblookup.c index 9a37c09d..e14518b4 100644 --- a/bin/mmdblookup.c +++ b/bin/mmdblookup.c @@ -8,6 +8,7 @@ #include "maxminddb.h" #include #include +#include #ifndef _WIN32 #include #endif @@ -229,7 +230,7 @@ static const char **get_options(int argc, static int version = 0; #ifdef _WIN32 - char *program = alloca(strlen(argv[0])); + char *program = alloca(strlen(argv[0]) + 1); _splitpath(argv[0], NULL, NULL, program, NULL); _splitpath(argv[0], NULL, NULL, NULL, program + strlen(program)); #else @@ -346,17 +347,22 @@ static MMDB_s open_or_die(const char *fname) { static void dump_meta(MMDB_s *mmdb) { const char *meta_dump = "\n" " Database metadata\n" - " Node count: %i\n" - " Record size: %i bits\n" - " IP version: IPv%i\n" - " Binary format: %i.%i\n" - " Build epoch: %llu (%s)\n" + " Node count: %" PRIu32 "\n" + " Record size: %" PRIu16 " bits\n" + " IP version: IPv%" PRIu16 "\n" + " Binary format: %" PRIu16 ".%" PRIu16 "\n" + " Build epoch: %" PRIu64 " (%s)\n" " Type: %s\n" " Languages: "; char date[40]; const time_t epoch = (const time_t)mmdb->metadata.build_epoch; - strftime(date, 40, "%F %T UTC", gmtime(&epoch)); + struct tm *tm = gmtime(&epoch); + if (tm != NULL) { + strftime(date, sizeof(date), "%F %T UTC", tm); + } else { + snprintf(date, sizeof(date), "out of range"); + } fprintf(stdout, meta_dump, diff --git a/doc/libmaxminddb.md b/doc/libmaxminddb.md index 4f64ffcd..5684f8e8 100644 --- a/doc/libmaxminddb.md +++ b/doc/libmaxminddb.md @@ -489,6 +489,10 @@ This function always returns an `MMDB_lookup_result_s` structure, but you should also check the `gai_error` and `mmdb_error` parameters. If either of these indicates an error then the returned structure is meaningless. +When `*gai_error` is non-zero (i.e., `getaddrinfo()` failed), `*mmdb_error` +is set to `MMDB_SUCCESS` because no database error occurred. You should always +check `*gai_error` first. + If no error occurred you still need to make sure that the `found_entry` member in the returned result is true. If it's not, this means that the IP address does not have an entry in the database. diff --git a/src/maxminddb.c b/src/maxminddb.c index 838a2669..925acde9 100644 --- a/src/maxminddb.c +++ b/src/maxminddb.c @@ -178,7 +178,8 @@ static int lookup_path_in_map(const char *path_elem, const MMDB_s *const mmdb, MMDB_entry_data_s *entry_data); static int skip_map_or_array(const MMDB_s *const mmdb, - MMDB_entry_data_s *entry_data); + MMDB_entry_data_s *entry_data, + int depth); static int decode_one_follow(const MMDB_s *const mmdb, uint32_t offset, MMDB_entry_data_s *entry_data); @@ -366,7 +367,7 @@ static LPWSTR utf8_to_utf16(const char *utf8_str) { } static int map_file(MMDB_s *const mmdb) { - DWORD size; + ssize_t size; int status = MMDB_SUCCESS; HANDLE mmh = NULL; HANDLE fd = INVALID_HANDLE_VALUE; @@ -386,12 +387,17 @@ static int map_file(MMDB_s *const mmdb) { status = MMDB_FILE_OPEN_ERROR; goto cleanup; } - size = GetFileSize(fd, NULL); - if (size == INVALID_FILE_SIZE) { - status = MMDB_FILE_OPEN_ERROR; + LARGE_INTEGER file_size; + if (!GetFileSizeEx(fd, &file_size)) { + status = MMDB_IO_ERROR; + goto cleanup; + } + if (file_size.QuadPart < 0 || file_size.QuadPart > SSIZE_MAX) { + status = MMDB_IO_ERROR; goto cleanup; } - mmh = CreateFileMapping(fd, NULL, PAGE_READONLY, 0, size, NULL); + size = (ssize_t)file_size.QuadPart; + mmh = CreateFileMapping(fd, NULL, PAGE_READONLY, 0, 0, NULL); /* Microsoft documentation for CreateFileMapping indicates this returns NULL not INVALID_HANDLE_VALUE on error */ if (NULL == mmh) { @@ -882,6 +888,11 @@ MMDB_lookup_result_s MMDB_lookup_string(const MMDB_s *const mmdb, if (!*gai_error) { result = MMDB_lookup_sockaddr(mmdb, addresses->ai_addr, mmdb_error); + } else { + /* No MMDB error occurred; the GAI failure is reported via + * *gai_error. Set *mmdb_error to a defined value so callers + * don't read indeterminate memory. */ + *mmdb_error = MMDB_SUCCESS; } if (NULL != addresses) { @@ -979,7 +990,7 @@ static int find_address_in_search_tree(const MMDB_s *const mmdb, result->netmask = current_bit; - if (value >= node_count + mmdb->data_section_size) { + if (value >= (uint64_t)node_count + mmdb->data_section_size) { // The pointer points off the end of the database. return MMDB_CORRUPT_SEARCH_TREE_ERROR; } @@ -1039,7 +1050,8 @@ static int find_ipv4_start_node(MMDB_s *const mmdb) { uint32_t node_count = mmdb->metadata.node_count; for (netmask = 0; netmask < 96 && node_value < node_count; netmask++) { - record_pointer = &search_tree[node_value * record_info.record_length]; + record_pointer = + &search_tree[(uint64_t)node_value * record_info.record_length]; if (record_pointer + record_info.record_length > mmdb->data_section) { return MMDB_CORRUPT_SEARCH_TREE_ERROR; } @@ -1097,13 +1109,16 @@ int MMDB_read_node(const MMDB_s *const mmdb, return MMDB_UNKNOWN_DATABASE_FORMAT_ERROR; } - if (node_number > mmdb->metadata.node_count) { + if (node_number >= mmdb->metadata.node_count) { return MMDB_INVALID_NODE_NUMBER_ERROR; } const uint8_t *search_tree = mmdb->file_content; const uint8_t *record_pointer = - &search_tree[node_number * record_info.record_length]; + &search_tree[(uint64_t)node_number * record_info.record_length]; + if (record_pointer + record_info.record_length > mmdb->data_section) { + return MMDB_CORRUPT_SEARCH_TREE_ERROR; + } node->left_record = record_info.left_record_getter(record_pointer); record_pointer += record_info.right_record_offset; node->right_record = record_info.right_record_getter(record_pointer); @@ -1272,7 +1287,7 @@ static int lookup_path_in_array(const char *path_elem, /* We don't want to follow a pointer here. If the next element is a * pointer we simply skip it and keep going */ CHECKED_DECODE_ONE(mmdb, entry_data->offset_to_next, entry_data); - int status = skip_map_or_array(mmdb, entry_data); + int status = skip_map_or_array(mmdb, entry_data, 0); if (MMDB_SUCCESS != status) { return status; } @@ -1314,7 +1329,7 @@ static int lookup_path_in_map(const char *path_elem, /* We don't want to follow a pointer here. If the next element is * a pointer we simply skip it and keep going */ CHECKED_DECODE_ONE(mmdb, offset_to_value, &value); - int status = skip_map_or_array(mmdb, &value); + int status = skip_map_or_array(mmdb, &value, 0); if (MMDB_SUCCESS != status) { return status; } @@ -1327,7 +1342,13 @@ static int lookup_path_in_map(const char *path_elem, } static int skip_map_or_array(const MMDB_s *const mmdb, - MMDB_entry_data_s *entry_data) { + MMDB_entry_data_s *entry_data, + int depth) { + if (depth >= MAXIMUM_DATA_STRUCTURE_DEPTH) { + DEBUG_MSG("reached the maximum data structure depth"); + return MMDB_INVALID_DATA_ERROR; + } + if (entry_data->type == MMDB_DATA_TYPE_MAP) { uint32_t size = entry_data->data_size; while (size-- > 0) { @@ -1335,7 +1356,7 @@ static int skip_map_or_array(const MMDB_s *const mmdb, mmdb, entry_data->offset_to_next, entry_data); // key CHECKED_DECODE_ONE( mmdb, entry_data->offset_to_next, entry_data); // value - int status = skip_map_or_array(mmdb, entry_data); + int status = skip_map_or_array(mmdb, entry_data, depth + 1); if (MMDB_SUCCESS != status) { return status; } @@ -1345,7 +1366,7 @@ static int skip_map_or_array(const MMDB_s *const mmdb, while (size-- > 0) { CHECKED_DECODE_ONE( mmdb, entry_data->offset_to_next, entry_data); // value - int status = skip_map_or_array(mmdb, entry_data); + int status = skip_map_or_array(mmdb, entry_data, depth + 1); if (MMDB_SUCCESS != status) { return status; } @@ -1707,6 +1728,12 @@ static int get_entry_data_list(const MMDB_s *const mmdb, case MMDB_DATA_TYPE_ARRAY: { uint32_t array_size = entry_data_list->entry_data.data_size; uint32_t array_offset = entry_data_list->entry_data.offset_to_next; + /* Each array element needs at least 1 byte. */ + if (array_offset >= mmdb->data_section_size || + array_size > mmdb->data_section_size - array_offset) { + DEBUG_MSG("array size exceeds remaining data section"); + return MMDB_INVALID_DATA_ERROR; + } while (array_size-- > 0) { MMDB_entry_data_list_s *entry_data_list_to = data_pool_alloc(pool); @@ -1730,6 +1757,12 @@ static int get_entry_data_list(const MMDB_s *const mmdb, uint32_t size = entry_data_list->entry_data.data_size; offset = entry_data_list->entry_data.offset_to_next; + /* Each map entry needs at least a key and a value (1 byte each). */ + if (offset >= mmdb->data_section_size || + size > (mmdb->data_section_size - offset) / 2) { + DEBUG_MSG("map size exceeds remaining data section"); + return MMDB_INVALID_DATA_ERROR; + } while (size-- > 0) { MMDB_entry_data_list_s *list_key = data_pool_alloc(pool); if (!list_key) { @@ -1894,6 +1927,12 @@ static void free_mmdb_struct(MMDB_s *const mmdb) { #pragma clang diagnostic pop #endif #endif + mmdb->file_content = NULL; + mmdb->file_size = 0; + mmdb->data_section = NULL; + mmdb->data_section_size = 0; + mmdb->metadata_section = NULL; + mmdb->metadata_section_size = 0; } if (NULL != mmdb->metadata.database_type) { @@ -1931,6 +1970,7 @@ static void free_languages_metadata(MMDB_s *mmdb) { #endif } FREE_AND_SET_NULL(mmdb->metadata.languages.names); + mmdb->metadata.languages.count = 0; } static void free_descriptions_metadata(MMDB_s *mmdb) { @@ -1973,6 +2013,7 @@ static void free_descriptions_metadata(MMDB_s *mmdb) { } FREE_AND_SET_NULL(mmdb->metadata.description.descriptions); + mmdb->metadata.description.count = 0; } const char *MMDB_lib_version(void) { return PACKAGE_VERSION; } @@ -2158,7 +2199,7 @@ dump_entry_data_list(FILE *stream, static void print_indentation(FILE *stream, int i) { char buffer[1024]; - int size = i >= 1024 ? 1023 : i; + int size = i < 0 ? 0 : (i >= 1024 ? 1023 : i); memset(buffer, 32, (size_t)size); buffer[size] = '\0'; fputs(buffer, stream); diff --git a/t/CMakeLists.txt b/t/CMakeLists.txt index f0e91f76..8ea56c44 100644 --- a/t/CMakeLists.txt +++ b/t/CMakeLists.txt @@ -5,11 +5,14 @@ add_library(tap # test programs set(TEST_TARGET_NAMES bad_pointers_t + bad_search_tree_t basic_lookup_t data_entry_list_t data-pool-t data_types_t + double_close_t dump_t + gai_error_t get_value_pointer_bug_t get_value_t ipv4_start_cache_t @@ -17,6 +20,7 @@ set(TEST_TARGET_NAMES metadata_pointers_t metadata_t no_map_get_value_t + overflow_bounds_t read_node_t version_t ) @@ -24,6 +28,10 @@ set(TEST_TARGET_NAMES if(UNIX) # or if (NOT WIN32) list(APPEND TEST_TARGET_NAMES bad_databases_t + bad_data_size_t + bad_epoch_t + bad_indent_t + max_depth_t threads_t ) find_package(Threads) diff --git a/t/Makefile.am b/t/Makefile.am index d8d416f1..8468f868 100644 --- a/t/Makefile.am +++ b/t/Makefile.am @@ -16,10 +16,14 @@ EXTRA_DIST = compile_c++_t.pl external_symbols_t.pl mmdblookup_t.pl \ libtap/tap.c libtap/tap.h maxmind-db check_PROGRAMS = \ - bad_pointers_t bad_databases_t basic_lookup_t data_entry_list_t \ - data-pool-t data_types_t dump_t get_value_t get_value_pointer_bug_t \ - ipv4_start_cache_t ipv6_lookup_in_ipv4_t metadata_t metadata_pointers_t \ - no_map_get_value_t read_node_t threads_t version_t + bad_pointers_t bad_databases_t bad_data_size_t bad_epoch_t bad_indent_t \ + bad_search_tree_t \ + basic_lookup_t data_entry_list_t \ + data-pool-t data_types_t double_close_t dump_t gai_error_t get_value_t \ + get_value_pointer_bug_t \ + ipv4_start_cache_t ipv6_lookup_in_ipv4_t max_depth_t metadata_t \ + metadata_pointers_t no_map_get_value_t overflow_bounds_t read_node_t \ + threads_t version_t data_pool_t_LDFLAGS = $(AM_LDFLAGS) -lm data_pool_t_SOURCES = data-pool-t.c ../src/data-pool.c diff --git a/t/bad_data_size_t.c b/t/bad_data_size_t.c new file mode 100644 index 00000000..b5ac9d26 --- /dev/null +++ b/t/bad_data_size_t.c @@ -0,0 +1,78 @@ +#include "maxminddb_test_helper.h" + +void test_bad_data_size_rejected(void) { + char *db_file = bad_database_path("libmaxminddb-oversized-array.mmdb"); + + MMDB_s mmdb; + int status = MMDB_open(db_file, MMDB_MODE_MMAP, &mmdb); + cmp_ok(status, "==", MMDB_SUCCESS, "opened bad-data-size MMDB"); + + if (status != MMDB_SUCCESS) { + diag("MMDB_open failed: %s", MMDB_strerror(status)); + free(db_file); + return; + } + + int gai_error, mmdb_error; + MMDB_lookup_result_s result = + MMDB_lookup_string(&mmdb, "1.2.3.4", &gai_error, &mmdb_error); + + cmp_ok(mmdb_error, "==", MMDB_SUCCESS, "lookup succeeded"); + ok(result.found_entry, "entry found"); + + if (result.found_entry) { + MMDB_entry_data_list_s *entry_data_list = NULL; + status = MMDB_get_entry_data_list(&result.entry, &entry_data_list); + cmp_ok(status, + "==", + MMDB_INVALID_DATA_ERROR, + "MMDB_get_entry_data_list returns INVALID_DATA_ERROR " + "for array with size exceeding remaining data"); + MMDB_free_entry_data_list(entry_data_list); + } + + MMDB_close(&mmdb); + free(db_file); +} + +void test_bad_map_size_rejected(void) { + char *db_file = bad_database_path("libmaxminddb-oversized-map.mmdb"); + + MMDB_s mmdb; + int status = MMDB_open(db_file, MMDB_MODE_MMAP, &mmdb); + cmp_ok(status, "==", MMDB_SUCCESS, "opened bad-map-size MMDB"); + + if (status != MMDB_SUCCESS) { + diag("MMDB_open failed: %s", MMDB_strerror(status)); + free(db_file); + return; + } + + int gai_error, mmdb_error; + MMDB_lookup_result_s result = + MMDB_lookup_string(&mmdb, "1.2.3.4", &gai_error, &mmdb_error); + + cmp_ok(mmdb_error, "==", MMDB_SUCCESS, "lookup succeeded"); + ok(result.found_entry, "entry found"); + + if (result.found_entry) { + MMDB_entry_data_list_s *entry_data_list = NULL; + status = MMDB_get_entry_data_list(&result.entry, &entry_data_list); + cmp_ok(status, + "==", + MMDB_INVALID_DATA_ERROR, + "MMDB_get_entry_data_list returns INVALID_DATA_ERROR " + "for map with size exceeding remaining data"); + MMDB_free_entry_data_list(entry_data_list); + } + + MMDB_close(&mmdb); + free(db_file); +} + +int main(void) { + plan(NO_PLAN); + test_bad_data_size_rejected(); + test_bad_map_size_rejected(); + done_testing(); +} diff --git a/t/bad_databases_t.c b/t/bad_databases_t.c index 54003d72..2324f1ec 100644 --- a/t/bad_databases_t.c +++ b/t/bad_databases_t.c @@ -12,7 +12,7 @@ int test_read(const char *path, const struct stat *UNUSED(sbuf), int flags, struct FTW *UNUSED(ftw)) { - // Check if path is a regular file) + // Check if path is a regular file if (flags != FTW_F) { return 0; } @@ -32,13 +32,38 @@ int test_read(const char *path, } int gai_error, mmdb_error; - MMDB_lookup_string(mmdb, "1.1.1.1", &gai_error, &mmdb_error); + MMDB_lookup_result_s result = + MMDB_lookup_string(mmdb, "1.1.1.1", &gai_error, &mmdb_error); if (gai_error != 0) { BAIL_OUT("could not parse IP address"); } - cmp_ok( - mmdb_error, "!=", MMDB_SUCCESS, "opening %s returned an error", path); + if (mmdb_error != MMDB_SUCCESS) { + ok(1, "received error on lookup for %s", path); + MMDB_close(mmdb); + free(mmdb); + return 0; + } + + if (result.found_entry) { + MMDB_entry_data_list_s *entry_data_list = NULL; + status = MMDB_get_entry_data_list(&result.entry, &entry_data_list); + MMDB_free_entry_data_list(entry_data_list); + + if (status != MMDB_SUCCESS) { + ok(1, + "received error from MMDB_get_entry_data_list for %s", + path); + MMDB_close(mmdb); + free(mmdb); + return 0; + } + } + + // Some bad-data files (e.g. uint64-max-epoch) are valid databases with + // extreme metadata values. They don't produce errors in libmaxminddb + // but are useful for testing other reader implementations. + ok(1, "no error reading %s (database may have extreme but valid data)", path); MMDB_close(mmdb); free(mmdb); diff --git a/t/bad_epoch_t.c b/t/bad_epoch_t.c new file mode 100644 index 00000000..918adca0 --- /dev/null +++ b/t/bad_epoch_t.c @@ -0,0 +1,53 @@ +#include "maxminddb_test_helper.h" + +void test_bad_epoch(void) { + char *db_file = bad_database_path("libmaxminddb-uint64-max-epoch.mmdb"); + + /* Verify we can at least open the DB without crashing */ + MMDB_s mmdb; + int status = MMDB_open(db_file, MMDB_MODE_MMAP, &mmdb); + cmp_ok(status, "==", MMDB_SUCCESS, "opened bad-epoch MMDB"); + + if (status != MMDB_SUCCESS) { + diag("MMDB_open failed: %s", MMDB_strerror(status)); + free(db_file); + return; + } + + /* Run mmdblookup --verbose via system() and check it doesn't crash. + * We redirect output to /dev/null; the return code tells us + * whether the process survived. Try both possible paths since tests + * may run from either the project root or the t/ directory. */ + char cmd[512]; + const char *binary = "../bin/mmdblookup"; + FILE *test_bin = fopen(binary, "r"); + if (!test_bin) { + binary = "./bin/mmdblookup"; + test_bin = fopen(binary, "r"); + } + if (test_bin) { + fclose(test_bin); + } + + skip(!test_bin, 1, "mmdblookup binary not found"); + snprintf(cmd, + sizeof(cmd), + "%s -f %s -i 1.2.3.4 -v > /dev/null 2>&1", + binary, + db_file); + int ret = system(cmd); + /* system() returns the exit status; a signal-killed process gives + * a non-zero status. WIFEXITED checks for normal exit. */ + ok(WIFEXITED(ret) && WEXITSTATUS(ret) == 0, + "mmdblookup --verbose with UINT64_MAX build_epoch does not crash"); + end_skip; + + MMDB_close(&mmdb); + free(db_file); +} + +int main(void) { + plan(NO_PLAN); + test_bad_epoch(); + done_testing(); +} diff --git a/t/bad_indent_t.c b/t/bad_indent_t.c new file mode 100644 index 00000000..21075ce9 --- /dev/null +++ b/t/bad_indent_t.c @@ -0,0 +1,64 @@ +#include "maxminddb_test_helper.h" +#include + +void test_negative_indent(void) { + char *db_file = test_database_path("MaxMind-DB-test-ipv4-24.mmdb"); + MMDB_s *mmdb = open_ok(db_file, MMDB_MODE_MMAP, "mmap mode"); + free(db_file); + + if (!mmdb) { + return; + } + + int gai_error, mmdb_error; + MMDB_lookup_result_s result = + MMDB_lookup_string(mmdb, "1.1.1.1", &gai_error, &mmdb_error); + cmp_ok(mmdb_error, "==", MMDB_SUCCESS, "lookup succeeded"); + + if (result.found_entry) { + MMDB_entry_data_list_s *entry_data_list = NULL; + int status = MMDB_get_entry_data_list(&result.entry, &entry_data_list); + cmp_ok(status, "==", MMDB_SUCCESS, "get_entry_data_list succeeded"); + + if (MMDB_SUCCESS == status && entry_data_list) { + FILE *devnull = fopen("/dev/null", "w"); + if (!devnull) { + BAIL_OUT("could not open /dev/null"); + } + + /* Negative indent should not crash — it should be clamped to 0 */ + status = MMDB_dump_entry_data_list(devnull, entry_data_list, -1); + cmp_ok(status, + "==", + MMDB_SUCCESS, + "MMDB_dump_entry_data_list with indent=-1 returns success"); + + status = MMDB_dump_entry_data_list(devnull, entry_data_list, -100); + cmp_ok( + status, + "==", + MMDB_SUCCESS, + "MMDB_dump_entry_data_list with indent=-100 returns success"); + + status = + MMDB_dump_entry_data_list(devnull, entry_data_list, INT_MIN); + cmp_ok(status, + "==", + MMDB_SUCCESS, + "MMDB_dump_entry_data_list with indent=INT_MIN returns " + "success"); + + fclose(devnull); + } + MMDB_free_entry_data_list(entry_data_list); + } + + MMDB_close(mmdb); + free(mmdb); +} + +int main(void) { + plan(NO_PLAN); + test_negative_indent(); + done_testing(); +} diff --git a/t/bad_search_tree_t.c b/t/bad_search_tree_t.c new file mode 100644 index 00000000..06e8cb85 --- /dev/null +++ b/t/bad_search_tree_t.c @@ -0,0 +1,49 @@ +#include "maxminddb_test_helper.h" + +/* + * Test the off-by-one fix in MMDB_read_node: node_number >= node_count + * must return MMDB_INVALID_NODE_NUMBER_ERROR. Previously the check used + * >, allowing node_number == node_count to read past the tree. + */ +void test_read_node_bounds(void) { + char *db_file = test_database_path("MaxMind-DB-test-ipv4-24.mmdb"); + MMDB_s *mmdb = open_ok(db_file, MMDB_MODE_MMAP, "mmap mode"); + free(db_file); + + if (!mmdb) { + return; + } + + MMDB_search_node_s node; + + /* node_count - 1 is the last valid node */ + int status = + MMDB_read_node(mmdb, mmdb->metadata.node_count - 1, &node); + cmp_ok(status, + "==", + MMDB_SUCCESS, + "MMDB_read_node succeeds for last valid node"); + + /* node_count itself is out of bounds (the off-by-one fix) */ + status = MMDB_read_node(mmdb, mmdb->metadata.node_count, &node); + cmp_ok(status, + "==", + MMDB_INVALID_NODE_NUMBER_ERROR, + "MMDB_read_node rejects node_number == node_count"); + + /* node_count + 1 is also out of bounds */ + status = MMDB_read_node(mmdb, mmdb->metadata.node_count + 1, &node); + cmp_ok(status, + "==", + MMDB_INVALID_NODE_NUMBER_ERROR, + "MMDB_read_node rejects node_number > node_count"); + + MMDB_close(mmdb); + free(mmdb); +} + +int main(void) { + plan(NO_PLAN); + test_read_node_bounds(); + done_testing(); +} diff --git a/t/double_close_t.c b/t/double_close_t.c new file mode 100644 index 00000000..942b4abf --- /dev/null +++ b/t/double_close_t.c @@ -0,0 +1,45 @@ +#include "maxminddb_test_helper.h" + +void test_double_close(void) { + char *db_file = test_database_path("MaxMind-DB-test-ipv4-24.mmdb"); + + MMDB_s mmdb; + int status = MMDB_open(db_file, MMDB_MODE_MMAP, &mmdb); + free(db_file); + cmp_ok(status, "==", MMDB_SUCCESS, "MMDB_open succeeded"); + + if (status != MMDB_SUCCESS) { + return; + } + + /* First close should work normally */ + MMDB_close(&mmdb); + + ok(mmdb.file_content == NULL, "file_content is NULL after first close"); + ok(mmdb.data_section == NULL, "data_section is NULL after close"); + ok(mmdb.metadata_section == NULL, "metadata_section is NULL after close"); + cmp_ok(mmdb.metadata.languages.count, + "==", + 0, + "languages.count is 0 after close"); + cmp_ok(mmdb.metadata.description.count, + "==", + 0, + "description.count is 0 after close"); + cmp_ok(mmdb.file_size, "==", 0, "file_size is 0 after close"); + cmp_ok(mmdb.data_section_size, "==", 0, + "data_section_size is 0 after close"); + cmp_ok(mmdb.metadata_section_size, "==", 0, + "metadata_section_size is 0 after close"); + + /* Second close should be a safe no-op (file_content was NULLed) */ + MMDB_close(&mmdb); + + ok(1, "calling MMDB_close twice does not crash"); +} + +int main(void) { + plan(NO_PLAN); + test_double_close(); + done_testing(); +} diff --git a/t/gai_error_t.c b/t/gai_error_t.c new file mode 100644 index 00000000..9e9f0304 --- /dev/null +++ b/t/gai_error_t.c @@ -0,0 +1,33 @@ +#include "maxminddb_test_helper.h" + +void test_mmdb_error_set_on_gai_failure(void) { + char *db_file = test_database_path("MaxMind-DB-test-ipv4-24.mmdb"); + MMDB_s *mmdb = open_ok(db_file, MMDB_MODE_MMAP, "mmap mode"); + free(db_file); + + if (!mmdb) { + return; + } + + /* Set mmdb_error to a known non-zero value to detect if it gets written */ + int gai_error = 0; + int mmdb_error = 0xDEAD; + + /* ".." is not a valid IP address - getaddrinfo will fail */ + MMDB_lookup_string(mmdb, "..", &gai_error, &mmdb_error); + + ok(gai_error != 0, "gai_error is non-zero for invalid IP '..'"); + cmp_ok(mmdb_error, + "==", + MMDB_SUCCESS, + "mmdb_error is set to MMDB_SUCCESS when gai_error is non-zero"); + + MMDB_close(mmdb); + free(mmdb); +} + +int main(void) { + plan(NO_PLAN); + test_mmdb_error_set_on_gai_failure(); + done_testing(); +} diff --git a/t/max_depth_t.c b/t/max_depth_t.c new file mode 100644 index 00000000..f703b8d0 --- /dev/null +++ b/t/max_depth_t.c @@ -0,0 +1,118 @@ +#include "maxminddb_test_helper.h" + +void test_deep_nesting_rejected(void) { + char *db_file = bad_database_path("libmaxminddb-deep-nesting.mmdb"); + + MMDB_s mmdb; + int status = MMDB_open(db_file, MMDB_MODE_MMAP, &mmdb); + cmp_ok(status, "==", MMDB_SUCCESS, "opened deeply nested MMDB"); + + if (status != MMDB_SUCCESS) { + diag("MMDB_open failed: %s", MMDB_strerror(status)); + free(db_file); + return; + } + + int gai_error, mmdb_error; + MMDB_lookup_result_s result = + MMDB_lookup_string(&mmdb, "1.2.3.4", &gai_error, &mmdb_error); + + cmp_ok(mmdb_error, "==", MMDB_SUCCESS, "lookup succeeded"); + ok(result.found_entry, "entry found"); + + if (result.found_entry) { + /* Looking up non-existent key "z" forces skip_map_or_array to + * recurse through all 600 nesting levels. With the depth limit, + * this should return MMDB_INVALID_DATA_ERROR instead of crashing. */ + MMDB_entry_data_s entry_data; + const char *lookup_path[] = {"z", NULL}; + status = MMDB_aget_value(&result.entry, &entry_data, lookup_path); + cmp_ok(status, + "==", + MMDB_INVALID_DATA_ERROR, + "MMDB_aget_value returns MMDB_INVALID_DATA_ERROR for " + "deeply nested data exceeding max depth"); + } + + MMDB_close(&mmdb); + free(db_file); +} + +void test_valid_nesting_allowed(void) { + char *db_file = test_database_path("MaxMind-DB-test-nested.mmdb"); + + MMDB_s mmdb; + int status = MMDB_open(db_file, MMDB_MODE_MMAP, &mmdb); + cmp_ok(status, "==", MMDB_SUCCESS, "opened moderately nested MMDB"); + + if (status != MMDB_SUCCESS) { + diag("MMDB_open failed: %s", MMDB_strerror(status)); + free(db_file); + return; + } + + int gai_error, mmdb_error; + MMDB_lookup_result_s result = + MMDB_lookup_string(&mmdb, "1.1.1.1", &gai_error, &mmdb_error); + + cmp_ok(mmdb_error, "==", MMDB_SUCCESS, "lookup succeeded"); + ok(result.found_entry, "entry found"); + + if (result.found_entry) { + MMDB_entry_data_list_s *entry_data_list = NULL; + status = MMDB_get_entry_data_list(&result.entry, &entry_data_list); + cmp_ok(status, + "==", + MMDB_SUCCESS, + "MMDB_get_entry_data_list succeeds for " + "valid nesting depth"); + MMDB_free_entry_data_list(entry_data_list); + } + + MMDB_close(&mmdb); + free(db_file); +} + +void test_deep_array_nesting_rejected(void) { + char *db_file = + bad_database_path("libmaxminddb-deep-array-nesting.mmdb"); + + MMDB_s mmdb; + int status = MMDB_open(db_file, MMDB_MODE_MMAP, &mmdb); + cmp_ok(status, "==", MMDB_SUCCESS, "opened deeply nested array MMDB"); + + if (status != MMDB_SUCCESS) { + diag("MMDB_open failed: %s", MMDB_strerror(status)); + free(db_file); + return; + } + + int gai_error, mmdb_error; + MMDB_lookup_result_s result = + MMDB_lookup_string(&mmdb, "1.2.3.4", &gai_error, &mmdb_error); + + cmp_ok(mmdb_error, "==", MMDB_SUCCESS, "lookup succeeded"); + ok(result.found_entry, "entry found"); + + if (result.found_entry) { + MMDB_entry_data_list_s *entry_data_list = NULL; + status = MMDB_get_entry_data_list(&result.entry, &entry_data_list); + cmp_ok(status, + "==", + MMDB_INVALID_DATA_ERROR, + "MMDB_get_entry_data_list returns MMDB_INVALID_DATA_ERROR " + "for deeply nested arrays exceeding max depth"); + MMDB_free_entry_data_list(entry_data_list); + } + + MMDB_close(&mmdb); + free(db_file); +} + +int main(void) { + plan(NO_PLAN); + test_deep_nesting_rejected(); + test_deep_array_nesting_rejected(); + test_valid_nesting_allowed(); + done_testing(); +} diff --git a/t/maxmind-db b/t/maxmind-db index 880f6b4b..29cab1fe 160000 --- a/t/maxmind-db +++ b/t/maxmind-db @@ -1 +1 @@ -Subproject commit 880f6b4b5eb6c12ea9d5c70dd201dec2cb5639a2 +Subproject commit 29cab1feb43abacb7f33c14db1052f99882fb7c6 diff --git a/t/maxminddb_test_helper.c b/t/maxminddb_test_helper.c index 612c7a65..a049800f 100644 --- a/t/maxminddb_test_helper.c +++ b/t/maxminddb_test_helper.c @@ -70,6 +70,29 @@ char *test_database_path(const char *filename) { return path; } +char *bad_database_path(const char *filename) { + char *bad_db_dir; +#ifdef _WIN32 + bad_db_dir = "../t/maxmind-db/bad-data/libmaxminddb"; +#else + char cwd[500]; + char *UNUSED(tmp) = getcwd(cwd, 500); + + if (strcmp(basename(cwd), "t") == 0) { + bad_db_dir = "./maxmind-db/bad-data/libmaxminddb"; + } else { + bad_db_dir = "./t/maxmind-db/bad-data/libmaxminddb"; + } +#endif + + char *path = malloc(500); + assert(NULL != path); + + snprintf(path, 500, "%s/%s", bad_db_dir, filename); + + return path; +} + char *dup_entry_string_or_bail(MMDB_entry_data_s entry_data) { char *string = mmdb_strndup(entry_data.utf8_string, entry_data.data_size); if (NULL == string) { diff --git a/t/maxminddb_test_helper.h b/t/maxminddb_test_helper.h index 53e14dce..0b66d0b9 100644 --- a/t/maxminddb_test_helper.h +++ b/t/maxminddb_test_helper.h @@ -48,6 +48,7 @@ extern void for_all_record_sizes(const char *filename_fmt, const char *description)); extern void for_all_modes(void (*tests)(int mode, const char *description)); extern char *test_database_path(const char *filename); +extern char *bad_database_path(const char *filename); extern char *dup_entry_string_or_bail(MMDB_entry_data_s entry_data); extern MMDB_s *open_ok(const char *db_file, int mode, const char *mode_desc); extern MMDB_lookup_result_s lookup_string_ok(MMDB_s *mmdb, diff --git a/t/overflow_bounds_t.c b/t/overflow_bounds_t.c new file mode 100644 index 00000000..136cd370 --- /dev/null +++ b/t/overflow_bounds_t.c @@ -0,0 +1,67 @@ +#include "maxminddb_test_helper.h" + +/* + * Test that the bounds check in find_address_in_search_tree uses 64-bit + * arithmetic. We can't realistically create a database large enough to + * trigger the uint32 overflow (it would require billions of nodes), but + * we can verify the fix doesn't regress normal lookups. + * + * The real protection is the cast to uint64_t before addition, + * matching the pattern used elsewhere in the codebase. + */ +void test_normal_lookup_still_works(void) { + char *db_file = test_database_path("MaxMind-DB-test-ipv4-24.mmdb"); + MMDB_s *mmdb = open_ok(db_file, MMDB_MODE_MMAP, "mmap mode"); + free(db_file); + + if (!mmdb) { + return; + } + + int gai_error, mmdb_error; + (void)MMDB_lookup_string(mmdb, "1.1.1.1", &gai_error, &mmdb_error); + + cmp_ok(gai_error, "==", 0, "no gai error"); + cmp_ok(mmdb_error, "==", MMDB_SUCCESS, "no mmdb error"); + + MMDB_close(mmdb); + free(mmdb); +} + +void test_record_type_bounds(void) { + /* Test that record_type correctly handles values near the boundary. + * With the uint64_t cast fix, valid records should still be classified + * correctly. */ + char *db_file = test_database_path("MaxMind-DB-test-ipv4-24.mmdb"); + MMDB_s *mmdb = open_ok(db_file, MMDB_MODE_MMAP, "mmap mode"); + free(db_file); + + if (!mmdb) { + return; + } + + /* Read node 0 and verify records are valid types */ + MMDB_search_node_s node; + int status = MMDB_read_node(mmdb, 0, &node); + cmp_ok(status, "==", MMDB_SUCCESS, "MMDB_read_node succeeded"); + + ok(node.left_record_type == MMDB_RECORD_TYPE_SEARCH_NODE || + node.left_record_type == MMDB_RECORD_TYPE_EMPTY || + node.left_record_type == MMDB_RECORD_TYPE_DATA, + "left record type is valid"); + + ok(node.right_record_type == MMDB_RECORD_TYPE_SEARCH_NODE || + node.right_record_type == MMDB_RECORD_TYPE_EMPTY || + node.right_record_type == MMDB_RECORD_TYPE_DATA, + "right record type is valid"); + + MMDB_close(mmdb); + free(mmdb); +} + +int main(void) { + plan(NO_PLAN); + test_normal_lookup_still_works(); + test_record_type_bounds(); + done_testing(); +} diff --git a/t/read_node_t.c b/t/read_node_t.c index da4ec849..b6ab6d83 100644 --- a/t/read_node_t.c +++ b/t/read_node_t.c @@ -252,10 +252,43 @@ void run_32_bit_record_tests(int mode, const char *mode_desc) { free(mmdb); } +void run_read_node_invalid_node_number_tests(int mode, const char *mode_desc) { + const char *filename = "MaxMind-DB-test-mixed-24.mmdb"; + char *path = test_database_path(filename); + MMDB_s *mmdb = open_ok(path, mode, mode_desc); + free(path); + + uint32_t node_count = mmdb->metadata.node_count; + + MMDB_search_node_s node; + int status; + + /* node_count is one past the last valid node (nodes are 0-indexed). + * This must be rejected. */ + status = MMDB_read_node(mmdb, node_count, &node); + cmp_ok(status, + "==", + MMDB_INVALID_NODE_NUMBER_ERROR, + "MMDB_read_node with node_number == node_count returns " + "MMDB_INVALID_NODE_NUMBER_ERROR"); + + /* node_count + 1 should also be rejected. */ + status = MMDB_read_node(mmdb, node_count + 1, &node); + cmp_ok(status, + "==", + MMDB_INVALID_NODE_NUMBER_ERROR, + "MMDB_read_node with node_number > node_count returns " + "MMDB_INVALID_NODE_NUMBER_ERROR"); + + MMDB_close(mmdb); + free(mmdb); +} + void run_tests(int mode, const char *mode_desc) { run_24_bit_record_tests(mode, mode_desc); run_28_bit_record_tests(mode, mode_desc); run_32_bit_record_tests(mode, mode_desc); + run_read_node_invalid_node_number_tests(mode, mode_desc); } int main(void) {