#21022 Switch to using bcrypt for hashing passwords and security keys #7333
Conversation
…hey are validated.
This comment was marked as outdated.
This comment was marked as outdated.
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
johnbillion
changed the title
…efault bcrypt cost.
…eck_password` filter.
|
Again, I've covered this already in the PR description. sodium_compat still requires the optional libsodium extension in order to support argon2 or scrypt. There is no polyfill for either in sodium_compat. I think we should put a pin in this conversation about the 72 byte limit of bcrypt. It's an interesting computer science problem but it's so far removed from any real life problem that it distracts from assessing the underlying switch to bcrypt. A 72 byte password has 576 bits of entropy. As soon as a password is over 16 bytes in length, you're beyond the widely recommended 128 bits of entropy to consider a password secure. There is no practical need to retain a higher entropy for passwords greater than 72 bytes. |
I respectfully disagree, for two reasons:
The 72-byte limit isn't the only footgun of vanilla bcrypt. Truncating on NUL characters is the other (which is why eschewing the base64 encoding is a bad move). That being said, consider users with long passphrases. but where each word in the passphrase is low-entropy (e.g. diceware). They're well-served by this change. That being said:
That's fine. This is my final word on this topic, unless explicitly queried by another user. |
The criterion for WordPress to stop supporting PHP is the usage rate (as far as I know, this value is less than 5%). |
|
I'm with @johnbillion on straight password->bcrypt. It's counterintuitive, but it will be more secure than password->sha512->bcrypt. Plus, why are we arguing about 470 vs 500+ bits of entropy when ANY form of bcrypt is >30,000,000 more time-consuming to brute force than md5. The cryptographers who designed bcrypt were well aware it limited possible entropy to ~470 bits. They compromised there on purpose. At modern cost factors it's 1,000's of years of complexity to brute force a 72 character password. But that limitation prevents resource exhaustion DoS attacks on lower-end servers. Hashing password->base64(sha512)->bcrypt is less secure in several ways. It enables breach correlation / shucking more easily than raw bcrypt. Plus because of the base64 encoding it LOSES entropy from ALL input passwords. This gets dangerous for the large % of users who have 10 char passwords. They start with a max of ~65 bits of entropy, then clipping from 88 -> 72 characters loses some of that already limited entropy. Progressively more pre-bcrypt steps to deal with this clipping just risks opening DoS vectors. ...meanwhile most users (70-80% per HaveIBeenPwned) are using common, short, and/or recycled passwords. If we're really worried about password security we should raise WP's minimum password length, not bikeshed something cryptographers already considered when designing bcrypt. |
|
@mbijon Your comment is wrong on several levels, including one I had been ignoring in the previous discussion because I didn't want to derail it with pedantry. Unfortunately, this pedantry is now necessary.
The most obvious evidence against this statement is the real-world security impact of bcrypt's truncation.
This is nonsense, which I will explain below.
This is also nonsense, which I will explain below.
The performance impact of a SHA512 hash compared to a password hashing algorithm is negligible. You can benchmark it yourself. <?php
// Initialize up front
$start = $stop = microtime(true);
// reasonably long example "password"
$x = str_repeat('A', 128);
$y = hash('sha512', $x, true);
$i = 0;
$start = microtime(true);
for ($i = 0; $i < 100_000; ++$i) {
$y = hash('sha512', $y, true);
}
$stop = microtime(true);
echo '100,000 iterations of SHA-512 took ', number_format($stop - $start, 3), ' seconds.', PHP_EOL;
$start = microtime(true);
$y = password_hash($x, PASSWORD_BCRYPT);
$stop = microtime(true);
echo '1 iteration of bcrypt took ', number_format($stop - $start, 3), ' seconds.', PHP_EOL;Here's the output on my machine: We aren't doing 100,000 iterations of SHA-512 here, only 1 or 2 (in the case of HMAC). The DoS risk for this pre-bcrypt hashing isn't real and can't hurt you.
You... do realize that a lot of cryptographers don't like bcrypt, right? That's why the whole Password Hashing Competition existed. We settled on Argon2id and yescrypt. The whole cryptography community was there. EDIT - On My Remark About Cryptographers' Dislike of BcryptI've received feedback that the previous statement wasn't precise (which is fair; I was aiming for clarity, not precision). So here's a bit of a pedantic explanation. When comparing Bcrypt and Argon2:
Some of the Password Hashing Competition judges have lamented that they didn't prioritize cache-hardness over memory-hardness, and they recommend bcrypt instead of Argon2 because Argon2 is better as a KDF than a password hash for low latency settings. I wrote about a lot of these nuances in my blog post, which was linked above. One of the PHC judges went on to design an algorithm called bscrypt to provide stronger cache-hardness guarantees. But then he found a vulnerability in his own code, and he urged people not to use it until it's fixed. Perhaps in a year or two we can have a successor to the PHC that focuses on password hashing, not password KDFs. When I said "cryptographers don't like bcrypt", I mean something very specific: If you walk up to a cryptographer and say, "I'm designing an authentication system. It uses bcrypt for password hashing." you will get one of two responses:
Very rarely will you hear, "Okay, yes, thank you for using bcrypt and not scrypt or Argon2." Separately, OWASP (which is not famously a home for cryptographers but is highly regarded in appsec circles) in particular wants bcrypt to die in a fire precisely because of the 72-char and null-truncation vulnerabilities. I'm personally in the "as long as it's not, like, SHA256(password), and you're using an actual password hash function" camp, I just wanted to suggest removing the bcrypt footguns. On Password ShuckingThis isn't a real concern for bcrypt(SHA512()) because the previous password hash was phpass, which is based on MD5. If we were storing the SHA512 hash of the password somewhere, a) that's stupid and b) we should domain-separate our use of SHA512. But I don't think WordPress or any plugins are doing that particular snafu, so it's unlikely to matter. Revisiting the Misleading Claims About EntropyThe competing proposals looks like this (vaguely):
SHA-512 is a hash function. Aside from length extension (which isn't relevant to our threat model), you can model any of the SHA-2 family hash functions as a Random Oracle. SHA-512 maps an input domain of 256^(2^(63)-1) possible inputs to a mere 512 bits. The input isn't guaranteed to be uniformly random (as they are passwords), but the output is. Base64-encoding the output of a hash function does not "[enable] breach correlation / shucking more easily than raw bcrypt". At the layer of base64, we have a uniformly random distribution of 512 bits, which gets encoded as slightly more characters from the base64 alphabet. There is no entropy loss at the base64 layer. The base64 encoding does not, at all, "[lose] entropy from ALL input passwords." The "entropy" of the password chosen is preserved as long as SHA-512 collisions are computationally infeasible. One objection to my previous statement might be, "But what about the 88 -> 72 truncation?" This is still secure. 72 characters from the base64 alphabet is still a keyspace representing about 432 bits (or 54 bytes without base64 encoding). This is only 10 bytes short of an untruncated SHA-512, and 6 bytes longer than SHA-384. The best attack against 432 bits of uncertainty is a birthday collision attack. You'd need about 2^216 samples for a 50% chance of a collision (or 2^144 samples for a 1/2^144 chance of a collision).
Any concerns about the reduction of entropy from the strategies we've been discussing are either rooted in a poor understanding of password-based cryptography, or are bikeshedding over differences that might as well be zero to any WordPress user. Meanwhile, that Okta breach is a real thing that happened because of bcrypt's truncation. Maybe we should learn from real world incidents? (EDIT: Updated script, increased iteration count to be closer to bcrypt.) |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Stop it @soatok. You're undermining a clear improvement on md5 with incorrect information: Okta's breach was caused by using bcrypt outside it's design params as a cache key generator. They prefixed heavily in a way that consumed most of the 72 bytes in bcrypt's input. This PR does the right thing and uses password-only as input and without hashing, as recommended by both OWASP and NIST. The hobbyist community's concern over collisions between two 73-char passwords passed directly to bcrypt is wrong in two ways:
SHA hashing a password does not increase entropy beyond the original password's. SHA may randomize the characters but there's a symbolic (1:1) correlation between those characters and a low-entropy password. Clipping the length of a base64'd string does matter. It reduces the space to store the shortened SHAs of all 10-char passwords. Regarding cryptographers, NIST does approve of bcrypt and will for several more years. (...it's looking to be past 2029 before FIPS 140-3 validates any modules that might not include bcrypt. More like 2032-2035 if this DOGE department cuts NIST funding). You are right that Argon2id would be preferable. However the way to make that happen is to push it to the PHP community and not the WP community. md5 however, has been deprecated well over a decade and @johnbillion's approach is the simplest, most recommended way to implement bcrypt. |
Even if WordPress decides against my recommendations, it's worth discussing the facts so it's an informed decision. I had rested my case until you came along and posted falsehoods. (Accusing me of "incorrect information" is funny.)
I didn't say it did. Using SHA2 to hash a password permutes the bits through the entire hash output, which means the reduction of the base64-encoded hash output is not a meaningful security reduction. That is my argument. I made no claims about the entropy of the input domain, only the output.
This is a meaningless distinction, with my previous statement in mind.
I genuinely do not understand this argument. If you take a raw SHA-512 hash and encode it with base64, there is no entropy loss due to the encoding of the raw hash, and the truncation of such a large hash still has a sufficiently enormous probability space that it doesn't help attackers. The best possible attack against a truncated SHA-512 is a collision attack. I provided the risk for bcrypt-sha512+base64: After 2^144 passwords, there is a 2^-144 probability of a collision. For vanilla bcrypt, it's 2^192 for 2^-192. If you really want to argue that 2^-192 matters more than 2^-144, I invite you to write a paper for your argument and submit it to a journal for an IACR-associated event (e.g., Real World Cryptography) and then share your inevitable rejection letter publicly. Analyzing how the algorithms transform data can tell you if there is a meaningful security reduction in the steps being taken. The short answer is: No. The truncation of an encoded SHA-512 hash is still longer (and of the same quality) as a SHA-384 hash, which is what the US government recommends in CNSA. If we were truncating to less than 256 bits, there might be cause for concern, but that's not the recommendation here. A weak, 10-character password is just as weak whether or not you use SHA-512+base64 as described above. No meaningful weakness is introduced by this process, and it prevents one that has broken real-world systems.
The point of writing misuse-resistant cryptography (which is what the bcrypt with SHA-512+base64 is) is that you cannot envision all of the possible ways that an API will be used. Okta previously used bcrypt in a way that wasn't recommended by experts. Developers do this all the time. See also: JWT libraries that accept alg=none. If someone decides to write a WordPress plugin that uses the WordPress password hashing code for application passwords, and encodes the passwords as
You're arguing for the second state, I'm saying the third is the best of both worlds. In order for it to be correct that I'm "undermining a clear improvement on md5 with incorrect information", I would have to a) be arguing for phpass to continue and b) be stating falsehoods.
This is incorrect. Bcrypt is not a NIST recommend algorithm, and you cannot use it in a FIPS module. You have to use PBKDF2 with a NIST-approved hash function. NIST does vaguely recommend a "memory hard" function, but they go on to reference Balloon, not bcrypt.
Right. It's a trade-off. I'm not pushing for Argon2id today, I'm telling you what cryptographers prefer today. I know this because cryptography is my job. Aside: I'm not the first person to recommend this construction. It's had nearly a decade of scrutiny, even if you only look at the PHP community. It's comical to suggest that vanilla bcrypt is safer than the construction I recommend. |
We should try testing this with wp session too, both the standard version and any extended implementation if possible - the expectation is that they aren't affected (user would remain logged in), but it wouldn't harm to check.
@johnbillion and I also discussed the potential result of this on large datasets, and assumed it wouldn't result in much higher load, and that a bulk process to update this wouldn't be possible due to how passwords are stored/retrieved. More view on this welcome though! |
I made this Laravel PR to put a bcrypt length check into a validation step, rather than having it just truncate silently, but that was rejected too. |
|
Meta comment: I've updated my comment earlier in the thread to clarify one of my statements. I wanted to call attention to this in case it is overlooked. |
|
Having read and reviewed this entire PR, I agree with @johnbillion here with retaining the original bcrypt implementation and limitations. Initially I was onboard with SHA384+bcrypt, but after reading, I agree with just bcrypt. I would suggest that if using bcrypt for >72char is deemed inappropriate, perhaps it should fall back to the current phpass stretched-md5 hashing method, especially if no consensus can be achieved. That would result in maximum compatibility for existing WordPress users who use the Password hashes outside of WordPress, while also not introducing yet-another-custom-hash into the web where it's not overly obviously necessary, but while still gaining the bcrypt advantages for where it's possible. Edit: (13/Dec/2024) If the consensus from committers and others here is to pre-hash, I'm onboard with that, this was meant as a "If no consensus can be reached" approach |
I strongly advise against doing this. While it's generally assumed that the entropy of a 72+ character password is sufficiently good that nobody is going to crack them, even if it's a weaker password hash like phpass, doing so would kill any chance of a world where everyone is on bcrypt or better. If possible, I'd suggest trying to measure if this is a real problem before committing to it. /**
* Is this string longer than the target length?
* Avoids side-channels.
* @param string $input
* @param int $targetLength
* @return bool true if the length of $input exceeds $targetLength
*/
function isLongerThanCT(string $input, int $targetLength): bool
{
$amount = PHP_INT_SIZE === 8 ? 63 : 31;
$len = strlen($input); // use mb_strlen($input, '8bit'); on older PHP
$res = (($targetLength - $len) >> $amount) & 1;
/*
* It's worth taking a moment to explain what the hell the line above is doing.
*
* ($targetLength - $len) will return a negative value if $targetLength is smaller than $len.
*
* By two's complement, negative values when shifted 31 or 63 places to the right will have a lower
* bit set. We then use a bit mask of `1` and the bitwise `&` operator on the result of the bitshift.
*
* End result? It's the same as if we did the following, except constant-time:
* $res = (int) ($len > $targetLength);
*/
return (bool) ($res);
}This function returns You can use this to measure if the length of users' passwords exceeds some threshold. Demo: https://3v4l.org/BZK5d Separately, I wrote a blog post exploring the details of bcrypt with SHA2 titled, Beyond Bcrypt. |
|
Please @dd32 let’s not fallback to any form of md5 hashing. The form of md5 stretching in PHPass isn’t built to consume time and can’t be configured to extend the time (rounds). Also, it creates a herd vulnerability on large sites like wordpress.com or WPEngine because it doesn’t have per-user salts (it only peppers passwords with what’s named a “SALT” in wp-config). Plus, there are already enough vulnerabilities in md5 for it to be considered broken since 2008. Any not-broken form of bcrypt password storage (ie: not like Okta’s old implementation) is more secure than any form of md5 storage. |
I'd just like to highlight my edit to the above comment:
I'm totally onboard with double-hashing if it's agreed by all involved. |
|
Comment from forum https://core.trac.wordpress.org/ticket/21022#comment:184 here again just for the record: Context: authentication can take even longer if it is one time process in one system and everything else is later handled with tokens, in WP is different e.g. we perform authentication on every request in many places. Let we check how XMLRPC system.multicall would behave from DoS or application passwords from TTB perspective. for the following code a like |
That's horrifying. Bcrypt should be used for logging in actual humans with human-memorable passwords. (These should, in turn, be handled by a password manager, such as 1Password or Bitwarden.) In an ideal world, XMLRPC authentication should use a high-entropy app password, for which a simple HMAC is more than sufficient. This is outside the scope of how password hashing is done in WordPress, but I shall ask: How do we get the ecosystem from here to there? |
Yeah, even more performance intensive algorithms would be ok and can't be a show stopper, because Troubling parts are applications passwords for rest api (here is per request and one time) and xmlrpc (here is where party starts), but also old xmlrpc access (user/pass). Multicall with 20 methods call would cause One thing that comes on my mind is how application passwords are implemented, at the moment functionality is with Yet old xmlrpc access with user/pass remains. |
As long as a solution can be offered, I think that's acceptable. We should take extra care to document this, and provide actionable instructions (e.g., "here's how to migrate to app passwords to improve performance after the bcrypt migration"). |
Only after application passwords are changed, atm it will be the same :) |
|
It's not only xmlrpc and application passwords that are affected. Many plugins also (mistakenly) use |
| } | ||
|
|
||
| $hashed = $wp_hasher->HashPassword( $key ); | ||
| $hashed = wp_hash_password( $key ); |
There was a problem hiding this comment.
There's no need to use wp_hash_password here.
wp_hash is appropriate.
Latest approach to switching from phpass to bcrypt via the native PHP password hashing functions.
This covers:
Lots more info about the change can be found in the draft for the make/core announcement post.
Tickets
Notes
wp_check_password()remains compatible with phpass hashes, so password checks will continue to succeed when checking a password against its existing hash. There is no need for users to change or resave their password.wp_authenticate_username_password()orwp_authenticate_email_password().wp_authenticate_application_password().wp_check_password()and the rehashing that occurs in the above functions are forward-compatible with future changes to the default bcrypt cost (the default bcrypt cost was increased in PHP 8.4), so password checks will continue to succeed when checking a password against its existing hash.wp_hash_password()andwp_check_password()retain support for a global$wp_hasherobject which can be used to override the password hashing and checking mechanisms.Elsewhere
FAQs
What about salting?
Salting a bcrypt hash is handled by
password_hash()andpassword_verify(). There is no need to implement salting in userland code.What about peppering?
Peppering effectively eliminates the ability to crack a password given only its hash by introducing a secret which needs to be stored outside of the database and is used as part of the value that's hashed, as long as the pepper remains secret. This is compelling, however the portability of a password hash is also eliminated and if the secret pepper is lost, changed, or differs between environments, then all password hashes are invalidated. All users will need to reset their passwords in order to log in, and all application passwords and post passwords will need to be changed.
While a secret pepper does prevent an attacker from being able to crack a password hash if they gain access only to the database, the potential usability tradeoff is high. In addition, the intention of switching to bcrypt is to switch to a password hashing algorithm that is highly resistant to cracking in the first place, thus reducing the benefit gained from peppering.
What about layering hashes to immediately protect legacy hashes?
Hash layering is the process of taking an existing password hash (for example one hashed with phpass) and applying the new hashing algorithm on top of it (bcrypt in this case). The intention is to immediately protect stored passwords with the new hashing algorithm instead of waiting for users to log in or change their passwords in order to rehash the password.
A concern is the length of time it could take to rehash all passwords in the database during the upgrade routine. Handling would need to be implemented to cover usage of passwords (for example logging in or changing passwords) while the password upgrade routine runs.
Additional risks include null byte characters present in the raw output of other hashing algorithms, and password shucking. OWASP warns that layering hashes can make the password easier to crack.
None of this is insurmountable, but it adds complexity for what is in most cases a short term benefit. For this reason, hash layering has not been implemented.
What about the 72 byte limit of bcrypt?
This is an ongoing consideration that will likely be addressed by pre-hashing the password with sha384 and base64 encoding it prior to hashing with bcrypt. There is discussion about this in the comments here and on #21022. A separate PR at johnbillion#5 is in progress.
Out of the platforms listed above, only Symfony provides any specific handling for passwords greater than 72 bytes in length (introduced in Symfony 5.3 in 2021). Passwords longer than 72 bytes are hashed with sha512 and then base64 encoded prior to being hashed with bcrypt.
This PR in its current form does not include any specific handling for the 72 byte limit, but there are options:
maxlengthattributes because the input value is obscured when typing/pasting/autofilling.Unless new objections are raised, johnbillion#5 will be merged into this PR soon.
What about DoS attacks from long passwords?
The bcrypt implementation in PHP is not susceptible to a DoS attack from long passwords because only the first 72 bytes of the password value are read, therefore there is no need to guard against a long password value prior to it being hashed or checked.
While phpass can be susceptible to a DoS attack via a very long password value, the phpass implementation in WordPress protects against this via a 4096 byte limit when hashing a password and when checking a password. This is unrelated to the password length limit discussed above.
What about the cost factor?
The default cost factor will be used. This is 10 in PHP up to 8.3 and has been increased to 12 in PHP 8.4. Hashes remain portable between installations of PHP that use different cost factors because the cost factor is encoded in the hash output.
It's beyond the scope of WordPress to make adjustments to the cost factor used by bcrypt. If you are planning on updating to PHP 8.4 then you should consider whether the default cost is appropriate for the resources available on your server. The
wp_hash_password_optionsfilter is available to change the cost factor should it be needed.What about using
PASSWORD_DEFAULTinstead ofPASSWORD_BCRYPT?The intention of the
PASSWORD_DEFAULTconstant in PHP is to take advantage of future changes to the default algorithm that's used to hash passwords. There are currently no public plans to change this algorithm (at least, I haven't found any), but for safety it makes sense to be explicit about the use of bcrypt in WordPress. This can easily be changed in the future.What about Argon2?
Unfortunately it's not possible to rely on argon2 being available because it requires both
libargon2to be available on the server and for PHP to be built with argon2 support enabled. Using argon2 viasodium_compatstill requires the optionallibsodiumextension to be installed. Conditionally using argon2i, argon2id, or bcrypt depending on what is available on the server would increase complexity and limit the portability of hashes.What about scrypt?
There is no native support for scrypt in PHP, and using scrypt via
sodium_compatstill requires the optionallibsodiumextension to be installedIs this change compatible with existing plugins that implement bcrypt hashing?
It should be, yes. If you've used such a plugin to hash passwords with bcrypt then those hashes should be compatible with this bcrypt implementation and you should be able to remove the plugin.
What effect will this have on my database when a user logs in?
The first time that each user subsequently logs in after this change is deployed to your site, their password will be rehashed using bcrypt and the value stored in the database. This will result in an additional
UPDATEquery to update theiruser_passfield in theuserstable.The query will look something like this:
This query performs an
UPDATEbut makes use of the primary key to target the row that needs updating, therefore it should remain very performant. The query only runs once for each user. When they subsequently log in again at a later date their password will not need to be rehashed again.Note that when a user logs in, WordPress already writes to the database via an
UPDATEquery on theusermetatable to store their updated user session information in thesession_tokensmeta field. This happens every time a user logs in.How do I use an algorithm other than bcrypt on my website?
The
wp_hash_password(),wp_check_password(), andwp_password_needs_rehash()functions are all pluggable. See wp-password-bcrypt as an example of overwriting them in a plugin.Alternatively, if you need to temporarily or permanently stick with phpass you can instantiate the
$wp_hasherglobal and this will be used instead:Todo
password_hash()should be filterable.wp_hash_password()andwp_check_password()need to retain back-compat support for the global$wp_hasherTesting
There's good test coverage for this change. Here are some manual testing steps that can be taken.
Remaining logged in after the update
user_passfield for your user account in thewp_userstable in the database has been updated -- it should be prefixed with$2y$instead of$P$Post passwords
Password resets
Personal data requests