compareString(): timing depends only on length of $actual input, unit test

framework/CHANGELOG.md

@@ -73,6 +73,7 @@ Yii Framework 2 Change Log
 - Bug #4412: Formatter used SI Prefixes for base 1024, now uses binary prefixes (kmindi)
 - Bug #4427: Formatter could do one division too much (kmindi)
 - Bug #4453: `yii message/extract` wasn't properly writing to po files in case of multiple categories (samdark)
+- Bug #4469: Make `Security::compareString()` timing depend only on length of `$actual` input and add unit test. (tom--)
 - Bug: Fixed inconsistent return of `\yii\console\Application::runAction()` (samdark)
 - Bug: URL encoding for the route parameter added to `\yii\web\UrlManager` (klimov-paul)
 - Bug: Fixed the bug that requesting protected or private action methods would cause 500 error instead of 404 (qiangxue)

framework/base/Security.php

@@ -45,6 +45,7 @@ class Security extends Component
     public $passwordHashStrategy = 'crypt';
 
     /**
+     * Cipher algorithm for mcrypt module.
      * AES has 128-bit block size and three key sizes: 128, 192 and 256 bits.
      * mcrypt offers the Rijndael cipher with block sizes of 128, 192 and 256
      * bits but only the 128-bit Rijndael is standardized in AES.
@@ -52,9 +53,12 @@ class Security extends Component
      * chooses the appropriate AES based on the length of the supplied key.
      */
     const MCRYPT_CIPHER = 'rijndael-128';
+    /**
+     * Block cipher operation mode for mcrypt module.
+     */
     const MCRYPT_MODE = 'cbc';
     /**
-     * Same size for encryption keys, auth keys and KDF salt
+     * Size in bytes of encryption key, message authentication key and KDF salt.
      */
     const KEY_SIZE = 16;
     /**
@@ -62,11 +66,11 @@ class Security extends Component
      */
     const KDF_HASH = 'sha256';
     /**
-     * Hash algorithm for authentication.
+     * Hash algorithm for message authentication.
      */
     const MAC_HASH = 'sha256';
     /**
-     * HKDF info value for auth keys
+     * HKDF info value for derivation of message authentication key.
      */
     const AUTH_KEY_INFO = 'AuthorizationKey';
 
@@ -606,19 +610,18 @@ class Security extends Component
      * Performs string comparison using timing attack resistant approach.
      * @see http://codereview.stackexchange.com/questions/13512
      * @param string $expected string to compare.
-     * @param string $actual string to compare.
+     * @param string $actual user-supplied string.
      * @return boolean whether strings are equal.
      */
     public function compareString($expected, $actual)
     {
-        // timing attack resistant approach:
+        $expected .= "\0";
-        $length = StringHelper::byteLength($expected);
+        $actual .= "\0";
-        if ($length !== StringHelper::byteLength($actual)) {
+        $expectedLength = StringHelper::byteLength($expected);
-            return false;
+        $actualLength = StringHelper::byteLength($actual);
-        }
+        $diff = $expectedLength - $actualLength;
-        $diff = 0;
+        for ($i = 0; $i < $actualLength; $i++) {
-        for ($i = 0; $i < $length; $i++) {
+            $diff |= (ord($actual[$i]) ^ ord($expected[$i % $expectedLength]));
-            $diff |= (ord($actual[$i]) ^ ord($expected[$i]));
         }
         return $diff === 0;
     }

tests/unit/framework/base/SecurityTest.php

@@ -312,4 +312,41 @@ class SecurityTest extends TestCase
         $dk = $this->security->hkdf($hash, hex2bin($ikm), hex2bin($salt), hex2bin($info), $l);
         $this->assertEquals($okm, bin2hex($dk));
     }
+
+    public function dataProviderCompareStrings()
+    {
+        return [
+            ["", ""],
+            [false, ""],
+            [null, ""],
+            [0, ""],
+            [0.00, ""],
+            ["", null],
+            ["", false],
+            ["", 0],
+            ["", "\0"],
+            ["\0", ""],
+            ["\0", "\0"],
+            ["0", "\0"],
+            [0, "\0"],
+            ["user", "User"],
+            ["password", "password"],
+            ["password", "passwordpassword"],
+            ["password1", "password"],
+            ["password", "password2"],
+            ["", "password"],
+            ["password", ""],
+        ];
+    }
+
+    /**
+     * @dataProvider dataProviderCompareStrings
+     *
+     * @param $expected
+     * @param $actual
+     */
+    public function testCompareStrings($expected, $actual)
+    {
+        $this->assertEquals(strcmp($expected, $actual) === 0, $this->security->compareString($expected, $actual));
+    }
 }