Skip to content

Commit

Permalink
crypt: Fix validation of malformed BCrypt hashes
Browse files Browse the repository at this point in the history 
PHP’s implementation of crypt_blowfish differs from the upstream Openwall
version by adding a “PHP Hack”, which allows one to cut short the BCrypt salt
by including a `$` character within the characters that represent the salt.

Hashes that are affected by the “PHP Hack” may erroneously validate any
password as valid when used with `password_verify` and when comparing the
return value of `crypt()` against the input.

The PHP Hack exists since the first version of PHP’s own crypt_blowfish
implementation that was added in 1e820ec.

No clear reason is given for the PHP Hack’s existence. This commit removes it,
because BCrypt hashes containing a `$` character in their salt are not valid
BCrypt hashes.
  • Loading branch information
@TimWolla @smalyshev
TimWolla authored and smalyshev committed Feb 13, 2023
1 parent 255e08a commit c840f71
Show file tree
Hide file tree
Showing 2 changed files with 82 additions and 8 deletions.
8 changes: 0 additions & 8 deletions ext/standard/crypt_blowfish.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,6 @@ static const unsigned char BF_atoi64[0x60] = {
#define BF_safe_atoi64(dst, src) \
{ \
tmp = (unsigned char)(src); \
if (tmp == '$') break; /* PHP hack */ \
if ((unsigned int)(tmp -= 0x20) >= 0x60) return -1; \
tmp = BF_atoi64[tmp]; \
if (tmp > 63) return -1; \
Expand Down Expand Up @@ -399,13 +398,6 @@ static int BF_decode(BF_word *dst, const char *src, int size)
*dptr++ = ((c3 & 0x03) << 6) | c4;
} while (dptr < end);

if (end - dptr == size) {
return -1;
}

while (dptr < end) /* PHP hack */
*dptr++ = 0;

return 0;
}

Expand Down
82 changes: 82 additions & 0 deletions ext/standard/tests/crypt/bcrypt_salt_dollar.phpt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
--TEST--
bcrypt correctly rejects salts containing $
--FILE--
<?php
for ($i = 0; $i < 23; $i++) {
$salt = '$2y$04$' . str_repeat('0', $i) . '$';
$result = crypt("foo", $salt);
var_dump($salt);
var_dump($result);
var_dump($result === $salt);
}
?>
--EXPECT--
string(8) "$2y$04$$"
string(2) "*0"
bool(false)
string(9) "$2y$04$0$"
string(2) "*0"
bool(false)
string(10) "$2y$04$00$"
string(2) "*0"
bool(false)
string(11) "$2y$04$000$"
string(2) "*0"
bool(false)
string(12) "$2y$04$0000$"
string(2) "*0"
bool(false)
string(13) "$2y$04$00000$"
string(2) "*0"
bool(false)
string(14) "$2y$04$000000$"
string(2) "*0"
bool(false)
string(15) "$2y$04$0000000$"
string(2) "*0"
bool(false)
string(16) "$2y$04$00000000$"
string(2) "*0"
bool(false)
string(17) "$2y$04$000000000$"
string(2) "*0"
bool(false)
string(18) "$2y$04$0000000000$"
string(2) "*0"
bool(false)
string(19) "$2y$04$00000000000$"
string(2) "*0"
bool(false)
string(20) "$2y$04$000000000000$"
string(2) "*0"
bool(false)
string(21) "$2y$04$0000000000000$"
string(2) "*0"
bool(false)
string(22) "$2y$04$00000000000000$"
string(2) "*0"
bool(false)
string(23) "$2y$04$000000000000000$"
string(2) "*0"
bool(false)
string(24) "$2y$04$0000000000000000$"
string(2) "*0"
bool(false)
string(25) "$2y$04$00000000000000000$"
string(2) "*0"
bool(false)
string(26) "$2y$04$000000000000000000$"
string(2) "*0"
bool(false)
string(27) "$2y$04$0000000000000000000$"
string(2) "*0"
bool(false)
string(28) "$2y$04$00000000000000000000$"
string(2) "*0"
bool(false)
string(29) "$2y$04$000000000000000000000$"
string(2) "*0"
bool(false)
string(30) "$2y$04$0000000000000000000000$"
string(60) "$2y$04$000000000000000000000u2a2UpVexIt9k3FMJeAVr3c04F5tcI8K"
bool(false)

14 comments on commit c840f71

@solardiz
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to see the hack finally removed. Some historical context I could dig up, where I suggested removing the hack in 2009:

