From dd937a0db4ea897ee2c8d9a93197505679f7a476 Mon Sep 17 00:00:00 2001
From: Mark Story <mark@mark-story.com>
Date: Thu, 19 Apr 2018 22:01:47 -0400
Subject: [PATCH] Make CSRF token comparisions time constant.

Felix Wiedemann and Nils Rokita contacted us about a possible timing
side channel in CSRF token comparisons. This issue can be mitigated by
using the constant time checks in Security. Exposing this method as
a public method allows us and application developers to use it as
needed.
---
 src/Controller/Component/CsrfComponent.php    |  2 +-
 .../Middleware/CsrfProtectionMiddleware.php   |  2 +-
 src/Utility/Security.php                      | 22 ++++++++-------
 tests/TestCase/Utility/SecurityTest.php       | 27 +++++++++++++++++++
 4 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/src/Controller/Component/CsrfComponent.php b/src/Controller/Component/CsrfComponent.php
index 1d880b27221..e43455710b7 100644
--- a/src/Controller/Component/CsrfComponent.php
+++ b/src/Controller/Component/CsrfComponent.php
@@ -157,7 +157,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
index 70a4d18afcc..5414b1d61e0 100644
--- 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
index 2169199d498..1918d85194f 100644
--- a/src/Utility/Security.php
+++ b/src/Utility/Security.php
@@ -277,7 +277,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;
         }
 
@@ -289,24 +289,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
index c783f4dc920..8767f678b31 100644
--- a/tests/TestCase/Utility/SecurityTest.php
+++ b/tests/TestCase/Utility/SecurityTest.php
@@ -334,4 +334,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));
+    }
 }