Application Passwords BC Break in 6.8's new hashing

[snicco]

Pre 6.8, application passwords are created with wp_hash_password which is pluggable and can be replaced by users.

6.8 uses a new function wp_fast_hash to hash application passwords, and wp_verify_fast_hash to check them.

For backwards compatibility, the following is included in the verification code:

<?php
function wp_verify_fast_hash(
        #[\SensitiveParameter]
        string $message,
        string $hash
): bool {
        if ( ! str_starts_with( $hash, '$generic$' ) ) {
                // Back-compat for old phpass hashes.
                require_once ABSPATH . WPINC . '/class-phpass.php';
                return ( new PasswordHash( 8, true ) )->CheckPassword( $message, $hash );
        }

        return hash_equals( $hash, wp_fast_hash( $message ) );
}

But this will only work if the site has previously not been using a custom password hashing implementation.

That's not given (Fortress, WP password bcrypt, etc.), and is a BC break.

All previously created application passwords would be invalid because they fail validation.

The correct, backwards compat preserving code would be:

<?php
function wp_verify_fast_hash(
        #[\SensitiveParameter]
        string $message,
        string $hash
): bool {
        if ( ! str_starts_with( $hash, '$generic$' ) ) {
                // Back-compat for old phpass hashes.
                return wp_check_password($message, $hash);
        }

        return hash_equals( $hash, wp_fast_hash( $message ) );
}

By default, wp_check_password will use PHPass. But if a custom implementation was used, it will use that instead.

Trac Ticket: Core-63203

[jorbin]

@johnbillion are you able to take a look?

[peterwilsoncc]

I've verified this issue by creating custom implementations of wp_hash_password() and wp_check_password(). (They're very basic and wildly insecure so I won't be sharing them here -- think rot13).

I think the new methods in WP_Application_Passwords will either need to be removed or converted to wrappers for the pluggable functions wp_hash_password() and wp_check_password().

My inclination is to remove them as they won't serve much purpose as wrappers.

@Debarghya-Banerjee Thank you for creating the pull request.

Unfortunately, this isn't the approach that will be needed. wp_verify_fash_hash() will need to remain unchanged, the functionality that will need a change is for application passwords.

In WP_Application_Passwords::create_new_application_password() the variable $hashed_password will need to be modified to use wp_hash_password()

​https://github.com/WordPress/wordpress-develop/blob/f444639e0852f7c0635a09d0dcee561af22a6515/src/wp-includes/class-wp-application-passwords.php#L99

In wp_authenticate_application_password() the password check will need to be modified to use wp_check_password()

​https://github.com/WordPress/wordpress-develop/blob/fd3f4e381884f825eaa969d8ff0c430d0b124d58/src/wp-includes/user.php#L460

With those changes, it should be possible to remove the methods introduced in WP 6.8 from WP_Application_Passwords

Trac Ticket: Core-63203

• Modifies WP_Application_Passwords::create_new_application_password() to use wp_has_password
• Modifies the wp_authenticate_application_password() password check to use wp_check_password()

Hi @peterwilsoncc, can you please have a look into this PR. Thanks.

Closing this and have raised a new PR with the suggested change:
​PR#8626

Closing this and have raised a new PR with the suggested change:
​PR#8626

[johnbillion]

I think there's some confusion in the pull requests between passwords hashed with bcrypt and passwords hashed with BLAKE2b. Application passwords use the latter via wp_fast_hash() and should not be switched to hashing with wp_hash_password(). This report from @snicco has identified that existing passwords hashed prior to 6.8 with a custom implementation of the pluggable wp_hash_password() need to be checked with that function for backwards compatibility, but new ones must not be hashed with it.

This needs additional test coverage because the existing tests should have failed with the proposed changes in the PRs.

[peterwilsoncc]

Replying to johnbillion:

I think there's some confusion in the pull requests between passwords hashed with bcrypt and passwords hashed with BLAKE2b. Application passwords use the latter via wp_fast_hash() and should not be switched to hashing with wp_hash_password().

@johnbillion Are you able to provide a little more detail here. I find it counter-intuitive to use different hashing algorithms for different types of passwords.

I also know some systems use WordPress authentication for external systems--for example WordPress.org for SVN, trac, etc--so switching to a forked approach for password storage could be problematic for that use case.

Sorry if this was discussed elsewhere, the original PR and trac ticket are long so I couldn't find it when I took a look yesterday.

[johnbillion]

Yeah it's covered in #21022 but that ticket might take all afternoon to read by now so the tl;dr can be found under the "Why switch to BLAKE2b for application passwords and security keys?" heading ​on this PR:

Switching from phpass to the cryptographically secure but fast BLAKE2b algorithm via Sodium is safe for application passwords and security keys which are randomly generated with sufficiently high entropy. Security keys and application passwords are all randomly generated with high entropy via wp_generate_password() from an alpha-numeric character set of size 62. BLAKE2b is highly resistant to preimage attacks (being able to reverse a hash to determine its input) while having a low computational cost.

This algorithm allows high-entropy passwords to be protected with a hashing algorithm that's much faster than bcrypt, which is just what we want for application passwords.

The sharing of password hashes between wordpress.org systems ​has been accounted for by Dion (note: this is a link to a private security channel).

[johnbillion]

In 60123:

Application Passwords: Correct the fallback behaviour for application passwords that don't use a generic hash.

Application passwords that aren't hashed using BLAKE2b should be checked using wp_check_password() rather than assuming they were hashed with phpass. This provides full back compat support for application passwords that were created via an overridden wp_hash_password() function that uses an alternative hashing algorithm.

Props snicco, debarghyabanerjee, peterwilsoncc, jorbin, johnbillion.

Fixes #63203

[johnbillion]

Re-opening to backport [60123] to 6.8.

[audrasjb]

I already reviewed the former PR and this changeset which adds unit tests looks good to me. Marking as ready for 6.8 backport. Thanks for handling this John.

[johnbillion]

In 60125:

Application Passwords: Correct the fallback behaviour for application passwords that don't use a generic hash.

Application passwords that aren't hashed using BLAKE2b should be checked using wp_check_password() rather than assuming they were hashed with phpass. This provides full back compat support for application passwords that were created via an overridden wp_hash_password() function that uses an alternative hashing algorithm.

Reviewed by audrasjb.
Merges [60123] into the 6.8 branch.

Props snicco, debarghyabanerjee, peterwilsoncc, jorbin, johnbillion.

Fixes #63203