7 ms·

PHP bug: Password_verify() always return true with some hash

43 points by dutchbrit 4y ago

mjburgess 4y ago
More info: https://github.com/php/php-src/security/advisories/GHSA-7fj2-8x79-rjf4 https://github.com/php/php-src/security/advisories/GHSA-7fj2... seems to be caused by a strange non-std php-specific crypto implementation
- cesarb 4y ago
  What I want to know, is why. > The “PHP Hack” exists since the very first version of PHP’s own crypt_blowfish implementation and no clear reasoning is given for its existence in the commentary or commit history.
  - TimWolla 4y ago
    Disclosure: I am not the author of the initial bug, but I investigated the issue together with another developer. I'm the author of the linked advisory and I've authored the commit that fixed the issue. As the advisory states I don't know about the why, but I have a suspicion. PHP initially didn't implement BCrypt itself, but delegated to the system crypt, making the behavior of crypt() system-dependent. Now the PHP manual for crypt() showcases this example: crypt('rasmuslerdorf', '$2a$07$usesomesillystringforsalt$'); which uses a horrible salt that incidentally ends with a dollar sign. I suspect to keep compatibility for users that thought the dollar sign would be necessary at the end of the salt, the “PHP Hack” was included. In fact such broken hashes appear to actually exist in the wild as showcased by this Stack Overflow question: https://stackoverflow.com/q/75519073/782822 https://stackoverflow.com/q/75519073/782822
mikehall314 4y ago
I'm actually quite surprised at the initial response by the PHP Core to this vulnerability. At very least I would have thought a sensible approach would be to fail securely - so if supplied with a bad hash you return false, not true!
- jefftk 4y ago
  It looks to me like it's trying to return false on a bad hash but missed one of the three ways crypt can signal failure (returning the input unmodified).
  - TimWolla 4y ago
    > but missed one of the three ways crypt can signal failure (returning the input unmodified) Returning the input unmodified is not failure, but success. That's how you check that a password is valid without having a specialized API.
    - jefftk 4y ago
      EDIT: I misread the linked post, ignore me! <strike>crypt is the hashing function, not the password checking function</strike>
      - TimWolla 4y ago
        I assume s/bit/not/. The checking function is “does the given password with the stored parameters hash to the same value as the stored hash”. Hash functions are deterministic. So returning the original hash for a valid password is the success case.
bawolff 4y ago
That's interesting, but i don't see how this can actually be leveraged into a vulnerability. But gosh, between this and the sha with null truncating bcrypt bug, php has had bad luck implementing password routines.
- Zr40 4y ago
  As described in the CVE[0]: > In PHP 8.0.X before 8.0.28, 8.1.X before 8.1.16 and 8.2.X before 8.2.3, password_verify() function may accept some invalid Blowfish hashes as valid. If such invalid hash ever ends up in the password database, it may lead to an application allowing any password for this entry as valid. [0] https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-0567 https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-0567
  - bawolff 4y ago
    > If such invalid hash ever ends up in the password database, it may lead to an application allowing any password for this entry as valid Again - how would that possibly be a security vulnerability? Like, its a really interesting security adjacent bug, but clearly not a security issue. If the attacker has the ability to set a hash, they can just set the hash to a known password.
    - brookst 4y ago
      One scenario would be setting another user’s password to the invalid hash so they can continue to log in with their existing password, and you can also log in without knowing that password.
      - donatj 4y ago
        That requires a database vulnerability to be exploited though, the average user can’t go setting hashes.
        masklinn 4y ago
jefftk 4y ago
password_verify was erroneously returning true for include hashes. In the most common case where the user gives you a password and you immediately hash it and call password_verify this doesn't come up, but any case where an attacker can influence the hash is at risk (ex: hashing on the client). The first comment on the bug report is pretty depressing.
- bawolff 4y ago
  I don't understand the scenario. You're saying this is bad if you hash on client side and compare to a plaintext password stored on the server? I mean, i suppose, but such a setup is so broken does it matter?
  - jefftk 4y ago
    I agree that this is broken, and loses much of the benefit of hashing. But this is the php ecosystem; I'd be surprised if no one was doing that!
