Fix the implementation of BlowFish to solve bugs concerning NUL bytes truncating strings

[LamentXU123]

Now, since including \0 in password_hash is not allowed in PASSWORD_BCRYPT. It is reasonable to add checkers that, makes input that includes \0 in password_verify directly fails.

This PR align password_verify() with password_hash() for bcrypt by rejecting passwords containing NUL bytes before verification.

Because bcrypt treats passwords as NUL-terminated C strings, this avoids ambiguous verification behavior for inputs such as "foo\0bar". A regression test is included.

Fix#21673

To be short: password encrypted by BYCRYPT should never includes \0. So directly rejecting it is always correct. It also avoids ambiguous verification where something like this happens:

<?php$hash = password_hash("secret", PASSWORD_BCRYPT);
var_dump(password_verify("secret" . chr(0) . "suffix", $hash)); //True, WTF??>

[LamentXU123]

nit: I also refactor this by the way.

[alecpl]

Shouldn't that be rather done in php_password_bcrypt_verify()?

[LamentXU123]

Shouldn't that be rather done in php_password_bcrypt_verify()?

It is before, see the initial fix in 6f14bee

Actually I think that'd be better. Anyway, lets wait for code owners.

[Girgias]

Can we not fix the blowfish implementation? It seems like it requires providing the length of the string all the way down to BF_set_key. Or does BlowFish specifically not support null bytes?

[LamentXU123]

Does BlowFish specifically not support null bytes?

Blowfish do support null bytes, but in php implementation it specifically doesn't. Any password with NUL will fail when hashing by using password_hash in bcrypt mode.

bugs with NUL bytes is inevitable since PHP is based on C string. But in this case, I think directly rejecting all inputs containing NUL bytes will be safer and always correct.

[Girgias]

Does BlowFish specifically not support null bytes?

Blowfish do support null bytes, but in php implementation it specifically doesn't. Any password with NUL will fail when hashing by using password_hash in bcrypt mode.

bugs with NUL bytes is inevitable since PHP is based on C string. But in this case, I think directly rejecting all inputs containing NUL bytes will be safer and always correct.

Sure, but we "own" the implementation of BF, so why not actually fix the implementation? I'm happy to reject them as a bug fix that is backported, but in the long run we should fix the implementation.

[LamentXU123]

Does BlowFish specifically not support null bytes?

Blowfish do support null bytes, but in php implementation it specifically doesn't. Any password with NUL will fail when hashing by using password_hash in bcrypt mode.
bugs with NUL bytes is inevitable since PHP is based on C string. But in this case, I think directly rejecting all inputs containing NUL bytes will be safer and always correct.

Sure, but we "own" the implementation of BF, so why not actually fix the implementation? I'm happy to reject them as a bug fix that is backported, but in the long run we should fix the implementation.

In the long run we should. But for now, password_hash does not accept password containing NUL, so fixing the implementation would create a BC break on this which requires a RFC. I would be happy to write such RFC in the future but I think this bug could be fix before we get it passed.

[Girgias]

I don't think fixing the implementation to be correct is a BC break.
Your bug fix is a BC break by this logic...

[LamentXU123]

I don't think fixing the implementation to be correct is a BC break.

That'd be good. I will work on this hours later.

I assume that the best practice here would be passing the correct length throughout the implementation so the strings will not be cut by NUL

[Girgias]

I don't think fixing the implementation to be correct is a BC break.

That'd be good. I will work on this hours later.

I assume that the best practice here would be passing the correct length throughout the implementation so the strings will not be cut by NUL

Correct :)

[Girgias]

We may want these to be Z_PARAM_PATH if we use the system crypt. Or Maybe it should indeed always be PATH so that the behaviour is consistent with the system API...

[LamentXU123]

We may want these to be Z_PARAM_PATH

I implement the bcrypt() to reject NUL byte, and make password_* not to.

Not sure about this change.

[Girgias]

Yeah, not sure how I feel about this either. Maybe @TimWolla has an opinion?

[TimWolla]

This file is a vendored library and must not be touched.

[TimWolla]

Or does BlowFish specifically not support null bytes?

It's not BlowFish, it's Bcrypt. And no, most Bcrypt implementation don't support NULs.

[ndossche]

The idiotic thing about how PHP implements password hashing is that it doesn't always use the builtin bcrypt implementation: it can defer to crypt_r which doesn't support NUL bytes. So if you want to fix this you'll need to ditch that other code path but that runs into political issues about "bundling too much code".

[bukka]

I guess the idea was that crypt implementation would be more secure but not sure if there's much value in it.

[bukka]

I'm also not sure if there's much value in allowing nul bytes in password. I think the initial idea of not allowing in the verify was more reasonable. That was sort of the plan when we were handling GHSA-h746-cjrr-wfmr - prohibit the hashing as low security issue and add warning for verify in master.

[LamentXU123]

I don't have strong opinions on both solutions, but I would revert back to the initial fix I guess if we are following GHSA-h746-cjrr-wfmr

plus: Is this issue worth a separate security entry? I mean that password_verify doesn't check if the input has NUL bytes could have low impact on some login scenarios.

[TimWolla]

I mean that password_verify doesn't check if the input has NUL bytes could have low impact on some login scenarios.

Please explain the impact you're seeing. I'm seeing none and I don't think this change is necessary either. For password_hash it's useful to not accidentally generate “weak hashes”, but for password_verify rejecting NULs will just break some working logic without any upside.

[LamentXU123]

I mean that password_verify doesn't check if the input has NUL bytes could have low impact on some login scenarios.

Please explain the impact you're seeing. I'm seeing none and I don't think this change is necessary either. For password_hash it's useful to not accidentally generate “weak hashes”, but for password_verify rejecting NULs will just break some working logic without any upside.

I dont think it will break "working logic". As I have said, originally, password containing NUL doesn't work right in password_verify, and, it is rejected in password_hash. In this case, rejecting it is always correct.

The impact is, we can avoid something like

<?php
$hash = password_hash("secret", PASSWORD_BCRYPT);
var_dump(password_verify("secret" . chr(0) . "suffix", $hash)); //True, WTF?
?>

While not touching existing logic. I really dont think the above behavior is intentional.

Password containing NUL byte in original will cause ambiguous verification, so the original code doesn't treat it correctly.

[Sjord]

Looks good to me.

I can imagine someone creating bcrypt hashes of strings with null bytes outside of PHP, and then verifying them with password_verify. We cannot reliably verify those, and returning false is a more secure result than checking a subset of the password.

However, this will be pretty rare. Users don't typically put null bytes in their password. bcrypt, password_hash and password_verify are specifically for user passwords and documented as such, not general cryptographic functions for random data. So returning false for null bytes seems reasonable.