Browse Source

Fix TypeError in CsrfProtectionMiddleware

When a cookie contains non-empty data that does not match the old token
format and also is not base64 encoded we should assume it is invalid and
generate a new CSRF token on GET requests. For non-GET requests this
will result in a CSRF validation error.

Fixes #15868
Mark Story 4 years ago
parent
commit
4044e676b4

+ 9 - 1
src/Http/Middleware/CsrfProtectionMiddleware.php

@@ -149,7 +149,11 @@ class CsrfProtectionMiddleware implements MiddlewareInterface
         $cookieData = Hash::get($cookies, $this->_config['cookieName']);
 
         if (is_string($cookieData) && strlen($cookieData) > 0) {
-            $request = $request->withAttribute('csrfToken', $this->saltToken($cookieData));
+            try {
+                $request = $request->withAttribute('csrfToken', $this->saltToken($cookieData));
+            } catch (RuntimeException $e) {
+                $cookieData = null;
+            }
         }
 
         if ($method === 'GET' && $cookieData === null) {
@@ -277,6 +281,10 @@ class CsrfProtectionMiddleware implements MiddlewareInterface
             return $token;
         }
         $decoded = base64_decode($token, true);
+        if ($decoded === false) {
+            throw new RuntimeException('Invalid token data.');
+        }
+
         $length = strlen($decoded);
         $salt = Security::randomBytes($length);
         $salted = '';

+ 41 - 1
tests/TestCase/Http/Middleware/CsrfProtectionMiddlewareTest.php

@@ -171,6 +171,27 @@ class CsrfProtectionMiddlewareTest extends TestCase
     }
 
     /**
+     * Test that the CSRF tokens are regenerated when token is not valid
+     *
+     * @return void
+     */
+    public function testRegenerateTokenOnGetWithInvalidData()
+    {
+        $request = new ServerRequest([
+            'environment' => [
+                'REQUEST_METHOD' => 'GET',
+            ],
+            'cookies' => ['csrfToken' => "\x20\x26"],
+        ]);
+
+        $middleware = new CsrfProtectionMiddleware();
+        /** @var \Cake\Http\Response $response */
+        $response = $middleware->process($request, $this->_getRequestHandler());
+        $this->assertInstanceOf(Response::class, $response);
+        $this->assertGreaterThan(32, strlen($response->getCookie('csrfToken')['value']));
+    }
+
+    /**
      * Test that the CSRF tokens are set for redirect responses
      *
      * @return void
@@ -373,13 +394,32 @@ class CsrfProtectionMiddlewareTest extends TestCase
     }
 
     /**
+     * Test that invalid string cookies are rejected.
+     *
+     * @return void
+     */
+    public function testInvalidTokenStringCookies()
+    {
+        $this->expectException(InvalidCsrfTokenException::class);
+        $request = new ServerRequest([
+            'environment' => [
+                'REQUEST_METHOD' => 'POST',
+            ],
+            'post' => ['_csrfToken' => ["\x20\x26"]],
+            'cookies' => ['csrfToken' => ["\x20\x26"]],
+        ]);
+        $middleware = new CsrfProtectionMiddleware();
+        $middleware->process($request, $this->_getRequestHandler());
+    }
+
+    /**
      * Test that request non string cookies are ignored.
      *
      * @return void
      */
     public function testInvalidTokenNonStringCookies()
     {
-        $this->expectException(\Cake\Http\Exception\InvalidCsrfTokenException::class);
+        $this->expectException(InvalidCsrfTokenException::class);
         $request = new ServerRequest([
             'environment' => [
                 'REQUEST_METHOD' => 'POST',