Bound decoded values to prevent a pointer fan-out DoS (STF-1488)

Changes.md

@@ -1,5 +1,28 @@
 ## next release
 
+- Fixed a denial-of-service issue in `MMDB_get_entry_data_list()`. A crafted
+  database could nest data-section pointers to shared targets so that decoding
+  one entry cost exponential time and memory from a small file. The decoder now
+  limits each returned list to 65,536 values and returns
+  `MMDB_DECODER_LIMIT_ERROR` when an entry exceeds it. The largest real records
+  MaxMind produces decode a few hundred values. This follows the proposed
+  Reader Resource Limits guidance for the MaxMind DB specification. See
+  GHSA-hj94-g986-h9r7.
+- Fixed a related payload-amplification denial of service. A crafted database
+  can point many times at one large value, so `MMDB_get_entry_data_list()`
+  returns a bounded number of nodes that together reference far more bytes than
+  the file holds. A caller that copies each node into a string then materializes
+  that amplified total. The decoder now also limits the total string and bytes
+  payload it exposes for a single entry to 2 MiB. Exceeding either new limit
+  returns `MMDB_DECODER_LIMIT_ERROR` and leaves the output list set to `NULL`.
+  These limits also protect the `languages` and `description` structures read
+  by `MMDB_open()`, where an over-limit structure is reported as
+  `MMDB_INVALID_METADATA_ERROR`. Both limits can be raised when rebuilding the
+  library with `-DMAXIMUM_DATA_STRUCTURE_VALUES=<values>` and
+  `-DMAXIMUM_DATA_STRUCTURE_BYTES=<bytes>`. Applications using a packaged
+  library can retrieve individual values with `MMDB_get_value()` or
+  `MMDB_aget_value()` without expanding the complete structure. See
+  GHSA-hj94-g986-h9r7.
 - 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

README.fuzzing.md

@@ -34,7 +34,7 @@ $ cmake --build . -j$(nproc)
 
 ```shell
 $ mkdir -p fuzz_mmdb_seed fuzz_mmdb_seed_corpus
-$ find ../t/maxmind-db/test-data/ -type f -size -4k -exec cp {} ./fuzz_mmdb_seed_corpus/ \;
+$ find ../t/maxmind-db/test-data/ -type f -size -256k -exec cp {} ./fuzz_mmdb_seed_corpus/ \;
 $ ./t/fuzz_mmdb fuzz_mmdb_seed/ fuzz_mmdb_seed_corpus/
 ```
 

doc/libmaxminddb.md

@@ -395,6 +395,9 @@ status codes are:
   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`.
+- `MMDB_DECODER_LIMIT_ERROR` - decoding an entry as a complete list would
+  exceed the configured value-count or string/bytes payload limit. The entry
+  may still be valid MaxMind DB data.
 
 All status codes should be treated as `int` values.
 
@@ -452,6 +455,11 @@ You can also pass `0` as the `flags` value in which case the database will be
 opened with the default flags. However, these defaults may change in future
 releases. The current default is `MMDB_MODE_MMAP`.
 
+Opening a database decodes its `languages` and `description` metadata. If one
+of these structures exceeds the decoder resource limits described under
+`MMDB_get_entry_data_list()`, this function returns
+`MMDB_INVALID_METADATA_ERROR`.
+
 ## `MMDB_close()`
 
 ```c
@@ -640,6 +648,24 @@ This function allows you to get all of the data for a complex data structure at
 once, rather than looking up each piece using repeated calls to
 `MMDB_get_value()`.
 
+To bound the work and caller-visible payload produced by crafted databases,
+this function decodes at most 65,536 list values and at most 2 MiB of UTF-8
+string and bytes payload per call. A structure exactly at either limit is
+accepted. If a structure exceeds either limit, the function returns
+`MMDB_DECODER_LIMIT_ERROR` and sets `entry_data_list` to `NULL`.
+
+The limits are per call and may be changed when rebuilding libmaxminddb by
+defining the positive integer macros `MAXIMUM_DATA_STRUCTURE_VALUES` and
+`MAXIMUM_DATA_STRUCTURE_BYTES`. For example, pass
+`-DMAXIMUM_DATA_STRUCTURE_BYTES=3145728` in the library's compiler flags. This
+requires rebuilding the library itself; defining the macro only while building
+an application does not change a packaged shared library.
+
+`MMDB_get_value()`, `MMDB_vget_value()`, and `MMDB_aget_value()` do not expand a
+complete structure and therefore do not charge these two budgets. Applications
+that cannot rebuild a packaged library can use those functions to retrieve a
+specific field from an otherwise over-limit record.
+
 ```c
 MMDB_lookup_result_s result =
     MMDB_lookup_sockaddr(&mmdb, address->ai_addr, &mmdb_error);
@@ -717,7 +743,9 @@ int MMDB_get_metadata_as_entry_data_list(
 
 This function allows you to retrieve the database metadata as a linked list of
 `MMDB_entry_data_list_s` structures. This can be a more convenient way to deal
-with the metadata than using the metadata structure directly.
+with the metadata than using the metadata structure directly. It uses the same
+per-call limits as `MMDB_get_entry_data_list()` and returns
+`MMDB_DECODER_LIMIT_ERROR` if the complete metadata list exceeds either one.
 
 ```c
     MMDB_entry_data_list_s *entry_data_list, *first;

