Skip to content

Commit 716de0c

Browse files
bukkabukka
committed
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.
1 parent e45850c commit 716de0c

6 files changed

+262
-15
lines changed

‎main/main.c

+1
Original file line numberDiff line numberDiff line change
@@ -751,6 +751,7 @@ PHP_INI_BEGIN()
751751
PHP_INI_ENTRY("disable_functions", "", PHP_INI_SYSTEM, NULL)
752752
PHP_INI_ENTRY("disable_classes", "", PHP_INI_SYSTEM, NULL)
753753
PHP_INI_ENTRY("max_file_uploads", "20", PHP_INI_SYSTEM|PHP_INI_PERDIR, NULL)
754+
PHP_INI_ENTRY("max_multipart_body_parts", "-1", PHP_INI_SYSTEM|PHP_INI_PERDIR, NULL)
754755

755756
STD_PHP_INI_BOOLEAN("allow_url_fopen", "1", PHP_INI_SYSTEM, OnUpdateBool, allow_url_fopen, php_core_globals, core_globals)
756757
STD_PHP_INI_BOOLEAN("allow_url_include", "0", PHP_INI_SYSTEM, OnUpdateBool, allow_url_include, php_core_globals, core_globals)

‎main/rfc1867.c

+11
Original file line numberDiff line numberDiff line change
@@ -687,6 +687,7 @@ SAPI_API SAPI_POST_HANDLER_FUNC(rfc1867_post_handler) /* {{{ */
687687
void *event_extra_data = NULL;
688688
unsigned int llen = 0;
689689
int upload_cnt = INI_INT("max_file_uploads");
690+
int body_parts_cnt = INI_INT("max_multipart_body_parts");
690691
const zend_encoding *internal_encoding = zend_multibyte_get_internal_encoding();
691692
php_rfc1867_getword_t getword;
692693
php_rfc1867_getword_conf_t getword_conf;
@@ -708,6 +709,11 @@ SAPI_API SAPI_POST_HANDLER_FUNC(rfc1867_post_handler) /* {{{ */
708709
return;
709710
}
710711

712+
if (body_parts_cnt < 0) {
713+
body_parts_cnt = PG(max_input_vars) + upload_cnt;
714+
}
715+
int body_parts_limit = body_parts_cnt;
716+
711717
/* Get the boundary */
712718
boundary = strstr(content_type_dup, "boundary");
713719
if (!boundary) {
@@ -792,6 +798,11 @@ SAPI_API SAPI_POST_HANDLER_FUNC(rfc1867_post_handler) /* {{{ */
792798
char *pair = NULL;
793799
int end = 0;
794800

801+
if (--body_parts_cnt < 0) {
802+
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);
803+
goto fileupload_done;
804+
}
805+
795806
while (isspace(*cd)) {
796807
++cd;
797808
}

‎sapi/fpm/tests/ghsa-54hq-v5wp-fqgv-max-body-parts-custom.phpt

+53
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
--TEST--
2+
FPM: GHSA-54hq-v5wp-fqgv - max_multipart_body_parts ini custom value
3+
--SKIPIF--
4+
<?php include "skipif.inc"; ?>
5+
--FILE--
6+
<?php
7+
8+
require_once "tester.inc";
9+
10+
$cfg = <<<EOT
11+
[global]
12+
error_log = {{FILE:LOG}}
13+
[unconfined]
14+
listen = {{ADDR}}
15+
pm = dynamic
16+
pm.max_children = 5
17+
pm.start_servers = 1
18+
pm.min_spare_servers = 1
19+
pm.max_spare_servers = 3
20+
php_admin_value[html_errors] = false
21+
php_admin_value[max_input_vars] = 20
22+
php_admin_value[max_file_uploads] = 5
23+
php_admin_value[max_multipart_body_parts] = 10
24+
php_flag[display_errors] = On
25+
EOT;
26+
27+
$code = <<<EOT
28+
<?php
29+
var_dump(count(\$_POST));
30+
EOT;
31+
32+
$tester = new FPM\Tester($cfg, $code);
33+
$tester->start();
34+
$tester->expectLogStartNotices();
35+
echo $tester
36+
->request(stdin: [
37+
'parts' => [
38+
'count' => 30,
39+
]
40+
])
41+
->getBody();
42+
$tester->terminate();
43+
$tester->close();
44+
45+
?>
46+
--EXPECT--
47+
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
48+
int(10)
49+
--CLEAN--
50+
<?php
51+
require_once "tester.inc";
52+
FPM\Tester::clean();
53+
?>

‎sapi/fpm/tests/ghsa-54hq-v5wp-fqgv-max-body-parts-default.phpt

