Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add compatibility for PHP 5.6's hash_equals function #61

Closed

Conversation

timemachine3030
Copy link

  • Updated verify_password to use new function
  • Tests to cover warnings and use cases.

 - Updated `verify_password` to use new function
 - Tests to cover warnings and use cases.
@timemachine3030
Copy link
Author

There are a lot of suggestions popping up around the web to patch this compatibility. Many of them are not timing safe (they return too soon if the strings are of different length). As your password_compat repo has been the authority for the 5.5 updates I wanted to add a correct implementation to it for the new 5.6 function.

@@ -222,16 +222,65 @@ function password_verify($password, $hash) {
return false;
}
$ret = crypt($password, $hash);
if (!is_string($ret) || PasswordCompat\binary\_strlen($ret) != PasswordCompat\binary\_strlen($hash) || PasswordCompat\binary\_strlen($ret) <= 13) {
if (!is_string($hash) || !is_string($ret)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice PasswordCompat\binary\_strlen($ret) <= 13 omitted in your revision.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure the purpose of the <= 13 check. I checked the php source code and the comparison is there as well so I pushed a change to add that in.

- Matches `ext/standard/password.c` from the php source.
- Comparing the ASCII value of characters.
@ircmaxell
Copy link
Owner

Closing as -1.

This library only supports 5.3.7 - 5.4.latest. It does not support 5.5+, and most definitely not 5.6, as it will disable itself.

So the only reason to add support for hash_equals would be cosmetic. And considering it would add complexity, -1 from me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants