WordPress.org

Make WordPress Core

Opened 7 months ago

Last modified 6 months ago

#46748 new defect (bug)

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

Reported by: robert.peake Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.7
Component: Login and Registration Keywords:
Focuses: Cc:
PR Number:

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.

Change History (4)

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

Note: See TracTickets for help on using tickets.