+54
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
--TEST--
2+
FPM: GHSA-54hq-v5wp-fqgv - max_multipart_body_parts ini default
3+
--SKIPIF--
4+
<?php include "skipif.inc"; ?>
5+
--FILE--
6+
<?php
7+
8+
require_once "tester.inc";
9+
10+
$cfg = <<<EOT
11+
[global]
12+
error_log = {{FILE:LOG}}
13+
[unconfined]
14+
listen = {{ADDR}}
15+
pm = dynamic
16+
pm.max_children = 5
17+
pm.start_servers = 1
18+
pm.min_spare_servers = 1
19+
pm.max_spare_servers = 3
20+
php_admin_value[html_errors] = false
21+
php_admin_value[max_input_vars] = 20
22+
php_admin_value[max_file_uploads] = 5
23+
php_flag[display_errors] = On
24+
EOT;
25+
26+
$code = <<<EOT
27+
<?php
28+
var_dump(count(\$_POST));
29+
EOT;
30+
31+
$tester = new FPM\Tester($cfg, $code);
32+
$tester->start();
33+
$tester->expectLogStartNotices();
34+
echo $tester
35+
->request(stdin: [
36+
'parts' => [
37+
'count' => 30,
38+
]
39+
])
40+
->getBody();
41+
$tester->terminate();
42+
$tester->close();
43+
44+
?>
45+
--EXPECT--
46+
Warning: Unknown: Input variables exceeded 20. To increase the limit change max_input_vars in php.ini. in Unknown on line 0
47+
48+
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
49+
int(20)
50+
--CLEAN--
51+
<?php
52+
require_once "tester.inc";
53+
FPM\Tester::clean();
54+
?>

‎sapi/fpm/tests/ghsa-54hq-v5wp-fqgv-max-file-uploads.phpt

+52
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
--TEST--
2+
FPM: GHSA-54hq-v5wp-fqgv - exceeding max_file_uploads
3+
--SKIPIF--
4+
<?php include "skipif.inc"; ?>
5+
--FILE--
6+
<?php
7+
8+
require_once "tester.inc";
9+
10+
$cfg = <<<EOT
11+
[global]
12+
error_log = {{FILE:LOG}}
13+
[unconfined]
14+
listen = {{ADDR}}
15+
pm = dynamic
16+
pm.max_children = 5
17+
pm.start_servers = 1
18+
pm.min_spare_servers = 1
19+
pm.max_spare_servers = 3
20+
php_admin_value[html_errors] = false
21+
php_admin_value[max_file_uploads] = 5
22+
php_flag[display_errors] = On
23+
EOT;
24+
25+
$code = <<<EOT
26+
<?php
27+
var_dump(count(\$_FILES));
28+
EOT;
29+
30+
$tester = new FPM\Tester($cfg, $code);
31+
$tester->start();
32+
$tester->expectLogStartNotices();
33+
echo $tester
34+
->request(stdin: [
35+
'parts' => [
36+
'count' => 10,
37+
'param' => 'filename'
38+
]
39+
])
40+
->getBody();
41+
$tester->terminate();
42+
$tester->close();
43+
44+
?>
45+
--EXPECT--
46+
Warning: Maximum number of allowable file uploads has been exceeded in Unknown on line 0
47+
int(5)
48+
--CLEAN--
49+
<?php
50+
require_once "tester.inc";
51+
FPM\Tester::clean();
52+
?>

‎sapi/fpm/tests/tester.inc

+91-15
Original file line numberDiff line numberDiff line change
@@ -567,13 +567,17 @@ class Tester
567567
* @param string $query
568568
* @param array $headers
569569
* @param string|null $uri
570+
* @param string|null $scriptFilename
571+
* @param string|null $stdin
570572
*
571573
* @return array
572574
*/
573575
private function getRequestParams(
574576
string $query = '',
575577
array $headers = [],
576-
string $uri = null
578+
string $uri = null,
579+
string $scriptFilename = null,
580+
?string $stdin = null
577581
): array {
578582
if (is_null($uri)) {
579583
$uri = $this->makeSourceFile();
@@ -582,8 +586,8 @@ class Tester
582586
$params = array_merge(
583587
[
584588
'GATEWAY_INTERFACE' => 'FastCGI/1.0',
585-
'REQUEST_METHOD' => 'GET',
586-
'SCRIPT_FILENAME' => $uri,
589+
'REQUEST_METHOD' => is_null($stdin) ? 'GET' : 'POST',
590+
'SCRIPT_FILENAME' => $scriptFilename ?: $uri,
587591
'SCRIPT_NAME' => $uri,
588592
'QUERY_STRING' => $query,
589593
'REQUEST_URI' => $uri . ($query ? '?' . $query : ""),
@@ -597,7 +601,7 @@ class Tester
597601
'SERVER_PROTOCOL' => 'HTTP/1.1',
598602
'DOCUMENT_ROOT' => __DIR__,
599603
'CONTENT_TYPE' => '',
600-
'CONTENT_LENGTH' => 0
604+
'CONTENT_LENGTH' => strlen($stdin ?? "") // Default to 0
601605
],
602606
$headers
603607
);
@@ -607,20 +611,86 @@ class Tester
607611
});
608612
}
609613

