👍

Note that hash_equals is constant time only for strings of the same length, so its use might not be appropriate here. See https://github.com/php/php-src/blob/PHP-5.6.0/ext/hash/hash.c#L731

You're right @aforsblo. I've just updated the PR to use hash_equals only if strings have the same size.

@dunglas I'm afraid that won't help. Now the strings will be compared by the interpreted implementation of the algorithm if the lengths differ, and by the presumably much faster native implementation if they are of the same length, thus potentially enabling an attacker to figure out the length of the secret string.

I agree with @aforsblo, this is also warned in the description - "Compares two strings using the same time whether they're equal or not. A difference in length will leak"

Ah this method is for comparing hashes of the same algorithm (thus same length).

Closing as this is not appropriate.

Maybe can we add a C implementation of the algorithm used in Symfony in php.net?

@fabpot @aforsblo I've submitted a PR to php.net: #11802 (comment) / http://marc.info/?l=php-internals&m=140948282903040&w=2

From the comments in this PR, on the PHP internals list and on the Python issue tracker (http://bugs.python.org/issue15061) it seems that this is almost impossible to be sure that an advanced attacker cannot recover the length. Maybe should we add a warning about that in the PHPDoc of this implementation?

It is impossible in the general case to prevent length information leakage. See this comment where I go into more detail.

Symfony's current setup does indeed leak length information today. To understand why, let's talk about large strings. CPUs use caches to speed up and prevent needing to fetch from main memory (RAM). We can determine the length of our known string by watching the cache miss pattern. If the known string is 10 bytes, then by passing in a 100mb string, we know that known never cache misses due to the execution time (cache misses are quite expensive). So we are able to deduce length information even though the function is "timing safe".

And that doesn't even approach the problem that it's PHP code that's executing, meaning that there are branches all over the place. Making things like % not timing safe.

So while the current equals function is quite good, it's definitely not perfect. This is why many systems have moved to Double HMAC Verification which isn't cheap, but it's significantly stronger than iterating. But it's also quite expensive.

The C implementation of hash_equals() is demonstrably better, in that it never leaks content information. And it doesn't pretend to not leak length information (which is impossible to do in the general case anyway).

So instead, I'd strongly recommend to always use the hash_equals() function, even if the lengths are different. Instead of relying on length not being leaked (it is being leaked anyway), design systems so that length is not a secret.

Thank you @ircmaxell for sharing your insights, you have convinced me that using hash_equals() here makes sense. Still, while I agree that the length shouldn't need to be kept secret, the risk of length leakage needs to be clearly documented.

@dunglas @fabpot I'm now in favor of merging the original version of this PR (without the length check), with the addition of a documentation warning about the length leakage.

@aforsblo @fabpot I will reopen the PR when #11808 will be merged. The original PR doesn't work when arguments' types aren't strings (I propose casting them to string to avoid a BC break).

As mentioned in php/php-src#792 (comment), HHVM tries hiding length information while PHP.net doesn't. I agree to update the PHPDoc of StringUtils::equals making clear that a difference in length will leak, even with the old plain PHP implementation and HHVM.

Well, my main issue is tries to hide is not hiding. So even HHVMs version will leak. It's just not as obvious, and much more subtle. But the leak is most definitely still there (and a savy attacker will be able to exploit it).