Browse Source

Add missing type checks to SecurityComponent and CSRF middleware.

Port changes in #14680 to 4.x, and update FormProtector as it replaces
SecurityComponent.
Mark Story 5 years ago
parent
commit
1f30a17a64

+ 3 - 0
src/Controller/Component/SecurityComponent.php

@@ -260,6 +260,9 @@ class SecurityComponent extends Component
         if (!isset($check['_Token']['fields'])) {
             throw new AuthSecurityException(sprintf($message, '_Token.fields'));
         }
+        if (!is_string($check['_Token']['fields'])) {
+            throw new AuthSecurityException("'_Token.fields' was invalid.");
+        }
         if (!isset($check['_Token']['unlocked'])) {
             throw new AuthSecurityException(sprintf($message, '_Token.unlocked'));
         }

+ 5 - 0
src/Form/FormProtector.php

@@ -237,6 +237,11 @@ class FormProtector
 
             return null;
         }
+        if (!is_string($formData['_Token']['fields'])) {
+            $this->debugMessage = '`_Token.fields` was invalid.';
+
+            return null;
+        }
         if (!isset($formData['_Token']['unlocked'])) {
             $this->debugMessage = sprintf($message, '_Token.unlocked');
 

+ 3 - 3
src/Http/Middleware/CsrfProtectionMiddleware.php

@@ -117,7 +117,7 @@ class CsrfProtectionMiddleware implements MiddlewareInterface
         $cookies = $request->getCookieParams();
         $cookieData = Hash::get($cookies, $this->_config['cookieName']);
 
-        if ($cookieData !== null && strlen($cookieData) > 0) {
+        if (is_string($cookieData) && strlen($cookieData) > 0) {
             $request = $request->withAttribute('csrfToken', $cookieData);
         }
 
@@ -246,8 +246,8 @@ class CsrfProtectionMiddleware implements MiddlewareInterface
     {
         $cookie = Hash::get($request->getCookieParams(), $this->_config['cookieName']);
 
-        if (!$cookie) {
-            throw new InvalidCsrfTokenException(__d('cake', 'Missing CSRF cookie.'));
+        if (!$cookie || !is_string($cookie)) {
+            throw new InvalidCsrfTokenException(__d('cake', 'Missing or incorrect CSRF cookie type.'));
         }
 
         if (!$this->_verifyToken($cookie)) {

+ 27 - 2
tests/TestCase/Controller/Component/SecurityComponentTest.php

@@ -382,14 +382,14 @@ class SecurityComponentTest extends TestCase
     }
 
     /**
-     * testValidatePostFormHacking method
+     * testValidatePostFormTampering method
      *
      * Test that validatePost fails if any of its required fields are missing.
      *
      * @return void
      * @triggers Controller.startup $this->Controller
      */
-    public function testValidatePostFormHacking(): void
+    public function testValidatePostFormTampering(): void
     {
         $event = new Event('Controller.startup', $this->Controller);
         $this->Security->startup($event);
@@ -888,6 +888,7 @@ class SecurityComponentTest extends TestCase
         ]));
         Configure::write('debug', false);
         $result = $this->validatePost('SecurityException', 'The request has been black-holed');
+        $this->assertFalse($result);
     }
 
     /**
@@ -928,6 +929,30 @@ class SecurityComponentTest extends TestCase
     }
 
     /**
+     * Test that invalid types cause failures.
+     *
+     * @return void
+     */
+    public function testValidatePostFailArrayData()
+    {
+        $event = new Event('Controller.startup', $this->Controller);
+        $this->Security->startup($event);
+        $this->Controller->setRequest($this->Controller->getRequest()->withParsedBody([
+            'Model' => [
+                'username' => 'mark',
+                'password' => 'sekret',
+            ],
+            '_Token' => [
+                'fields' => [],
+                'unlocked' => '',
+            ],
+        ]));
+        Configure::write('debug', false);
+        $result = $this->validatePost('SecurityException', "'_Token.fields' was invalid.");
+        $this->assertFalse($result);
+    }
+
+    /**
      * testValidateHiddenMultipleModel method
      *
      * @return void

+ 19 - 0
tests/TestCase/Form/FormProtectorTest.php

@@ -136,6 +136,25 @@ class FormProtectorTest extends TestCase
     }
 
     /**
+     * testValidate array fields method
+     *
+     * Test that validate fails if empty form is submitted.
+     *
+     * @return void
+     */
+    public function testValidateInvalidFields(): void
+    {
+        $data = [
+            '_Token' => [
+                'debug' => '',
+                'unlocked' => '',
+                'fields' => [],
+            ],
+        ];
+        $this->validate($data, '`_Token.fields` was invalid.');
+    }
+
+    /**
      * testValidateObjectDeserialize
      *
      * Test that objects can't be passed into the serialized string. This was a vector for RFI and LFI

+ 19 - 0
tests/TestCase/Http/Middleware/CsrfProtectionMiddlewareTest.php

@@ -250,6 +250,25 @@ class CsrfProtectionMiddlewareTest extends TestCase
     }
 
     /**
+     * Test that request non string cookies are ignored.
+     *
+     * @return void
+     */
+    public function testInvalidTokenNonStringCookies()
+    {
+        $this->expectException(\Cake\Http\Exception\InvalidCsrfTokenException::class);
+        $request = new ServerRequest([
+            'environment' => [
+                'REQUEST_METHOD' => 'POST',
+            ],
+            'post' => ['_csrfToken' => ['nope']],
+            'cookies' => ['csrfToken' => ['nope']],
+        ]);
+        $middleware = new CsrfProtectionMiddleware();
+        $middleware->process($request, $this->_getRequestHandler());
+    }
+
+    /**
      * Test that request data works with the various http methods.
      *
      * @dataProvider httpMethodProvider