WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 8 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: 5.9 Priority: normal
Severity: normal Version: 3.7
Component: Login and Registration Keywords: needs-unit-tests has-patch
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 (1)

46748.diff (1.1 KB) - added by SergeyBiryukov 6 months ago.

Download all attachments as: .zip

Change History (11)

#1 follow-up: @swissspidy
2 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
2 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
2 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
2 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
6 months ago

#52439 was marked as a duplicate.

#6 @johnbillion
6 months ago

  • Keywords needs-patch needs-unit-tests added

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


2 months ago

#9 @audrasjb
2 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
8 weeks 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.

Note: See TracTickets for help on using tickets.