-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#21022 Switch to using bcrypt for hashing passwords #7333
base: trunk
Are you sure you want to change the base?
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. |
…efault bcrypt cost.
…eck_password` filter.
Fair enough, and I understand what you're saying here,. However, let's be damn sure of that because failure means not having passwords work correctly, in all languages. There have to be unit tests for this and those unit tests have to cover a lot of cases. Please make sure we get those right, first. |
I'm happy with the test coverage that's already in place for utf8mb4 and the other supported database character sets. The tests include string length assertions among many others. Variable length encoding is a fundamental feature of utf8, it's not something that needs specific testing as part of this change. |
…t keys, and the recovery mode key, which have a known high level of entropy.
@soatok Happy to take some further guidance on this. I was going to use sha256 but it's technically possible for ext/hash to be disabled in PHP < 7.4 which makes this algo unavailable for use in |
Second that. I guarantee that if SHA1 is used, it will be flagged in every pentest and code audit of a WordPress site for the next 20 years... |
Scrap that, it looks like sha256 is already unconditionally used in PHPMailer as well as well as the updated Gravatar hash handling in https://core.trac.wordpress.org/ticket/60638 . I'll make the change. |
That's my department. SHA256 is used to make MIME boundaries, DKIM signatures, S/MIME signing and other things. |
@dd32 Are you able to pull usage numbers for what % of sites running PHP 7.2-7.3 have ext/hash disabled? In https://core.trac.wordpress.org/ticket/60638 the Gravatar functions now use sha256 which requires hash to be available on those versions of PHP, and ideally we'd use it in place of my proposed use of sha1 for high entropy security keys. |
By using |
There's a few places with conditional logic for
Looks like we'll just need similar logic here. |
Something I have had to do in PHPMailer is to check for extensions in a different way because it seems that some hosting providers (in their infinite wisdom) disable |
@johnbillion why not use: https://www.php.net/manual/en/function.sodium-crypto-generichash.php For all the high entropy use cases? It's in sodium_compat AFAIK |
require_once ABSPATH . WPINC . '/class-phpass.php'; | ||
// By default, use the portable hash from phpass. | ||
$wp_hasher = new PasswordHash( 8, true ); | ||
if ( ! empty( $wp_hasher ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this has the potential for a lot of breakage.
IIRC, a ton of plugins that do custom user creation and/or profile updates do checks like "if empty, create new password hash instance".
This opens up the possibility that password hashes are created with $wp_hasher, but not validated with it, since those same plugins would not re-instantiate on the fly during validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so we can boil that down to: plugins that manually hash a password with PasswordHash
and then let core do the hash validation at a later date. That should be supported by this PR and not be a problem because wp_check_password()
will check a password with PasswordHash
when its hash starts with $P$
, regardless of the $wp_hasher
global.
The problem you describe would only take effect if a plugin specifically assigns a hasher instance to the $wp_hasher
global (the instance isn't a singleton).
Am I understanding correctly?
Search results for instances of that here: https://wpdirectory.net/search/01JH19AV75Q6TJA6VRZ0A9EMG7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reverse case is also true, there are plugins calling $wp_hasher->checkPassword()
It's not immediately clear to me what the implications are. There are several scenarios that'd need to be taken into account.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have this documented in the Fortress docs here
There's nothing that can be done to support this particular edge case.
For me, that was an acceptable trade-off. But the scale is obviously different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do some digging through plugins which are using PasswordHash::HashPassword/CheckPassword
directly and/or messing with the $wp_hasher
global.
Benefits: https://libsodium.gitbook.io/doc/hashing/generic_hashing#notes
|
This is what I proposed for Laravel nearly 5 years ago! No movement there either :) |
I would much prefer Performance wise, it's obviously fastest if you have ext-sodium installed, but the polyfill isn't too bad. |
return $wp_hasher->HashPassword( trim( $password ) ); | ||
} | ||
|
||
if ( strlen( $password ) > 4096 ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Footgun.
This is not part of the public API and many plugins use wp_hash_password for non-passwords, sadly.
A PHP notice or error_log would probably be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is part of the public API because it matches the current behaviour of the phpass implementation, which limits passwords to 4096 characters in length for DoS mitigation. I am not attached to it though, and the new use of sha384+base64+bcrypt shouldn't be subject to DoS concerns from overly long passwords. Shall we pull it out?
Edit for clarification: by "pull it out" I mean from wp_hash_password()
and wp_check_password()
, not from the phpass implementation.
Refs:
wordpress-develop/src/wp-includes/class-phpass.php
Lines 216 to 218 in 09459f1
if ( strlen( $password ) > 4096 ) { return '*'; } wordpress-develop/src/wp-includes/class-phpass.php
Lines 249 to 251 in 09459f1
if ( strlen( $password ) > 4096 ) { return false; }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, it's implicitly exposed as the public API if phpass is used
* @param string $hash Hash of a password to check. | ||
* @return bool Whether the hash needs to be rehashed. | ||
*/ | ||
function wp_password_needs_rehash( $hash ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a new pluggable function? As-is, the WP password functionality now exposes a new public API for plugins to use, which also means that any custom code that changes pluggable functions needs to implement this "interface" even if not needed.
Is there a reason why this can't be done inline inside wp_check_password
with a $needs_rehash variable?
Usually, rehashing is only performed if you know that a valid password was provided.
The function signature with just $hash
seems strange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wp_password_needs_rehash()
function is called outside of wp_check_password()
because, as we know, wp_hash_password()
and wp_check_password()
can be used for data other than user passwords (currently application passwords and post passwords). Rehashing and resaving cannot reliably be performed inside wp_check_password()
because it's not always a user password being checked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How should an implementor even know what $hash
is?
It could be any kind of hash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, At a minimum, the new API should also receive a user id argument, and a "type" argument.
Type being an enum of login_password|app_password
making it clear that this is not for use of third party plugins in other scenarios
I'm working on the switch to |
continue; | ||
} | ||
|
||
if ( wp_password_needs_rehash( $item['password'] ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Independently from
I don't think this is needed.
Application passwords are high-entropy.
Rehashing them from phpass to bcrypt is not needed, and not desired as its very slow on the validation part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point. The idea was to opportunistically upgrade from phpass to bcrypt (blake2d in johnbillion#7) to take advantage of the added preimage resistance. Given the known high entropy, perhaps that's not needed, but it's worth bearing in mind that application passwords are valid permanently until revoked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A fast hash is enough for application passwords.
They have 24 bit entropy, IIRC. The relevant threat model is SQL injection, so you don't want plaintext in the DB.
But fast hashes are fine.
I'd not add any upgrading of app passwords tbh, the less moving parts the better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good points, thanks. I'll give it some more thought and likely remove the opportunistic hash upgrade for application passwords in johnbillion#7.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments on that PR. :)
I've opened a Trac ticket to start discussing enforcing application passwords for XML-RPC. |
Latest approach to switching from phpass to bcrypt via the native PHP password hashing functions for the following features:
The following features are switched from phpass to a SHA-256 HMAC:
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_hasher
object 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 addressed by pre-hashing the password with SHA-384 and base64 encoding it prior to hashing with bcrypt. There is discussion about this in the comments here and on #21022.
Supporting references:
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_options
filter is available to change the cost factor should it be needed.What about using
PASSWORD_DEFAULT
instead ofPASSWORD_BCRYPT
?The intention of the
PASSWORD_DEFAULT
constant 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
libargon2
to be available on the server and for PHP to be built with argon2 support enabled. Using argon2 viasodium_compat
still requires the optionallibsodium
extension 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.Argon2id can be enabled via a filter if your server supports it. See the "How do I use an algorithm other than bcrypt on my website?" section further down.
What about scrypt?
There is no native support for scrypt in PHP, and using scrypt via
sodium_compat
still requires the optionallibsodium
extension to be installedIs this change compatible with existing plugins that implement bcrypt hashing?
Yes. If you've used such a plugin to hash passwords with bcrypt then those hashes are 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
UPDATE
query to update theiruser_pass
field in theusers
table.The query will look something like this:
This query performs an
UPDATE
but 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
UPDATE
query on theusermeta
table to store their updated user session information in thesession_tokens
meta field. This happens every time a user logs in.How do I use an algorithm other than bcrypt on my website?
If your server supports the argon2id algorithm, switching to it is now a one-liner:
If necessary, the
hash_algos()
function can be used to check for argon2id support.Alternatively, 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 again, if you need to temporarily or permanently stick with phpass you can instantiate the
$wp_hasher
global to restore the previous behaviour:Todo
hash_hmac( 'sha382' )
means that thehash
extension is required on PHP 7.2 and 7.3 because thehash_hmac()
compat function in compat.php only supports md5 and sha1.password_hash()
should be filterable.wp_hash_password()
andwp_check_password()
need to retain back-compat support for the global$wp_hasher
Testing
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_pass
field for your user account in thewp_users
table in the database has been updated -- it should be prefixed with$wp$2y$
instead of$P$
Post passwords
Password resets
Personal data requests