diff --git a/framework/CHANGELOG.md b/framework/CHANGELOG.md index 249c65a143..d7718b3617 100644 --- a/framework/CHANGELOG.md +++ b/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) diff --git a/framework/base/Security.php b/framework/base/Security.php index 0c955f40f8..2b78636f43 100644 --- a/framework/base/Security.php +++ b/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: - $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; } diff --git a/tests/unit/framework/base/SecurityTest.php b/tests/unit/framework/base/SecurityTest.php index eb4e1e7433..a807c8e747 100644 --- a/tests/unit/framework/base/SecurityTest.php +++ b/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