Introduce max_multipart_body_parts INI

This fixes GHSA-54hq-v5wp-fqgv DOS vulnerabality by limitting number of parsed multipart body parts as currently all parts were always parsed.
php · Feb 14, 2023 · 94fce68 · 94fce68
1 parent 830bdb5
commit 94fce68
Show file tree

Hide file tree

Showing 7 changed files with 262 additions and 15 deletions.
diff --git a/NEWS b/NEWS
@@ -8,6 +8,10 @@ PHP                                                                        NEWS
   . Fixed bug #81746 (1-byte array overrun in common path resolve code).
     (CVE-2023-0568). (Niels Dossche)
 
+- FPM
+  . Fixed bug GHSA-54hq-v5wp-fqgv (DOS vulnerability when parsing multipart
+    request body). (CVE-2023-0662) (Jakub Zelenka)
+
 02 Feb 2023, PHP 8.1.15
 
 - Apache:

diff --git a/main/main.c b/main/main.c
@@ -746,6 +746,7 @@ PHP_INI_BEGIN()
 	PHP_INI_ENTRY("disable_functions",			"",			PHP_INI_SYSTEM,		NULL)
 	PHP_INI_ENTRY("disable_classes",			"",			PHP_INI_SYSTEM,		NULL)
 	PHP_INI_ENTRY("max_file_uploads",			"20",			PHP_INI_SYSTEM|PHP_INI_PERDIR,		NULL)
+	PHP_INI_ENTRY("max_multipart_body_parts",	"-1",			PHP_INI_SYSTEM|PHP_INI_PERDIR,		NULL)
 
 	STD_PHP_INI_BOOLEAN("allow_url_fopen",		"1",		PHP_INI_SYSTEM,		OnUpdateBool,		allow_url_fopen,		php_core_globals,		core_globals)
 	STD_PHP_INI_BOOLEAN("allow_url_include",	"0",		PHP_INI_SYSTEM,		OnUpdateBool,		allow_url_include,		php_core_globals,		core_globals)

diff --git a/main/rfc1867.c b/main/rfc1867.c
@@ -686,6 +686,7 @@ SAPI_API SAPI_POST_HANDLER_FUNC(rfc1867_post_handler) /* {{{ */
 	void *event_extra_data = NULL;
 	unsigned int llen = 0;
 	int upload_cnt = INI_INT("max_file_uploads");
+	int body_parts_cnt = INI_INT("max_multipart_body_parts");
 	const zend_encoding *internal_encoding = zend_multibyte_get_internal_encoding();
 	php_rfc1867_getword_t getword;
 	php_rfc1867_getword_conf_t getword_conf;
@@ -707,6 +708,11 @@ SAPI_API SAPI_POST_HANDLER_FUNC(rfc1867_post_handler) /* {{{ */
 		return;
 	}
 
