Zend\Crypt\Password\Bcrypt does not report inability to generate hash

[Ocramius]

As of PHP 5.3.7, the hashing algorithm has changed.

Zend\Crypt\Password\Bcrypt::create($password) does indeed report that using PHP < 5.3.7 is dangerous, but Zend\Crypt\Password\Bcrypt::verify($password, $hash) just silently fails when a newer hash is compared on an older PHP installation.

That's a very sneaky problem that occurs when you generate the hashes on (for example) a deployment machine and then use them on a production machine with a lower PHP version: all tests pass except the fact that hashes cannot be verified, resulting in all users being basically unable to login (in my case).

A possible fix is to insert an exception in Zend\Crypt\Password\Bcrypt::verify($password, $hash) that looks for the $2a$ prefix in the hash, and to suggest PHP > 5.3.7 in zendframework/zend-crypt with a short explanation of the problem.

Ping @ezimuel @ircmaxell

[ezimuel]

I see the your point here and I think the workaround that you suggested is correct. We need to check in Zend\Crypt\Password\Bcrypt::verify() if the hash start with $2y$ and in that case if PHP < 5.3.7 throw an exception suggesting to migrate to PHP >= 5.3.7 in order to verify the hash. The $2a$ format is supported in all the versions of PHP.

[ircmaxell]

I've been thinking about this for a while, and I think the only way to do this would be to test another hash... So, in the verify function:

if ($hash === crypt($pw, $hash)) {
    return true;
}
if ($testHash !== crypt("test", $testHash)) {
    throw new \LogicException("Could not test hash");
}
return false;

The basic problem is referenced in this issue. Checking version number isn't reliable (thanks Linux Distro maintainers). Therefore, you'd have to run a test (at cost 4, the lowest you can) to prove that it can be checked to distinguish between a false-negative and a negative.

Ideally this should never be an issue, and as such should never even be a problem. But even more so, it should never become a problem in production (it should be identified a lot sooner). But I can see how some people would want that functionality. Just that to me, it's more of a hack around problems in other areas (deployment sanity, etc)...

[ezimuel]

I forgot to mention that we have the setBackwardCompatibility() method of Bcrypt that helps to generate hash password using the old format $2a$. If you use a dev environment with PHP >= 5.3.7 and you must deploy in a production environment with PHP < 5.3.7, I suggest to set the setBackwardCompatibility to true before you generate the hash.

[Ocramius]

@ezimuel shouldn't that happen by default?

[ezimuel]

@Ocramius The goal is to offer the best security by default, the $2y$ is a fingerprint that guarantee that the password has been generated without the possible bug reported here: http://php.net/security/crypt_blowfish.php. That said I don't think it's a good idea to setBackwardComaptibility() to true by default. From a security point of view, if you care about backward compatible, you need to set it explicit.

[ircmaxell]

Well, you "could" set it to true when php < 5.3.6... So ->setBackwardCompatibility(version_compare(PHP_VERSION, '5.3.6', '<')); or something like that (a conditional based on the version). Which would allow those users of CentOS/RHEL to turn it back off, but the usual use-case would have it on by default... Not sure though....

[Ocramius]

@ezimuel agreed - a secure default is surely the way to go.

I'm wondering if double-checking a dummy hash would be acceptable: I'd rather see an exception (or something logged) than spending hours wondering why things don't work.
A warning works too for me, since it doesn't stop the application from working (without aggressive error handlers).

I don't think there is a performance impact for those using <=5.3.6 - thoughts?

@ircmaxell I agree this should never be a problem, but in fact it is, because we work with so many different environments all together (not just me)

Here's the pseudo-code I'd like to see after seeing what @ircmaxell suggested:

if ($hash === crypt($password, $hash)) {
    return true;
}

if ($testHash !== crypt("yadda!", $testHash)) {
    trigger_error("Your php version yadda yadda yadda", E_USER_WARNING);
}

return false;

[ezimuel]

I just executed some tests on different PHP versions and I found that in a generated hash the $2y$ flag can be replaced with $2a$ for backward compatibility with PHP < 5.3.7. I come out with this solution for the Bcrypt::verify method:

public function verify($password, $hash)                                                                                         
{                                                                                                                                
    if ($hash === crypt($password, $hash)) {                                                                                     
        return true;                                                                                                             
    }                                                                                                                            
    if (substr($hash, 0, 3) === '$2y' && version_compare(PHP_VERSION, '5.3.7', '<')) {                                           
        $hash = str_replace('$2y$', '$2a$', $hash);                                                                              
        trigger_error("Please upgrade to PHP 5.3.7+ to be sure to use a secure version of crypt()", E_USER_WARNING);
        return ($hash === crypt($password, $hash)); 
    }                                                                                                                            
    return false;                                                               
} 

I think that in this way we can solve the issue that @Ocramius reported.
What do you think?

[Ocramius]

@ezimuel looks good, but what is the str_replace part about? Is PHP still able to compute the correct hash?

[ircmaxell]

You can't reliably just replace it. You would need to check for high bytes in the passwords. And if you find one reject it.

The short of it is you shouldn't try to run new hashes on old versions (that is the definition of a problem that must be avoided)... In other words: throw an exception that the hash is too new and die. Don't try to validate it or you'll open other vectors...

[ezimuel]

The str_replace works if the original password did not contain characters with more than 7 bit. I tried using 3v4l.org and it works: http://3v4l.org/nMsYb

[Bittarman]

I agree with @ircmaxell here, in the vast majority of cases, people will not be using mixed versions of php with a given set of hashes, as people tend to ensure the same version all the way across their environment.

Ryan Mauger

On Wednesday, 5 June 2013 at 16:48, Anthony Ferrara wrote:

You can't reliably just replace it. You would need to check for high bytes in the passwords. And if you find one reject it.
The short of it is you shouldn't try to run new hashes on old versions (that is the definition of a problem that must be avoided)... In other words: throw an exception that the hash is too new and die. Don't try to validate it or you'll open other vectors...

—
Reply to this email directly or view it on GitHub (#4538 (comment)).

[ezimuel]

@ircmaxell but it will fails always, in this case we are just trying to recover the valid passwords just using the old flag for crypt(). @Bittarman I know, this is just an idea to prevent the issue reported by @Ocramius.

[Ocramius]

We don't live in a perfect world, as the fact that we have a silent failure on < 5.3.7 demonstrates. I don't see anything negative in throwing a warning in a discouraged PHP version in such an edge case.

[ezimuel]

I proposed the PR #4607 to solve the issue reported to @Ocramius . The user must explicit set the backwardCompatibility mode to true in order to get this feature. As @ircmaxell suggested this can represent a potential security problem with false positive due to the crypt() bug reported here: http://php.net/security/crypt_blowfish.php. This PR mitigate the issue suggesting to upgrade, generating a WARNING in any case.

[Maks3w]

Merged #4623 instead