Skip to content
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

Open
wants to merge 69 commits into
base: trunk
Choose a base branch
from

Conversation

johnbillion
Copy link
Member

@johnbillion johnbillion commented Sep 11, 2024

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:

  • User password reset keys
  • Personal data request keys
  • The recovery mode key

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.
  • User passwords are rehashed using bcrypt after successful authentication in wp_authenticate_username_password() or wp_authenticate_email_password().
  • Application passwords are rehashed using bcrypt after successful authentication in 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() and wp_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() and password_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 of PASSWORD_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 via sodium_compat still requires the optional libsodium 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 optional libsodium extension to be installed

Is 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 their user_pass field in the users table.

The query will look something like this:

UPDATE wp_users
SET user_pass = '<hash>', user_activation_key = ''
WHERE ID = <id>

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 the usermeta table to store their updated user session information in the session_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:

add_filter( 'wp_hash_password_algorithm', fn() => PASSWORD_ARGON2ID );

If necessary, the hash_algos() function can be used to check for argon2id support.

Alternatively, the wp_hash_password(), wp_check_password(), and wp_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:

add_action(
  'plugins_loaded',
  function(): void {
    global $wp_hasher;
    require_once ABSPATH . WPINC . '/class-phpass.php';
    $wp_hasher = new PasswordHash( 8, true );
  }
);

Todo

  • Uh oh! The use of hash_hmac( 'sha382' ) means that the hash extension is required on PHP 7.2 and 7.3 because the hash_hmac() compat function in compat.php only supports md5 and sha1.
  • Make the decision on SHA-256 or BLAKE2d via Sodium for security key HMACs
  • Make a decision on the increased DoS impact that results from switching from phpass to bcrypt for endpoints
    • XML-RPC (uses user passwords)
    • REST API (uses application passwords)
  • Make a decision on the 72 byte limit imposed by bcrypt
  • Decide whether the options array for password_hash() should be filterable.
  • Decide whether wp_hash_password() and wp_check_password() need to retain back-compat support for the global $wp_hasher
  • Fully support changes to the default bcrypt cost
  • Tests for the hash upgrade routine for a user password
  • Tests for the hash upgrade routine for an application password
  • Tests for handling post password hashes
  • Address md5 hashing of passwords in repair.php

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

  • Start on the trunk branch
  • Log in to WordPress
  • Switch to this branch
  • Ensure you have remained logged in
  • Log out and back in again
  • Confirm it works as expected
  • Confirm that the user_pass field for your user account in the wp_users table in the database has been updated -- it should be prefixed with $wp$2y$ instead of $P$

Post passwords

  • Start on the trunk branch
  • Publish a post and set a password for it
  • View the post and enter the password to view it
  • Switch to this branch
  • Confirm you can still see the post without having to re-enter its password
  • Edit the post and change its password
  • View the post and confirm that you can re-enter the new password and view it

Password resets

  • Start with the "Lost your password?" link on the login screen
  • Click the confirmation link sent to your email
  • Follow the process of resetting your user password
  • Confirm you can log in with your new password

Personal data requests

  • Start on the trunk branch
  • Log in to WordPress as an Administrator
  • Initiate a data export from Tools -> Export Personal Data
  • Switch to this branch
  • Click the confirmation link sent to the email address and confirm that it still works
  • Initiate another data export from Tools -> Export Personal Data
  • Click the confirmation link sent to the email address and confirm that it works

@johnbillion johnbillion marked this pull request as draft September 11, 2024 18:38

This comment was marked as outdated.

Copy link

Test using WordPress Playground

The 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

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@johnbillion johnbillion changed the title #21022 Switch to using bcrypt for password and security key hashing #21022 Switch to using bcrypt for hashing passwords and security keys Sep 11, 2024
@Otto42
Copy link
Member

Otto42 commented Jan 5, 2025

That's not how UTF-8 works. For example, café is 5 bytes in UTF-8; just because one character uses > 1 byte doesn't mean they all do.

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.

@johnbillion
Copy link
Member Author

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.

@soatok
Copy link

soatok commented Jan 6, 2025

413e81f -- I don't want to belabor the point, but since shattered and shambles, choosing SHA-1 is at least a code smell. Can you use a SHA-2 family hash function instead?

@johnbillion
Copy link
Member Author

@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 hash_hmac(). I will ask the .org team if they have any helpful usage numbers. Any other suggestions?

@Synchro
Copy link

