From 3c95f0cde306c64fa718ffd388d065d1f3208bff Mon Sep 17 00:00:00 2001 From: michalsn Date: Sat, 9 Sep 2023 09:06:15 +0200 Subject: [PATCH] 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 dae668909711..537a6ac9a5a4 100644 --- a/user_guide_src/source/changelogs/v4.4.2.rst +++ b/user_guide_src/source/changelogs/v4.4.2.rst @@ -24,6 +24,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.