Merge pull request #416 from maxmind/greg/eng-4239

Harden libmaxminddb against crafted/malformed databases

Author: horgh

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 `<inttypes.h>`.
+* 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.

bin/mmdblookup.c

@@ -8,6 +8,7 @@
 #include "maxminddb.h"
 #include <errno.h>
 #include <getopt.h>
+#include <inttypes.h>
 #ifndef _WIN32
     #include <pthread.h>
 #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,

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.

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,15 +1342,21 @@ 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) {
             CHECKED_DECODE_ONE(
                 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);

t/CMakeLists.txt

@@ -5,25 +5,33 @@ 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
   ipv6_lookup_in_ipv4_t
   metadata_pointers_t
   metadata_t
   no_map_get_value_t
+  overflow_bounds_t
   read_node_t
   version_t
 )
 
 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)

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