+	if (body_parts_cnt < 0) {
+		body_parts_cnt = PG(max_input_vars) + upload_cnt;
+	}
+	int body_parts_limit = body_parts_cnt;
+
 	/* Get the boundary */
 	boundary = strstr(content_type_dup, "boundary");
 	if (!boundary) {
@@ -791,6 +797,11 @@ SAPI_API SAPI_POST_HANDLER_FUNC(rfc1867_post_handler) /* {{{ */
 			char *pair = NULL;
 			int end = 0;
 
+			if (--body_parts_cnt < 0) {
+				php_error_docref(NULL, E_WARNING, "Multipart body parts limit exceeded %d. To increase the limit change max_multipart_body_parts in php.ini.", body_parts_limit);
+				goto fileupload_done;
+			}
+
 			while (isspace(*cd)) {
 				++cd;
 			}

diff --git a/sapi/fpm/tests/ghsa-54hq-v5wp-fqgv-max-body-parts-custom.phpt b/sapi/fpm/tests/ghsa-54hq-v5wp-fqgv-max-body-parts-custom.phpt
@@ -0,0 +1,53 @@
+--TEST--
+FPM: GHSA-54hq-v5wp-fqgv - max_multipart_body_parts ini custom value
+--SKIPIF--
+<?php include "skipif.inc"; ?>
+--FILE--
+<?php
+
+require_once "tester.inc";
+
+$cfg = <<<EOT
+[global]
+error_log = {{FILE:LOG}}
+[unconfined]
+listen = {{ADDR}}
+pm = dynamic
+pm.max_children = 5
+pm.start_servers = 1
+pm.min_spare_servers = 1
+pm.max_spare_servers = 3
+php_admin_value[html_errors] = false
+php_admin_value[max_input_vars] = 20
+php_admin_value[max_file_uploads] = 5
+php_admin_value[max_multipart_body_parts] = 10
+php_flag[display_errors] = On
+EOT;
+
+$code = <<<EOT
+<?php
+var_dump(count(\$_POST));
+EOT;
+
+$tester = new FPM\Tester($cfg, $code);
+$tester->start();
+$tester->expectLogStartNotices();
+echo $tester
+    ->request(stdin: [
+        'parts' => [
+            'count' => 30,
+        ]
+    ])
+    ->getBody();
+$tester->terminate();
+$tester->close();
+
+?>
+--EXPECT--
+Warning: Unknown: Multipart body parts limit exceeded 10. To increase the limit change max_multipart_body_parts in php.ini. in Unknown on line 0
+int(10)
+--CLEAN--
+<?php
+require_once "tester.inc";
+FPM\Tester::clean();
+?>
diff --git a/sapi/fpm/tests/ghsa-54hq-v5wp-fqgv-max-body-parts-default.phpt b/sapi/fpm/tests/ghsa-54hq-v5wp-fqgv-max-body-parts-default.phpt
@@ -0,0 +1,54 @@
+--TEST--
+FPM: GHSA-54hq-v5wp-fqgv - max_multipart_body_parts ini default
+--SKIPIF--
+<?php include "skipif.inc"; ?>
+--FILE--
+<?php
+
+require_once "tester.inc";
+
+$cfg = <<<EOT
+[global]
+error_log = {{FILE:LOG}}
+[unconfined]
+listen = {{ADDR}}
+pm = dynamic
+pm.max_children = 5
+pm.start_servers = 1
+pm.min_spare_servers = 1
+pm.max_spare_servers = 3
+php_admin_value[html_errors] = false
+php_admin_value[max_input_vars] = 20
+php_admin_value[max_file_uploads] = 5
+php_flag[display_errors] = On
+EOT;
+
+$code = <<<EOT
+<?php
+var_dump(count(\$_POST));
+EOT;
+
+$tester = new FPM\Tester($cfg, $code);
+$tester->start();
+$tester->expectLogStartNotices();
+echo $tester
+    ->request(stdin: [
+        'parts' => [
+            'count' => 30,
+        ]
+    ])
+    ->getBody();
+$tester->terminate();
+$tester->close();
+
+?>
+--EXPECT--
+Warning: Unknown: Input variables exceeded 20. To increase the limit change max_input_vars in php.ini. in Unknown on line 0
+
+Warning: Unknown: Multipart body parts limit exceeded 25. To increase the limit change max_multipart_body_parts in php.ini. in Unknown on line 0
+int(20)
+--CLEAN--
+<?php
+require_once "tester.inc";
+FPM\Tester::clean();
+?>
diff --git a/sapi/fpm/tests/ghsa-54hq-v5wp-fqgv-max-file-uploads.phpt b/sapi/fpm/tests/ghsa-54hq-v5wp-fqgv-max-file-uploads.phpt
@@ -0,0 +1,52 @@
+--TEST--
+FPM: GHSA-54hq-v5wp-fqgv - exceeding max_file_uploads
+--SKIPIF--
+<?php include "skipif.inc"; ?>
+--FILE--
+<?php
+
+require_once "tester.inc";
+
+$cfg = <<<EOT
+[global]
+error_log = {{FILE:LOG}}
+[unconfined]
+listen = {{ADDR}}
+pm = dynamic
+pm.max_children = 5
+pm.start_servers = 1
+pm.min_spare_servers = 1
+pm.max_spare_servers = 3
+php_admin_value[html_errors] = false
+php_admin_value[max_file_uploads] = 5
+php_flag[display_errors] = On
+EOT;
+
+$code = <<<EOT
+<?php
+var_dump(count(\$_FILES));
+EOT;
+
+$tester = new FPM\Tester($cfg, $code);
+$tester->start();
+$tester->expectLogStartNotices();
+echo $tester
+    ->request(stdin: [
+        'parts' => [
+            'count' => 10,
+            'param' => 'filename'
+        ]
+    ])
+    ->getBody();
+$tester->terminate();
+$tester->close();
+
+?>
+--EXPECT--
+Warning: Maximum number of allowable file uploads has been exceeded in Unknown on line 0
+int(5)
+--CLEAN--
+<?php
+require_once "tester.inc";
+FPM\Tester::clean();
+?>
diff --git a/sapi/fpm/tests/tester.inc b/sapi/fpm/tests/tester.inc
@@ -574,14 +574,16 @@ class Tester
      * @param array       $headers
      * @param string|null $uri
      * @param string|null $scriptFilename
+     * @param string|null $stdin
      *
      * @return array
      */
     private function getRequestParams(
         string $query = '',
         array $headers = [],
         string $uri = null,
-        string $scriptFilename = null
+        string $scriptFilename = null,
+        ?string $stdin = null
     ): array {
         if (is_null($uri)) {
             $uri = $this->makeSourceFile();
@@ -590,7 +592,7 @@ class Tester
         $params = array_merge(
             [
                 'GATEWAY_INTERFACE' => 'FastCGI/1.0',
-                'REQUEST_METHOD'    => 'GET',
+                'REQUEST_METHOD'    => is_null($stdin) ? 'GET' : 'POST',
                 'SCRIPT_FILENAME'   => $scriptFilename ?: $uri,
                 'SCRIPT_NAME'       => $uri,
                 'QUERY_STRING'      => $query,
@@ -605,7 +607,7 @@ class Tester
                 'SERVER_PROTOCOL'   => 'HTTP/1.1',
                 'DOCUMENT_ROOT'     => __DIR__,
                 'CONTENT_TYPE'      => '',
-                'CONTENT_LENGTH'    => 0
+                'CONTENT_LENGTH'    => strlen($stdin ?? "") // Default to 0
             ],
             $headers
         );
@@ -615,21 +617,86 @@ class Tester
         });
     }
 
