Make WordPress Core

Opened 17 months ago

Closed 12 months ago

Last modified 12 months ago

#56787 closed defect (bug) (fixed)

Recovery mode tokens can't be validated successfully if pluggable function wp_check_password is overwritten.

Reported by: calvinalkan's profile calvinalkan Owned by: timothyblynjacobs's profile TimothyBlynJacobs
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: @TimothyBlynJacobs
17 months 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

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.

#2 in reply to: ↑ 1 @calvinalkan
17 months 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.

Last edited 17 months ago by calvinalkan (previous) (diff)

This ticket was mentioned in Slack in #core by costdev. View the logs.


13 months ago

#4 @hellofromTonya
13 months ago

  • Keywords needs-patch needs-testing needs-unit-tests needs-testing-info added

This ticket was mentioned in PR #4058 on WordPress/wordpress-develop by @TimothyBlynJacobs.


13 months 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 @TimothyBlynJacobs
13 months 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.

#7 @TimothyBlynJacobs
12 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 55397:

Recovery Mode: Use PasswordHash API directly when validating keys.

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.

Props calvinalkan.
Fixes #56787.

Note: See TracTickets for help on using tickets.