Repository navigation
Conversation
…ic logic - Fixes a bug where the libargon2 implementation uses less entropy than intended (still safe, but below baseline) - Aligned bcrypt salt encoding more closely to bcrypt's variant (previously, it relied on php_crypt() doing the error correction) - Stricter bcrypt sanity checks, and slightly optimized code
Sjord
left a comment
Sjord
left a comment
There was a problem hiding this comment.
Interesting bug. So the argon2 salt was base64-encoded twice?
Yes, and any new algorithm would inherit the flaw if unnoticed. That's how it came to affect argon2 - if you don't look closely at |
| return 0; | ||
| } | ||
|
|
||
| if (ZSTR_LEN(hash) < 13) { |
There was a problem hiding this comment.
FYI would've changed this to fixed length too, but evidently all non-recognized algos default here during password_verify() calls and it's breaking a test.
I want to fix that, so the default is its own thing and bcrypt stays isolated. Will do that separately.
|
I think the code can be made a little bit nicer, but functionally this looks good to me. |
|
Looks good to me! |
Originally designed with only bcrypt in mind and contributed to the argon2 salt bug. Today it only exists to emit the ignored salt warning and is only complicating things. Moved the warning trigger in PHP_FUNCTION(password_hash) to avoid a breaking change, since argon2 inherited it even though it never accepted a user-provided salt. This is a bug on its own, but it can be argued both ways so being safe with it. I reckon the warning should eventually be dropped entirely in a major release.
|
I think I've rooted-out everything that would've contributed to the argon2 bug and/or remnants from when user-provided salt was supported (i.e. there's no reason for it to be a Will limit myself to reviewer-requested changes only from here on, and ready to squash. |
2 participants
Uh oh!
There was an error while loading. Please reload this page.