https://news-web.php.net/php.internals/44200
https://news-web.php.net/php.internals/44201
https://news-web.php.net/php.internals/44206

@solardiz
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, quick testing suggests that the hack wasn't yet a vulnerability when this code (apparently from Suhosin) was first introduced in PHP 5.3+, but became exposed as a vulnerability via other changes in 5.5+. Versions 5.3.x and 5.4.x appear to either compute classic Unix descrypt hashes (13 chars) for those invalid salt strings (using $2 as the salt) or pad the bcrypt salts to proper length with $ chars. I did not check where in the code the '$' padding was, I just observed the behavior - maybe there was more of a hack in those versions, whereas that part of the original hack was dropped in an update for 5.5+, making the hack internally inconsistent.

@syntaxseed
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised that there was not more discussion of the backward breaking fallout of this change.

This now renders passwords that were hashed with this 'hack' un-reproduceable. This means that projects who have this problem cannot validate the old hashes before sending out password reset emails (ie done when the user logs in instead of in bulk).

@solardiz
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@syntaxseed What are those projects?

Wouldn't they previously "erroneously validate any password as valid" (from this commit's description) on PHP 5.5+ and until this commit? If so, that's not acceptable backwards compatibility behavior. Maybe, if many/popular projects are affected, PHP could reintroduce pre-5.5 behavior, but would this really help those projects (I suppose it would only if those hashes were generated on 5.3+ pre-5.5 as well, which is a rather narrow range of PHP versions)?

Can you show an example affected hash and its corresponding password?

@syntaxseed
Copy link

@syntaxseed syntaxseed commented on c840f71 Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@syntaxseed What are those projects?

Wouldn't they previously "erroneously validate any password as valid" (from this commit's description) on PHP 5.5+ and until this commit? If so, that's not acceptable backwards compatibility behavior. Maybe, if many/popular projects are affected, PHP could reintroduce pre-5.5 behavior, but would this really help those projects (I suppose it would only if those hashes were generated on 5.3+ pre-5.5 as well, which is a rather narrow range of PHP versions)?

Can you show an example affected hash and its corresponding password?

I can't say much about versions 5.x, but I have an 8.0 project that had legacy users with hashes generated with crypt(). This suddenly stopped working for these users (newer users have password hashes using the password_hash() function).

They didn't erroneously validate if using crypt() to properly compare against the stored hash.

People who followed an example years ago (and still) found on W3Schools to use blowfish with a 21 character salt followed by a $, the hash is reproduced properly in version 8.0.27, but in 8.0.28 and later it generates the *0 error result.

I can't be the only one with a project that used the W3Schools example and was working fine on 8.0.27 and prior.

See example: https://onlinephp.io/c/a7d05

@syntaxseed
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the "erroneously validate" concern applies to situations using the password_validate() method. If an app was just using crypt() on the submitted password and comparing to the stored hash... then *0 would obviously not match the stored hash.

@solardiz
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with a 21 character salt followed by a $, the hash is reproduced properly in version 8.0.27, but in 8.0.28 and later it generates the *0 error result.

See example: https://onlinephp.io/c/a7d05

Testing this at that link, I see that if you shorter the salt further to 19 characters or less, the returned string is just the prefix+salt (no hash), so any password would be "valid".

I believe the "erroneously validate" concern applies to situations using the password_validate() method.

Per the above, no, it would also apply to crypt(), but apparently only for even shorter salts.

If an app was just using crypt() on the submitted password and comparing to the stored hash... then *0 would obviously not match the stored hash.

If crypt() were to return *0, then a valid password would not validate as well - which is precisely the backwards compatibility problem you're facing.

Updating your test further to:

<?php
echo (crypt('testing', '$2a$10$78b367f8d5feaa9b34b61$')) . "\n"; // 21 char salt
echo (crypt('testing', '$2a$10$78b367f8d5feaa9b34b61.laVwoQ7KOfxRpkp7Vf5DOHt8AeO7wZC'));

Result for 8.0.27:

$2a$10$78b367f8d5feaa9b34b61.laVwoQ7KOfxRpkp7Vf5DOHt8AeO7wZC
$2a$10$78b367f8d5feaa9b34b61.W5.ENlgA1S0ccDJhHflb.UFCCyD3lI6

Result for 8.0.28:

*0
$2a$10$78b367f8d5feaa9b34b61.W5.ENlgA1S0ccDJhHflb.UFCCyD3lI6

