[Security] Use hash_equals for constant-time string comparison (again) #11822
Conversation
|
👍 |
| @@ -35,6 +36,10 @@ private function __construct() {} | |||
| */ | |||
| public static function equals($knownString, $userInput) | |||
| { | |||
| if (function_exists('hash_equals')) { | |||
| return hash_equals((string) $knownString, (string) $userInput); | |||
There was a problem hiding this comment.
no need to cast to string since according to phpdoc only strings are accepted by equals
There was a problem hiding this comment.
If you don't cast, the C implementation will throw a warning: https://github.com/php/php-src/blob/master/ext/hash/hash.c#L736
The PHP implementation doesn't throw anything. The cast is mandatory to avoid BC break.
There was a problem hiding this comment.
Btw, I've added tests for that: #11808
There was a problem hiding this comment.
Well the tests you did are wrong. You pass null and int to a function that excepts a string. Of course the behavior is undefined then.
There was a problem hiding this comment.
The current PHP implementation implicitly casts inputs to string: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Core/Util/StringUtils.php#L39
If you don't add the cast, the behavior of the function will change (throw warning, was not throwing anything before). This matter especially if arguments are objects implementing the __toString() method.
There was a problem hiding this comment.
Then why did you add tests for something that is not supported?
There was a problem hiding this comment.
To be sure that this patch (and future work on this method) behave in a similar fashion than the previous implementation. Especially because it has been asked to merge it in 2.3.
There was a problem hiding this comment.
(And I don't know a way to indicate in a PHPDoc "string or an object implementing __toString()" and mixed is not appropriate because passing an array will throw a notice and an object without __toString() method will throw an error [http://dunglas.fr/2014/02/casting-php-types-to-string/]).
There was a problem hiding this comment.
Well ok the string cast might be good to be more fault-tolerant. I think it's just important to do so for good reasons. Otherwise we could add string casts in every method in symfony where strings are expected.
There was a problem hiding this comment.
Yes, this is only to keep consistency with the old behavior. Anyway, a new PHP project targeting PHP 5.6+ should directly use hash_equals to avoid overhead.
|
👍 |
f8b5ef6 to
03bd74b
Compare
|
Cast to string earlier. |
| array('', ''), | ||
| array(123, 123), | ||
| array(null, ''), | ||
| array(null, null), |
There was a problem hiding this comment.
I've just copied data of the php.net implementation. I'll add it.
|
Thank you @dunglas. |
…arison (again) (dunglas) This PR was merged into the 2.3 branch. Discussion ---------- [Security] Use hash_equals for constant-time string comparison (again) | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Use the `hash_equals` function (introduced in PHP 5.6) for timing attack safe string comparison when available. Add in the DocBlock that length will leak (#11797 (comment)). Commits ------- 3071557 [Security] Add more tests for StringUtils::equals 03bd74b [Security] Use hash_equals for constant-time string comparison
Use the
hash_equalsfunction (introduced in PHP 5.6) for timing attack safe string comparison when available.Add in the DocBlock that length will leak (#11797 (comment)).