Merge pull request #4474 from tom--/4469-compareString-without-shortcut

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

@@ -610,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:
-        $length = StringHelper::byteLength($expected);
-        if ($length !== StringHelper::byteLength($actual)) {
-            return false;
-        }
-        $diff = 0;
-        for ($i = 0; $i < $length; $i++) {
-            $diff |= (ord($actual[$i]) ^ ord($expected[$i]));
+        $expected .= "\0";
+        $actual .= "\0";
+        $expectedLength = StringHelper::byteLength($expected);
+        $actualLength = StringHelper::byteLength($actual);
+        $diff = $expectedLength - $actualLength;
+        for ($i = 0; $i < $actualLength; $i++) {
+            $diff |= (ord($actual[$i]) ^ ord($expected[$i % $expectedLength]));
         }
         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));
+    }
 }
\ No newline at end of file