From b4dda021be5f9fc6fd6da90acebc5d3526a75492 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Tue, 29 Jun 2021 17:10:04 +0200 Subject: [PATCH 1/5] Fix #73630: Built-in Weberver - overwrite $_SERVER['request_uri'] The built-in Webserver's `on_path`, `on_query_string` and `on_url` callbacks may be called multiple times from the parser; we must not simply replace the old values, but need to concatenate the new values instead. This appears to be tricky for `on_path` due to the path normalization, so we fail if the function is called again. --- sapi/cli/php_cli_server.c | 28 ++++++++++++++++++----- sapi/cli/tests/bug73630.phpt | 43 ++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 5 deletions(-) create mode 100644 sapi/cli/tests/bug73630.phpt diff --git a/sapi/cli/php_cli_server.c b/sapi/cli/php_cli_server.c index befa4c65f5de2..16f291b0d5228 100644 --- a/sapi/cli/php_cli_server.c +++ b/sapi/cli/php_cli_server.c @@ -1629,6 +1629,9 @@ static int php_cli_server_client_read_request_on_path(php_http_parser *parser, c { char *vpath; size_t vpath_len; + if (client->request.vpath != NULL) { + return 1; + } normalize_vpath(&vpath, &vpath_len, at, length, 1); client->request.vpath = vpath; client->request.vpath_len = vpath_len; @@ -1639,17 +1642,32 @@ static int php_cli_server_client_read_request_on_path(php_http_parser *parser, c static int php_cli_server_client_read_request_on_query_string(php_http_parser *parser, const char *at, size_t length) { php_cli_server_client *client = parser->data; - client->request.query_string = pestrndup(at, length, 1); - client->request.query_string_len = length; + if (client->request.query_string == NULL) { + client->request.query_string = pestrndup(at, length, 1); + client->request.query_string_len = length; + } else { + client->request.query_string = perealloc(client->request.query_string, client->request.query_string_len + length + 1, 1); + memcpy(client->request.query_string + client->request.query_string_len, at, length); + client->request.query_string_len += length; + client->request.query_string[client->request.query_string_len] = '\0'; + } return 0; } static int php_cli_server_client_read_request_on_url(php_http_parser *parser, const char *at, size_t length) { php_cli_server_client *client = parser->data; - client->request.request_method = parser->method; - client->request.request_uri = pestrndup(at, length, 1); - client->request.request_uri_len = length; + if (client->request.request_uri == NULL) { + client->request.request_method = parser->method; + client->request.request_uri = pestrndup(at, length, 1); + client->request.request_uri_len = length; + } else { + ZEND_ASSERT(client->request.request_method == parser->method); + client->request.request_uri = perealloc(client->request.request_uri, client->request.request_uri_len + length + 1, 1); + memcpy(client->request.request_uri + client->request.request_uri_len, at, length); + client->request.request_uri_len += length; + client->request.request_uri[client->request.request_uri_len] = '\0'; + } return 0; } diff --git a/sapi/cli/tests/bug73630.phpt b/sapi/cli/tests/bug73630.phpt new file mode 100644 index 0000000000000..42240bcfa9c32 --- /dev/null +++ b/sapi/cli/tests/bug73630.phpt @@ -0,0 +1,43 @@ +--TEST-- +Bug #73630 (Built-in Weberver - overwrite $_SERVER['request_uri']) +--SKIPIF-- + +--FILE-- + +--EXPECTF-- +HTTP/1.1 200 OK +Host: %s +Date: %s +Connection: close +X-Powered-By: PHP/%s +Content-type: text/html; charset=UTF-8 + +int(0) From d0397b05629125080a8a31a231f1b8c8d447617e Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Wed, 30 Jun 2021 11:41:19 +0200 Subject: [PATCH 2/5] Clarify that overflow can't happen The total length of the HTTP headers is already restricted to at most `PHP_HTTP_MAX_HEADER_SIZE` bytes, so no integer overflow can occur in our calculations. --- sapi/cli/php_cli_server.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sapi/cli/php_cli_server.c b/sapi/cli/php_cli_server.c index 16f291b0d5228..9683a549c28d3 100644 --- a/sapi/cli/php_cli_server.c +++ b/sapi/cli/php_cli_server.c @@ -1646,6 +1646,7 @@ static int php_cli_server_client_read_request_on_query_string(php_http_parser *p client->request.query_string = pestrndup(at, length, 1); client->request.query_string_len = length; } else { + ZEND_ASSERT(length <= PHP_HTTP_MAX_HEADER_SIZE && PHP_HTTP_MAX_HEADER_SIZE - length >= client->request.query_string_len); client->request.query_string = perealloc(client->request.query_string, client->request.query_string_len + length + 1, 1); memcpy(client->request.query_string + client->request.query_string_len, at, length); client->request.query_string_len += length; @@ -1663,6 +1664,7 @@ static int php_cli_server_client_read_request_on_url(php_http_parser *parser, co client->request.request_uri_len = length; } else { ZEND_ASSERT(client->request.request_method == parser->method); + ZEND_ASSERT(length <= PHP_HTTP_MAX_HEADER_SIZE && PHP_HTTP_MAX_HEADER_SIZE - length >= client->request.query_string_len); client->request.request_uri = perealloc(client->request.request_uri, client->request.request_uri_len + length + 1, 1); memcpy(client->request.request_uri + client->request.request_uri_len, at, length); client->request.request_uri_len += length; From 79acd2112401bb7b55907905b9727bfe97878d92 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Wed, 30 Jun 2021 11:56:50 +0200 Subject: [PATCH 3/5] Verify strlen of QUERY_STRING, too --- sapi/cli/tests/bug73630.phpt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sapi/cli/tests/bug73630.phpt b/sapi/cli/tests/bug73630.phpt index 42240bcfa9c32..431b323feb218 100644 --- a/sapi/cli/tests/bug73630.phpt +++ b/sapi/cli/tests/bug73630.phpt @@ -9,6 +9,7 @@ include "skipif.inc"; $code = <<<'EOF' var_dump(strncmp($_SERVER['REQUEST_URI'], "/overflow.php", strlen("/overflow.php"))); +var_dump(strlen($_SERVER['QUERY_STRING'])); EOF; include "php_cli_server.inc"; @@ -41,3 +42,4 @@ X-Powered-By: PHP/%s Content-type: text/html; charset=UTF-8 int(0) +int(16413) From e030f34040134a417141ec12b8dc407da5c545c4 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Wed, 30 Jun 2021 13:38:40 +0200 Subject: [PATCH 4/5] Add test regarding too long paths The built-in Webserver logs errors during request parsing to stderr, but this is ignored by the php_cli_server framework, and apparently the Webserver does not send a resonse at all in such cases (instead of an 4xx). Thus we can only check that a request with an overly long path fails. --- sapi/cli/tests/bug73630a.phpt | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 sapi/cli/tests/bug73630a.phpt diff --git a/sapi/cli/tests/bug73630a.phpt b/sapi/cli/tests/bug73630a.phpt new file mode 100644 index 0000000000000..2ad9829510f8b --- /dev/null +++ b/sapi/cli/tests/bug73630a.phpt @@ -0,0 +1,25 @@ +--TEST-- +Bug #73630 (Built-in Weberver - overwrite $_SERVER['request_uri']) +--DESCRIPTION-- +Check that too long paths result in invalid request +--SKIPIF-- + +--FILE-- + +--EXPECTF-- +Warning: file_get_contents(http://%s//example.com): failed to open stream: HTTP request failed! in %s on line %d +bool(false) From 1a804d0cb6df90bfae41474ecf8bfc638eecc40a Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Wed, 30 Jun 2021 13:46:43 +0200 Subject: [PATCH 5/5] EXPECT the callbacks to be called at most once --- sapi/cli/php_cli_server.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sapi/cli/php_cli_server.c b/sapi/cli/php_cli_server.c index 9683a549c28d3..c3097861e3f28 100644 --- a/sapi/cli/php_cli_server.c +++ b/sapi/cli/php_cli_server.c @@ -1629,7 +1629,7 @@ static int php_cli_server_client_read_request_on_path(php_http_parser *parser, c { char *vpath; size_t vpath_len; - if (client->request.vpath != NULL) { + if (UNEXPECTED(client->request.vpath != NULL)) { return 1; } normalize_vpath(&vpath, &vpath_len, at, length, 1); @@ -1642,7 +1642,7 @@ static int php_cli_server_client_read_request_on_path(php_http_parser *parser, c static int php_cli_server_client_read_request_on_query_string(php_http_parser *parser, const char *at, size_t length) { php_cli_server_client *client = parser->data; - if (client->request.query_string == NULL) { + if (EXPECTED(client->request.query_string == NULL)) { client->request.query_string = pestrndup(at, length, 1); client->request.query_string_len = length; } else { @@ -1658,7 +1658,7 @@ static int php_cli_server_client_read_request_on_query_string(php_http_parser *p static int php_cli_server_client_read_request_on_url(php_http_parser *parser, const char *at, size_t length) { php_cli_server_client *client = parser->data; - if (client->request.request_uri == NULL) { + if (EXPECTED(client->request.request_uri == NULL)) { client->request.request_method = parser->method; client->request.request_uri = pestrndup(at, length, 1); client->request.request_uri_len = length;