Synchro commented Jan 6, 2025

413e81f -- I don't want to belabor the point, but since shattered and shambles, choosing SHA-1 is at least a code smell. Can you use a SHA-2 family hash function instead?

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

@johnbillion
Copy link
Member Author

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.

@Synchro
Copy link

Synchro commented Jan 6, 2025

Scrap that, it looks like sha256 is already unconditionally used in PHPMailer...

That's my department. SHA256 is used to make MIME boundaries, DKIM signatures, S/MIME signing and other things.

@johnbillion
Copy link
Member Author

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

@soatok
Copy link

soatok commented Jan 6, 2025

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

By using extension_loaded(), you can detect when this extension is disabled. If you absolutely need a fallback for "ext/hash is disabled", you can add SHA1 as a fallback. But making it the default just because an edge case is possible will cause years of headache.

@johnbillion
Copy link
Member Author

johnbillion commented Jan 7, 2025

There's a few places with conditional logic for hash() already in place:

  • // If ext/hash is not present, compat.php's hash_hmac() does not support sha256.
    $algo = function_exists( 'hash' ) ? 'sha256' : 'sha1';
    // Old WP installs may not have AUTH_SALT defined.
    $salt = defined( 'AUTH_SALT' ) && AUTH_SALT ? AUTH_SALT : (string) rand();
    $placeholder = '{' . hash_hmac( $algo, uniqid( $salt, true ), $salt ) . '}';
  • // If ext/hash is not present, compat.php's hash_hmac() does not support sha256.
    $algo = function_exists( 'hash' ) ? 'sha256' : 'sha1';
    $hash = hash_hmac( $algo, $username . '|' . $expiration . '|' . $token, $key );
  • // If ext/hash is not present, compat.php's hash_hmac() does not support sha256.
    $algo = function_exists( 'hash' ) ? 'sha256' : 'sha1';
    $hash = hash_hmac( $algo, $user->user_login . '|' . $expiration . '|' . $token, $key );
  • // If ext/hash is not present, use sha1() instead.
    if ( function_exists( 'hash' ) ) {
    return hash( 'sha256', $token );
    } else {
    return sha1( $token );
    }

Looks like we'll just need similar logic here.

@Synchro
Copy link

Synchro commented Jan 7, 2025

There's a few places with conditional logic for hash() already in place:
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 extension_loaded, so a safer, more indirect way is to check whether a constant defined by an extension exists, for example by defined('OPENSSL_ALGO_SHA256'). However, since WP is already using extension_loaded it's safe to continue using it.

@calvinalkan
Copy link

@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 ) ) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnbillion

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

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

Copy link

@calvinalkan calvinalkan Jan 7, 2025

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.

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.

Copy link
Member Author

@johnbillion johnbillion Jan 7, 2025

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.

@calvinalkan
Copy link

@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

Benefits:

https://libsodium.gitbook.io/doc/hashing/generic_hashing#notes

secure hash function that is as strong as SHA-3 but faster than MD5 and SHA-1.

Unlike MD5, SHA-1, and SHA-256, this function is safe against hash length extension attacks.

@Synchro
Copy link

Synchro commented Jan 7, 2025

@johnbillion why not use:
https://www.php.net/manual/en/function.sodium-crypto-generichash.php
For all the high entropy use cases?

This is what I proposed for Laravel nearly 5 years ago! No movement there either :)

@soatok
Copy link

soatok commented Jan 7, 2025

I would much prefer sodium_crypto_generichash(), which is included in sodium_compat, and provides a baked-in keyed hashing mode (so you can replace hash_hmac() calls too).

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

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.

Copy link
Member Author

@johnbillion johnbillion Jan 7, 2025

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:

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnbillion

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.

Copy link
Member Author

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.

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.

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

@johnbillion
Copy link
Member Author

I'm working on the switch to sodium_crypto_generichash() in johnbillion#7.

continue;
}

if ( wp_password_needs_rehash( $item['password'] ) ) {
Copy link

@calvinalkan calvinalkan Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independently from

johnbillion#7

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.

Copy link
Member Author

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.

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.

Copy link
Member Author

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.

Copy link

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

@johnbillion johnbillion changed the title #21022 Switch to using bcrypt for hashing passwords and security keys #21022 Switch to using bcrypt for hashing passwords Jan 8, 2025
@johnbillion
Copy link
Member Author

I've opened a Trac ticket to start discussing enforcing application passwords for XML-RPC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.