From 5f8aa24280fb09947897d6b322bf1f0e038b13b6 Mon Sep 17 00:00:00 2001 From: Michal Sniatala Date: Sat, 18 Jan 2025 11:54:38 +0100 Subject: [PATCH 1/2] Merge commit from fork Co-authored-by: neznaika0 Co-authored-by: John Paul E. Balandan, CPA --- system/HTTP/Header.php | 72 +++++++++++++++++- tests/system/HTTP/HeaderTest.php | 81 +++++++++++++++++++++ user_guide_src/source/changelogs/v4.5.8.rst | 8 ++ 3 files changed, 159 insertions(+), 2 deletions(-) diff --git a/system/HTTP/Header.php b/system/HTTP/Header.php index 3832a4413a47..a21f9961536a 100644 --- a/system/HTTP/Header.php +++ b/system/HTTP/Header.php @@ -13,6 +13,7 @@ namespace CodeIgniter\HTTP; +use InvalidArgumentException; use Stringable; /** @@ -54,7 +55,7 @@ class Header implements Stringable */ public function __construct(string $name, $value = null) { - $this->name = $name; + $this->setName($name); $this->setValue($value); } @@ -81,9 +82,12 @@ public function getValue() * Sets the name of the header, overwriting any previous value. * * @return $this + * + * @throws InvalidArgumentException */ public function setName(string $name) { + $this->validateName($name); $this->name = $name; return $this; @@ -95,10 +99,16 @@ public function setName(string $name) * @param array|string>|string|null $value * * @return $this + * + * @throws InvalidArgumentException */ public function setValue($value = null) { - $this->value = is_array($value) ? $value : (string) $value; + $value = is_array($value) ? $value : (string) $value; + + $this->validateValue($value); + + $this->value = $value; return $this; } @@ -110,6 +120,8 @@ public function setValue($value = null) * @param array|string|null $value * * @return $this + * + * @throws InvalidArgumentException */ public function appendValue($value = null) { @@ -117,6 +129,8 @@ public function appendValue($value = null) return $this; } + $this->validateValue($value); + if (! is_array($this->value)) { $this->value = [$this->value]; } @@ -135,6 +149,8 @@ public function appendValue($value = null) * @param array|string|null $value * * @return $this + * + * @throws InvalidArgumentException */ public function prependValue($value = null) { @@ -142,6 +158,8 @@ public function prependValue($value = null) return $this; } + $this->validateValue($value); + if (! is_array($this->value)) { $this->value = [$this->value]; } @@ -193,4 +211,54 @@ public function __toString(): string { return $this->name . ': ' . $this->getValueLine(); } + + /** + * Validate header name. + * + * Regex is based on code from a guzzlehttp/psr7 library. + * + * @see https://datatracker.ietf.org/doc/html/rfc7230#section-3.2 + * + * @throws InvalidArgumentException + */ + private function validateName(string $name): void + { + if (preg_match('/^[a-zA-Z0-9\'`#$%&*+.^_|~!-]+$/D', $name) !== 1) { + throw new InvalidArgumentException('The header name is not valid as per RFC 7230.'); + } + } + + /** + * Validate header value. + * + * Regex is based on code from a guzzlehttp/psr7 library. + * + * @see https://datatracker.ietf.org/doc/html/rfc7230#section-3.2 + * + * @param array|string>|int|string $value + * + * @throws InvalidArgumentException + */ + private function validateValue(array|int|string $value): void + { + if (is_int($value)) { + return; + } + + if (is_array($value)) { + foreach ($value as $key => $val) { + $this->validateValue($key); + $this->validateValue($val); + } + + return; + } + + // The regular expression excludes obs-fold per RFC 7230#3.2.4, as sending folded lines + // is deprecated and rare. This obscure HTTP/1.1 feature is unlikely to impact legitimate + // use cases. Libraries like Guzzle and AMPHP follow the same principle. + if (preg_match('/^[\x20\x09\x21-\x7E\x80-\xFF]*$/D', $value) !== 1) { + throw new InvalidArgumentException('The header value is not valid as per RFC 7230.'); + } + } } diff --git a/tests/system/HTTP/HeaderTest.php b/tests/system/HTTP/HeaderTest.php index 6ca5180ff838..3dab6faa3784 100644 --- a/tests/system/HTTP/HeaderTest.php +++ b/tests/system/HTTP/HeaderTest.php @@ -15,6 +15,8 @@ use CodeIgniter\Test\CIUnitTestCase; use Error; +use InvalidArgumentException; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Group; use stdClass; @@ -234,4 +236,83 @@ public function testHeaderToStringShowsEntireHeader(): void $this->assertSame($expected, (string) $header); } + + /** + * @param string $name + */ + #[DataProvider('invalidNamesProvider')] + public function testInvalidHeaderNames($name): void + { + $this->expectException(InvalidArgumentException::class); + + new Header($name, 'text/html'); + } + + /** + * @return list> + */ + public static function invalidNamesProvider(): array + { + return [ + ["Content-Type\r\n\r\n"], + ["Content-Type\r\n"], + ["Content-Type\n"], + ["\tContent-Type\t"], + ["\n\nContent-Type\n\n"], + ["\r\nContent-Type"], + ["\nContent-Type"], + ["Content\r\n-Type"], + ["\n"], + ["\r\n"], + ["\t"], + [' Content-Type '], + ['Content - Type'], + ["Content\x00Type"], + [':Content-Type'], + ['Content-Type:'], + [''], + ]; + } + + /** + * @param array|string>|string|null $value + */ + #[DataProvider('invalidValuesProvider')] + public function testInvalidHeaderValues($value): void + { + $this->expectException(InvalidArgumentException::class); + + new Header('X-Test-Header', $value); + } + + /** + * @return list|string>> + */ + public static function invalidValuesProvider(): array + { + return [ + ["Header\n Value"], + ["Header\r\n Value"], + ["Header\r Value"], + ["Header Value\n"], + ["\nHeader Value"], + ["Header Value\r\n"], + ["\n\rHeader Value"], + ["\n\nHeader Value\n\n"], + [ + ["Header\n Value"], + ["Header\r\n Value"], + ], + [ + [ + "Header\n" => 'Value', + ], + ], + [ + [ + 'Header' => "Value\r\n", + ], + ], + ]; + } } diff --git a/user_guide_src/source/changelogs/v4.5.8.rst b/user_guide_src/source/changelogs/v4.5.8.rst index ae29b59fb165..e62944df9334 100644 --- a/user_guide_src/source/changelogs/v4.5.8.rst +++ b/user_guide_src/source/changelogs/v4.5.8.rst @@ -10,6 +10,14 @@ Release Date: Unreleased :local: :depth: 3 +******** +SECURITY +******** + +- **Header:** *Validation of header name and value* was fixed. + See the `Security advisory GHSA-x5mq-jjr3-vmx6 `_ + for more information. + ******** BREAKING ******** From 97a6d66640ac15520387dcd4196ac0657f36927e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ng=C3=B4=20Qu=E1=BB=91c=20=C4=90=E1=BA=A1t?= Date: Sat, 18 Jan 2025 21:33:37 +0700 Subject: [PATCH 2/2] fix: ensure csrf token is string (#9365) * fix: ensure csrf token is string * fix: ensure csrf token is string * handle request php://input * handle request php://input * wip * wip * wip * wip Co-authored-by: Michal Sniatala * wip Co-authored-by: Michal Sniatala * Update user_guide_src/source/changelogs/v4.5.8.rst * Use data providers --------- Co-authored-by: Michal Sniatala Co-authored-by: John Paul E. Balandan, CPA --- system/Security/Security.php | 17 ++-- tests/system/Security/SecurityTest.php | 98 +++++++++++++++++---- user_guide_src/source/changelogs/v4.5.8.rst | 1 + 3 files changed, 94 insertions(+), 22 deletions(-) diff --git a/system/Security/Security.php b/system/Security/Security.php index 5d86dbbe9984..574da8d50dc1 100644 --- a/system/Security/Security.php +++ b/system/Security/Security.php @@ -307,13 +307,13 @@ private function getPostedToken(RequestInterface $request): ?string // Does the token exist in POST, HEADER or optionally php:://input - json data or PUT, DELETE, PATCH - raw data. if ($tokenValue = $request->getPost($this->config->tokenName)) { - return $tokenValue; + return is_string($tokenValue) ? $tokenValue : null; } - if ($request->hasHeader($this->config->headerName) - && $request->header($this->config->headerName)->getValue() !== '' - && $request->header($this->config->headerName)->getValue() !== []) { - return $request->header($this->config->headerName)->getValue(); + if ($request->hasHeader($this->config->headerName)) { + $tokenValue = $request->header($this->config->headerName)->getValue(); + + return (is_string($tokenValue) && $tokenValue !== '') ? $tokenValue : null; } $body = (string) $request->getBody(); @@ -321,12 +321,15 @@ private function getPostedToken(RequestInterface $request): ?string if ($body !== '') { $json = json_decode($body); if ($json !== null && json_last_error() === JSON_ERROR_NONE) { - return $json->{$this->config->tokenName} ?? null; + $tokenValue = $json->{$this->config->tokenName} ?? null; + + return is_string($tokenValue) ? $tokenValue : null; } parse_str($body, $parsed); + $tokenValue = $parsed[$this->config->tokenName] ?? null; - return $parsed[$this->config->tokenName] ?? null; + return is_string($tokenValue) ? $tokenValue : null; } return null; diff --git a/tests/system/Security/SecurityTest.php b/tests/system/Security/SecurityTest.php index fa7d29e775c3..2f3aea06470b 100644 --- a/tests/system/Security/SecurityTest.php +++ b/tests/system/Security/SecurityTest.php @@ -24,6 +24,7 @@ use CodeIgniter\Test\Mock\MockSecurity; use Config\Security as SecurityConfig; use PHPUnit\Framework\Attributes\BackupGlobals; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Group; /** @@ -42,13 +43,23 @@ protected function setUp(): void $this->resetServices(); } - private function createMockSecurity(?SecurityConfig $config = null): MockSecurity + private static function createMockSecurity(SecurityConfig $config = new SecurityConfig()): MockSecurity { - $config ??= new SecurityConfig(); - return new MockSecurity($config); } + private static function createIncomingRequest(): IncomingRequest + { + $config = new MockAppConfig(); + + return new IncomingRequest( + $config, + new SiteURI($config), + null, + new UserAgent(), + ); + } + public function testBasicConfigIsSaved(): void { $security = $this->createMockSecurity(); @@ -108,18 +119,6 @@ public function testCSRFVerifyPostThrowsExceptionOnNoMatch(): void $security->verify($request); } - private function createIncomingRequest(): IncomingRequest - { - $config = new MockAppConfig(); - - return new IncomingRequest( - $config, - new SiteURI($config), - null, - new UserAgent(), - ); - } - public function testCSRFVerifyPostReturnsSelfOnMatch(): void { $_SERVER['REQUEST_METHOD'] = 'POST'; @@ -315,4 +314,73 @@ public function testGetters(): void $this->assertIsString($security->getCookieName()); $this->assertIsBool($security->shouldRedirect()); } + + public function testGetPostedTokenReturnsTokenFromPost(): void + { + $_POST['csrf_test_name'] = '8b9218a55906f9dcc1dc263dce7f005a'; + $request = $this->createIncomingRequest(); + $method = $this->getPrivateMethodInvoker($this->createMockSecurity(), 'getPostedToken'); + + $this->assertSame('8b9218a55906f9dcc1dc263dce7f005a', $method($request)); + } + + public function testGetPostedTokenReturnsTokenFromHeader(): void + { + $_POST = []; + $request = $this->createIncomingRequest()->setHeader('X-CSRF-TOKEN', '8b9218a55906f9dcc1dc263dce7f005a'); + $method = $this->getPrivateMethodInvoker($this->createMockSecurity(), 'getPostedToken'); + + $this->assertSame('8b9218a55906f9dcc1dc263dce7f005a', $method($request)); + } + + public function testGetPostedTokenReturnsTokenFromJsonBody(): void + { + $_POST = []; + $jsonBody = json_encode(['csrf_test_name' => '8b9218a55906f9dcc1dc263dce7f005a']); + $request = $this->createIncomingRequest()->setBody($jsonBody); + $method = $this->getPrivateMethodInvoker($this->createMockSecurity(), 'getPostedToken'); + + $this->assertSame('8b9218a55906f9dcc1dc263dce7f005a', $method($request)); + } + + public function testGetPostedTokenReturnsTokenFromFormBody(): void + { + $_POST = []; + $formBody = 'csrf_test_name=8b9218a55906f9dcc1dc263dce7f005a'; + $request = $this->createIncomingRequest()->setBody($formBody); + $method = $this->getPrivateMethodInvoker($this->createMockSecurity(), 'getPostedToken'); + + $this->assertSame('8b9218a55906f9dcc1dc263dce7f005a', $method($request)); + } + + #[DataProvider('provideGetPostedTokenReturnsNullForInvalidInputs')] + public function testGetPostedTokenReturnsNullForInvalidInputs(string $case, IncomingRequest $request): void + { + $method = $this->getPrivateMethodInvoker($this->createMockSecurity(), 'getPostedToken'); + + $this->assertNull( + $method($request), + sprintf('Failed asserting that %s returns null on invalid input.', $case), + ); + } + + /** + * @return iterable + */ + public static function provideGetPostedTokenReturnsNullForInvalidInputs(): iterable + { + $testCases = [ + 'empty_post' => self::createIncomingRequest(), + 'invalid_post_data' => self::createIncomingRequest()->setGlobal('post', ['csrf_test_name' => ['invalid' => 'data']]), + 'empty_header' => self::createIncomingRequest()->setHeader('X-CSRF-TOKEN', ''), + 'invalid_json_data' => self::createIncomingRequest()->setBody(json_encode(['csrf_test_name' => ['invalid' => 'data']])), + 'invalid_json' => self::createIncomingRequest()->setBody('{invalid json}'), + 'missing_token_in_body' => self::createIncomingRequest()->setBody('other=value&another=test'), + 'invalid_form_data' => self::createIncomingRequest()->setBody('csrf_test_name[]=invalid'), + ]; + + foreach ($testCases as $case => $request) { + yield $case => [$case, $request]; + } + } } diff --git a/user_guide_src/source/changelogs/v4.5.8.rst b/user_guide_src/source/changelogs/v4.5.8.rst index e62944df9334..910912ae5dc4 100644 --- a/user_guide_src/source/changelogs/v4.5.8.rst +++ b/user_guide_src/source/changelogs/v4.5.8.rst @@ -39,6 +39,7 @@ Bugs Fixed ********** - **Database:** Fixed a bug where ``Builder::affectedRows()`` threw an error when the previous query call failed in ``Postgre`` and ``SQLSRV`` drivers. +- **Security:** Fixed a bug where the CSRF token validation could fail on malformed input, causing a generic HTTP 500 status code instead of handling the input gracefully. See the repo's `CHANGELOG.md `_