Merge pull request #11972 from cakephp/issue-csrf

Make CSRF token comparisions time constant.
cakephp · Apr 21, 2018 · d4bcd8b · d4bcd8b
2 parents 7f97eaf + 87bab29
commit d4bcd8b
Show file tree

Hide file tree

Showing 4 changed files with 42 additions and 11 deletions.
diff --git a/src/Controller/Component/CsrfComponent.php b/src/Controller/Component/CsrfComponent.php
@@ -162,7 +162,7 @@ protected function _validateToken(ServerRequest $request)
             throw new InvalidCsrfTokenException(__d('cake', 'Missing CSRF token cookie'));
         }
 
-        if ($post !== $cookie && $header !== $cookie) {
+        if (!Security::constantEquals($post, $cookie) && !Security::constantEquals($header, $cookie)) {
             throw new InvalidCsrfTokenException(__d('cake', 'CSRF token mismatch.'));
         }
     }

diff --git a/src/Http/Middleware/CsrfProtectionMiddleware.php b/src/Http/Middleware/CsrfProtectionMiddleware.php
@@ -190,7 +190,7 @@ protected function _validateToken(ServerRequest $request)
             throw new InvalidCsrfTokenException(__d('cake', 'Missing CSRF token cookie'));
         }
 
-        if ($post !== $cookie && $header !== $cookie) {
+        if (!Security::constantEquals($post, $cookie) && !Security::constantEquals($header, $cookie)) {
             throw new InvalidCsrfTokenException(__d('cake', 'CSRF token mismatch.'));
         }
     }

diff --git a/src/Utility/Security.php b/src/Utility/Security.php
@@ -301,7 +301,7 @@ public static function decrypt($cipher, $key, $hmacSalt = null)
         $cipher = mb_substr($cipher, $macSize, null, '8bit');
 
         $compareHmac = hash_hmac('sha256', $cipher, $key);
-        if (!static::_constantEquals($hmac, $compareHmac)) {
+        if (!static::constantEquals($hmac, $compareHmac)) {
             return false;
         }
 
@@ -313,24 +313,28 @@ public static function decrypt($cipher, $key, $hmacSalt = null)
     /**
      * A timing attack resistant comparison that prefers native PHP implementations.
      *
-     * @param string $hmac The hmac from the ciphertext being decrypted.
-     * @param string $compare The comparison hmac.
+     * @param string $original The original value.
+     * @param string $compare The comparison value.
      * @return bool
      * @see https://github.com/resonantcore/php-future/
+     * @since 3.6.2
      */
-    protected static function _constantEquals($hmac, $compare)
+    public static function constantEquals($original, $compare)
     {
+        if (!is_string($original) || !is_string($compare)) {
+            return false;
+        }
         if (function_exists('hash_equals')) {
-            return hash_equals($hmac, $compare);
+            return hash_equals($original, $compare);
         }
-        $hashLength = mb_strlen($hmac, '8bit');
+        $originalLength = mb_strlen($original, '8bit');
         $compareLength = mb_strlen($compare, '8bit');
-        if ($hashLength !== $compareLength) {
+        if ($originalLength !== $compareLength) {
             return false;
         }
         $result = 0;
-        for ($i = 0; $i < $hashLength; $i++) {
-            $result |= (ord($hmac[$i]) ^ ord($compare[$i]));
+        for ($i = 0; $i < $originalLength; $i++) {
+            $result |= (ord($original[$i]) ^ ord($compare[$i]));
         }
 
         return $result === 0;

diff --git a/tests/TestCase/Utility/SecurityTest.php b/tests/TestCase/Utility/SecurityTest.php
@@ -365,4 +365,31 @@ public function testInsecureRandomBytes()
 
         $this->assertRegExp('/[^0-9a-f]/', $value, 'should return a binary string');
     }
+
+    /**
+     * test constantEquals
+     *
+     * @return void
+     */
+    public function testConstantEquals()
+    {
+        $this->assertFalse(Security::constantEquals('abcde', null));
+        $this->assertFalse(Security::constantEquals('abcde', false));
+        $this->assertFalse(Security::constantEquals('abcde', true));
+        $this->assertFalse(Security::constantEquals('abcde', 1));
+
+        $this->assertFalse(Security::constantEquals(null, 'abcde'));
+        $this->assertFalse(Security::constantEquals(false, 'abcde'));
+        $this->assertFalse(Security::constantEquals(1, 'abcde'));
+        $this->assertFalse(Security::constantEquals(true, 'abcde'));
+
+        $this->assertTrue(Security::constantEquals('abcde', 'abcde'));
+        $this->assertFalse(Security::constantEquals('abcdef', 'abcde'));
+        $this->assertFalse(Security::constantEquals('abcde', 'abcdef'));
+        $this->assertFalse(Security::constantEquals('a', 'abcdef'));
+
+        $snowman = "\xe2\x98\x83";
+        $this->assertTrue(Security::constantEquals($snowman, $snowman));
+        $this->assertFalse(Security::constantEquals(str_repeat($snowman, 3), $snowman));
+    }
 }