jupp0r 4y ago
Still one of the best blog articles ever written: "PHP: a fractal of bad design" https://eev.ee/blog/2012/04/09/php-a-fractal-of-bad-design/ https://eev.ee/blog/2012/04/09/php-a-fractal-of-bad-design/
- smt88 4y ago
  I hate PHP, but that article is so old and outdated now that it's meaningless in this context. PHP is a completely different language with far more strict typing to opt into and more consistent stdlib APIs.
  - derrida 4y ago
    I hope it never changes. PHP reminds me of my socially awkward uncle who is endearing and means well and caused many cringes & me and my sister to laugh with his faux pas. I love reading articles about its bugs. The Mr Bean of programming languages. We need PHP just as the film industry needs its gaffer tape. It's just that we have a very good standard of Gaffer tape (357 or "perl" in programming). It is kinda expensive though in dollars or cognitive cycles. So sometimes we use the inferior stuff that is not the standard 357. But then, we find, after the film bumps out - it caused you to rip the paint off the floor in the venue the film hired and not get your $10,000k deposit back. We need to have our humour.
    - smt88 4y ago
      Honestly we don't need PHP, which we've proven because basically no one uses it for anything other than WordPress, Drupal, and (internally) Facebook now. Facebook may even have finished porting all their PHP to Hack and other languages. There will be a few HN commenters who say they still use Laravel or something, but in reality it's a tiny percentage of people using it now.
  - hiccuphippo 4y ago
    Most of what it says is still true today. Stdlib did add new functions and flags (which only add to the inconsistency) but most the ones the article mentions still have the same behavior today. Plus new issues as well. Something I found the other day, typed parameters won't error if you change the type, even with strict_types turned on: function foo(string $foo) { $foo = 10; }
hiccuphippo 4y ago
This issue is caused by a PHP specific modification to the crypt_blowfish implementation that is fittingly named “PHP hack”: php-src/ext/standard/crypt_blowfish.c Line 374 in 2740920 if (tmp == '$') break; /* PHP hack */ Cheeky hacker.
0xFEE1DEAD 4y ago
> I fail to see how this is a bug if you feed it garbage What an awful response to someone reporting a pretty serious security vulnerability.
- giancarlostoro 4y ago
  Then they proceed to explain why, but why in the world would you return true if the underlying function calls failed???
- bawolff 4y ago
  "Pretty serious" is a major overstatement. Regardless if you want to report a security bug you should include the impact. If you've never been on the other end of this - the number of false security reports with minimal details popular open source projects get are staggering. Many are unrealistic. People saying things like, if i give the attacker my password then the attacker has my password.
  - manuelabeledo 4y ago
    > Regardless if you want to report a security bug you should include the impact. Assigning impact, even only as a measure of risk, is not the responsibility of the reporter. A reporter must provide a fair description and a proof of concept. Calculating impact involves much more than that.
    - bawolff 4y ago
      I would disagree that it is a fair description without some description of impact. Hell, without impact, its not even a normal bug, let alone a security bug. The fundamentals of a bug report are "I expect X to happen when I do Y, but instead Z happens". Without impact, all you are saying is "I expect X to happen".
      - manuelabeledo 4y ago
        "Impact" is much more than just what happens when a vulnerability is exploited in a vacuum. That is the reason behind CVSS scores and incident response.
captn3m0 4y ago
The GitHub advisory is a better location to point this submission to: https://github.com/php/php-src/security/advisories/GHSA-7fj2-8x79-rjf4 https://github.com/php/php-src/security/advisories/GHSA-7fj2...
aldousd666 4y ago
Man, he tried so hard to defend that bug.
derrida 4y ago
For some reason when ever I've had to work with PHP which was last about 10 years ago I got giggles & "TNT" by ACDC would play in my head. I'm remembering why... P.H.P DYNAMITE
donatj 4y ago
Many comments here vastly overstate the seriousness of this. For it to be used as an attack, the attacker needs general write access to your database. You probably have much bigger issues on your hands if that is the case. If they have write access they can already just set the hash to a known value for the same result. It’s a bug certainly but of very little practical security concern.
- CGamesPlay 4y ago
  I think you're understating it. You're imagining a scenario where an attacker spears a specific row in the database. A much more likely failure mode is that a bungled database migration simply leaves the doors wide open: all users can now log in with any password.
  - donatj 4y ago
    Hmm… You make a very interesting point. I wonder how long it would take for someone to spot and report “I entered my password wrong and it worked anyway” I was thinking of an attacker with specific goals of accessing things who doesn’t want to be found out. Your hypothetical attacker would be more of a general chaos agent. I have certainly encountered far more of the latter.
- pytness 4y ago
  As @TimWolla commented somewhere else: https://stackoverflow.com/questions/75519073/password-verify-works-in-php-7-4-but-doesnt-work-in-php-8-2 https://stackoverflow.com/questions/75519073/password-verify...
  - TimWolla 4y ago
    Note: The hash in the SO question is not vulnerable, because it is too long. The issue exists specifically for hashes that both contain a `$` and are too short. The “too short” is necessary for the trailing NUL byte of the input to be copied into the output and thus to truncate early because of the `strlen()`.
