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?
| return false; | ||
| } | ||
|
|
||
| if (ZSTR_LEN(buffer) != out_len || ZSTR_VAL(buffer)[out_len] == '=') { |
There was a problem hiding this comment.
Why the check for = here? The code already passses PHP_BASE64_NO_PADDING above, is that not reliable?
There was a problem hiding this comment.
It should be reliable if nothing changes, but it's also an assumption. This guards against coding errors elsewhere remaining undetected.
|
|
||
| /* bcrypt implementation */ | ||
|
|
||
| static bool php_password_bcrypt_salt_to64(const char *in, const size_t in_len, const size_t out_len, char *out) /* {{{ */ |
There was a problem hiding this comment.
The order of these parameters is unusual. Also, it seems only fixed values are accepted for in_len and out_len, so is it necessary to pass them at all?
There was a problem hiding this comment.
Not obvious from the diff, but this is in part just renaming the old method and preserving its signature. It was already weird, and I kind of liked having the 2 sizes be adjacent on the caller side - easier to debug.
Having the lengths even if they're fixed is primarily for easier bug detection. I'm split on whether it's better to do away with them, but the output one in particular is annoying indeed.
There was a problem hiding this comment.
I can't detect the lengths without them being passed in, so there's that.
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.
| default: break; | ||
| } | ||
|
|
||
| out[out_len] = 0; |
There was a problem hiding this comment.
This fixes the overread, but I like the API even less now. out_len is clearly not the length of *out, or this would write beyond the end of the buffer. So it makes even less sense to check whether out_len == 22.
There was a problem hiding this comment.
Huh? in[in_len] would also be the null byte. Both lengths exclude termination, just like ZSTR_LEN() and virtually everything else.
This is just for extra safety btw, I've modified the printf pattern to write exactly 22 bytes no matter what, so the overflow would be fixed with or without this assignment.
2 participants
Uh oh!
There was an error while loading. Please reload this page.