614+
/**
615+
* Parse stdin and generate data for multipart config.
616+
*
617+
* @param array $stdin
618+
* @param array $headers
619+
*
620+
* @return void
621+
* @throws \Exception
622+
*/
623+
private function parseStdin(array $stdin, array &$headers)
624+
{
625+
$parts = $stdin['parts'] ?? null;
626+
if (empty($parts)) {
627+
throw new \Exception('The stdin array needs to contain parts');
628+
}
629+
$boundary = $stdin['boundary'] ?? 'AaB03x';
630+
if ( ! isset($headers['CONTENT_TYPE'])) {
631+
$headers['CONTENT_TYPE'] = 'multipart/form-data; boundary=' . $boundary;
632+
}
633+
$count = $parts['count'] ?? null;
634+
if ( ! is_null($count)) {
635+
$dispositionType = $parts['disposition'] ?? 'form-data';
636+
$dispositionParam = $parts['param'] ?? 'name';
637+
$namePrefix = $parts['prefix'] ?? 'f';
638+
$nameSuffix = $parts['suffix'] ?? '';
639+
$value = $parts['value'] ?? 'test';
640+
$parts = [];
641+
for ($i = 0; $i < $count; $i++) {
642+
$parts[] = [
643+
'disposition' => $dispositionType,
644+
'param' => $dispositionParam,
645+
'name' => "$namePrefix$i$nameSuffix",
646+
'value' => $value
647+
];
648+
}
649+
}
650+
$out = '';
651+
$nl = "\r\n";
652+
foreach ($parts as $part) {
653+
if (!is_array($part)) {
654+
$part = ['name' => $part];
655+
} elseif ( ! isset($part['name'])) {
656+
throw new \Exception('Each part has to have a name');
657+
}
658+
$name = $part['name'];
659+
$dispositionType = $part['disposition'] ?? 'form-data';
660+
$dispositionParam = $part['param'] ?? 'name';
661+
$value = $part['value'] ?? 'test';
662+
$partHeaders = $part['headers'] ?? [];
663+
664+
$out .= "--$boundary$nl";
665+
$out .= "Content-disposition: $dispositionType; $dispositionParam=\"$name\"$nl";
666+
foreach ($partHeaders as $headerName => $headerValue) {
667+
$out .= "$headerName: $headerValue$nl";
668+
}
669+
$out .= $nl;
670+
$out .= "$value$nl";
671+
}
672+
$out .= "--$boundary--$nl";
673+
674+
return $out;
675+
}
676+
610677
/**
611678
* Execute request.
612679
*
613-
* @param string $query
614-
* @param array $headers
615-
* @param string|null $uri
616-
* @param string|null $address
617-
* @param string|null $successMessage
618-
* @param string|null $errorMessage
619-
* @param bool $connKeepAlive
620-
* @param bool $expectError
621-
* @param int $readLimit
680+
* @param string $query
681+
* @param array $headers
682+
* @param string|null $uri
683+
* @param string|null $address
684+
* @param string|null $successMessage
685+
* @param string|null $errorMessage
686+
* @param bool $connKeepAlive
687+
* @param string|null $scriptFilename = null
688+
* @param string|array|null $stdin = null
689+
* @param bool $expectError
690+
* @param int $readLimit
622691
*
623692
* @return Response
693+
* @throws \Exception
624694
*/
625695
public function request(
626696
string $query = '',
@@ -630,19 +700,25 @@ class Tester
630700
string $successMessage = null,
631701
string $errorMessage = null,
632702
bool $connKeepAlive = false,
703+
string $scriptFilename = null,
704+
string|array $stdin = null,
633705
bool $expectError = false,
634706
int $readLimit = -1,
635707
): Response {
636708
if ($this->hasError()) {
637709
return new Response(null, true);
638710
}
639711

640-
$params = $this->getRequestParams($query, $headers, $uri);
712+
if (is_array($stdin)) {
713+
$stdin = $this->parseStdin($stdin, $headers);
714+
}
715+
716+
$params = $this->getRequestParams($query, $headers, $uri, $scriptFilename, $stdin);
641717
$this->trace('Request params', $params);
642718

643719
try {
644720
$this->response = new Response(
645-
$this->getClient($address, $connKeepAlive)->request_data($params, false, $readLimit)
721+
$this->getClient($address, $connKeepAlive)->request_data($params, $stdin, $readLimit)
646722
);
647723
if ($expectError) {
648724
$this->error('Expected request error but the request was successful');

0 commit comments

Comments
 (0)