From 309c8990dcadcf4391967f4b01aa84c58f84b8d8 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Mon, 2 May 2022 16:04:21 -0400 Subject: [PATCH] Add configuration option to enable more secure CSRF token (#16481) * Add configuration option to enable more secure CSRF token Backport the fixes from #14431 to 3.x. This will resolve GHSA-j33j-fg2g-mcv2 aka CVE-2020-15400 in a backwards compatible way. I didn't initially make this change because of the backwards compatibility risk. However, I think a setting based approach at least gives application developers a way to have a clean security audit and maintain backwards compatibility. Fixes #16326 --- .../Middleware/CsrfProtectionMiddleware.php | 87 +++++++++++++++- src/TestSuite/IntegrationTestTrait.php | 8 +- .../CsrfProtectionMiddlewareTest.php | 99 +++++++++++++++++++ 3 files changed, 187 insertions(+), 7 deletions(-) diff --git a/src/Http/Middleware/CsrfProtectionMiddleware.php b/src/Http/Middleware/CsrfProtectionMiddleware.php index 5c6b410a7a3..2756408339f 100644 --- a/src/Http/Middleware/CsrfProtectionMiddleware.php +++ b/src/Http/Middleware/CsrfProtectionMiddleware.php @@ -46,9 +46,14 @@ class CsrfProtectionMiddleware * Defaults to browser session. * - `secure` Whether or not the cookie will be set with the Secure flag. Defaults to false. * - `httpOnly` Whether or not the cookie will be set with the HttpOnly flag. Defaults to false. - * - `samesite` Value for "SameSite" attribute. Default to null. + * - `samesite` Value for "SameSite" attribute. Default to null. * - `field` The form field to check. Changing this will also require configuring * FormHelper. + * - `verifyTokenSource` Generate and verify tokens that include the application salt + * value. This prevents tokens from being manipulated by an attacker via XSS or physical + * access. This behavior is disabled by default as it is not cross compatible with tokens + * created in earlier versions of CakePHP. It is recommended that you enable this setting + * if possible as it is the default in 4.x. * * @var array */ @@ -59,6 +64,7 @@ class CsrfProtectionMiddleware 'httpOnly' => false, 'samesite' => null, 'field' => '_csrfToken', + 'verifyTokenSource' => false, ]; /** @@ -77,6 +83,11 @@ class CsrfProtectionMiddleware */ protected $whitelistCallback; + /** + * @var int + */ + const TOKEN_VALUE_LENGTH = 16; + /** * Constructor * @@ -115,7 +126,7 @@ public function __invoke(ServerRequest $request, Response $response, $next) $method = $request->getMethod(); if ($method === 'GET' && $cookieData === null) { - $token = $this->_createToken(); + $token = $this->createToken(); $request = $this->_addTokenToRequest($token, $request); $response = $this->_addTokenCookie($token, $request, $response); @@ -169,7 +180,22 @@ protected function _validateAndUnsetTokenField(ServerRequest $request) */ protected function _createToken() { - return hash('sha512', Security::randomBytes(16), false); + return $this->createToken(); + } + + /** + * Create a new token to be used for CSRF protection. + * + * @return string + */ + public function createToken() + { + $value = Security::randomBytes(static::TOKEN_VALUE_LENGTH); + if (!$this->_config['verifyTokenSource']) { + return hash('sha512', $value, false); + } + + return $value . hash_hmac('sha1', $value, Security::getSalt()); } /** @@ -231,8 +257,61 @@ protected function _validateToken(ServerRequest $request) throw new InvalidCsrfTokenException(__d('cake', 'Missing CSRF token cookie')); } - if (!Security::constantEquals($post, $cookie) && !Security::constantEquals($header, $cookie)) { + if ($this->_config['verifyTokenSource']) { + // This path validates that the token was generated by our application. + if ($this->_compareToken($post, $cookie) || $this->_compareToken($header, $cookie)) { + return; + } + throw new InvalidCsrfTokenException(__d('cake', 'CSRF token mismatch.')); } + + // Backwards compatibility mode. This path compares tokens as opaque strings. + if (Security::constantEquals($post, $cookie) || Security::constantEquals($header, $cookie)) { + return; + } + + throw new InvalidCsrfTokenException(__d('cake', 'CSRF token mismatch.')); + } + + /** + * Ensure that the request token matches the cookie value and that + * both were generated by us. + * + * @param mixed $post The request token. + * @param mixed $cookie The cookie token. + * @return bool + */ + protected function _compareToken($post, $cookie) + { + if (!is_string($post)) { + $post = ''; + } + if (!is_string($cookie)) { + $cookie = ''; + } + $postKey = (string)substr($post, 0, static::TOKEN_VALUE_LENGTH); + $postHmac = (string)substr($post, static::TOKEN_VALUE_LENGTH); + $cookieKey = (string)substr($cookie, 0, static::TOKEN_VALUE_LENGTH); + $cookieHmac = (string)substr($cookie, static::TOKEN_VALUE_LENGTH); + + // Put all checks in a list + // so they all burn time reducing timing attack window. + $checks = [ + hash_equals($postKey, $cookieKey), + hash_equals($postHmac, $cookieHmac), + hash_equals( + $postHmac, + hash_hmac('sha1', $postKey, Security::getSalt()) + ), + ]; + + foreach ($checks as $check) { + if ($check !== true) { + return false; + } + } + + return true; } } diff --git a/src/TestSuite/IntegrationTestTrait.php b/src/TestSuite/IntegrationTestTrait.php index 0936daad763..0953b2102f6 100644 --- a/src/TestSuite/IntegrationTestTrait.php +++ b/src/TestSuite/IntegrationTestTrait.php @@ -15,7 +15,7 @@ use Cake\Core\Configure; use Cake\Database\Exception as DatabaseException; -use Cake\Http\ServerRequest; +use Cake\Http\Middleware\CsrfProtectionMiddleware; use Cake\Http\Session; use Cake\Routing\Router; use Cake\TestSuite\Constraint\Response\BodyContains; @@ -51,7 +51,6 @@ use Cake\Utility\CookieCryptTrait; use Cake\Utility\Hash; use Cake\Utility\Security; -use Cake\Utility\Text; use Cake\View\Helper\SecureFieldTokenTrait; use Exception; use Laminas\Diactoros\Uri; @@ -701,8 +700,11 @@ protected function _addTokens($url, $data) } if ($this->_csrfToken === true) { + // While most applications will not be using verify tokens, we enable + // it for tests so that if applications upgrade they don't face testing failures. + $middleware = new CsrfProtectionMiddleware(['verifyTokenSource' => true]); if (!isset($this->_cookie['csrfToken'])) { - $this->_cookie['csrfToken'] = Text::uuid(); + $this->_cookie['csrfToken'] = $middleware->createToken(); } if (!isset($data['_csrfToken'])) { $data['_csrfToken'] = $this->_cookie['csrfToken']; diff --git a/tests/TestCase/Http/Middleware/CsrfProtectionMiddlewareTest.php b/tests/TestCase/Http/Middleware/CsrfProtectionMiddlewareTest.php index c60214eb386..71e7a3421ae 100644 --- a/tests/TestCase/Http/Middleware/CsrfProtectionMiddlewareTest.php +++ b/tests/TestCase/Http/Middleware/CsrfProtectionMiddlewareTest.php @@ -189,6 +189,105 @@ public function testValidTokenRequestData($method) $middleware($request, $response, $closure); } + /** + * Test that the X-CSRF-Token works with the various http methods. + * + * @dataProvider httpMethodProvider + * @return void + */ + public function testValidTokenInHeaderVerifySource($method) + { + $middleware = new CsrfProtectionMiddleware(['verifyTokenSource' => true]); + $token = $middleware->createToken(); + $request = new ServerRequest([ + 'environment' => [ + 'REQUEST_METHOD' => $method, + 'HTTP_X_CSRF_TOKEN' => $token, + ], + 'post' => ['a' => 'b'], + 'cookies' => ['csrfToken' => $token], + ]); + $response = new Response(); + + // No exception means the test is valid + $response = $middleware($request, $response, $this->_getNextClosure()); + $this->assertInstanceOf(Response::class, $response); + } + + /** + * Test that the X-CSRF-Token works with the various http methods. + * + * @dataProvider httpMethodProvider + * @return void + */ + public function testInvalidTokenInHeaderVerifySource($method) + { + $this->expectException(\Cake\Http\Exception\InvalidCsrfTokenException::class); + $request = new ServerRequest([ + 'environment' => [ + 'REQUEST_METHOD' => $method, + // Even though the values match they are not signed. + 'HTTP_X_CSRF_TOKEN' => 'nope', + ], + 'post' => ['a' => 'b'], + 'cookies' => ['csrfToken' => 'nope'], + ]); + $response = new Response(); + + $middleware = new CsrfProtectionMiddleware(['verifyTokenSource' => true]); + $middleware($request, $response, $this->_getNextClosure()); + } + + /** + * Test that request data works with the various http methods. + * + * @dataProvider httpMethodProvider + * @return void + */ + public function testValidTokenRequestDataVerifySource($method) + { + $middleware = new CsrfProtectionMiddleware(['verifyTokenSource' => true]); + $token = $middleware->createToken(); + $request = new ServerRequest([ + 'environment' => [ + 'REQUEST_METHOD' => $method, + ], + 'post' => ['_csrfToken' => $token], + 'cookies' => ['csrfToken' => $token], + ]); + $response = new Response(); + + $closure = function ($request, $response) { + $this->assertNull($request->getData('_csrfToken')); + }; + + // No exception means everything is OK + $middleware($request, $response, $closure); + } + + /** + * Test that request data works with the various http methods. + * + * @dataProvider httpMethodProvider + * @return void + */ + public function testInvalidTokenRequestDataVerifySource($method) + { + $this->expectException(\Cake\Http\Exception\InvalidCsrfTokenException::class); + $request = new ServerRequest([ + 'environment' => [ + 'REQUEST_METHOD' => $method, + ], + // Even though the tokens match they are not signed. + 'post' => ['_csrfToken' => 'example-token'], + 'cookies' => ['csrfToken' => 'example-token'], + ]); + $response = new Response(); + + $middleware = new CsrfProtectionMiddleware(['verifyTokenSource' => true]); + $middleware($request, $response, $this->_getNextClosure()); + } + /** * Test that request data works with the various http methods. *