mgrund 4y ago
Could also be a backdoor in combination with another yet-to-be-discovered bug…
deleted 4y ago
[deleted]

43 points by dutchbrit 4y ago

More info: https://github.com/php/php-src/security/advisories/GHSA-7fj2-8x79-rjf4 https://github.com/php/php-src/security/advisories/GHSA-7fj2... seems to be caused by a strange non-std php-specific crypto implementation

cesarb 4y ago
What I want to know, is why. > The “PHP Hack” exists since the very first version of PHP’s own crypt_blowfish implementation and no clear reasoning is given for its existence in the commentary or commit history.
- TimWolla 4y ago
  Disclosure: I am not the author of the initial bug, but I investigated the issue together with another developer. I'm the author of the linked advisory and I've authored the commit that fixed the issue. As the advisory states I don't know about the why, but I have a suspicion. PHP initially didn't implement BCrypt itself, but delegated to the system crypt, making the behavior of crypt() system-dependent. Now the PHP manual for crypt() showcases this example: crypt('rasmuslerdorf', '$2a$07$usesomesillystringforsalt$'); which uses a horrible salt that incidentally ends with a dollar sign. I suspect to keep compatibility for users that thought the dollar sign would be necessary at the end of the salt, the “PHP Hack” was included. In fact such broken hashes appear to actually exist in the wild as showcased by this Stack Overflow question: https://stackoverflow.com/q/75519073/782822 https://stackoverflow.com/q/75519073/782822

mikehall314 4y ago

I'm actually quite surprised at the initial response by the PHP Core to this vulnerability. At very least I would have thought a sensible approach would be to fail securely - so if supplied with a bad hash you return false, not true!

jefftk 4y ago
It looks to me like it's trying to return false on a bad hash but missed one of the three ways crypt can signal failure (returning the input unmodified).
- TimWolla 4y ago
  > but missed one of the three ways crypt can signal failure (returning the input unmodified) Returning the input unmodified is not failure, but success. That's how you check that a password is valid without having a specialized API.
  - jefftk 4y ago
    EDIT: I misread the linked post, ignore me! <strike>crypt is the hashing function, not the password checking function</strike>
    - TimWolla 4y ago
      I assume s/bit/not/. The checking function is “does the given password with the stored parameters hash to the same value as the stored hash”. Hash functions are deterministic. So returning the original hash for a valid password is the success case.

bawolff 4y ago

That's interesting, but i don't see how this can actually be leveraged into a vulnerability. But gosh, between this and the sha with null truncating bcrypt bug, php has had bad luck implementing password routines.

Zr40 4y ago
As described in the CVE[0]: > In PHP 8.0.X before 8.0.28, 8.1.X before 8.1.16 and 8.2.X before 8.2.3, password_verify() function may accept some invalid Blowfish hashes as valid. If such invalid hash ever ends up in the password database, it may lead to an application allowing any password for this entry as valid. [0] https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-0567 https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-0567
- bawolff 4y ago
  > If such invalid hash ever ends up in the password database, it may lead to an application allowing any password for this entry as valid Again - how would that possibly be a security vulnerability? Like, its a really interesting security adjacent bug, but clearly not a security issue. If the attacker has the ability to set a hash, they can just set the hash to a known password.
  - brookst 4y ago
    One scenario would be setting another user’s password to the invalid hash so they can continue to log in with their existing password, and you can also log in without knowing that password.
    - donatj 4y ago
      That requires a database vulnerability to be exploited though, the average user can’t go setting hashes.
      - masklinn 4y ago

jefftk 4y ago

password_verify was erroneously returning true for include hashes. In the most common case where the user gives you a password and you immediately hash it and call password_verify this doesn't come up, but any case where an attacker can influence the hash is at risk (ex: hashing on the client). The first comment on the bug report is pretty depressing.

bawolff 4y ago
I don't understand the scenario. You're saying this is bad if you hash on client side and compare to a plaintext password stored on the server? I mean, i suppose, but such a setup is so broken does it matter?
- jefftk 4y ago
  I agree that this is broken, and loses much of the benefit of hashing. But this is the php ecosystem; I'd be surprised if no one was doing that!

jupp0r 4y ago

Still one of the best blog articles ever written: "PHP: a fractal of bad design" https://eev.ee/blog/2012/04/09/php-a-fractal-of-bad-design/ https://eev.ee/blog/2012/04/09/php-a-fractal-of-bad-design/

