Bug #66293 [Asn->Opn]: Password Hashing extension returns incorrect hashes for binary salts

Edit report at https://bugs.php.net/bug.php?id=66293&edit=1 ID: 66293 Updated by: kalle@php.net Reported by: jan at zahlen-kern dot de Summary: Password Hashing extension returns incorrect hashes for binary salts -Status: Assigned +Status: Open Type: Bug Package: *Encryption and hash functions Operating System: Windows 7 PHP Version: 5.5Git-2013-12-13 (snap) -Assigned To: ircmaxell +Assigned To: Block user comment: N Private report: N Previous Comments: ------------------------------------------------------------------------ [2014-03-07 11:14:15] narf at devilix dot net I didn't say it's not a bug, although some people could argue about that. I just pointed out that _because_ it's practically irrelevant, fixing it doesn't break backwards compatibility. ------------------------------------------------------------------------ [2014-03-07 01:52:11] jan at zahlen-kern dot de No need to repeat the old "It's a feature, not a bug" discussion. This is a bug, and it has already been fixed in the compatibility library: https://github.com/ircmaxell/password_compat/commit/fbbfdebebc4b35d203bca7ec650d0f66f15d2fd9 I'm sure it's only a matter of time when it will be fixed in PHP as well. ------------------------------------------------------------------------ [2014-03-06 16:40:26] narf at devilix dot net There wouldn't be any BC break if the encoding is changed, nor would there be any practical issue if it's left as is. The way I see it, current behavior is just inconsistent with other implementations, but that isn't even a "portability" issue because the desired effect is achieved - a random salt matching [a-zA-Z0-9./] ------------------------------------------------------------------------ [2014-01-06 08:55:13] ab@php.net As for me, an invalid salt should cause zero output and warning. Any hidden error correction is confusing and isn't worth it. That's however probably some BC breach as the erroneous behavior is known, lets see what Anthony says. ------------------------------------------------------------------------ [2014-01-04 11:14:30] jan at zahlen-kern dot de It's not a problem of pre-encoded salts. If the salt only contains Base64 digits (which is the case for alphanumeric strings), it will be passed straight to crypt(). The bug occurs when the library tries to convert a binary salt into Base64. This happens in two places: -- If the salt is generated automatically -- If you specify a custom salt which does not only contain Base64 digits The first case isn't visible from the outside. While the library does pass an "impossible" salt to crypt(), the current bcrypt implementation fixes this. That's why we don't see the bug in the resulting hash. Without this error correction, we would in fact get nonsensical hashes at all times. The second case can be verified with any salt that does not only contain Base64 digits. For example: $password = 'foo'; $cost = 10; $salt = str_repeat("\x01", 22); // we actually only need 16 bytes $hash = password_hash($password, PASSWORD_BCRYPT, array('salt' => $salt)); Expected result: "$2y$10$.OC/.OC/.OC/.OC/.OC/.OgAsrO3gl0RHNcFPE3BIzZ1Bz0qZsc6K" Actual result: "$2y$10$AQEBAQEBAQEBAQEBAQEBAO7r8cBYh78G83ccYofvR5QAV0LN00wTu" Also note that the last salt digit is "O" instead of "Q". This is due to the error correction. The author of the extension has already acknowledged the wrong encoding in the context of the password_compat library (which is a PHP implementation of the API). ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=66293 -- Edit this bug report at https://bugs.php.net/bug.php?id=66293&edit=1