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

Changes.md

@@ -1,5 +1,25 @@
 ## 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 the number of values it decodes for a single entry to 65,536 and
+  returns `MMDB_INVALID_DATA_ERROR` when an entry exceeds it. The largest real
+  records MaxMind produces decode a few hundred values. This matches the reader
+  resource limits recommended by a proposed update to 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 decodes for a single entry to 2 MiB and returns
+  `MMDB_INVALID_DATA_ERROR` when an entry exceeds it. This also rejects a rare
+  format-valid record whose own string and bytes fields total more than the
+  limit, since a single field can be up to 16,843,036 bytes. For such a
+  database, raise the limit at build time with
+  `-DMAXIMUM_DATA_STRUCTURE_BYTES=<bytes>`. The largest records MaxMind produces
+  hold about a kilobyte of payload. 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

src/data-pool.h

@@ -42,6 +42,21 @@ typedef struct MMDB_data_pool_s {
     // An array of pointers to blocks of memory holding space for list
     // elements.
     MMDB_entry_data_list_s *blocks[DATA_POOL_NUM_BLOCKS];
+
+    // Number of decode visits charged for a single entry, across all blocks.
+    // get_entry_data_list charges one per call, so following a pointer into a
+    // container charges every level it expands even where those visits reuse
+    // one output list node. This bounds the fan-out work an attacker inflates,
+    // which is not the same as the count of output elements (see
+    // MAXIMUM_DATA_STRUCTURE_VALUES).
+    size_t length;
+
+    // Total bytes of string and bytes payloads decoded so far, across all
+    // blocks. The node count stays under MAXIMUM_DATA_STRUCTURE_VALUES even
+    // when many pointers target one large value, so this separately bounds the
+    // total payload a caller would copy out of the list (see
+    // MAXIMUM_DATA_STRUCTURE_BYTES).
+    uint64_t bytes;
 } MMDB_data_pool_s;
 
 bool can_multiply(size_t const, size_t const, size_t const);

src/maxminddb.c

@@ -35,6 +35,25 @@ typedef ADDRESS_FAMILY sa_family_t;
 
 #define MMDB_DATA_SECTION_SEPARATOR (16)
 #define MAXIMUM_DATA_STRUCTURE_DEPTH (512)
+// The maximum number of data-section values decoded for a single entry. This
+// bounds a pointer fan-out, where nested pointers to shared targets would
+// otherwise cost 2**depth decode operations. The largest real records decode a
+// few hundred values, so this leaves a wide margin. See the MaxMind DB
+// specification's "Reader Resource Limits".
+#define MAXIMUM_DATA_STRUCTURE_VALUES ((size_t)1 << 16)
+
+// The maximum total bytes of string and bytes payloads decoded for a single
+// entry. libmaxminddb borrows payload bytes (each node points into the data
+// section, it does not copy), so the value count above already bounds the
+// library's own memory. But a fan-out of pointers to one large value produces
+// many nodes that all reference it, and a caller that copies each node into a
+// language string then materializes far more than the file holds. This bounds
+// that copied total. The largest real records hold about a kilobyte of
+// payload, so 2 MiB leaves a wide margin while stopping the amplification. It
+// can be raised at build time with -DMAXIMUM_DATA_STRUCTURE_BYTES=<n>.
+#ifndef MAXIMUM_DATA_STRUCTURE_BYTES
+    #define MAXIMUM_DATA_STRUCTURE_BYTES ((size_t)1 << 21)
+#endif
 
 #ifdef MMDB_DEBUG
     #define DEBUG_MSG(msg) fprintf(stderr, msg "\n")
@@ -1725,6 +1744,10 @@ static int get_entry_data_list(const MMDB_s *const mmdb,
         return MMDB_INVALID_DATA_ERROR;
     }
     depth++;
