From b0d4b663924c649fa33831bf92f67c3d066c83e9 Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Thu, 4 May 2023 14:54:29 -0500 Subject: [PATCH 01/10] feature: filter out integer keys when marshaling headers from SAPI Integer strings have the nasty habit of being cast to actual integers by PHP, making them problematic for usage in an associative array, despite being valid per the RFC 7230 ABNF. Additionally, having them pass through `marshal_headers_from_sapi()` means that once `ServerRequest` gets them and tries to use them, `HeaderSecurity::assertValidName()` will raise an exception for integers, which could lead to unexpected server errors. This patch chooses to filter such header names out entirely. Doing so prevents those server errors. Signed-off-by: Matthew Weier O'Phinney --- psalm-baseline.xml | 92 ++++++++++--------- src/functions/marshal_headers_from_sapi.php | 10 +- test/functions/MarshalHeadersFromSapiTest.php | 3 + 3 files changed, 59 insertions(+), 46 deletions(-) diff --git a/psalm-baseline.xml b/psalm-baseline.xml index e78b48d5..aafe7966 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -1,5 +1,5 @@ - + null|callable @@ -80,7 +80,7 @@ $protocolVersion $requestTarget $uri - + self::getValueFromKey($serializedRequest, 'body') $headers @@ -114,7 +114,7 @@ $protocolVersion $reasonPhrase $statusCode - + self::getValueFromKey($serializedResponse, 'body') $headers @@ -186,7 +186,7 @@ - + $headers['cookie'] $headers @@ -250,42 +250,48 @@ - - - - + $spec['error'] + $spec['name'] ?? null + $spec['tmp_name'] + $spec['type'] ?? null + + is_string($key), ARRAY_FILTER_USE_KEY)]]> + $headers[$name] $headers[$name] $value + + ]]> + string - - + $server['REQUEST_METHOD'] ?? 'GET' + $server['REQUEST_METHOD'] ?? 'GET' - + $server['SERVER_PROTOCOL'] - - + $apacheRequestHeaders['Authorization'] + $apacheRequestHeaders['authorization'] $apacheRequestHeaders - - + $server['HTTP_AUTHORIZATION'] + $server['HTTP_AUTHORIZATION'] @@ -308,25 +314,25 @@ $nameTree[$key] ?? null, $typeTree[$key] ?? null ) - $recursiveNormalize( $files['tmp_name'], $files['size'], $files['error'], $files['name'] ?? null, $files['type'] ?? null - )]]> + ) array - $recursiveNormalize( $files['tmp_name'], $files['size'], $files['error'], $files['name'] ?? null, $files['type'] ?? null - )]]> + ) @@ -386,7 +392,7 @@ - + $normalizedFiles['fooFiles'] @@ -434,23 +440,23 @@ - - + $normalised['my-form']['details']['avatars'] + $normalised['slide-shows'][0]['slides'] - - - - - - - - - - - - - + $normalised['my-form']['details']['avatar'] + $normalised['my-form']['details']['avatars'] + $normalised['my-form']['details']['avatars'] + $normalised['my-form']['details']['avatars'] + $normalised['my-form']['details']['avatars'] + $normalised['my-form']['details']['avatars'][0] + $normalised['my-form']['details']['avatars'][1] + $normalised['my-form']['details']['avatars'][2] + $normalised['slide-shows'][0]['slides'] + $normalised['slide-shows'][0]['slides'] + $normalised['slide-shows'][0]['slides'] + $normalised['slide-shows'][0]['slides'][0] + $normalised['slide-shows'][0]['slides'][1] getClientFilename @@ -461,14 +467,14 @@ getClientFilename - - - - - - - - + $normalised['my-form'] + $normalised['my-form'] + $normalised['my-form'] + $normalised['my-form'] + $normalised['my-form'] + $normalised['slide-shows'] + $normalised['slide-shows'] + $normalised['slide-shows'] diff --git a/src/functions/marshal_headers_from_sapi.php b/src/functions/marshal_headers_from_sapi.php index 7fd09996..cdeaa369 100644 --- a/src/functions/marshal_headers_from_sapi.php +++ b/src/functions/marshal_headers_from_sapi.php @@ -4,6 +4,7 @@ namespace Laminas\Diactoros; +use function array_filter; use function array_key_exists; use function is_string; use function str_starts_with; @@ -11,9 +12,11 @@ use function strtr; use function substr; +use const ARRAY_FILTER_USE_KEY; + /** * @param array $server Values obtained from the SAPI (generally `$_SERVER`). - * @return array Header/value pairs + * @return array Header/value pairs */ function marshalHeadersFromSapi(array $server): array { @@ -30,7 +33,7 @@ function marshalHeadersFromSapi(array $server): array $headers = []; foreach ($server as $key => $value) { - if (! is_string($key)) { + if (! is_string($key) || $key === '') { continue; } @@ -63,5 +66,6 @@ function marshalHeadersFromSapi(array $server): array } } - return $headers; + // Filter out integer keys + return array_filter($headers, fn(string|int $key): bool => is_string($key), ARRAY_FILTER_USE_KEY); } diff --git a/test/functions/MarshalHeadersFromSapiTest.php b/test/functions/MarshalHeadersFromSapiTest.php index b5fa2ddb..d7b42405 100644 --- a/test/functions/MarshalHeadersFromSapiTest.php +++ b/test/functions/MarshalHeadersFromSapiTest.php @@ -34,6 +34,9 @@ public function testReturnsHeaders(): void 'CONTENT_TEST_XY' => '', 'CONTENT_TEST_ZZ' => null, 123 => 'integer', + '1' => 'string-integer', + '0' => 'string-zero', + '-1' => 'string-negative-integer', ]; $expectedHeaders = [ From c958ceca1f4ab1f6df179cf788b5fdfe252b56ef Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Thu, 4 May 2023 15:14:13 -0500 Subject: [PATCH 02/10] docs: document change to filter integer header names Creates a "security features" document that brings in the former v2 "forward migration" document around filtering x-forwarded-* headers, and also adds narrative around filtering integer headers. Signed-off-by: Matthew Weier O'Phinney --- docs/book/v3/forward-migration.md | 20 ---------------- docs/book/v3/security.md | 39 +++++++++++++++++++++++++++++++ mkdocs.yml | 1 + 3 files changed, 40 insertions(+), 20 deletions(-) delete mode 100644 docs/book/v3/forward-migration.md create mode 100644 docs/book/v3/security.md diff --git a/docs/book/v3/forward-migration.md b/docs/book/v3/forward-migration.md deleted file mode 100644 index 375ad512..00000000 --- a/docs/book/v3/forward-migration.md +++ /dev/null @@ -1,20 +0,0 @@ -# Preparing for Version 3 - -## ServerRequestFilterInterface defaults - -Introduced in version 2.11.1, the `Laminas\Diactoros\ServerRequestFilter\FilterServerRequestInterface` is used by `ServerRequestFactory::fromGlobals()` to allow modifying the generated `ServerRequest` instance prior to returning it. -The primary use case is to allow modifying the generated URI based on the presence of headers such as `X-Forwarded-Host`. -When operating behind a reverse proxy, the `Host` header is often rewritten to the name of the node to which the request is being forwarded, and an `X-Forwarded-Host` header is generated with the original `Host` value to allow the server to determine the original host the request was intended for. -(We have always examined the `X-Forwarded-Proto` header; as of 2.11.1, we also examine the `X-Forwarded-Port` header.) - -To accommodate this use case, we created `Laminas\Diactoros\ServerRequestFilter\FilterUsingXForwardedHeaders`. - -Due to potential security issues, it is generally best to only accept these headers if you trust the reverse proxy that has initiated the request. -(This value is found in `$_SERVER['REMOTE_ADDR']`, which is present as `$request->getServerParams()['REMOTE_ADDR']` within PSR-7 implementations.) -`FilterUsingXForwardedHeaders` provides named constructors to allow you to trust these headers from any source (which has been the default behavior of Diactoros since the beginning), or to specify specific IP addresses or CIDR subnets to trust, along with which headers are trusted. -To prevent backwards compatibility breaks, we use this filter by default, marked to trust **only proxies on private subnets**. - -Features will be added to the 3.11.0 version of [mezzio/mezzio](https://github.com/mezzio/mezzio) that will allow configuring the `Laminas\Diactoros\ServerRequestFilter\FilterServerRequestInterface` instance, and we recommend explicitly configuring this to utilize the `FilterUsingXForwardedHeaders` if you depend on this functionality. -If you **do not** need the functionality, we recommend specifying `Laminas\Diactoros\ServerRequestFilter\DoNotFilter` as the configured `FilterServerRequestInterface` in your application immediately. - -We will update this documentation with a link to the related functionality in mezzio/mezzio when it is published. diff --git a/docs/book/v3/security.md b/docs/book/v3/security.md new file mode 100644 index 00000000..a4698f22 --- /dev/null +++ b/docs/book/v3/security.md @@ -0,0 +1,39 @@ +# Security Features + +## ServerRequestFilterInterface defaults + +`Laminas\Diactoros\ServerRequestFilter\FilterServerRequestInterface` is used by `ServerRequestFactory::fromGlobals()` to allow modifying the generated `ServerRequest` instance prior to returning it. +The primary use case is to allow modifying the generated URI based on the presence of headers such as `X-Forwarded-Host`. +When operating behind a reverse proxy, the `Host` header is often rewritten to the name of the node to which the request is being forwarded, and an `X-Forwarded-Host` header is generated with the original `Host` value to allow the server to determine the original host the request was intended for. +We also similarly examine the `X-Forwarded-Port` header. + +To accommodate this use case, we provide `Laminas\Diactoros\ServerRequestFilter\FilterUsingXForwardedHeaders`. + +Due to potential security issues, it is generally best to only accept these headers if you trust the reverse proxy that has initiated the request. +(This value is found in `$_SERVER['REMOTE_ADDR']`, which is present as `$request->getServerParams()['REMOTE_ADDR']` within PSR-7 implementations.) +`FilterUsingXForwardedHeaders` provides named constructors to allow you to trust these headers from any source (which has been the default behavior of Diactoros since the beginning), or to specify specific IP addresses or CIDR subnets to trust, along with which headers are trusted. +We use this filter by default, marked to trust **only proxies on private subnets**. + +If you **do not** need the functionality, we recommend specifying `Laminas\Diactoros\ServerRequestFilter\DoNotFilter` as the configured `FilterServerRequestInterface` in your application. + +## Filtering of integer header names + +[PSR-7](https://www.php-fig.org/psr/psr-7/) targets [RFC 7230](https://www.rfc-editor.org/rfc/rfc7230). +RFC-7230 defines an ABNF pattern for header field names that allows the possibility of using an integer as a header field; e.g., + +```http +1234: header value +``` + +The PSR-7, `Psr\Http\MessageInterface::getHeaders()` method requires implementations to return an associative array, where the key is the header field name. +This triggers an interesting quirk in PHP: when adding a value to an array using a string that consists of an integer value, PHP will convert this value to an integer. +This presents several issues: + +- First, it means that consumers cannot depend on the header field name returned being a string. +- Second, our own validation of header field name will fail, as it will not see a string. + +Normally, this will not present an issue, as the way to add headers to a message is via the `MessageInterface::withHeader()` and `MessageInterface::withAddedHeader()` methods, which both require a `string` name argument. +However, when using `Laminas\Diactoros\ServerRequestFactory::fromGlobals()`, it can present a problem if any discovered headers have field names that evaluate to integers. + +To prevent issues, as of version 3.0.0, the `ServerRequestFactory` implementation in Diactoros filters out any headers that evaluate to integers. +If you wish to accept these anyways, we strongly recommend that you modify your web server to rewrite the incoming header field name to add a prefix or suffix string (e.g., `X-Digit-1`, `1-Digit`). diff --git a/mkdocs.yml b/mkdocs.yml index 5143f1bd..41ba7814 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -17,6 +17,7 @@ nav: - Factories: v3/factories.md - "Server Request Filters": v3/server-request-filters.md - "Custom Responses": v3/custom-responses.md + - "Security Features": v3/security.md - Serialization: v3/serialization.md - API: v3/api.md - Migration: From 8ae3a5e2d774c773f9dca5c634007c3ff86b7987 Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Thu, 4 May 2023 15:17:41 -0500 Subject: [PATCH 03/10] docs: adds section to migration doc to cover marshalHeadersFromSapi change Signed-off-by: Matthew Weier O'Phinney --- docs/book/v3/migration.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/book/v3/migration.md b/docs/book/v3/migration.md index 6d0d5731..909c74dd 100644 --- a/docs/book/v3/migration.md +++ b/docs/book/v3/migration.md @@ -24,6 +24,12 @@ For consumers, usage should be completely backwards compatible, however. The factory `Laminas\Diactoros\ServerRequestFactory::fromGlobals()` was modified such that passing empty array values for arguments that accept `null` or an array now will not use the associated superglobal in that scenario. Previously, an empty array value was treated as identical to `null`, and would cause the factory to fallback to superglobals; now, this is a way to provide an empty set for the associated value(s). +### marshalHeadersFromSapi + +The function `Laminas\Diactoros\marshalHeadersFromSapi()`, which is consumed by `Laminas\Diactoros\ServerRequestFactory::fromGlobals()`, was modified such that it will no filter out header field names that evaluate to integers. +Please see the [Security Features](security-features.md) document for more details. + + ## Removed The following features were removed for version 3. From 608f0250cd924c86158f6f75d4c7f3b06b08e476 Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Thu, 4 May 2023 16:01:41 -0500 Subject: [PATCH 04/10] Update docs/book/v3/migration.md Co-authored-by: Aleksei Khudiakov Signed-off-by: Matthew Weier O'Phinney --- docs/book/v3/migration.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/book/v3/migration.md b/docs/book/v3/migration.md index 909c74dd..51dcbec7 100644 --- a/docs/book/v3/migration.md +++ b/docs/book/v3/migration.md @@ -26,7 +26,7 @@ Previously, an empty array value was treated as identical to `null`, and would c ### marshalHeadersFromSapi -The function `Laminas\Diactoros\marshalHeadersFromSapi()`, which is consumed by `Laminas\Diactoros\ServerRequestFactory::fromGlobals()`, was modified such that it will no filter out header field names that evaluate to integers. +The function `Laminas\Diactoros\marshalHeadersFromSapi()`, which is consumed by `Laminas\Diactoros\ServerRequestFactory::fromGlobals()`, was modified such that it will now filter out header field names that evaluate to integers. Please see the [Security Features](security-features.md) document for more details. From 4186a4b8affbca03a083b58485321a0aaddf8e35 Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Thu, 4 May 2023 15:57:37 -0500 Subject: [PATCH 05/10] docs: document why we filter integer keys from the $headers array Signed-off-by: Matthew Weier O'Phinney --- src/functions/marshal_headers_from_sapi.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/functions/marshal_headers_from_sapi.php b/src/functions/marshal_headers_from_sapi.php index cdeaa369..21258cab 100644 --- a/src/functions/marshal_headers_from_sapi.php +++ b/src/functions/marshal_headers_from_sapi.php @@ -66,6 +66,9 @@ function marshalHeadersFromSapi(array $server): array } } - // Filter out integer keys + // Filter out integer keys. + // These can occor if the translated header name is a string integer. + // PHP will cast those to integers when assigned to an array. + // This filters them out. return array_filter($headers, fn(string|int $key): bool => is_string($key), ARRAY_FILTER_USE_KEY); } From 458ceb387e3672b091a7d75ce3018dddd3d892bc Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Thu, 4 May 2023 15:59:17 -0500 Subject: [PATCH 06/10] qa: add integer keys to header lists while testing Adds integer and string-integer keys from the `marshalHeadersFromSapi()` test case to the ServerRequestFactory test cases to demonstrate they get filtered out. Signed-off-by: Matthew Weier O'Phinney --- test/ServerRequestFactoryTest.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/ServerRequestFactoryTest.php b/test/ServerRequestFactoryTest.php index b10bddf5..73e90303 100644 --- a/test/ServerRequestFactoryTest.php +++ b/test/ServerRequestFactoryTest.php @@ -43,6 +43,10 @@ public function testMarshalsExpectedHeadersFromServerArray(): void 'HTTP_X_FOO_BAR' => 'FOOBAR', 'CONTENT_MD5' => 'CONTENT-MD5', 'CONTENT_LENGTH' => 'UNSPECIFIED', + 123 => 'integer', + '1' => 'string-integer', + '0' => 'string-zero', + '-1' => 'string-negative-integer', ]; $expected = [ @@ -65,6 +69,10 @@ public function testMarshalInvalidHeadersStrippedFromServerArray(): void 'HTTP_AUTHORIZATION' => 'token', 'MD5' => 'CONTENT-MD5', 'CONTENT_LENGTH' => 'UNSPECIFIED', + 123 => 'integer', + '1' => 'string-integer', + '0' => 'string-zero', + '-1' => 'string-negative-integer', ]; //Headers that don't begin with HTTP_ or CONTENT_ will not be returned From 97ba84a175a2d42651c5e8d23b83ccbb1b0197d6 Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Thu, 4 May 2023 16:03:24 -0500 Subject: [PATCH 07/10] docs: link to PHP bug 80309 Links to PHP bug 80309 in discussion of the string integer value to integer conversion. Signed-off-by: Matthew Weier O'Phinney --- docs/book/v3/security.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/book/v3/security.md b/docs/book/v3/security.md index a4698f22..5218d53b 100644 --- a/docs/book/v3/security.md +++ b/docs/book/v3/security.md @@ -26,7 +26,7 @@ RFC-7230 defines an ABNF pattern for header field names that allows the possibil ``` The PSR-7, `Psr\Http\MessageInterface::getHeaders()` method requires implementations to return an associative array, where the key is the header field name. -This triggers an interesting quirk in PHP: when adding a value to an array using a string that consists of an integer value, PHP will convert this value to an integer. +This triggers an interesting quirk in PHP: when adding a value to an array using a string that consists of an integer value, PHP will convert this value to an integer (see [PHP bug 80309](https://bugs.php.net/bug.php?id=80309) for more details). This presents several issues: - First, it means that consumers cannot depend on the header field name returned being a string. From 1fce7e8b9900087dc848d245b1243bf0e6413c36 Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Thu, 4 May 2023 16:07:29 -0500 Subject: [PATCH 08/10] docs: add a note about integer keys added via other means When a header field is added with `withHeader()` or `withHeaderLine()`, these will accept strings that have digits only, which can also lead to the issues presented elsewhere. Signed-off-by: Matthew Weier O'Phinney --- docs/book/v3/security.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/book/v3/security.md b/docs/book/v3/security.md index 5218d53b..7588baf3 100644 --- a/docs/book/v3/security.md +++ b/docs/book/v3/security.md @@ -37,3 +37,7 @@ However, when using `Laminas\Diactoros\ServerRequestFactory::fromGlobals()`, it To prevent issues, as of version 3.0.0, the `ServerRequestFactory` implementation in Diactoros filters out any headers that evaluate to integers. If you wish to accept these anyways, we strongly recommend that you modify your web server to rewrite the incoming header field name to add a prefix or suffix string (e.g., `X-Digit-1`, `1-Digit`). + +> NOTE: **Integer keys can still be returned from getHeaders()** +> While `withHeader()` and `withHeaderLine()` require string name values, please be aware that these can be presented as string integers. +> These names will be considered valid, and that means that when you call `getHeaders()`, any such names will become integers at this time. From 21c70c7847e571fa845ccb2829409b73df4ee9fd Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Thu, 4 May 2023 16:08:53 -0500 Subject: [PATCH 09/10] docs: remove extra linebreak Signed-off-by: Matthew Weier O'Phinney --- docs/book/v3/migration.md | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/book/v3/migration.md b/docs/book/v3/migration.md index 51dcbec7..44a234fe 100644 --- a/docs/book/v3/migration.md +++ b/docs/book/v3/migration.md @@ -29,7 +29,6 @@ Previously, an empty array value was treated as identical to `null`, and would c The function `Laminas\Diactoros\marshalHeadersFromSapi()`, which is consumed by `Laminas\Diactoros\ServerRequestFactory::fromGlobals()`, was modified such that it will now filter out header field names that evaluate to integers. Please see the [Security Features](security-features.md) document for more details. - ## Removed The following features were removed for version 3. From 5535fe55941802d84619ffff40a6bc8600e3ed13 Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Thu, 4 May 2023 16:13:47 -0500 Subject: [PATCH 10/10] Update src/functions/marshal_headers_from_sapi.php Co-authored-by: Aleksei Khudiakov Signed-off-by: Matthew Weier O'Phinney --- src/functions/marshal_headers_from_sapi.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/functions/marshal_headers_from_sapi.php b/src/functions/marshal_headers_from_sapi.php index 21258cab..2819840d 100644 --- a/src/functions/marshal_headers_from_sapi.php +++ b/src/functions/marshal_headers_from_sapi.php @@ -67,7 +67,7 @@ function marshalHeadersFromSapi(array $server): array } // Filter out integer keys. - // These can occor if the translated header name is a string integer. + // These can occur if the translated header name is a string integer. // PHP will cast those to integers when assigned to an array. // This filters them out. return array_filter($headers, fn(string|int $key): bool => is_string($key), ARRAY_FILTER_USE_KEY);