Bug #66293 [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 User updated by: jan at zahlen-kern dot de Reported by: jan at zahlen-kern dot de Summary: Password Hashing extension returns incorrect hashes for binary salts Status: Open Type: Bug Package: *Encryption and hash functions Operating System: Windows 7 PHP Version: 5.5Git-2013-12-13 (snap) Block user comment: N Private report: N New Comment: 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. Previous Comments: ------------------------------------------------------------------------ [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). ------------------------------------------------------------------------ [2014-01-04 09:42:50] ab@php.net I think it's not correct using a nullbyted string as a salt. What's the result with some alnum salt string? ------------------------------------------------------------------------ [2013-12-14 01:05:20] jan at zahlen-kern dot de Description: ------------ The password_hash() function accepts raw salt strings through the $options parameter, but it fails to encode them correctly. As a result, the hash is different than it would be with common bcrypt. This is caused by an incorrect encoding procedure: The function applies standard Base64, removes the padding and replaces "+" with ".". This does yield the right alphabet, but in the Base64 variant used by bcrypt, the digits have different values: Common Base64 is ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/ bcrypt Base 64 is ./ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789 So the function effectively adds 2 modulo 64 to every salt byte. The last salt digit in the resulting hash is correct, though, because the current bcrypt implementation internally fixes it. Test script: --------------- <?php $salt = str_repeat("\x00", 22); echo password_hash('foo', PASSWORD_BCRYPT, array('salt' => $salt)); Expected result: ---------------- The resulting hash should be "$2y$10$......................0li5vIK0lccG/IXHAOP2wBncDW/oa2u" Actual result: -------------- Instead, it is "$2y$10$AAAAAAAAAAAAAAAAAAAAA.A6.6PO3Wv4OxvTWdUxMVpdLT2d5g/FG" ------------------------------------------------------------------------ -- Edit this bug report at https://bugs.php.net/bug.php?id=66293&edit=1