I see that the hash produced by the first crypt() call in it wouldn't validate with the second call (where I put the first hash output by the first call on 8.0.27) - the second call returns a different hash even on 8.0.27. So this wouldn't have worked right on 8.0.27 anyway.

@solardiz
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

People who followed an example years ago (and still) found on W3Schools to use blowfish with a 21 character salt followed by a $

The example I currently see there has 22 characters before the $, and works the same on 8.0.27 and 8.0.28:

echo "Blowfish: ".crypt('something','$2a$09$anexamplestringforsalt$')."\n<br>";
Blowfish: $2a$09$anexamplestringforsaleLouKejcjRlExmf1671qw3Khl49R3dfu

@syntaxseed
Copy link

@syntaxseed syntaxseed commented on c840f71 Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@solardiz ... I'm not sure what more to say. I have a live project with a database full of password hashes that look like this:

$2a$10$78b367f8d5feaa9b34b61.JKGar4N5ibBXe6lVjdtsLATzqp3UAR.

That used to validate fine and now they don't. And this is the only change I can find in this version that is even remotely connected.

The legacy code parses out the 21 character salt (not including the .) and appends a dollar sign on the end.

It's a relief though if the W3Schools example is fine then hopefully this isn't widespread. 🤞

Thankfully my particular problem resulted in legacy users becoming unable to log in... rather than anyone being able to log in with any password!

@solardiz
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The legacy code parses out the 21 character salt (not including the .) and appends a dollar sign on the end.

Maybe you should simply remove that legacy code? Let the full 60-character hash string be passed into crypt().

@solardiz
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you should simply remove that legacy code? Let the full 60-character hash string be passed into crypt().

Actually, while such legacy code was wrong and shouldn't have ever been needed, simply dropping it now isn't expected to make those old hashes work for you, unfortunately - that's based on my test with 8.0.27 above, where a different hash was produced for the truncated salt. So perhaps your hashes in the DB are similarly different. I'm sorry I don't have a good workaround for you.

@solardiz
Copy link

@solardiz solardiz commented on c840f71 Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry I don't have a good workaround for you.

Or maybe I do. The early break; for $ caused a full byte of the salt value to become 0, not just the last character (which encodes only 2 bits). Which is why simply replacing the one $ with a . doesn't (always) produce the same hash. However, if you also replace the preceding character with a ., it does! That's because the last two salt characters together exactly correspond to 1 byte (6+2 bits), we got lucky there (this exact matching saved us from the need to calculate the replacement character for this workaround, making it always .).

echo (crypt('testing', '$2a$10$78b367f8d5feaa9b34b61$')) . "\n"; // 21 char salt
echo (crypt('testing', '$2a$10$78b367f8d5feaa9b34b6..')); // 22 char corrected salt

Result for 8.0.27:

$2a$10$78b367f8d5feaa9b34b61.laVwoQ7KOfxRpkp7Vf5DOHt8AeO7wZC
$2a$10$78b367f8d5feaa9b34b6..laVwoQ7KOfxRpkp7Vf5DOHt8AeO7wZC

Result for 8.0.28:

*0
$2a$10$78b367f8d5feaa9b34b6..laVwoQ7KOfxRpkp7Vf5DOHt8AeO7wZC

https://onlinephp.io?s=s7EvyCjg5UpNzshX0Eguqiwo0VAvSS0uycxLV9dRUFcxSlQxNFAxt0gyNjNPs0gxTUtNTLRMMjZJMjNUUdfUVNBTUIrJU7JW0NdXMDJUSM5ILFIoTswpIctEPT2giRCjjCBGJecXFaUml6SmgA0FAA%2C%2C&v=8.0.28%2C8.0.27

@syntaxseed
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe I do. The early break; for $ caused a full byte of the salt value to become 0, not just the last character (which encodes only 2 bits). Which is why simply replacing the one $ with a . doesn't (always) produce the same hash. However, if you also replace the preceding character with a ., it does! That's because the last two salt characters together exactly correspond to 1 byte (6+2 bits), we got lucky there (this exact matching saved us from the need to calculate the replacement character for this workaround, making it always .).

Holy smokes @solardiz ... that's what I was trying to figure out! 🥇

Thank you so much, this means I can validate users BEFORE performing the password reset/rehash. 🥇

@solardiz
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much, this means I can validate users BEFORE performing the password reset/rehash.

You're welcome. This also means you don't need the password reset/rehash - you can update your hash encodings in the database (put the .. into them where needed) or keep the equivalent magic in code.

Please sign in to comment.