ext/standard: notice on long passwords in bcrypt

[Sjord]

Bcrypt supports passwords up to 72 characters. The remainder is ignored.

Earlier proposed here:

• https://news-web.php.net/php.internals/75692
• https://wiki.php.net/rfc/password_hash_spec
• https://bugs.php.net/bug.php?id=67653

Bcrypt truncation can result in serious security bugs:

• https://pentesterlab.com/blog/freshrss-bcrypt-truncation-auth-bypass
• https://www.invicti.com/blog/web-security/okta-vulnerability-bcrypt-auth

[Sjord]

This could also apply to crypt. However, with crypt there is not a distinction between creating the hash and verifying the hash, as there is with password_hash / password_verify, which means that anyone that can access to login screen can trigger the notice by entering a long password. That makes the notice less meaningful and bigger chance of BC break.

[Sjord]

@nikic @narfbg @ircmaxell I am looking into this again, and you seemed to have objected last time. Do you feel the same now?

This is mostly for the case where some static string is prepended to the password before being passed to password_hash, and not so much for the case where users actually use 72 character passwords.

[narfbg]

Thanks for the ping, @Sjord.

I do still think that warnings would do more harm than good.

However, I might not be against a fail-closed approach where an exception is thrown. Not to say that I'd outright support that (the BC break can be too big a price to pay) but it would solve most of the problems I have with the warning idea.

[Sjord]

password_hash and password_verify are supposed to take a user's password. For both this is clear from the documentation. In that case, it is really rare and no problem if it's longer than 72 bytes.

However, in the FreshRSS bug (and more or less the Okta bug) mentioned above, they don't just supply the password, but prepend a nonce or a username. This is a bad idea and not a supported way of using password_hash/verify.

I think throwing an error is warranted. For FreshRSS this would mean severe BC break: their authentication method stops working when they upgrade their PHP version. Perhaps that's acceptable? It could be better than that users can bypass the authentication altogether. And if you use functions in undocumented and unsupported ways, you can expect BC break.

[Sjord]

We could add a new algorithm to password_hash:

• existing PASSWORD_BCRYPT silently truncates;
• new PASSWORD_BCRYPT_EX throws exception when password is longer than 72 characters.

Then at some point we make PASSWORD_BCRYPT_EX the default, but it would be easy to avoid BC break by specifying PASSWORD_BCRYPT algorithm.

[Sjord]

https://wiki.php.net/rfc/bcrypt_max_password_length

[Sjord]

This is on hold, at least until we make a decision on PASSWORD_BCRYPT_SHA256.