From c48f34b5237eec2ed643722950ada833893ca4c4 Mon Sep 17 00:00:00 2001 From: michalsn Date: Sat, 9 Sep 2023 09:06:15 +0200 Subject: [PATCH 1/7] fix: check for CSRF token in the raw body --- system/Security/Security.php | 31 ++++++++----- .../SecurityCSRFSessionRandomizeTokenTest.php | 13 ++++++ .../Security/SecurityCSRFSessionTest.php | 13 ++++++ tests/system/Security/SecurityTest.php | 44 +++++++++++++++++++ user_guide_src/source/changelogs/v4.4.2.rst | 2 + 5 files changed, 93 insertions(+), 10 deletions(-) diff --git a/system/Security/Security.php b/system/Security/Security.php index 648856321363..b8bed67dfe11 100644 --- a/system/Security/Security.php +++ b/system/Security/Security.php @@ -318,16 +318,22 @@ private function removeTokenInRequest(RequestInterface $request): void { assert($request instanceof Request); - $json = json_decode($request->getBody() ?? ''); - if (isset($_POST[$this->config->tokenName])) { // We kill this since we're done and we don't want to pollute the POST array. unset($_POST[$this->config->tokenName]); $request->setGlobal('post', $_POST); - } elseif (isset($json->{$this->config->tokenName})) { - // We kill this since we're done and we don't want to pollute the JSON data. - unset($json->{$this->config->tokenName}); - $request->setBody(json_encode($json)); + } else { + $body = $request->getBody() ?? ''; + if (! empty($json = json_decode($body)) && json_last_error() === JSON_ERROR_NONE) { + // We kill this since we're done and we don't want to pollute the JSON data. + unset($json->{$this->config->tokenName}); + $request->setBody(json_encode($json)); + } else { + parse_str($body, $parsed); + // We kill this since we're done and we don't want to pollute the BODY data. + unset($parsed[$this->config->tokenName]); + $request->setBody(http_build_query($parsed)); + } } } @@ -335,7 +341,7 @@ private function getPostedToken(RequestInterface $request): ?string { assert($request instanceof IncomingRequest); - // Does the token exist in POST, HEADER or optionally php:://input - json data. + // 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; @@ -346,10 +352,15 @@ private function getPostedToken(RequestInterface $request): ?string } $body = (string) $request->getBody(); - $json = json_decode($body); - if ($body !== '' && ! empty($json) && json_last_error() === JSON_ERROR_NONE) { - return $json->{$this->config->tokenName} ?? null; + if ($body !== '') { + if (! empty($json = json_decode($body)) && json_last_error() === JSON_ERROR_NONE) { + return $json->{$this->config->tokenName} ?? null; + } + + parse_str($body, $parsed); + + return $parsed[$this->config->tokenName] ?? null; } return null; diff --git a/tests/system/Security/SecurityCSRFSessionRandomizeTokenTest.php b/tests/system/Security/SecurityCSRFSessionRandomizeTokenTest.php index 2a148c902e0f..7c8af63040f4 100644 --- a/tests/system/Security/SecurityCSRFSessionRandomizeTokenTest.php +++ b/tests/system/Security/SecurityCSRFSessionRandomizeTokenTest.php @@ -246,6 +246,19 @@ public function testCSRFVerifyPUTHeaderReturnsSelfOnMatch(): void $this->assertLogged('info', 'CSRF token verified.'); } + public function testCSRFVerifyPUTBodyReturnsSelfOnMatch(): void + { + $_SERVER['REQUEST_METHOD'] = 'PUT'; + + $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); + $request->setBody("csrf_test_name={$this->randomizedToken}&foo=bar"); + + $security = $this->createSecurity(); + + $this->assertInstanceOf(Security::class, $security->verify($request)); + $this->assertLogged('info', 'CSRF token verified.'); + } + public function testCSRFVerifyJsonThrowsExceptionOnNoMatch(): void { $this->expectException(SecurityException::class); diff --git a/tests/system/Security/SecurityCSRFSessionTest.php b/tests/system/Security/SecurityCSRFSessionTest.php index a8f784a5464b..1d701520488c 100644 --- a/tests/system/Security/SecurityCSRFSessionTest.php +++ b/tests/system/Security/SecurityCSRFSessionTest.php @@ -201,6 +201,19 @@ public function testCSRFVerifyPUTHeaderReturnsSelfOnMatch(): void $this->assertLogged('info', 'CSRF token verified.'); } + public function testCSRFVerifyPUTBodyReturnsSelfOnMatch(): void + { + $_SERVER['REQUEST_METHOD'] = 'PUT'; + + $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); + $request->setBody('csrf_test_name=8b9218a55906f9dcc1dc263dce7f005a&foo=bar'); + + $security = $this->createSecurity(); + + $this->assertInstanceOf(Security::class, $security->verify($request)); + $this->assertLogged('info', 'CSRF token verified.'); + } + public function testCSRFVerifyJsonThrowsExceptionOnNoMatch(): void { $this->expectException(SecurityException::class); diff --git a/tests/system/Security/SecurityTest.php b/tests/system/Security/SecurityTest.php index 972115de6a7b..ae4953607662 100644 --- a/tests/system/Security/SecurityTest.php +++ b/tests/system/Security/SecurityTest.php @@ -218,6 +218,50 @@ public function testCSRFVerifyJsonReturnsSelfOnMatch(): void $this->assertSame('{"foo":"bar"}', $request->getBody()); } + public function testCSRFVerifyPutBodyThrowsExceptionOnNoMatch(): void + { + $_SERVER['REQUEST_METHOD'] = 'PUT'; + $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005b'; + + $security = $this->createMockSecurity(); + $request = new IncomingRequest( + new MockAppConfig(), + new URI('http://badurl.com'), + null, + new UserAgent() + ); + + $request->setBody( + 'csrf_test_name=8b9218a55906f9dcc1dc263dce7f005a' + ); + + $this->expectException(SecurityException::class); + $security->verify($request); + } + + public function testCSRFVerifyPutBodyReturnsSelfOnMatch(): void + { + $_SERVER['REQUEST_METHOD'] = 'PUT'; + $_COOKIE['csrf_cookie_name'] = '8b9218a55906f9dcc1dc263dce7f005a'; + + $security = $this->createMockSecurity(); + $request = new IncomingRequest( + new MockAppConfig(), + new URI('http://badurl.com'), + null, + new UserAgent() + ); + + $request->setBody( + 'csrf_test_name=8b9218a55906f9dcc1dc263dce7f005a&foo=bar' + ); + + $this->assertInstanceOf(Security::class, $security->verify($request)); + $this->assertLogged('info', 'CSRF token verified.'); + + $this->assertSame('foo=bar', $request->getBody()); + } + public function testSanitizeFilename(): void { $security = $this->createMockSecurity(); diff --git a/user_guide_src/source/changelogs/v4.4.2.rst b/user_guide_src/source/changelogs/v4.4.2.rst index 3866a7664ee6..0b288350797a 100644 --- a/user_guide_src/source/changelogs/v4.4.2.rst +++ b/user_guide_src/source/changelogs/v4.4.2.rst @@ -29,6 +29,8 @@ Deprecations Bugs Fixed ********** +- **Security:** Fixed a bug where the CSRF token wasn't checked if we sent it in the raw body (not JSON format) for PUT, PATCH, and DELETE requests. + See the repo's `CHANGELOG.md `_ for a complete list of bugs fixed. From ca6c800d5cf9a9290d1c2f41297337b41926b92f Mon Sep 17 00:00:00 2001 From: michalsn Date: Sat, 9 Sep 2023 14:59:41 +0200 Subject: [PATCH 2/7] implement changes from code review --- system/Security/Security.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/system/Security/Security.php b/system/Security/Security.php index b8bed67dfe11..1ab893aeff8f 100644 --- a/system/Security/Security.php +++ b/system/Security/Security.php @@ -354,7 +354,8 @@ private function getPostedToken(RequestInterface $request): ?string $body = (string) $request->getBody(); if ($body !== '') { - if (! empty($json = json_decode($body)) && json_last_error() === JSON_ERROR_NONE) { + $json = json_decode($body); + if (! empty($json) && json_last_error() === JSON_ERROR_NONE) { return $json->{$this->config->tokenName} ?? null; } From 1528d0ea3bd653dc91b594d761a8d5bac65f334d Mon Sep 17 00:00:00 2001 From: michalsn Date: Sat, 9 Sep 2023 15:00:04 +0200 Subject: [PATCH 3/7] update changelog and user guide --- user_guide_src/source/changelogs/v4.4.2.rst | 3 +-- user_guide_src/source/libraries/security.rst | 1 + 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/user_guide_src/source/changelogs/v4.4.2.rst b/user_guide_src/source/changelogs/v4.4.2.rst index 0b288350797a..19f2a4f1782c 100644 --- a/user_guide_src/source/changelogs/v4.4.2.rst +++ b/user_guide_src/source/changelogs/v4.4.2.rst @@ -22,6 +22,7 @@ Changes command was removed. It did not work from the beginning. Also, the rollback command returns the database(s) state to a specified batch number and cannot specify only a specific database group. +- **Security:** The presence of the CSRF token is now also checked in the raw body (not JSON format) for PUT, PATCH, and DELETE type of requests. Deprecations ************ @@ -29,8 +30,6 @@ Deprecations Bugs Fixed ********** -- **Security:** Fixed a bug where the CSRF token wasn't checked if we sent it in the raw body (not JSON format) for PUT, PATCH, and DELETE requests. - See the repo's `CHANGELOG.md `_ for a complete list of bugs fixed. diff --git a/user_guide_src/source/libraries/security.rst b/user_guide_src/source/libraries/security.rst index bc5777f759fa..f78c1106e861 100644 --- a/user_guide_src/source/libraries/security.rst +++ b/user_guide_src/source/libraries/security.rst @@ -204,6 +204,7 @@ The order of checking the availability of the CSRF token is as follows: 1. ``$_POST`` array 2. HTTP header 3. ``php://input`` (JSON request) - bear in mind that this approach is the slowest one since we have to decode JSON and then re-encode it +4. ``php://input`` (raw body) - for PUT, PATCH, and DELETE type of requests ********************* Other Helpful Methods From 4335c6be165de5d3de8b76346ec931edf8e15218 Mon Sep 17 00:00:00 2001 From: michalsn Date: Sat, 9 Sep 2023 15:33:43 +0200 Subject: [PATCH 4/7] implement missing changes from the code review --- system/Security/Security.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/system/Security/Security.php b/system/Security/Security.php index 1ab893aeff8f..534487efcc41 100644 --- a/system/Security/Security.php +++ b/system/Security/Security.php @@ -324,7 +324,8 @@ private function removeTokenInRequest(RequestInterface $request): void $request->setGlobal('post', $_POST); } else { $body = $request->getBody() ?? ''; - if (! empty($json = json_decode($body)) && json_last_error() === JSON_ERROR_NONE) { + $json = json_decode($body); + if (! empty($json) && json_last_error() === JSON_ERROR_NONE) { // We kill this since we're done and we don't want to pollute the JSON data. unset($json->{$this->config->tokenName}); $request->setBody(json_encode($json)); From a63457f0695c0c8d292c950c6b87351c4cae4e87 Mon Sep 17 00:00:00 2001 From: michalsn Date: Fri, 15 Sep 2023 10:14:48 +0200 Subject: [PATCH 5/7] apply changes from code review --- system/Security/Security.php | 4 ++-- user_guide_src/source/libraries/security.rst | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/system/Security/Security.php b/system/Security/Security.php index 534487efcc41..aeb8526cfdc0 100644 --- a/system/Security/Security.php +++ b/system/Security/Security.php @@ -325,7 +325,7 @@ private function removeTokenInRequest(RequestInterface $request): void } else { $body = $request->getBody() ?? ''; $json = json_decode($body); - if (! empty($json) && json_last_error() === JSON_ERROR_NONE) { + if ($json !== null && json_last_error() === JSON_ERROR_NONE) { // We kill this since we're done and we don't want to pollute the JSON data. unset($json->{$this->config->tokenName}); $request->setBody(json_encode($json)); @@ -356,7 +356,7 @@ private function getPostedToken(RequestInterface $request): ?string if ($body !== '') { $json = json_decode($body); - if (! empty($json) && json_last_error() === JSON_ERROR_NONE) { + if ($json !== null && json_last_error() === JSON_ERROR_NONE) { return $json->{$this->config->tokenName} ?? null; } diff --git a/user_guide_src/source/libraries/security.rst b/user_guide_src/source/libraries/security.rst index f78c1106e861..6c39cbefdb5e 100644 --- a/user_guide_src/source/libraries/security.rst +++ b/user_guide_src/source/libraries/security.rst @@ -206,6 +206,8 @@ The order of checking the availability of the CSRF token is as follows: 3. ``php://input`` (JSON request) - bear in mind that this approach is the slowest one since we have to decode JSON and then re-encode it 4. ``php://input`` (raw body) - for PUT, PATCH, and DELETE type of requests +.. note:: ``php://input`` (raw body) is checked since v4.4.2. + ********************* Other Helpful Methods ********************* From be07d9c98fff5cf87664495d4e393a4910406378 Mon Sep 17 00:00:00 2001 From: michalsn Date: Fri, 15 Sep 2023 10:29:07 +0200 Subject: [PATCH 6/7] make phpstan happy --- system/Security/Security.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/system/Security/Security.php b/system/Security/Security.php index aeb8526cfdc0..4ba665639f36 100644 --- a/system/Security/Security.php +++ b/system/Security/Security.php @@ -348,7 +348,9 @@ private function getPostedToken(RequestInterface $request): ?string return $tokenValue; } - if ($request->hasHeader($this->config->headerName) && ! empty($request->header($this->config->headerName)->getValue())) { + 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(); } From 46f8d5f7e393ecb886cdc40b1d97d5c2872b31ba Mon Sep 17 00:00:00 2001 From: michalsn Date: Fri, 15 Sep 2023 10:51:57 +0200 Subject: [PATCH 7/7] update phpstan-baseline.php --- phpstan-baseline.php | 5 ----- 1 file changed, 5 deletions(-) diff --git a/phpstan-baseline.php b/phpstan-baseline.php index 106de3723ef0..d95d38fb3c6d 100644 --- a/phpstan-baseline.php +++ b/phpstan-baseline.php @@ -2421,11 +2421,6 @@ 'count' => 1, 'path' => __DIR__ . '/system/Router/RouterInterface.php', ]; -$ignoreErrors[] = [ - 'message' => '#^Construct empty\\(\\) is not allowed\\. Use more strict comparison\\.$#', - 'count' => 2, - 'path' => __DIR__ . '/system/Security/Security.php', -]; $ignoreErrors[] = [ 'message' => '#^Method CodeIgniter\\\\Session\\\\Exceptions\\\\SessionException\\:\\:forEmptySavepath\\(\\) has no return type specified\\.$#', 'count' => 1,