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).
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.
"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.
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.
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()`.
> 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:
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 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.
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.
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.
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"...
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.
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.
> 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.
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.
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.
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
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 */
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 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.
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:
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.
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.
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.
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«.
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.
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...
