#56787 closed defect (bug) (fixed)
Recovery mode tokens can't be validated successfully if pluggable function wp_check_password is overwritten.
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.2 | Priority: | normal |
Severity: | normal | Version: | 5.2 |
Component: | Site Health | Keywords: | needs-testing has-patch has-testing-info |
Focuses: | Cc: |
Description
WordPress allows users to override the wp_hash_password and wp_check_password functions with alternative implementations.
How passwords are hashes is an implementation detail. Call sites must not make assumptions about how they are implemented.
WordPress [generates recovery mode tokens using PHPass's PasswordHash class](https://github.com/WordPress/WordPress/blob/c03305852e7e40e61cad5798eba9ebc3b961e27a/wp-includes/class-wp-recovery-mode-key-service.php#L57).
To validate recovery tokens, wp_check_password
[is used](https://github.com/WordPress/WordPress/blob/c03305852e7e40e61cad5798eba9ebc3b961e27a/wp-includes/class-wp-recovery-mode-key-service.php#L109).
This is a bug. Any implementation of wp_check_password
that doesn't use PHPass will cause the recovery tokens to be always invalid.
There are two possibilities:
- Either use PasswordHash::HashPassword() + PasswordHash::CheckPassword() or
- Use wp_hash_password and wp_check_password
Mixing the two violates the Liskov substitution principle (if we consider pluggable functions as the WordPress version of interfaces).
In all other places in Core, this principle is respected. It looks like recovery tokens slipped through.
Change History (8)
#1
follow-up:
↓ 2
@
2 years ago
- Component changed from Login and Registration to Site Health
- Milestone changed from Awaiting Review to 6.2
- Owner set to TimothyBlynJacobs
- Status changed from new to accepted
- Version changed from 6.0.2 to 5.2
#2
in reply to:
↑ 1
@
2 years ago
Replying to TimothyBlynJacobs:
Thanks for the ticket @calvinalkan!
Agreed, this should've been consistent with using or not using a pluggable API. I'm in favor of using
PasswordHash
directly. Matching how we handle User Request and Password Reset tokens.
Arguably even plain sha256 is enough. PasswordHash is more than sufficient. Anything that uses slow hashes inside a custom pluggable function would only waste electricity.
<?php $key = wp_generate_password( 22, false );
This has enough entropy.
This ticket was mentioned in Slack in #core by costdev. View the logs.
2 years ago
This ticket was mentioned in PR #4058 on WordPress/wordpress-develop by @TimothyBlynJacobs.
2 years ago
#5
- Keywords has-patch added; needs-patch removed
Previously, the wp_check_password function was used for validating keys, while the PasswordHash class was used for creating keys. This would prevent Recovery Mode from working on sites that provide a custom implementation for the wp_check_password pluggable function.
Trac ticket: https://core.trac.wordpress.org/ticket/56787
#6
@
2 years ago
- Keywords has-testing-info added; needs-unit-tests needs-testing-info removed
This should be covered by the existing unit tests. To manually test, edit a plugin on your site to introduce a fatal error. For example:
<?php add_action( 'init', function() { $this->method(); } );
You should be able to receive a Recovery Mode email to the Admin Email address configured in Settings → General. Clicking on the emailed link should enter you into Recovery Mode.
Thanks for the ticket @calvinalkan!
Agreed, this should've been consistent with using or not using a pluggable API. I'm in favor of using
PasswordHash
directly. Matching how we handle User Request and Password Reset tokens.