From 716de0cff539f46294ef70fe75d548cd66766370 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. --- 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 | 106 +++++++++++++++--- 6 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/main/main.c b/main/main.c index 40684f32dc14..c58ea58bf5ac 100644 --- a/main/main.c +++ b/main/main.c @@ -751,6 +751,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 b43cfae5a1e2..3086e8da3dbe 100644 --- a/main/rfc1867.c +++ b/main/rfc1867.c @@ -687,6 +687,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; @@ -708,6 +709,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) { @@ -792,6 +798,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 000000000000..d2239ac3c410 #--- /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 000000000000..42b5afbf9ee7 #--- /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 000000000000..da81174c7280 #--- /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 6197cdba53f5..e51aa0f69143 100644 ##--- a/sapi/fpm/tests/tester.inc ##+++ b/sapi/fpm/tests/tester.inc #@@ -567,13 +567,17 @@ class Tester # * @param string $query # * @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 $uri = null, #+ string $scriptFilename = null, #+ ?string $stdin = null # ): array { # if (is_null($uri)) { # $uri = $this->makeSourceFile(); 3@@ -582,8 +586,8 @@ class Tester # $params = array_merge( # [ # 'GATEWAY_INTERFACE' => 'FastCGI/1.0', #- 'REQUEST_METHOD' => 'GET', #- 'SCRIPT_FILENAME' => $uri, #+ 'REQUEST_METHOD' => is_null($stdin) ? 'GET' : 'POST', #+ 'SCRIPT_FILENAME' => $scriptFilename ?: $uri, # 'SCRIPT_NAME' => $uri, # 'QUERY_STRING' => $query, # 'REQUEST_URI' => $uri . ($query ? '?' . $query : ""), #@@ -597,7 +601,7 @@ class Tester # 'SERVER_PROTOCOL' => 'HTTP/1.1', # 'DOCUMENT_ROOT' => __DIR__, # 'CONTENT_TYPE' => '', #- 'CONTENT_LENGTH' => 0 #+ 'CONTENT_LENGTH' => strlen($stdin ?? "") // Default to 0 # ], # $headers # ); #@@ -607,20 +611,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 $errorMessage #- * @param bool $connKeepAlive #- * @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 = '', #@@ -630,6 +700,8 @@ class Tester # string $successMessage = null, # string $errorMessage = null, # bool $connKeepAlive = false, #+ string $scriptFilename = null, #+ string|array $stdin = null, # bool $expectError = false, # int $readLimit = -1, # ): Response { #@@ -637,12 +709,16 @@ class Tester # return new Response(null, true); # } # #- $params = $this->getRequestParams($query, $headers, $uri); #+ 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');