Delete cookie with invalid CSRF token

src/Http/Middleware/CsrfProtectionMiddleware.php

@@ -194,6 +194,26 @@ class CsrfProtectionMiddleware implements MiddlewareInterface
     }
 
     /**
+     * Verify CSRF token.
+     *
+     * @param string $token The CSRF token.
+     * @return bool
+     */
+    protected function _verifyToken(string $token): bool
+    {
+        if (strlen($token) <= self::TOKEN_VALUE_LENGTH) {
+            return false;
+        }
+
+        $key = substr($token, 0, static::TOKEN_VALUE_LENGTH);
+        $hmac = substr($token, static::TOKEN_VALUE_LENGTH);
+
+        $expectedHmac = hash_hmac('sha1', $key, Security::getSalt());
+
+        return hash_equals($hmac, $expectedHmac);
+    }
+
+    /**
      * Add a CSRF token to the response cookies.
      *
      * @param string $token The token to add.
@@ -206,16 +226,7 @@ class CsrfProtectionMiddleware implements MiddlewareInterface
         ServerRequestInterface $request,
         ResponseInterface $response
     ): ResponseInterface {
-        $cookie = Cookie::create(
-            $this->_config['cookieName'],
-            $token,
-            [
-                'expires' => $this->_config['expiry'] ?: null,
-                'path' => $request->getAttribute('webroot'),
-                'secure' => $this->_config['secure'],
-                'httponly' => $this->_config['httpOnly'],
-            ]
-        );
+        $cookie = $this->_createCookie($token, $request);
         if ($response instanceof Response) {
             return $response->withCookie($cookie);
         }
@@ -238,16 +249,25 @@ class CsrfProtectionMiddleware implements MiddlewareInterface
             throw new InvalidCsrfTokenException(__d('cake', 'Missing CSRF token cookie'));
         }
 
+        if (!$this->_verifyToken($cookie)) {
+            $exception = new InvalidCsrfTokenException(__d('cake', 'Missing CSRF token cookie'));
+
+            $expiredCookie = $this->_createCookie('', $request)->withExpired();
+            $exception->responseHeader('Set-Cookie', $expiredCookie->toHeaderValue());
+
+            throw $exception;
+        }
+
         $body = $request->getParsedBody();
         if (is_array($body) || $body instanceof ArrayAccess) {
-            $post = Hash::get($body, $this->_config['field']);
-            if ($this->_compareToken($post, $cookie)) {
+            $post = (string)Hash::get($body, $this->_config['field']);
+            if (hash_equals($post, $cookie)) {
                 return;
             }
         }
 
         $header = $request->getHeaderLine('X-CSRF-Token');
-        if ($this->_compareToken($header, $cookie)) {
+        if (hash_equals($header, $cookie)) {
             return;
         }
 
@@ -255,43 +275,25 @@ class CsrfProtectionMiddleware implements MiddlewareInterface
     }
 
     /**
-     * Ensure that the request token matches the cookie value and that
-     * both were generated by us.
+     * Create response cookie
      *
-     * @param mixed $post The request token.
-     * @param mixed $cookie The cookie token.
-     * @return bool
+     * @param string $value Cookie value
+     * @param \Psr\Http\Message\ServerRequestInterface $request The request object.
+     * @return \Cake\Http\Cookie\Cookie
      */
-    protected function _compareToken($post, $cookie): bool
+    protected function _createCookie(string $value, ServerRequestInterface $request): 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;
-            }
-        }
+        $cookie = Cookie::create(
+            $this->_config['cookieName'],
+            $value,
+            [
+                'expires' => $this->_config['expiry'] ?: null,
+                'path' => $request->getAttribute('webroot'),
+                'secure' => $this->_config['secure'],
+                'httponly' => $this->_config['httpOnly'],
+            ]
+        );
 
-        return true;
+        return $cookie;
     }
 }

tests/TestCase/Http/Middleware/CsrfProtectionMiddlewareTest.php

@@ -16,6 +16,7 @@ declare(strict_types=1);
  */
 namespace Cake\Test\TestCase\Http\Middleware;
 
+use Cake\Http\Cookie\Cookie;
 use Cake\Http\Exception\InvalidCsrfTokenException;
 use Cake\Http\Middleware\CsrfProtectionMiddleware;
 use Cake\Http\Response;
@@ -204,8 +205,19 @@ class CsrfProtectionMiddlewareTest extends TestCase
 
         $middleware = new CsrfProtectionMiddleware();
 
-        $this->expectException(InvalidCsrfTokenException::class);
-        $middleware->process($request, $this->_getRequestHandler());
+        try {
+            $middleware->process($request, $this->_getRequestHandler());
+
+            $this->fail();
+        } catch (InvalidCsrfTokenException $exception) {
+            $responseHeaders = $exception->responseHeader();
+
+            $this->assertArrayHasKey('Set-Cookie', $responseHeaders);
+
+            $cookie = Cookie::createFromHeaderString($responseHeaders['Set-Cookie']);
+            $this->assertSame('csrfToken', $cookie->getName(), 'Should delete cookie with invalid CSRF token');
+            $this->assertTrue($cookie->isExpired(), 'Should delete cookie with invalid CSRF token');
+        }
     }
 
     /**
@@ -255,8 +267,18 @@ class CsrfProtectionMiddlewareTest extends TestCase
 
         $middleware = new CsrfProtectionMiddleware();
 
-        $this->expectException(InvalidCsrfTokenException::class);
-        $middleware->process($request, $this->_getRequestHandler());
+        try {
+            $middleware->process($request, $this->_getRequestHandler());
+
+            $this->fail();
+        } catch (InvalidCsrfTokenException $exception) {
+            $responseHeaders = $exception->responseHeader();
+            $this->assertArrayHasKey('Set-Cookie', $responseHeaders);
+
+            $cookie = Cookie::createFromHeaderString($responseHeaders['Set-Cookie']);
+            $this->assertSame('csrfToken', $cookie->getName(), 'Should delete cookie with invalid CSRF token');
+            $this->assertTrue($cookie->isExpired(), 'Should delete cookie with invalid CSRF token');
+        }
     }
 
     /**
@@ -317,8 +339,15 @@ class CsrfProtectionMiddlewareTest extends TestCase
         ]);
 
         $middleware = new CsrfProtectionMiddleware();
-        $this->expectException(InvalidCsrfTokenException::class);
-        $middleware->process($request, $this->_getRequestHandler());
+
+        try {
+            $middleware->process($request, $this->_getRequestHandler());
+
+            $this->fail();
+        } catch (InvalidCsrfTokenException $exception) {
+            $responseHeaders = $exception->responseHeader();
+            $this->assertEmpty($responseHeaders, 'Should not send any header');
+        }
     }
 
     /**