php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #66293 Password Hashing extension returns incorrect hashes for binary salts
Submitted: 2013-12-14 01:05 UTC Modified: 2017-10-24 06:56 UTC
Votes:1
Avg. Score:1.0 ± 0.0
Reproduced:1 of 1 (100.0%)
Same Version:0 (0.0%)
Same OS:0 (0.0%)
From: jan at zahlen-kern dot de Assigned:
Status: Open Package: *Encryption and hash functions
PHP Version: 5.5Git-2013-12-13 (snap) OS: Windows 7
Private report: No CVE-ID: None
View Add Comment Developer Edit
Anyone can comment on a bug. Have a simpler test case? Does it work for you on a different platform? Let us know!
Just going to say 'Me too!'? Don't clutter the database with that please — but make sure to vote on the bug!
Your email address:
MUST BE VALID
Solve the problem:
37 - 31 = ?
Subscribe to this entry?

 
 [2013-12-14 01:05 UTC] 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"

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2014-01-04 09:42 UTC] ab@php.net
-Status: Open +Status: Feedback
 [2014-01-04 09:42 UTC] 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?
 [2014-01-04 11:14 UTC] jan at zahlen-kern dot de
-Status: Feedback +Status: Open
 [2014-01-04 11:14 UTC] 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-06 08:55 UTC] 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-03-06 16:40 UTC] 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-03-07 01:52 UTC] 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-07 09:00 UTC] nikic@php.net
-Assigned To: +Assigned To: ircmaxell
 [2014-03-07 11:14 UTC] 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.
 [2017-10-24 06:56 UTC] kalle@php.net
-Status: Assigned +Status: Open -Assigned To: ircmaxell +Assigned To:
 
PHP Copyright © 2001-2019 The PHP Group
All rights reserved.
Last updated: Wed Nov 20 20:01:21 2019 UTC