Make WordPress Core

Opened 3 years ago

Last modified 3 weeks ago

#46748 accepted defect (bug)

authenticate filter hook does not behave as expected for priority values less than 20

Reported by: robert.peake Owned by: SergeyBiryukov
Milestone: 6.0 Priority: normal
Severity: normal Version: 3.7
Component: Login and Registration Keywords: has-unit-tests early needs-patch
Focuses: Cc:


Returning null or a WP_Error object from functions bound to the authenticate filter at priority values less than 20 does not prohibit a user from logging in.

Consider the following snippet:

Plugin Name: Prohibit Login
Description: Proves that the authenticate filter does not work as expected with lower priority
Author: Robert Peake
Version: 0.1

function prohibit_login($user_or_email, $username = null, $password = null) {
    return new WP_Error('authentication_failed','Prohibit Login plugin prohibited login');
add_filter('authenticate', 'prohibit_login', 20, 3);

This code when activated as a plugin prohibits an admin user from logging in using wp-login.php and displays the message "Prohibit Login plugin prohibited login" as expected.

Changing the value from 20 to e.g. 19 on the final line does not prohibit an admin user from logging in using wp-login.php. No message is displayed, and the login proceeds.

(Note: this has been tested with all other plugins deactivated with the Twentynineteen theme using the latest nightly build.)

While this is not a security problem in itself, because it is undocumented behaviour it could lead to security issues in plugins where an author assumes that, like other filter hooks, e.g. the default priority of 10 can be explicitly stated without side-effects.

For this reason, I initially submitted a ticket on the HackerOne platform just to be sure, but the ticket was closed as being a "hypothetical" vulnerability with out a "clear PoC", so I am filing this bug report instead.

Attachments (2)

46748.diff (1.1 KB) - added by SergeyBiryukov 10 months ago.
46748-test-before-after.gif (1.4 MB) - added by hellofromTonya 3 weeks ago.
Test Report: Confirmed reproducible. Apply PR 1903 and confirmed the filter less than 20 priority now works ✅

Download all attachments as: .zip

Change History (16)

#1 follow-up: @swissspidy
3 years ago

wp_authenticate_username_password runs on priority 20, so I guess this happens because the function does not immediately return $user if it's a WP_Error.

#2 in reply to: ↑ 1 @robert.peake
3 years ago

  • Version set to trunk

Replying to swissspidy:

wp_authenticate_username_password runs on priority 20, so I guess this happens because the function does not immediately return $user if it's a WP_Error.

That makes sense.

Since most other filter and action hooks work as expected more-or-less irrespective of priority in relation to core functionality, I would suggest at minimum that the documentation be updated to reflect the need for a priority of 20+ in order to prevent logins. As you know, the default is 10.

#3 @robert.peake
3 years ago

In the longer term, could wp_authenticate_username_password have its priority lowered (and all other auth-specific core functionality that needs to fire before it be likewise lowered) so that hooking into authenticate with the default priority value of 10 behaves as expected?

Could a proof-of-concept conceivably be created as a plugin to demonstrate this functionality? Or is it too intrinsic to core?

#4 @SergeyBiryukov
3 years ago

  • Version changed from trunk to 3.7

Introduced in [24848].

At a glance, there's no need to lower the priority of wp_authenticate_username_password(), it just needs to immediately return $user if it's a WP_Error, like it was already done for an empty username or password in [24850].

#5 @TimothyBlynJacobs
10 months ago

#52439 was marked as a duplicate.

#6 @johnbillion
10 months ago

  • Keywords needs-patch needs-unit-tests added

#7 @SergeyBiryukov
10 months ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Awaiting Review to 5.8
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

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

6 months ago

#9 @audrasjb
6 months ago

Hi, I tested the patch. It still applies against trunk and it works fine in my testing: it returns $user/$input_user when the result is a WP_Error object.

I'm not sure about which unit tests we should add here, though.

#10 @hellofromTonya
6 months ago

  • Milestone changed from 5.8 to 5.9

Thank you @audrasjb!

Today is 5.8 Beta 1. As this one needs tests, punting to 5.9 to continue progress towards resolution.

This ticket was mentioned in PR #1903 on WordPress/wordpress-develop by costdev.

3 weeks ago

  • Keywords has-unit-tests added; needs-unit-tests removed

This PR:

  • Adds the changes from the existing patch on the ticket.
  • Adds unit tests for wp_authenticate_username_password() with 100% line coverage.

Trac ticket: https://core.trac.wordpress.org/ticket/46748

3 weeks ago

Test Report: Confirmed reproducible. Apply PR 1903 and confirmed the filter less than 20 priority now works ✅

#12 @TimothyBlynJacobs
3 weeks ago

The whole authentication stack is pretty janky to be honest. I think this would be a good candidate for an early ticket.

#13 @audrasjb
3 weeks ago

  • Keywords early added
  • Milestone changed from 5.9 to 6.0

As per the comment above, moving to the next milestone with early keyword so it could get a proper review.

#14 @hellofromTonya
3 weeks ago

  • Keywords needs-patch added; has-patch removed

Hold on. In testing further, 46748.diff blocks email login in wp_authenticate_email_password(). The PR adds an is_email() check. This needs further investigation to determine if the changes are appropriate or if something else is going on. I agree with @TimothyBlynJacobs to punt this to 6.0. Give it time for more discussion and investigation.

Marking as needs-patch as the current patches do not fully resolve the reported issue: i.e. works for username login, but not email login.

Note: See TracTickets for help on using tickets.