include/maxminddb.h

@@ -87,6 +87,7 @@ extern "C" {
     #define MMDB_INVALID_NODE_NUMBER_ERROR (10)
     #define MMDB_IPV6_LOOKUP_IN_IPV4_DATABASE_ERROR (11)
     #define MMDB_INVALID_NETWORK_ADDRESS_ERROR (12)
+    #define MMDB_DECODER_LIMIT_ERROR (13)
 
     #if !(MMDB_UINT128_IS_BYTE_ARRAY)
         #if MMDB_UINT128_USING_MODE

src/data-pool.c

@@ -9,16 +9,22 @@
 #include <stddef.h>
 #include <stdlib.h>
 
-// Allocate an MMDB_data_pool_s. It initially has space for size
-// MMDB_entry_data_list_s structs.
-MMDB_data_pool_s *data_pool_new(size_t const size) {
+// Allocate an MMDB_data_pool_s. It initially has space for up to size
+// MMDB_entry_data_list_s structs and will never reserve more than max_size.
+MMDB_data_pool_s *data_pool_new(size_t size, size_t const max_size) {
     MMDB_data_pool_s *const pool = calloc(1, sizeof(MMDB_data_pool_s));
     if (!pool) {
         return NULL;
     }
 
-    if (size == 0 ||
-        !can_multiply(SIZE_MAX, size, sizeof(MMDB_entry_data_list_s))) {
+    if (size == 0 || max_size == 0) {
+        data_pool_destroy(pool);
+        return NULL;
+    }
+    if (size > max_size) {
+        size = max_size;
+    }
+    if (!can_multiply(SIZE_MAX, size, sizeof(MMDB_entry_data_list_s))) {
         data_pool_destroy(pool);
         return NULL;
     }
@@ -31,6 +37,8 @@ MMDB_data_pool_s *data_pool_new(size_t const size) {
     pool->blocks[0]->pool = pool;
 
     pool->sizes[0] = size;
+    pool->capacity = size;
+    pool->max_size = max_size;
 
     pool->block = pool->blocks[0];
 
@@ -75,6 +83,10 @@ MMDB_entry_data_list_s *data_pool_alloc(MMDB_data_pool_s *const pool) {
         return element;
     }
 
+    if (pool->capacity == pool->max_size) {
+        return NULL;
+    }
+
     // Take it from a new block of memory.
 
     size_t const new_index = pool->index + 1;
@@ -83,10 +95,9 @@ MMDB_entry_data_list_s *data_pool_alloc(MMDB_data_pool_s *const pool) {
         return NULL;
     }
 
-    if (!can_multiply(SIZE_MAX, pool->size, 2)) {
-        return NULL;
-    }
-    size_t const new_size = pool->size * 2;
+    size_t const remaining = pool->max_size - pool->capacity;
+    size_t const new_size =
+        pool->size <= remaining / 2 ? pool->size * 2 : remaining;
 
     if (!can_multiply(SIZE_MAX, new_size, sizeof(MMDB_entry_data_list_s))) {
         return NULL;
@@ -104,6 +115,7 @@ MMDB_entry_data_list_s *data_pool_alloc(MMDB_data_pool_s *const pool) {
 
     pool->size = new_size;
     pool->sizes[pool->index] = pool->size;
+    pool->capacity += new_size;
 
     MMDB_entry_data_list_s *const element = pool->block;
     pool->used = 1;

src/data-pool.h

@@ -7,12 +7,12 @@
 #include <stddef.h>
 
 // This should be large enough that we never need to grow the array of pointers
-// to blocks. 32 is enough. Even starting out of with size 1 (1 struct), the
-// 32nd element alone will provide 2**32 structs as we exponentially increase
+// to blocks. 64 is enough. Even starting with size 1 (1 struct), the
+// 64th element alone will provide 2**63 structs as we exponentially increase
 // the number in each block. Being confident that we do not have to grow the
 // array lets us avoid writing code to do that. That code would be risky as it
 // would rarely be hit and likely not be well tested.
-#define DATA_POOL_NUM_BLOCKS 32
+#define DATA_POOL_NUM_BLOCKS 64
 
 // A pool of memory for MMDB_entry_data_list_s structs. This is so we can
 // allocate multiple up front rather than one at a time for performance
@@ -33,6 +33,12 @@ typedef struct MMDB_data_pool_s {
     // How many used in the current block, counting by structs.
     size_t used;
 
+    // Total number of structs reserved across all blocks.
+    size_t capacity;
+
+    // Maximum number of structs this pool may reserve.
+    size_t max_size;
+
     // The current block we're allocating out of.
     MMDB_entry_data_list_s *block;
 
@@ -45,7 +51,7 @@ typedef struct MMDB_data_pool_s {
 } MMDB_data_pool_s;
 
 bool can_multiply(size_t const, size_t const, size_t const);
-MMDB_data_pool_s *data_pool_new(size_t const);
+MMDB_data_pool_s *data_pool_new(size_t const, size_t const);
 void data_pool_destroy(MMDB_data_pool_s *const);
 MMDB_entry_data_list_s *data_pool_alloc(MMDB_data_pool_s *const);
 MMDB_entry_data_list_s *data_pool_to_list(MMDB_data_pool_s *const);