smt88 4y ago
I hate PHP, but that article is so old and outdated now that it's meaningless in this context. PHP is a completely different language with far more strict typing to opt into and more consistent stdlib APIs.
- derrida 4y ago
  I hope it never changes. PHP reminds me of my socially awkward uncle who is endearing and means well and caused many cringes & me and my sister to laugh with his faux pas. I love reading articles about its bugs. The Mr Bean of programming languages. We need PHP just as the film industry needs its gaffer tape. It's just that we have a very good standard of Gaffer tape (357 or "perl" in programming). It is kinda expensive though in dollars or cognitive cycles. So sometimes we use the inferior stuff that is not the standard 357. But then, we find, after the film bumps out - it caused you to rip the paint off the floor in the venue the film hired and not get your $10,000k deposit back. We need to have our humour.
  - smt88 4y ago
    Honestly we don't need PHP, which we've proven because basically no one uses it for anything other than WordPress, Drupal, and (internally) Facebook now. Facebook may even have finished porting all their PHP to Hack and other languages. There will be a few HN commenters who say they still use Laravel or something, but in reality it's a tiny percentage of people using it now.
- hiccuphippo 4y ago
  Most of what it says is still true today. Stdlib did add new functions and flags (which only add to the inconsistency) but most the ones the article mentions still have the same behavior today. Plus new issues as well. Something I found the other day, typed parameters won't error if you change the type, even with strict_types turned on: function foo(string $foo) { $foo = 10; }

hiccuphippo 4y ago

This issue is caused by a PHP specific modification to the crypt_blowfish implementation that is fittingly named “PHP hack”: php-src/ext/standard/crypt_blowfish.c Line 374 in 2740920 if (tmp == '$') break; /* PHP hack */ Cheeky hacker.

0xFEE1DEAD 4y ago

> I fail to see how this is a bug if you feed it garbage What an awful response to someone reporting a pretty serious security vulnerability.

giancarlostoro 4y ago
Then they proceed to explain why, but why in the world would you return true if the underlying function calls failed???
bawolff 4y ago
"Pretty serious" is a major overstatement. Regardless if you want to report a security bug you should include the impact. If you've never been on the other end of this - the number of false security reports with minimal details popular open source projects get are staggering. Many are unrealistic. People saying things like, if i give the attacker my password then the attacker has my password.
- manuelabeledo 4y ago
  > Regardless if you want to report a security bug you should include the impact. Assigning impact, even only as a measure of risk, is not the responsibility of the reporter. A reporter must provide a fair description and a proof of concept. Calculating impact involves much more than that.
  - bawolff 4y ago
    I would disagree that it is a fair description without some description of impact. Hell, without impact, its not even a normal bug, let alone a security bug. The fundamentals of a bug report are "I expect X to happen when I do Y, but instead Z happens". Without impact, all you are saying is "I expect X to happen".
    - manuelabeledo 4y ago
      "Impact" is much more than just what happens when a vulnerability is exploited in a vacuum. That is the reason behind CVSS scores and incident response.

captn3m0 4y ago

The GitHub advisory is a better location to point this submission to: https://github.com/php/php-src/security/advisories/GHSA-7fj2-8x79-rjf4 https://github.com/php/php-src/security/advisories/GHSA-7fj2...

aldousd666 4y ago

Man, he tried so hard to defend that bug.

derrida 4y ago

For some reason when ever I've had to work with PHP which was last about 10 years ago I got giggles & "TNT" by ACDC would play in my head. I'm remembering why... P.H.P DYNAMITE

donatj 4y ago

Many comments here vastly overstate the seriousness of this. For it to be used as an attack, the attacker needs general write access to your database. You probably have much bigger issues on your hands if that is the case. If they have write access they can already just set the hash to a known value for the same result. It’s a bug certainly but of very little practical security concern.

CGamesPlay 4y ago
I think you're understating it. You're imagining a scenario where an attacker spears a specific row in the database. A much more likely failure mode is that a bungled database migration simply leaves the doors wide open: all users can now log in with any password.
- donatj 4y ago
  Hmm… You make a very interesting point. I wonder how long it would take for someone to spot and report “I entered my password wrong and it worked anyway” I was thinking of an attacker with specific goals of accessing things who doesn’t want to be found out. Your hypothetical attacker would be more of a general chaos agent. I have certainly encountered far more of the latter.
pytness 4y ago
As @TimWolla commented somewhere else: https://stackoverflow.com/questions/75519073/password-verify-works-in-php-7-4-but-doesnt-work-in-php-8-2 https://stackoverflow.com/questions/75519073/password-verify...
- TimWolla 4y ago
  Note: The hash in the SO question is not vulnerable, because it is too long. The issue exists specifically for hashes that both contain a `$` and are too short. The “too short” is necessary for the trailing NUL byte of the input to be copied into the output and thus to truncate early because of the `strlen()`.

mgrund 4y ago

Could also be a backdoor in combination with another yet-to-be-discovered bug…

deleted 4y ago

[deleted]