PHP bug: Password_verify() always return true with some hash | Hacker News Reader

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

bugs.php.net

43 pointsdutchbrit72 comments

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!

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).

> 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.

EDIT: I misread the linked post, ignore me!

<strike>crypt is the hashing function, not the password checking function</strike>

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.

> 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.

Then they proceed to explain why, but why in the world would you return true if the underlying function calls failed???

"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.

> 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.

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".

"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.

Sure, there are different levels of explaining "impact" and the reporter isn't responsible for being the final authority. They should have some idea though as to what the bad thing you can do with the bug is. (However, cvss scores are basically garbage)

That if you don’t meet the documented preconditions to calling a function you can’t assume the output is valid?

That seems fine to me, not every programming language has to be defensive about bad inputs, lord knows C isn’t. It seems like he got caught between two conflicting pieces of documentation one having stronger guarantees.

PHP has a lot of C-isms under the hood, not only in its infamous function names but also in how they signal errors in-band in the old functions. crypt() is one of those old functions with a crappy API, and password_verify() is just a thin wrapper around it. Maybe this explains the "it's undefined, so we can do whatever" attitude.

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.

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.

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.

As @TimWolla commented somewhere else: https://stackoverflow.com/questions/75519073/password-verify...

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()`.

More info: https://github.com/php/php-src/security/advisories/GHSA-7fj2...

seems to be caused by a strange non-std php-specific crypto implementation

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.

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

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.

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

> 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.

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.

That requires a database vulnerability to be exploited though, the average user can’t go setting hashes.

Chaining vulnerabilities is a common way to exploit systems.

True, but in this case if you can write an invalid hash into a database, you can likewise write a valid one, and as such this doesn't really enable anything.

The one thing this does get you is that the original password would still work (technically any password would still work) so it may make it harder to detect since the user wouldn't "suddenly be locked out"...

Which is one thing it enables: silent co-use. If your goals are related to long-term use or surveillance, this is useful.

PHP is the king of SQL injections, they are often not too hard to come by on a lot of deployments. Sure, if you use modern PHP with modern toolkits/frameworks/libraries and you follow OWASP and are a competent developer, you might never introduce such vulnerabilities in your codebase. But PHP didn't gain popularity because it had frameworks that were concerned about security or that it had a trove of security conscious developers - it thrived because any 13 year old could string together a website that was as minimum of a viable product as possible.

If you have a sql injection vulnerability they literally already have the keys to the castle. This in addition gets you nothing you didn’t already have simply by setting the hash to a known value.

How would you persistently log into another user’s account, without them knowing anything was wrong and without knowing their password, with just sql injection?

Well i kind of agree with you that the covert persistence aspect makes this a borderline low severity bug, but pivoting from DB->access isn't exactly unheard of.

I would first of all, wonder why i would want that. Why access someone's account when i could just dump the DB. Obviously it depends on context, but most of the time, DB access is more valuable than account access.

I would check if the DB is misconfigured (e.g. In mysql, does the DB user have File or Super rights?)

I would dump all the passwords and see if anyone's is weak enough to suffer from a crack attempt

I would check if there is anything in the DB that looks like php serialized data (People are saying unfair things about PHP like it being the king of sql injection, but its probably fair to say that unsafe deserialization is pretty common in php).

As a last resort, i would check if there is anything that looks like HTML or JS in the database, and see if i could get an XSS to leverage into an account take over.

> any 13 year old

you could say the same about python, javascript, ruby.. didn't implement a calculator at high school in C++?

> the king of SQL injections

that would be bob https://xkcd.com/327/

> If the attacker has the ability to set a hash, they can just set the hash to a known password.

That requires the attacker top also have access to the salt

I smell an underlying sentiment of "if the attacker has access to the DB, then it is broken anyways". This is not entirely true. Think a gateway service that lets the user to something on another service with access without access to the database immediately giving access to the system.

This is definitely a security bug.

> I smell an underlying sentiment of "if the attacker has access to the DB, then it is broken anyways"

To be more clear, my position is - if the service allows you to set the password for an arbitrary user, then it is broken anyways.

Again, not necessarily. This depends on the hashing scheme you use. Eg. if setting a correct password hash relies on you having access to private keys.

No it does not.

Either you allow bcrypt hashes, or this bug is inapplicable. If you are encrypting your hashes or something, then this bug cannot be leveraged.

Yes, for this very specific bug. This thread taked about having access to the database as a general attack vector.

Nope. Blowfish doesn’t use a manually defined salt. It generates one itself randomly every time and includes it in the hash result string which you generally store the entire string in the database. No secret salt involved.

> That requires the attacker top also have access to the salt

Which you must presume they do. If any part of your security relies on the salt being secret, that's a much bigger vulnerability than this.

That said, I do think there's a potential vulnerability here, because it allows you to break in if you can only corrupt another user's password hash (rather than controlling it entirely). Think a rowhammer attack or something.

There are scenarios, but they are pretty out there.

For example, a service might import hashed passwords from a directory, and an attacker has limited influence on the network connection to cause some random-ish data corruption.

> Like, its a really interesting security adjacent bug, but clearly not a security issue.

I kinda disagree. We often think of security in layers, and an unexpected fail-open behavior in any layer should be treated as a (potential) security issue.

The impact might be low because you expect another layer (like protection of the password database) to prevent exploits, but there could always be corner cases where that assumption doesn't hold 100%, especially in something as fundamental as a language built-in, a system call or something like that.

So IMHO it's pretty low severity, but still a security issue.

In effect all bugs are security bugs. When people offer an excuse, what they're telling you is that they don't care about correctness, which in turn means they don't actually care about security only about appearances.

Why are all bugs security bugs? Because security depends on the actual behaviour of the software, and bugs cause this behaviour to deviate from your intended and documented behaviour in unknown ways which means they have unknown impact on security. Figuring out whether any user of the software actually incurs a security impact as a result of any particular bug is likely to be far more work than fixing it, so just fix it.

That's like arguing everything causes cancer, therefore smoking is fine.

The equivalent of an argument that everything causes cancer => smoking is fine would be all bugs are security bugs => don't bother fixing the bug underpinning a known a remote code execution exploit.

Which is not at all what I was getting at. Again, don't spend time trying to argue why this or that bug is probably fine actually and not "really" a security problem, fix it and then it isn't.

Not much, but it would allow you to change a hash via sql injection while still allowing the regular user to login. So perhaps stealthier than using a real pre generated hash.

The database could be corrupted somehow and then any password will match. Some developer may just look at the RFC and assume random strings should fail the check, so may develop something that actually passes on random string. It’s probably not even a real security issue, but definitely strange behavior

Indeed, '$' is a just bit flip away from '4' or 'd', and we end up with a user account that can be accessed by any password.

    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.

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

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.

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?

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!

Man, he tried so hard to defend that bug.

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/

Obligatory "that article is from 10 years ago and it's really not the same PHP anymore"

OP’s link disagrees. A critical vulnerability being called »not a bug RTFM« is the same cultural and engineering level of »string length as hash function what’s the problem«.

This bug is obviously not critical.

The response is wrong though. The RFC for this function which someone quoted further down in the bug report clearly stated that it should return false even if you feed it a garbage hash.

The initial response to the report is obviously quite shocking, not disagreeing there.

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.

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.

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.

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;
    }

Yeah, I mean it's still a dynamic language with the pitfalls that come with it (many shared with JS and Python).

Do I think dynamic languages are terrible and hate all three of the above? Yes.

Do I think modern, idiomatic PHP still suffers from most of TFA's examples of bad design? No.

This is expected behavior and works like a charm. The type of a variable can always change at any time. The argument hint is designed to make sure you’re given the expected type. Not that the variable will only hold that type.

Most inflammatory? Yes. Best? No.

This guy should have monetized his blog, as in every discussion vaguely related to php someone come with a link to this page.

Instead it's just costing him money. PHP takes revenge.

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

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

Reply on news.ycombinator.com