+    /**
+     * Parse stdin and generate data for multipart config.
+     *
+     * @param array $stdin
+     * @param array $headers
+     *
+     * @return void
+     * @throws \Exception
+     */
+    private function parseStdin(array $stdin, array &$headers)
+    {
+        $parts = $stdin['parts'] ?? null;
+        if (empty($parts)) {
+            throw new \Exception('The stdin array needs to contain parts');
+        }
+        $boundary = $stdin['boundary'] ?? 'AaB03x';
+        if ( ! isset($headers['CONTENT_TYPE'])) {
+            $headers['CONTENT_TYPE'] = 'multipart/form-data; boundary=' . $boundary;
+        }
+        $count = $parts['count'] ?? null;
+        if ( ! is_null($count)) {
+            $dispositionType  = $parts['disposition'] ?? 'form-data';
+            $dispositionParam = $parts['param'] ?? 'name';
+            $namePrefix       = $parts['prefix'] ?? 'f';
+            $nameSuffix       = $parts['suffix'] ?? '';
+            $value            = $parts['value'] ?? 'test';
+            $parts            = [];
+            for ($i = 0; $i < $count; $i++) {
+                $parts[] = [
+                    'disposition' => $dispositionType,
+                    'param'       => $dispositionParam,
+                    'name'        => "$namePrefix$i$nameSuffix",
+                    'value'       => $value
+                ];
+            }
+        }
+        $out = '';
+        $nl  = "\r\n";
+        foreach ($parts as $part) {
+            if (!is_array($part)) {
+                $part = ['name' => $part];
+            } elseif ( ! isset($part['name'])) {
+                throw new \Exception('Each part has to have a name');
+            }
+            $name             = $part['name'];
+            $dispositionType  = $part['disposition'] ?? 'form-data';
+            $dispositionParam = $part['param'] ?? 'name';
+            $value            = $part['value'] ?? 'test';
+            $partHeaders          = $part['headers'] ?? [];
+
+            $out .= "--$boundary$nl";
+            $out .= "Content-disposition: $dispositionType; $dispositionParam=\"$name\"$nl";
+            foreach ($partHeaders as $headerName => $headerValue) {
+                $out .= "$headerName: $headerValue$nl";
+            }
+            $out .= $nl;
+            $out .= "$value$nl";
+        }
+        $out .= "--$boundary--$nl";
+
+        return $out;
+    }
+
     /**
      * Execute request.
      *
-     * @param string      $query
-     * @param array       $headers
-     * @param string|null $uri
-     * @param string|null $address
-     * @param string|null $successMessage
-     * @param string|null $errorMessagereadLimit
-     * @param bool        $connKeepAlive
-     * @param string|null $scriptFilename = null
-     * @param bool        $expectError
-     * @param int         $readLimit
+     * @param string            $query
+     * @param array             $headers
+     * @param string|null       $uri
+     * @param string|null       $address
+     * @param string|null       $successMessage
+     * @param string|null       $errorMessage
+     * @param bool              $connKeepAlive
+     * @param string|null       $scriptFilename = null
+     * @param string|array|null $stdin          = null
+     * @param bool              $expectError
+     * @param int               $readLimit
      *
      * @return Response
+     * @throws \Exception
      */
     public function request(
         string $query = '',
@@ -640,19 +707,24 @@ class Tester
         string $errorMessage = null,
         bool $connKeepAlive = false,
         string $scriptFilename = null,
+        string|array $stdin = null,
         bool $expectError = false,
         int $readLimit = -1,
     ): Response {
         if ($this->hasError()) {
             return new Response(null, true);
         }
 
-        $params = $this->getRequestParams($query, $headers, $uri, $scriptFilename);
+        if (is_array($stdin)) {
+            $stdin = $this->parseStdin($stdin, $headers);
+        }
+
+        $params = $this->getRequestParams($query, $headers, $uri, $scriptFilename, $stdin);
         $this->trace('Request params', $params);
 
         try {
             $this->response = new Response(
-                $this->getClient($address, $connKeepAlive)->request_data($params, false, $readLimit)
+                $this->getClient($address, $connKeepAlive)->request_data($params, $stdin, $readLimit)
             );
             if ($expectError) {
                 $this->error('Expected request error but the request was successful');