Bound decoder work to prevent a pointer fan-out DoS (STF-1570)

CHANGELOG.md

@@ -4,6 +4,24 @@ CHANGELOG
 1.14.0
 -------------------
 
+* Bounded the resources that the pure PHP decoder spends on a single lookup. A
+  crafted database could nest data-section pointers to shared targets so that
+  decoding one record cost exponential time and memory, or point many times at
+  one large value so that the decoder copied far more data than the file holds.
+  The decoder now follows the Reader Resource Limits section of the MaxMind DB
+  specification. Each lookup is limited to 65,536 values, 512 levels of
+  nesting, and 2 MiB of string and bytes payload.
+  * Exceeding a limit throws an `InvalidDatabaseException`.
+  * Opening a database whose metadata exceeds a limit throws the same
+    exception.
+  * A scalar that declares more than 16 bytes, the width of the widest
+    fixed-width type, is rejected as invalid data.
+* The bundled libmaxminddb used by `--with-maxminddb-bundled` builds of the
+  extension now applies the same decoder limits. The extension throws an
+  `InvalidDatabaseException` when a lookup exceeds them.
+* The pure PHP reader is about 40% faster on City lookups. It no longer seeks
+  before a read that continues where the last one ended, and it checks read
+  lengths with `strlen()` instead of `ftell()`.
 * The Windows build configuration now accepts either `libmaxminddb.lib` or
   `maxminddb.lib` when building the extension. The `lib` prefix was removed
   in libmaxminddb 1.6.0, but the libmaxminddb that PHP publishes for Windows

ext/bundled-include/maxminddb_config.h

@@ -31,7 +31,7 @@
  * every build, so a stale value here fails CI rather than shipping.
  */
 #ifndef PACKAGE_VERSION
-#define PACKAGE_VERSION "1.13.3"
+#define PACKAGE_VERSION "1.14.0"
 #endif
 
 #endif /* MAXMINDDB_CONFIG_H */

src/MaxMind/Db/Reader/Decoder.php

@@ -30,6 +30,29 @@ class Decoder
      */
     private $switchByteOrder;
 
+    /**
+     * Remaining decoded-value allowance for the current decode() call.
+     *
+     * @var int
+     */
+    private $budget = 0;
+
+    /**
+     * Remaining string and bytes payload allowance for the current decode()
+     * call.
+     *
+     * @var int
+     */
+    private $byteBudget = 0;
+
+    /**
+     * Stream position after the last read of the current decode() call, or -1
+     * when unknown.
+     *
+     * @var int
+     */
+    private $position = -1;
+
     private const _EXTENDED = 0;
     private const _POINTER = 1;
     private const _UTF8_STRING = 2;
@@ -47,6 +70,33 @@ class Decoder
     private const _BOOLEAN = 14;
     private const _FLOAT = 15;
 
+    // Per-lookup decode limits recommended by the MaxMind DB specification. The
+    // depth limit stops pointer cycles and over-deep data. The value limit
+    // stops a pointer fan-out, where nested pointers to shared targets would
+    // otherwise cost 2**depth decode operations. The count follows the
+    // specification's flat rule: the root is one value, each array and map
+    // charges its declared children (a map entry costs two, key and value),
+    // and a pointer costs nothing beyond the value it resolves to, which its
+    // container already charged. The largest real records decode a few hundred
+    // values, so the limit leaves a wide margin.
+    private const MAX_DEPTH = 512;
+    private const MAX_VALUES = 1 << 16;
+
+    // The value limit alone does not stop payload amplification: an array of
+    // pointers to one large string or bytes value keeps the value count low
+    // while forcing the reader to copy the target once per pointer. This
+    // second, independent limit bounds the total string and bytes payload
+    // copied for one lookup to 2 MiB, matching libmaxminddb and the Go reader.
+    // No real record approaches it, and re-decoding a shared target charges its
+    // payload again, so the fan-out is bounded.
+    private const MAX_PAYLOAD_BYTES = 1 << 21;
+
+    // A fixed-width scalar (a float, double, or integer) never needs more than
+    // 16 bytes (the width of a uint128). A larger declared size is either
+    // corrupt or an attempt to amplify the read of an oversized variable-length
+    // integer, so it is rejected before the bytes are materialized.
+    private const MAX_SCALAR_BYTES = 16;
+
     /**
      * @param resource $fileStream
      */