+    if (++pool->length > MAXIMUM_DATA_STRUCTURE_VALUES) {
+        DEBUG_MSG("reached the maximum number of data structure values");
+        return MMDB_INVALID_DATA_ERROR;
+    }
     CHECKED_DECODE_ONE(mmdb, offset, &entry_data_list->entry_data);
 
     switch (entry_data_list->entry_data.type) {
@@ -1826,6 +1849,23 @@ static int get_entry_data_list(const MMDB_s *const mmdb,
             break;
     }
 
+    // Charge the copied payload. Only string and bytes carry a variable-length
+    // payload that a caller copies. Integers are size-validated and tiny,
+    // floats are fixed width, and container data_size is an element count, not
+    // bytes. Pointers have been resolved to their target above, so a pointer to
+    // a string is charged here as the string. This runs once per node, so a
+    // fan-out that references one large value many times is charged each time.
+    // pool->bytes is a uint64 and the value-count limit caps how many payloads
+    // are charged, so this sum cannot overflow before the comparison.
+    if (entry_data_list->entry_data.type == MMDB_DATA_TYPE_UTF8_STRING ||
+        entry_data_list->entry_data.type == MMDB_DATA_TYPE_BYTES) {
+        pool->bytes += entry_data_list->entry_data.data_size;
+        if (pool->bytes > MAXIMUM_DATA_STRUCTURE_BYTES) {
+            DEBUG_MSG("reached the maximum data structure size");
+            return MMDB_INVALID_DATA_ERROR;
+        }
+    }
+
     return MMDB_SUCCESS;
 }
 

t/CMakeLists.txt

@@ -22,6 +22,7 @@ set(TEST_TARGET_NAMES
   metadata_t
   no_map_get_value_t
   overflow_bounds_t
+  pointer_dos_t
   read_node_t
   version_t
 )

t/Makefile.am

@@ -24,7 +24,7 @@ check_PROGRAMS = \
 	get_value_pointer_bug_t invalid_sockaddr_t \
 	ipv4_start_cache_t ipv6_lookup_in_ipv4_t max_depth_t metadata_t \
 	metadata_marker_t metadata_pointers_t no_map_get_value_t \
-	overflow_bounds_t read_node_t \
+	overflow_bounds_t pointer_dos_t read_node_t \
 	threads_t version_t
 
 data_pool_t_LDFLAGS = $(AM_LDFLAGS) -lm

t/pointer_dos_t.c

