Upgrade crypt_blowfish to 1.3 #868
Conversation
3 of the missing tests fail. // TODO
| @@ -196,7 +196,6 @@ PHPAPI int php_crypt(const char *password, const int pass_len, const char *salt, | |||
| } else if ( | |||
| salt[0] == '$' && | |||
| salt[1] == '2' && | |||
| salt[2] >= 'a' && salt[2] <= 'z' && | |||
There was a problem hiding this comment.
This is handled in crypt_blowfish.c. Removed to prevent fallback to DES when an invalid bcrypt revision is specified.
|
looks like it should go to 5.4. |
|
@smalyshev Hey Stas, is there any reason this wasn't merged up to 5.5, 5.6 and 7.0? (I noticed it wasn't in NEWS for todays releases, and a quick look at the tags show the code is not there either.) |
|
Ah, looks like it's in PHP 5.x.next, guess this release was mainly bugfix + CVE |
| @@ -196,7 +196,6 @@ PHPAPI int php_crypt(const char *password, const int pass_len, const char *salt, | |||
| } else if ( | |||
| salt[0] == '$' && | |||
| salt[1] == '2' && | |||
| salt[2] >= 'a' && salt[2] <= 'z' && | |||
| salt[3] == '$' && | |||
| salt[4] >= '0' && salt[4] <= '3' && | |||
| salt[5] >= '0' && salt[5] <= '9' && | |||
There was a problem hiding this comment.
Shouldn't this be removed and handled in crypt_blowfish.c as well? Now if you supply a cost of 44 it will fall back to DES rather than erroring in bcrypt.
There was a problem hiding this comment.
I agree, I think anything starting '$2?$' (maybe even '$2' .. not sure) should be handed off to crypt_blowfish.c and handled there.
I would like an opinion on whether this is a security fix, which affects it being merged to 5.4
Version 1.3 addresses: OpenBSD bcrypt 8-bit key_len wraparound
I have received and followed some advice from Solar Designer on implementing this change.
During implementation I added some tests that were missing from the official test suite, which actually failed. I have made some further changes to address these failures, and while all of the other existing tests still pass, I would again like an opinion on whether this is considered a BC break, or a bug fix. (I feel it is the latter)
Thanks.