December 16th, 2013, 06:26 PM
Warning: Several bugs in PHP Password Hashing extension and password_compat library
both the PHP Password Hashing extension and the password_compat library from the same author have several bugs.
Since many people (including myself) have recommended the extension or the library, I think you should be aware of those issues.
Both the extension and the library do not handle binary salts correctly. Binary means: You haven't encoded them with the special Base64 variant used by crypt(). If you pass a binary salt through the $options parameter, you will always get a wrong hash. This can easily be verified with other bcrypt libraries like PHPass.
For example, hashing the password foo with a salt consisting of 16 null bytes should yield the hash
Instead, you get
If you do encode the salt but make a mistake, the extension and the library will silently double-encode the salt, which reduces the effective length by ~1/4.
Also note that the documentation incorrectly states that the functions will return false on error. They may also return null.
The password_compat library has two additional bugs:
It is incompatible with mbstring.func_overload. If you're using this feature together with a multibyte encoding, the functions won't be able to measure the length of strings. This causes them to fail and throw nonsensical error messages.
If you're using open_basedir without /dev/urandom being included (like on a Windows system), and you neither have the OpenSSL nor the Mcrypt extension, you will get a warning for every call of password_hash().
December 17th, 2013, 09:48 AM
December 17th, 2013, 07:07 PM
if the bugs broke the security of the library, I wouldn't have posted them in public. So, no, they don't. And I didn't claim they do.
However, you are wrong about the effects of the bugs:
The incorrect Base64 encoding affects the auto-generated salt as well. The only reason why we don't see it breaking the hash is because the current bcrypt implementation by Solar Designer happens to repair the last salt digit. This is pure luck. Without this internal correction, we would in fact get nonsensical hashes.
So this is not just a problem of custom salts. In fact, the only way to circumvent the wrong encoding is to provide a custom Base64-encoded salt. In that case, it will be passed directly to crypt().
But even if the bug only affected custom salts, how would that make things better? I mean, if you don't want the users to provide a custom salt at all, you shouldn't have this option in the first place. But providing it and then blaming people for using it is kind of weird.
You claim that the mbstring.func_overload bug will only affect custom salts. This is wrong. Even a common encoding like UTF-16 will break both password_get_info() and password_needs_rehash() completely:
Regarding the open_basedir warning, I don't really get why you make things so complicated. PHP throws a nonsensical warning, so we suppress it and maybe generate our own warning. That's it.
// a valid bcrypt salt with a cost factor of 10
$hash = '$2y$10$......................0li5vIK0lccG/IXHAOP2wBncDW/oa2u';
$info = password_get_info($hash);
var_dump( $info ); // this yields "unknown algorithm", which is incorrect
// compare the hash with the exact same parameters
$needs_rehash = password_needs_rehash($hash, PASSWORD_BCRYPT, array('cost' => 10));
var_dump( $needs_rehash ); // this yields true, which is incorrect
I understand that the @ operator is often characterized as the ultimate evil, but this is nonsense. If used correctly, it's perfectly fine.
In fact, I'm pretty sure this is the only valid solution in this case. You talk about writing a workaround for Windows, but this is not a Windows issue. For example, OpenBSD also has a different concept of random number generators. They do maintain /dev/urandom for legacy reasons, but as far as I know, they actually only have /dev/random. Sounds like some really ugly OS detection.
And like I already said, some users may not have a chance to edit or remove the open_basedir setting. What are they supposed to do? Turn off the error reporting completely? Always write @password_hash() only to make the function shut up?
Last edited by Jacques1; December 17th, 2013 at 07:25 PM.
January 11th, 2014, 07:34 AM
short update: Most of the issues mentioned above are fixed now in the password_compat library. If you're using it, you should get the current version from GitHub.
The changes in particular:
- the salt encoding is fixed
- the compatibility issues with the mbstring extension are fixed
- no risk of a dangerous fallback to MD5-crypt
- no open_basedir warnings on systems which do not have /dev/urandom