From 94fce68f033e786e2b85ff5e16e39f40ac5dfa0f Mon Sep 17 00:00:00 2001 From: Jakub Zelenka Date: Thu, 19 Jan 2023 14:31:25 +0000 Subject: [PATCH] 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. --- NEWS | 4 + main/main.c | 1 + main/rfc1867.c | 11 ++ ...-54hq-v5wp-fqgv-max-body-parts-custom.phpt | 53 +++++++++ ...54hq-v5wp-fqgv-max-body-parts-default.phpt | 54 ++++++++++ .../ghsa-54hq-v5wp-fqgv-max-file-uploads.phpt | 52 +++++++++ sapi/fpm/tests/tester.inc | 102 +++++++++++++++--- 7 files changed, 262 insertions(+), 15 deletions(-) create mode 100644 sapi/fpm/tests/ghsa-54hq-v5wp-fqgv-max-body-parts-custom.phpt create mode 100644 sapi/fpm/tests/ghsa-54hq-v5wp-fqgv-max-body-parts-default.phpt create mode 100644 sapi/fpm/tests/ghsa-54hq-v5wp-fqgv-max-file-uploads.phpt diff --git a/NEWS b/NEWS index a0c177a06c1..6572edc3568 100644 --- 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 index b536504be72..3d7ca291383 100644 --- 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 index af0b3b0d237..2ddad7950fd 100644 --- 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 new file mode 100644 index 00000000000..d2239ac3c41 --- /dev/null +++ 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-- + +--FILE-- +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-- + 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 new file mode 100644 index 00000000000..42b5afbf9ee --- /dev/null +++ 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-- + +--FILE-- +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-- + 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 new file mode 100644 index 00000000000..da81174c728 --- /dev/null +++ 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-- + +--FILE-- +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-- + diff --git a/sapi/fpm/tests/tester.inc b/sapi/fpm/tests/tester.inc index c1f090ab145..e051a9d73db 100644 --- a/sapi/fpm/tests/tester.inc +++ b/sapi/fpm/tests/tester.inc @@ -574,6 +574,7 @@ class Tester * @param array $headers * @param string|null $uri * @param string|null $scriptFilename + * @param string|null $stdin * * @return array */ @@ -581,7 +582,8 @@ class Tester 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,6 +707,7 @@ class Tester string $errorMessage = null, bool $connKeepAlive = false, string $scriptFilename = null, + string|array $stdin = null, bool $expectError = false, int $readLimit = -1, ): Response { @@ -647,12 +715,16 @@ class Tester 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');