WordPress.org

Make WordPress Core

Opened 7 years ago

Last modified 2 years ago

#31686 new defect (bug) (maybelater)

wp_authenticate_username_password() should check for a WP_Error object

Reported by: kwisatz Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.7
Component: Security Keywords: reporter-feedback
Focuses: Cc:

Description

This is a follow-up to #19714 and #22516

These were closed, but IMHO, the bug as such is not resolved.
wp_authenticate_username_password() does only check whether $user is a WP_Error object when either password or username are empty:

75 if ( empty($username) || empty($password) ) {
76	if ( is_wp_error( $user ) )
77            return $user;
78 …


However, another plugin that hooks into authenticate might pass a WP_Error even if $username and $password were provided, but the specific authentication mechanism failed.
The current implementation completely ignores this.

The result is that users who exist locally can log in using a local, possibly older password, bypassing the plugins' additional authentication mechanisms completely.

I'd advocate to check for is_wp_error($user) much earlier in wp_authenticate_username_password(), so that the authentication process will fail as soon as one "authenticate" filter returns a WP_Error object.

Change History (7)

#1 follow-up: @kwisatz
7 years ago

Appendix:

I am no longer 100% sure about this.

The problem is, what happens when the plugin hooking into the authentication process is unable to perform authentication (perhaps the authentication server is unreachable). In such a case, you might also want to return a WP_Error, but in this case, it's not that the user is invalid, it's just that we cannot tell.

However, even in that case, the Login form should also display the WP_Error returned by plugins hooking into authenticate I guess.

Last edited 7 years ago by kwisatz (previous) (diff)

#2 @DrewAPicture
7 years ago

  • Version changed from trunk to 3.7

#3 @DrewAPicture
7 years ago

  • Component changed from General to Security
  • Keywords reporter-feedback added

Hi @kwisatz, welcome to Trac. Did you still want to try to pursue this? Care to try submitting a patch?

#4 in reply to: ↑ 1 @extendwings
6 years ago

Replying to kwisatz:

the Login form should also display the WP_Error returned by plugins hooking into authenticate

It seems the WP_Error contents from plugins are displayed by this code. (Sorry if I'm missing something.)

#6 @iandunn
3 years ago

  • Resolution set to maybelater

Switching from wontfix to maybelater, since that's more accurate.

xref: https://make.wordpress.org/core/2019/01/14/follow-up-on-recent-trac-bulk-edit/

#7 @donmhico
2 years ago

I'm not sure how to proceed with this. From the documentation of wp_authenticate_username_password()

@param WP_User|WP_Error|null $user  WP_User or WP_Error object from a previous callback. Default null.

It seems that it's expecting the $user input as a WP_Error instance in some cases. And considering that the WP_Error check is placed below the empty $username and empty $password check -

if ( empty( $username ) || empty( $password ) ) {
    if ( is_wp_error( $user ) ) {
        return $user;
    }
    .......
}

It maybe possible that wp_authenticate_username_password() is design to give a chance to a WP_Error $user to still be authenticated if the $username and $password are provided. I'm just not sure if there's a good use-case for such scenario or if this is really an intended behaviour.

IMHO, if the $user passed is a WP_Error then it should immediately return the WP_Error. Like @kwisatz 's concern, this maybe a potential security hole.

We need more feedback regarding this.

Reference:
https://developer.wordpress.org/reference/functions/wp_authenticate_username_password/

Last edited 2 years ago by donmhico (previous) (diff)

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


2 years ago

Note: See TracTickets for help on using tickets.