Merge pull request #431 from maxmind/greg/fix-findings

Fix several minor safety issues

Author: horgh

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.

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

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

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";
     }

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
   )

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

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();
 }

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();
+}

t/maxmind-db

@@ -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();
+}