@@ -0,0 +1,157 @@
+#include "maxminddb_test_helper.h"
+
+// A non-NULL sentinel for the output pointer. On a resource-limit error
+// MMDB_get_entry_data_list must set the caller's output to NULL, not leave a
+// stale pointer that a caller could later free. Starting from this sentinel and
+// asserting the call replaces it with NULL proves that clearing. The node is
+// not heap allocated, so it must never be freed.
+static MMDB_entry_data_list_s sentinel_node;
+#define OUTPUT_SENTINEL (&sentinel_node)
+
+/* Decoding a crafted fan-out record must be rejected, not run to exhaustion.
+ * The value-count and payload byte limits both surface as
+ * MMDB_INVALID_DATA_ERROR from MMDB_get_entry_data_list, and the output list
+ * must be set to NULL. */
+static void test_fan_out_rejected(const char *fixture, const char *desc) {
+    char *db_file = test_database_path(fixture);
+
+    MMDB_s mmdb;
+    int status = MMDB_open(db_file, MMDB_MODE_MMAP, &mmdb);
+    cmp_ok(status, "==", MMDB_SUCCESS, "opened %s fixture", desc);
+    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, "%s: lookup succeeded", desc);
+    ok(result.found_entry, "%s: entry found", desc);
+
+    if (result.found_entry) {
+        MMDB_entry_data_list_s *entry_data_list = OUTPUT_SENTINEL;
+        status = MMDB_get_entry_data_list(&result.entry, &entry_data_list);
+        cmp_ok(status,
+               "==",
+               MMDB_INVALID_DATA_ERROR,
+               "%s: MMDB_get_entry_data_list returns MMDB_INVALID_DATA_ERROR",
+               desc);
+        ok(entry_data_list == NULL,
+           "%s: output list is set to NULL after the error",
+           desc);
+        // Free a real partial list, or NULL as a no-op, but never the sentinel.
+        if (entry_data_list != OUTPUT_SENTINEL) {
+            MMDB_free_entry_data_list(entry_data_list);
+        }
+    }
+
+    MMDB_close(&mmdb);
+    free(db_file);
+}
+
+/* A normal record must still decode fully. Its payload is far below the limit,
+ * so the limits must not reject legitimate data. */
+static void test_normal_record_allowed(void) {
+    char *db_file = test_database_path("GeoIP2-City-Test.mmdb");
+
+    MMDB_s mmdb;
+    int status = MMDB_open(db_file, MMDB_MODE_MMAP, &mmdb);
+    cmp_ok(status, "==", MMDB_SUCCESS, "opened GeoIP2-City-Test");
+    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, "81.2.69.142", &gai_error, &mmdb_error);
+    ok(result.found_entry, "normal record: 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,
+               "normal record decodes with no false rejection");
+        ok(entry_data_list != NULL, "normal record: list returned");
+        MMDB_free_entry_data_list(entry_data_list);
+    }
+
+    MMDB_close(&mmdb);
+    free(db_file);
+}
+
+/* The counters are per call. A rejected decode must not leave state that
+ * changes a later decode on the same reader. */
+static void test_per_call_state(void) {
+    char *dos_file =
+        test_database_path("MaxMind-DB-test-payload-amplification-dos.mmdb");
+    MMDB_s dos;
+    if (MMDB_open(dos_file, MMDB_MODE_MMAP, &dos) == MMDB_SUCCESS) {
+        int gai, err;
+        MMDB_lookup_result_s result =
+            MMDB_lookup_string(&dos, "1.1.1.1", &gai, &err);
+        if (result.found_entry) {
+            MMDB_entry_data_list_s *first = OUTPUT_SENTINEL;
+            int s1 = MMDB_get_entry_data_list(&result.entry, &first);
+            cmp_ok(s1,
+                   "==",
+                   MMDB_INVALID_DATA_ERROR,
+                   "per-call: first decode of the attack record is rejected");
+            if (first != OUTPUT_SENTINEL) {
+                MMDB_free_entry_data_list(first);
+            }
+
+            MMDB_entry_data_list_s *second = OUTPUT_SENTINEL;
+            int s2 = MMDB_get_entry_data_list(&result.entry, &second);
+            cmp_ok(s2,
+                   "==",
+                   MMDB_INVALID_DATA_ERROR,
+                   "per-call: repeating it is still rejected, no leaked count");
+            if (second != OUTPUT_SENTINEL) {
+                MMDB_free_entry_data_list(second);
+            }
+
+            // Same-reader isolation. After the rejections a bounded decode on
+            // the same reader must still succeed. Offset 0 is the shared scalar
+            // the fan-out points at, a single small value well under the
+            // limits. A reader left with exhausted counters would reject it.
+            MMDB_entry_s scalar = {.mmdb = &dos, .offset = 0};
+            MMDB_entry_data_list_s *bounded = NULL;
+            int s3 = MMDB_get_entry_data_list(&scalar, &bounded);
+            cmp_ok(s3,
+                   "==",
+                   MMDB_SUCCESS,
+                   "per-call: a bounded decode on the same reader still works");
+            ok(bounded != NULL, "per-call: bounded decode returned a list");
+            MMDB_free_entry_data_list(bounded);
+        }
+        MMDB_close(&dos);
+    }
+    free(dos_file);
+}
+
+int main(void) {
+    plan(NO_PLAN);
+    /* Value-count limit: nested arrays of pointers to shared targets. */
+    test_fan_out_rejected("MaxMind-DB-test-pointer-decoder-dos.mmdb",
+                          "value-count fan-out");
+    /* Payload byte limit: many pointers to one large bytes value. */
+    test_fan_out_rejected("MaxMind-DB-test-payload-amplification-dos.mmdb",
+                          "payload amplification");
+    /* Worst case under the value-count limit, caught only by the byte limit. */
+    test_fan_out_rejected(
+        "MaxMind-DB-test-payload-amplification-dos-worst-case.mmdb",
+        "worst-case payload");
+    /* Payload byte limit via a shared UTF-8 string, the type bindings copy. */
+    test_fan_out_rejected(
+        "MaxMind-DB-test-payload-amplification-dos-string.mmdb",
+        "string payload amplification");
+    test_normal_record_allowed();
+    test_per_call_state();
+    done_testing();
+}