Update secure_tools.rst #4787
Update secure_tools.rst #4787
Conversation
|
This method can still be used to protect the information about the content of strings when comparing them, even though it does not protect the length. Your update of the title will let people think they should compared the length first, which is not the case (and will also leak information). |
|
What about changing the title to "comparing hash" (like the PHP functions). Hashes have by definition the same length. |
|
Or we can copy the statement of PHP.net/hash_equals:
|
|
Updated the wording according to @stof comments and copied warning from php.net. |
| @@ -18,13 +18,18 @@ algorithm; you can use the same strategy in your own code thanks to the | |||
|
|
|||
| use Symfony\Component\Security\Core\Util\StringUtils; | |||
|
|
|||
| // is some known string (e.g. password) equal to some user input? | |||
| $bool = StringUtils::equals($knownString, $userInput); | |||
| // is some known string (e.g. password hash) equal to another string (e.g. hash of some user input)? | |||
There was a problem hiding this comment.
Can you please wrap the comment on two lines?
|
done |
| // is some known string (e.g. password) equal to some user input? | ||
| $bool = StringUtils::equals($knownString, $userInput); | ||
| // is some known string (e.g. password hash) equal to another string | ||
| // (e.g. hash ofsome user input)? |
There was a problem hiding this comment.
looks like a typo here: "ofsome" should probably be "of some"
|
ping @sarciszewski @ircmaxell It would be nice if you could have a look here. |
^^ That's not true. No timing attack exists on the content of the strings. The length leak isn't really anything to worry about because the length is leaked *anyway. If length is critical to security, there's another problem in your setup. Because you can never depend on length not being leaked (in the general case at least). |
|
@ircmaxell thanks for your comment. If I understand you correctly, you would propose to remove "[...] and the length of the known string may be leaked in case of a timing attack."? |
|
@ircmaxell Can you confirm that we properly understood your suggestion? |
|
@ircmaxell I'm sorry to ping you again on this issue, but can you please confirm if we understood your comment? I don't want to merge something that's wrong according to the security experts :) |
|
I question the entire premise of the PR? Initially it was to document that different lengths leak timing info. When shown that was not the case, what is this PR intending to do? The original PR's description is incorrect. And hashing things isn't a magical fix. The hash operations are dependent upon the length, and as such the length will be leaked. Hence what's the point of this PR? |
|
Don't mind this PR I'll close it. But is the current doc OK? |
|
I don't see any issues about it. |
|
Thank you @ircmaxell |
Fix a critical mistake in the
secure_toolsdoc that can lead to security vulnerabilities.Using
StringUtils::equals()with strings of different lengths can lead to critical timing attacks.See the following PR for the technical background:
php/php-src#792
symfony/symfony#11822