Fix the implementation of BlowFish to solve bugs concerning NUL bytes truncating strings#21675
Fix the implementation of BlowFish to solve bugs concerning NUL bytes truncating strings#21675LamentXU123 wants to merge 4 commits intophp:masterfrom
Conversation
ext/standard/password.c
Outdated
| zend_long cost = PHP_PASSWORD_BCRYPT_COST; | ||
|
|
||
| if (memchr(ZSTR_VAL(password), '\0', ZSTR_LEN(password))) { | ||
| if (zend_str_has_nul_byte(password)) { |
There was a problem hiding this comment.
nit: I also refactor this by the way.
|
Shouldn't that be rather done in |
It is before, see the initial fix in 6f14bee Actually I think that'd be better. Anyway, lets wait for code owners. |
|
Can we not fix the blowfish implementation? It seems like it requires providing the length of the string all the way down to |
Blowfish do support null bytes, but in php implementation it specifically doesn't. Any password with NUL will fail when hashing by using 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. |
|
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 :) |
LamentXU123
changed the title
| } | ||
| /* }}} */ | ||
|
|
||
| PHPAPI zend_string *php_crypt(const char *password, const int pass_len, const char *salt, int salt_len, bool quiet) |
There was a problem hiding this comment.
Might make sense to fix the pass_len param to be size_t
| if (j) | ||
| sign |= tmp[1] & 0x80; | ||
| if (!*ptr) | ||
| ptr = key; | ||
| else | ||
| ptr++; | ||
| key_pos++; | ||
| if (key_pos > key_len) | ||
| key_pos = 0; |
There was a problem hiding this comment.
I know this is existing but we usually try to wrap if conditions in {}
| if (crypt("foo\0bar", $setting) === crypt("foo", $setting)) { | ||
| die("skip bcrypt backend truncates NUL bytes"); | ||
| } |
There was a problem hiding this comment.
Do we ever not rely on our own implementation of crypt? I feel like at one point we decided to always use the custom PHP one... but I may be misremembering.
4 participants
Now, since including \0 in
password_hashis not allowed in PASSWORD_BCRYPT. It is reasonable to add checkers that, makes input that includes \0 inpassword_verifydirectly fails.This PR align
password_verify()withpassword_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: