diff --git a/Changes.md b/Changes.md index df03c288..c4b2af66 100644 --- a/Changes.md +++ b/Changes.md @@ -1,3 +1,20 @@ +## next release + +- Fixed an out-of-bounds read in `MMDB_lookup_sockaddr()` when callers passed a + `sockaddr` with an unsupported address family. The function now rejects any + family other than `AF_INET` and `AF_INET6` with + `MMDB_INVALID_NETWORK_ADDRESS_ERROR`. +- Fixed metadata parsing for files that end immediately after the + `\xAB\xCD\xEFMaxMind.com` marker. Such files are now rejected as invalid + metadata instead of allowing a zero-length metadata section to reach the + decoder. +- Fixed search-tree validation for records that point into the 16-byte separator + before the data section. These records are now rejected as corrupt instead of + being exposed as apparent data entries with underflowed offsets. +- `MMDB_read_node()` now returns `MMDB_CORRUPT_SEARCH_TREE_ERROR` instead of + `MMDB_SUCCESS` with `MMDB_RECORD_TYPE_INVALID` record types when a node's + child record is invalid. + ## 1.13.3 - 2026-03-05 - Fixed validation of empty maps and arrays at the end of the metadata section. diff --git a/doc/libmaxminddb.md b/doc/libmaxminddb.md index ef416715..10ecc743 100644 --- a/doc/libmaxminddb.md +++ b/doc/libmaxminddb.md @@ -393,6 +393,8 @@ status codes are: array index larger than an array or smaller than the minimum offset from the end of an array. It can also happen when the path expects to find a map or array where none exist. +- `MMDB_INVALID_NETWORK_ADDRESS_ERROR` - `MMDB_lookup_sockaddr()` was given a + `sockaddr` whose family is neither `AF_INET` nor `AF_INET6`. All status codes should be treated as `int` values. @@ -518,7 +520,8 @@ MMDB_lookup_result_s MMDB_lookup_sockaddr( ``` This function looks up an IP address that has already been resolved by -`getaddrinfo()`. +`getaddrinfo()`. The `sockaddr` passed to this function must be an `AF_INET` or +`AF_INET6` address. Other than not calling `getaddrinfo()` itself, this function is identical to the `MMDB_lookup_string()` function. @@ -770,7 +773,11 @@ to an `MMDB_search_node_s` structure that will be populated by this function. The return value is a status code. If you pass a `node_number` that is greater than or equal to the number of nodes in the database, this function will return -`MMDB_INVALID_NODE_NUMBER_ERROR`, otherwise it will return `MMDB_SUCCESS`. +`MMDB_INVALID_NODE_NUMBER_ERROR`. If the node's child records are invalid or +point outside the search tree or data section, it will return +`MMDB_CORRUPT_SEARCH_TREE_ERROR`. Otherwise it will return `MMDB_SUCCESS`. On +any non-`MMDB_SUCCESS` return the contents of `*node` are unspecified and must +not be read. The first node in the search tree is always node 0. If you wanted to iterate over the whole search tree, you would start by reading node 0 and then following diff --git a/include/maxminddb.h b/include/maxminddb.h index 9c1a96b9..59f404db 100644 --- a/include/maxminddb.h +++ b/include/maxminddb.h @@ -86,6 +86,7 @@ extern "C" { #define MMDB_LOOKUP_PATH_DOES_NOT_MATCH_DATA_ERROR (9) #define MMDB_INVALID_NODE_NUMBER_ERROR (10) #define MMDB_IPV6_LOOKUP_IN_IPV4_DATABASE_ERROR (11) + #define MMDB_INVALID_NETWORK_ADDRESS_ERROR (12) #if !(MMDB_UINT128_IS_BYTE_ARRAY) #if MMDB_UINT128_USING_MODE diff --git a/src/maxminddb.c b/src/maxminddb.c index 5aa3b254..8d82a9e7 100644 --- a/src/maxminddb.c +++ b/src/maxminddb.c @@ -518,7 +518,7 @@ static const uint8_t *find_metadata(const uint8_t *file_content, } } while (NULL != tmp); - if (search_area == start) { + if (search_area == start || max_size <= 0) { return NULL; } @@ -926,23 +926,33 @@ MMDB_lookup_result_s MMDB_lookup_sockaddr(const MMDB_s *const mmdb, uint8_t mapped_address[16]; uint8_t const *address; + // Reject families other than AF_INET/AF_INET6 before casting to + // sockaddr_in/sockaddr_in6, which would otherwise read past the + // truncated struct sockaddr the caller passed in. if (mmdb->metadata.ip_version == 4) { if (sockaddr->sa_family == AF_INET6) { *mmdb_error = MMDB_IPV6_LOOKUP_IN_IPV4_DATABASE_ERROR; return result; } + if (sockaddr->sa_family != AF_INET) { + *mmdb_error = MMDB_INVALID_NETWORK_ADDRESS_ERROR; + return result; + } address = (uint8_t const *)&((struct sockaddr_in const *)sockaddr) ->sin_addr.s_addr; } else { if (sockaddr->sa_family == AF_INET6) { address = (uint8_t const *)&((struct sockaddr_in6 const *)sockaddr) ->sin6_addr.s6_addr; - } else { + } else if (sockaddr->sa_family == AF_INET) { address = mapped_address; memset(mapped_address, 0, 12); memcpy(mapped_address + 12, &((struct sockaddr_in const *)sockaddr)->sin_addr.s_addr, 4); + } else { + *mmdb_error = MMDB_INVALID_NETWORK_ADDRESS_ERROR; + return result; } } @@ -990,12 +1000,12 @@ static int find_address_in_search_tree(const MMDB_s *const mmdb, result->netmask = current_bit; - if (value >= (uint64_t)node_count + mmdb->data_section_size) { - // The pointer points off the end of the database. + uint8_t type = record_type(mmdb, value); + if (type == MMDB_RECORD_TYPE_INVALID) { return MMDB_CORRUPT_SEARCH_TREE_ERROR; } - if (value == node_count) { + if (type == MMDB_RECORD_TYPE_EMPTY) { // record is empty result->found_entry = false; return MMDB_SUCCESS; @@ -1083,7 +1093,14 @@ static uint8_t record_type(const MMDB_s *const mmdb, uint64_t record) { return MMDB_RECORD_TYPE_EMPTY; } - if (record - node_count < mmdb->data_section_size) { + uint64_t data_offset = record - node_count; + if (data_offset < MMDB_DATA_SECTION_SEPARATOR) { + DEBUG_MSG("record points into the data section separator"); + return MMDB_RECORD_TYPE_INVALID; + } + + data_offset -= MMDB_DATA_SECTION_SEPARATOR; + if (data_offset < mmdb->data_section_size) { return MMDB_RECORD_TYPE_DATA; } @@ -1126,6 +1143,11 @@ int MMDB_read_node(const MMDB_s *const mmdb, node->left_record_type = record_type(mmdb, node->left_record); node->right_record_type = record_type(mmdb, node->right_record); + if (node->left_record_type == MMDB_RECORD_TYPE_INVALID || + node->right_record_type == MMDB_RECORD_TYPE_INVALID) { + return MMDB_CORRUPT_SEARCH_TREE_ERROR; + } + // Note that offset will be invalid if the record type is not // MMDB_RECORD_TYPE_DATA, but that's ok. Any use of the record entry // for other data types is a programming error. @@ -1421,6 +1443,13 @@ static int decode_one(const MMDB_s *const mmdb, MMDB_entry_data_s *entry_data) { const uint8_t *mem = mmdb->data_section; + if (mmdb->data_section_size == 0) { + // decode_one is also called with a fake mmdb whose data_section + // points at the metadata; either way an empty section is invalid. + DEBUG_MSG("decode_one called with an empty section"); + return MMDB_INVALID_DATA_ERROR; + } + // We subtract rather than add as it possible that offset + 1 // could overflow for a corrupt database while an underflow // from data_section_size - 1 should not be possible. @@ -2254,6 +2283,9 @@ const char *MMDB_strerror(int error_code) { case MMDB_IPV6_LOOKUP_IN_IPV4_DATABASE_ERROR: return "You attempted to look up an IPv6 address in an IPv4-only " "database"; + case MMDB_INVALID_NETWORK_ADDRESS_ERROR: + return "The sockaddr family is unsupported; only AF_INET and " + "AF_INET6 are accepted"; default: return "Unknown error code"; } diff --git a/t/CMakeLists.txt b/t/CMakeLists.txt index 5065cca8..04627b60 100644 --- a/t/CMakeLists.txt +++ b/t/CMakeLists.txt @@ -17,6 +17,7 @@ set(TEST_TARGET_NAMES get_value_t ipv4_start_cache_t ipv6_lookup_in_ipv4_t + metadata_marker_t metadata_pointers_t metadata_t no_map_get_value_t @@ -32,6 +33,7 @@ if(UNIX) # or if (NOT WIN32) bad_epoch_t bad_indent_t empty_container_metadata_t + invalid_sockaddr_t max_depth_t threads_t ) diff --git a/t/Makefile.am b/t/Makefile.am index a85c507d..630c664c 100644 --- a/t/Makefile.am +++ b/t/Makefile.am @@ -21,9 +21,10 @@ check_PROGRAMS = \ basic_lookup_t data_entry_list_t \ data-pool-t data_types_t double_close_t dump_t empty_container_metadata_t \ gai_error_t get_value_t \ - get_value_pointer_bug_t \ + get_value_pointer_bug_t invalid_sockaddr_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 \ + metadata_marker_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 diff --git a/t/bad_search_tree_t.c b/t/bad_search_tree_t.c index 2ec8dd31..4eb0cf39 100644 --- a/t/bad_search_tree_t.c +++ b/t/bad_search_tree_t.c @@ -41,8 +41,63 @@ void test_read_node_bounds(void) { free(mmdb); } +// Open a pre-built corruption fixture and assert that all rejection paths +// fire. `lookup_ip` must traverse the corrupted record from node 0 — high +// bit 0 for a corrupted left record, 1 for a corrupted right record. +static void check_corrupt_record(const char *label, + const char *fixture, + const char *lookup_ip) { + char *db_file = bad_database_path(fixture); + MMDB_s mmdb; + int status = MMDB_open(db_file, MMDB_MODE_MMAP, &mmdb); + cmp_ok(status, "==", MMDB_SUCCESS, "%s: opened crafted bad MMDB", label); + if (status != MMDB_SUCCESS) { + free(db_file); + return; + } + + MMDB_search_node_s node; + status = MMDB_read_node(&mmdb, 0, &node); + cmp_ok(status, + "==", + MMDB_CORRUPT_SEARCH_TREE_ERROR, + "%s: MMDB_read_node rejects record as corrupt", + label); + + int gai_error = 0; + int mmdb_error = 0; + MMDB_lookup_result_s result = + MMDB_lookup_string(&mmdb, lookup_ip, &gai_error, &mmdb_error); + cmp_ok(gai_error, "==", 0, "%s: lookup string parse succeeds", label); + cmp_ok(mmdb_error, + "==", + MMDB_CORRUPT_SEARCH_TREE_ERROR, + "%s: MMDB_lookup_string rejects record", + label); + ok(!result.found_entry, "%s: lookup reports no entry", label); + + MMDB_close(&mmdb); + free(db_file); +} + +void test_separator_record_rejected(void) { + // Records in the half-open range [node_count + 1, node_count + 16) point + // into the 16-byte separator between the search tree and data section + // and must be rejected as corrupt. + check_corrupt_record("left record, first separator byte", + "libmaxminddb-separator-record-min-left.mmdb", + "1.1.1.1"); + check_corrupt_record("right record, first separator byte", + "libmaxminddb-separator-record-min-right.mmdb", + "128.0.0.0"); + check_corrupt_record("left record, last separator byte", + "libmaxminddb-separator-record-max-left.mmdb", + "1.1.1.1"); +} + int main(void) { plan(NO_PLAN); test_read_node_bounds(); + test_separator_record_rejected(); done_testing(); } diff --git a/t/invalid_sockaddr_t.c b/t/invalid_sockaddr_t.c new file mode 100644 index 00000000..32535437 --- /dev/null +++ b/t/invalid_sockaddr_t.c @@ -0,0 +1,51 @@ +#include "maxminddb_test_helper.h" + +static void test_invalid_sockaddr_family(const char *filename, + sa_family_t family, + const char *open_msg, + const char *family_msg) { + char *db_file = test_database_path(filename); + MMDB_s *mmdb = open_ok(db_file, MMDB_MODE_MMAP, open_msg); + free(db_file); + + if (!mmdb) { + return; + } + + struct sockaddr addr = {.sa_family = family}; + int mmdb_error = MMDB_SUCCESS; + MMDB_lookup_result_s result = + MMDB_lookup_sockaddr(mmdb, &addr, &mmdb_error); + + ok(!result.found_entry, "%s: no entry returned", family_msg); + cmp_ok(result.netmask, "==", 0, "%s: netmask left at zero", family_msg); + cmp_ok(mmdb_error, + "==", + MMDB_INVALID_NETWORK_ADDRESS_ERROR, + "%s: MMDB_lookup_sockaddr rejects unsupported family", + family_msg); + + MMDB_close(mmdb); + free(mmdb); +} + +int main(void) { + plan(NO_PLAN); + test_invalid_sockaddr_family("MaxMind-DB-test-ipv4-24.mmdb", + AF_UNIX, + "opened IPv4 test database (AF_UNIX)", + "AF_UNIX against IPv4 db"); + test_invalid_sockaddr_family("MaxMind-DB-test-ipv4-24.mmdb", + AF_UNSPEC, + "opened IPv4 test database (AF_UNSPEC)", + "AF_UNSPEC against IPv4 db"); + test_invalid_sockaddr_family("MaxMind-DB-test-ipv6-24.mmdb", + AF_UNIX, + "opened IPv6 test database (AF_UNIX)", + "AF_UNIX against IPv6 db"); + test_invalid_sockaddr_family("MaxMind-DB-test-ipv6-24.mmdb", + AF_UNSPEC, + "opened IPv6 test database (AF_UNSPEC)", + "AF_UNSPEC against IPv6 db"); + done_testing(); +} diff --git a/t/maxmind-db b/t/maxmind-db index 819f226f..1486d037 160000 --- a/t/maxmind-db +++ b/t/maxmind-db @@ -1 +1 @@ -Subproject commit 819f226fbf8290c2b171ac077e6e050618dd3574 +Subproject commit 1486d037d53617e3f111933753ac52f80e1b4b1c diff --git a/t/metadata_marker_t.c b/t/metadata_marker_t.c new file mode 100644 index 00000000..ee004bb3 --- /dev/null +++ b/t/metadata_marker_t.c @@ -0,0 +1,23 @@ +#include "maxminddb_test_helper.h" + +static void test_trailing_metadata_marker(void) { + char *db_file = bad_database_path("libmaxminddb-metadata-marker-only.mmdb"); + MMDB_s mmdb; + int status = MMDB_open(db_file, MMDB_MODE_MMAP, &mmdb); + cmp_ok(status, + "==", + MMDB_INVALID_METADATA_ERROR, + "MMDB_open rejects a file containing only the metadata marker"); + + if (status == MMDB_SUCCESS) { + MMDB_close(&mmdb); + } + + free(db_file); +} + +int main(void) { + plan(NO_PLAN); + test_trailing_metadata_marker(); + done_testing(); +}