fixup! Bound decoder work to prevent a pointer fan-out denial of service

Author: oschwald

maxminddb/decoder.py

@@ -25,12 +25,14 @@
 # DB specification. It stops 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 the limit leaves a wide margin.
-# Pointer cycles and over-deep data are caught separately by Python's own
-# recursion limit (see ``decode``).
+# Pointer cycles and over-deep data are caught separately by an explicit,
+# call-local depth limit (see ``decode``).
 _MAX_VALUES = 1 << 16
+_MAX_DEPTH = 512
 _TOO_MANY_VALUES = (
     "The MaxMind DB file's data section exceeds the maximum number of values"
 )
+_TOO_DEEP = "The MaxMind DB file's data section exceeds the maximum depth"
 
 
 class Decoder:
@@ -63,10 +65,14 @@ def _decode_array(
         budget[0] -= size
         if budget[0] < 0:
             raise InvalidDatabaseError(_TOO_MANY_VALUES)
+        budget[1] += 1
+        if budget[1] > _MAX_DEPTH:
+            raise InvalidDatabaseError(_TOO_DEEP)
         array = []
         for _ in range(size):
             (value, offset) = self._decode(offset, budget)
             array.append(value)
+        budget[1] -= 1
         return array, offset
 
     def _decode_boolean(
@@ -136,11 +142,15 @@ def _decode_map(
         budget[0] -= size * 2
         if budget[0] < 0:
             raise InvalidDatabaseError(_TOO_MANY_VALUES)
+        budget[1] += 1
+        if budget[1] > _MAX_DEPTH:
+            raise InvalidDatabaseError(_TOO_DEEP)
         container: dict[str, Record] = {}
         for _ in range(size):
             (key, offset) = self._decode(offset, budget)
             (value, offset) = self._decode(offset, budget)
             container[cast("str", key)] = value
+        budget[1] -= 1
         return container, offset
 
     def _decode_pointer(
@@ -169,7 +179,11 @@ def _decode_pointer(
         if self._pointer_test:
             return pointer, new_offset
 
+        budget[1] += 1
+        if budget[1] > _MAX_DEPTH:
+            raise InvalidDatabaseError(_TOO_DEEP)
         (value, _) = self._decode(pointer, budget)
+        budget[1] -= 1
         return value, new_offset
 
     def _decode_uint(
@@ -215,16 +229,16 @@ def decode(self, offset: int) -> tuple[Record, int]:
 
         """
         # Bound the work per lookup so a crafted database cannot exhaust CPU or
-        # memory. ``budget`` is a single-element list so the running count is
-        # shared across the recursion. It is call-local, which keeps the
-        # decoder safe for concurrent reads. There is no separate depth limit: a
-        # pointer cycle or over-deep data exhausts Python's own recursion limit,
-        # which is converted into an InvalidDatabaseError.
+        # memory. ``budget`` carries the remaining value count and current
+        # structural depth so both are shared across the recursion. It is
+        # call-local, which keeps the decoder safe for concurrent reads. The
+        # explicit depth limit is independent of Python's process-wide recursion
+        # limit; RecursionError remains a fallback on interpreters whose stack
+        # limit is reached first.
         try:
-            return self._decode(offset, [_MAX_VALUES])
+            return self._decode(offset, [_MAX_VALUES, 0])
         except RecursionError as ex:
-            msg = "The MaxMind DB file's data section exceeds the maximum depth"
-            raise InvalidDatabaseError(msg) from ex
+            raise InvalidDatabaseError(_TOO_DEEP) from ex
 
     def _decode(self, offset: int, budget: list[int]) -> tuple[Record, int]:
         new_offset = offset + 1

tests/decoder_test.py

@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 import mmap
+import sys
 import unittest
 from typing import TYPE_CHECKING, Any, ClassVar
 
@@ -255,11 +256,37 @@ def test_pointer_fan_out_is_bounded(self) -> None:
             Decoder(bytes(buf), pointer_base=0).decode(prev)
 
     def test_cyclic_pointer_raises(self) -> None:
-        # A pointer to itself must raise a catchable InvalidDatabaseError
-        # rather than recursing until the interpreter's stack limit.
+        # A pointer to itself must hit the decoder's own depth limit even when
+        # Python's process-wide recursion limit is much higher.
         cyclic = bytes([0x20, 0x00])  # pointer (base 0) to offset 0, itself
-        with self.assertRaises(InvalidDatabaseError):
-            Decoder(cyclic, pointer_base=0).decode(0)
+        old_recursion_limit = sys.getrecursionlimit()
+        try:
+            sys.setrecursionlimit(10_000)
+            with self.assertRaisesRegex(
+                InvalidDatabaseError,
+                "^The MaxMind DB file's data section exceeds the maximum depth$",
+            ):
+                Decoder(cyclic, pointer_base=0).decode(0)
+        finally:
+            sys.setrecursionlimit(old_recursion_limit)
+
+    def test_container_depth_is_bounded_independently_of_recursion_limit(self) -> None:
+        # Each prefix is an array with one element. Raising Python's global
+        # recursion limit proves that the decoder's call-local limit is what
+        # accepts 512 containers and rejects the 513th.
+        at_limit = bytes([0x01, 0x04]) * 512 + bytes([0xA0])
+        over_limit = bytes([0x01, 0x04]) * 513 + bytes([0xA0])
+        old_recursion_limit = sys.getrecursionlimit()
+        try:
+            sys.setrecursionlimit(10_000)
+            Decoder(at_limit, pointer_base=0).decode(0)
+            with self.assertRaisesRegex(
+                InvalidDatabaseError,
+                "^The MaxMind DB file's data section exceeds the maximum depth$",
+            ):
+                Decoder(over_limit, pointer_base=0).decode(0)
+        finally:
+            sys.setrecursionlimit(old_recursion_limit)
 
     def test_oversized_map_is_bounded(self) -> None:
         # A map entry decodes a key and a value, so a map of N entries costs