@@ -68,7 +118,31 @@ public function __construct(
      */
     public function decode(int $offset): array
     {
-        $ctrlByte = \ord(Util::read($this->fileStream, $offset, 1));
+        // Bound the work per lookup so a crafted database cannot exhaust CPU or
+        // memory. $budget counts decoded values and stops the pointer fan-out;
+        // $byteBudget counts copied string and bytes payload and stops payload
+        // amplification. Both live on the decoder and are reset here, so every
+        // call starts with the full allowance. Passing them by reference
+        // through each recursive call instead costs a few percent per lookup.
+        // No other lookup can observe them mid-decode: PHP runs one request
+        // per thread, and the decoder never yields while it decodes. The root
+        // value is charged here; containers charge their children.
+        $this->budget = self::MAX_VALUES - 1;
+        $this->byteBudget = self::MAX_PAYLOAD_BYTES;
+
+        // Other readers of the stream, such as the search tree walk, may have
+        // moved it since the last call.
+        $this->position = -1;
+
+        return $this->decodeWithBudget($offset, 0);
+    }
+
+    /**
+     * @return array<mixed>
+     */
+    private function decodeWithBudget(int $offset, int $depth): array
+    {
+        $ctrlByte = \ord($this->read($offset, 1));
         ++$offset;
 
         $type = $ctrlByte >> 5;
@@ -84,13 +158,22 @@ public function decode(int $offset): array
                 return [$pointer];
             }
 
-            [$result] = $this->decode($pointer);
+            if ($depth >= self::MAX_DEPTH) {
+                throw new InvalidDatabaseException(
+                    "The MaxMind DB file's data section exceeds the maximum depth"
+                );
+            }
+
+            // The value at the pointer's position was charged by its containing
+            // array or map, so the target costs nothing more. Only the depth
+            // grows.
+            [$result] = $this->decodeWithBudget($pointer, $depth + 1);
 
             return [$result, $offset];
         }
 
         if ($type === self::_EXTENDED) {
-            $nextByte = \ord(Util::read($this->fileStream, $offset, 1));
+            $nextByte = \ord($this->read($offset, 1));
 
             $type = $nextByte + 7;
 
@@ -108,35 +191,62 @@ public function decode(int $offset): array
 
         [$size, $offset] = $this->sizeFromCtrlByte($ctrlByte, $offset);
 
-        return $this->decodeByType($type, $offset, $size);
+        return $this->decodeByType($type, $offset, $size, $depth);
     }
 
     /**
      * @param int<0, max> $size
      *
      * @return array{0:mixed, 1:int}
      */
-    private function decodeByType(int $type, int $offset, int $size): array
+    private function decodeByType(int $type, int $offset, int $size, int $depth): array
     {
         switch ($type) {
             case self::_MAP:
-                return $this->decodeMap($size, $offset);
+                return $this->decodeMap($size, $offset, $depth);
 
             case self::_ARRAY:
-                return $this->decodeArray($size, $offset);
+                return $this->decodeArray($size, $offset, $depth);
 
             case self::_BOOLEAN:
                 return [$this->decodeBoolean($size), $offset];
+
+            case self::_BYTES:
+            case self::_UTF8_STRING:
+                // A string or bytes value is copied into a native string, so N
+                // pointers to one large value copy N times its length. Charge
+                // the payload against the byte budget wherever it is decoded,
+                // including inline inside a pointed-to container, so a shared
+                // target recharges each time it is followed. Compare before
+                // subtracting so an oversized declared size cannot drive the
+                // budget negative. A total exactly at the limit is allowed.
+                if ($size > $this->byteBudget) {
+                    throw new InvalidDatabaseException(
+                        "The MaxMind DB file's data section exceeds the maximum payload size"
+                    );
+                }
+                $this->byteBudget -= $size;
+
+                return [$this->read($offset, $size), $offset + $size];
+        }
+
+        // The remaining valid types are fixed-width scalars, none wider than a
+        // uint128. A few other control bytes also reach here: the container
+        // (12) and end-marker (13) types, and any unknown extended type. The
+        // size guard below rejects one that declares an oversized size, and the
+        // default case at the end of the switch rejects the rest. Reject an
+        // oversized declared size before materializing the bytes, so an
+        // oversized variable-length integer cannot amplify the read.
+        if ($size > self::MAX_SCALAR_BYTES) {
+            throw new InvalidDatabaseException(
+                "The MaxMind DB file's data section contains bad data (unknown data type or corrupt data)"
+            );
         }
 
         $newOffset = $offset + $size;
-        $bytes = Util::read($this->fileStream, $offset, $size);
+        $bytes = $this->read($offset, $size);
 
         switch ($type) {
-            case self::_BYTES:
-            case self::_UTF8_STRING:
-                return [$bytes, $newOffset];
-
             case self::_DOUBLE:
                 $this->verifySize(8, $size);
 
@@ -163,6 +273,39 @@ private function decodeByType(int $type, int $offset, int $size): array
         }
     }
 
