Make WordPress Core

Opened 5 years ago

Last modified 2 years ago

#46748 accepted defect (bug)

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

Reported by: robertpeake's profile robert.peake Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version: 3.7
Component: Login and Registration Keywords: has-unit-tests early needs-patch dev-feedback
Focuses: Cc:

Description

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:

<?php
<?php
/*
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 3 years ago.
46748-test-before-after.gif (1.4 MB) - added by hellofromTonya 2 years 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 (22)

#1 follow-up: @swissspidy
5 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
5 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
5 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
5 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
3 years ago

#52439 was marked as a duplicate.

#6 @johnbillion
3 years ago

  • Keywords needs-patch needs-unit-tests added

@SergeyBiryukov
3 years ago

#7 @SergeyBiryukov
3 years 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.


3 years ago

#9 @audrasjb
3 years 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
3 years 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.


2 years ago
#11

  • 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

@hellofromTonya
2 years ago

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

#12 @TimothyBlynJacobs
2 years 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
2 years 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
2 years 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.

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


2 years ago

#16 @costdev
2 years ago

  • Keywords dev-feedback added

Per the discussion in the bug scrub, I'm adding dev-feedback to this ticket to clarify its current state.

@SergeyBiryukov has kindly offered to take a look at the issue to help move this one forward. Everyone reading this ticket is encouraged to take a look through the discussion above and this discussion on the PR. All input and feedback is welcome to help us land this in 6.0.

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


2 years ago

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


2 years ago

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


2 years ago

#20 @chaion07
2 years ago

  • Milestone changed from 6.0 to Future Release

Thanks @robertpeake for reporting this. We recently reviewed this ticket during a bug-scrub session. Due to the lack of discussion and the progress we are moving it to Future Release.

Props to @costdev & @peterwilsoncc

Cheers!

Note: See TracTickets for help on using tickets.