+    /**
+     * Reads from the stream, seeking only when the read does not continue where
+     * the previous one ended. Most values in a record are laid out in order, and
+     * fseek() discards PHP's read buffer, so seeking before every read turned
+     * each small read into a system call. Skipping the seek makes a City lookup
+     * about 40% faster.
+     *
+     * @param int<0, max> $numberOfBytes
+     */
+    private function read(int $offset, int $numberOfBytes): string
+    {
+        if ($numberOfBytes === 0) {
+            return '';
+        }
+
+        $stream = $this->fileStream;
+        if ($offset !== $this->position && fseek($stream, $offset) !== 0) {
+            $this->position = -1;
+
+            throw new InvalidDatabaseException('The MaxMind DB file contains bad data');
+        }
+
+        $value = fread($stream, $numberOfBytes);
+        if ($value === false || \strlen($value) !== $numberOfBytes) {
+            $this->position = -1;
+
+            throw new InvalidDatabaseException('The MaxMind DB file contains bad data');
+        }
+        $this->position = $offset + $numberOfBytes;
+
+        return $value;
+    }
+
     private function verifySize(int $expected, int $actual): void
     {
         if ($expected !== $actual) {
@@ -172,15 +315,48 @@ private function verifySize(int $expected, int $actual): void
         }
     }
 
+    /**
+     * Applies the per-lookup limits when entering a container. The depth limit
+     * stops cycles and over-deep data (checked here and at pointer follows,
+     * the only places depth grows). The value budget is charged per declared
+     * element up front, so an oversized declared size is rejected before the
+     * loop reads anything. A pointer element costs nothing more when it is
+     * followed: its slot is charged here, and a container it resolves to
+     * charges its own children each time it is decoded, which is what bounds
+     * a fan-out through shared targets.
+     */
+    private function enterContainer(
+        int $size,
+        int $depth,
+        int $valuesPerEntry = 1
+    ): void {
+        if ($depth >= self::MAX_DEPTH) {
+            throw new InvalidDatabaseException(
+                "The MaxMind DB file's data section exceeds the maximum depth"
+            );
+        }
+        // Compare with a division rather than multiplying the declared size, so
+        // an oversized declaration cannot overflow the integer on 32-bit builds
+        // before the budget check runs.
+        if ($size > intdiv($this->budget, $valuesPerEntry)) {
+            throw new InvalidDatabaseException(
+                "The MaxMind DB file's data section exceeds the maximum number of values"
+            );
+        }
+        $this->budget -= $size * $valuesPerEntry;
+    }
+
     /**
      * @return array{0:array<mixed>, 1:int}
      */
-    private function decodeArray(int $size, int $offset): array
+    private function decodeArray(int $size, int $offset, int $depth): array
     {
+        $this->enterContainer($size, $depth);
+
         $array = [];
 
         for ($i = 0; $i < $size; ++$i) {
-            [$value, $offset] = $this->decode($offset);
+            [$value, $offset] = $this->decodeWithBudget($offset, $depth + 1);
             $array[] = $value;
         }
 
@@ -258,13 +434,16 @@ private function decodeInt32(string $bytes, int $size): int
     /**
      * @return array{0:array<string, mixed>, 1:int}
      */
-    private function decodeMap(int $size, int $offset): array
+    private function decodeMap(int $size, int $offset, int $depth): array
     {
+        // A map entry decodes a key and a value, so it costs two values.
+        $this->enterContainer($size, $depth, 2);
+
         $map = [];
 
         for ($i = 0; $i < $size; ++$i) {
-            [$key, $offset] = $this->decode($offset);
-            [$value, $offset] = $this->decode($offset);
+            [$key, $offset] = $this->decodeWithBudget($offset, $depth + 1);
+            [$value, $offset] = $this->decodeWithBudget($offset, $depth + 1);
             $map[$key] = $value;
         }
 
@@ -278,7 +457,7 @@ private function decodePointer(int $ctrlByte, int $offset): array
     {
         $pointerSize = (($ctrlByte >> 3) & 0x3) + 1;
 
-        $buffer = Util::read($this->fileStream, $offset, $pointerSize);
+        $buffer = $this->read($offset, $pointerSize);
         $offset += $pointerSize;
 
         switch ($pointerSize) {
@@ -404,7 +583,7 @@ private function sizeFromCtrlByte(int $ctrlByte, int $offset): array
         }
 
         $bytesToRead = $size - 28;
-        $bytes = Util::read($this->fileStream, $offset, $bytesToRead);
+        $bytes = $this->read($offset, $bytesToRead);
 
         if ($size === 29) {
             $size = 29 + \ord($bytes);

src/MaxMind/Db/Reader/Util.php

@@ -18,10 +18,8 @@ public static function read($stream, int $offset, int $numberOfBytes): string
         if (fseek($stream, $offset) === 0) {
             $value = fread($stream, $numberOfBytes);
 
-            // We check that the number of bytes read is equal to the number
-            // asked for. We use ftell as getting the length of $value is
-            // much slower.
-            if ($value !== false && ftell($stream) - $offset === $numberOfBytes) {
+            // Check that the number of bytes read is the number asked for.
+            if ($value !== false && \strlen($value) === $numberOfBytes) {
                 return $value;
             }
         }