Make WordPress Core

Opened 7 years ago

Last modified 6 years ago

#42183 reviewing defect (bug)

wp_update_user() conditional compares a plain-text password to the hashed old

Reported by: yudge's profile yudge Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version: 4.5.2
Component: Users Keywords: has-patch dev-feedback needs-refresh needs-unit-tests
Focuses: administration Cc:

Description

In file wp-includes/user.php, function wp_update_user()
On line 1767:

if ( ! empty( $userdata['user_pass'] ) && $userdata['user_pass'] !== $user_obj->user_pass)

The second conditional is comparing a plain-text password to a hashed version of password, so this would almost always evaluate to true except for the case where the new password itself matches the old hashed password. This block will then evaluate to false and therefore password itself won't be updated. It's a rare case but the logic here is incorrect. And obviously this code block would run when passwords are the same since it's comparing plain-text to the hashed version.

Attachments (3)

42183.patch (916 bytes) - added by rinkuyadav999 7 years ago.
42183.1.patch (720 bytes) - added by rinkuyadav999 7 years ago.
42183.2.patch (1.1 KB) - added by johnjamesjacoby 7 years ago.
Uses wp_check_password() and renames a variable to avoid keeping the plaintext password floating around in another variable

Download all attachments as: .zip

Change History (11)

@rinkuyadav999
7 years ago

#1 @rinkuyadav999
7 years ago

  • Focuses administration added
  • Keywords has-patch dev-feedback added

#2 @johnjamesjacoby
7 years ago

Hi @yudge, thanks for your ticket here. Hello @rinkuyadav999, thanks also for the patch!

I think the most-correct approach is to use wp_check_password() instead of loading the hasher directly. That function includes considerations for backwards compatibility issues, which are especially useful during this password change workflow.

Unfortunately, wp_insert_user() also still expects the $userdata['user_pass'] to be hashed already, so we'll need to use both functions back to back to maintain backwards compatibility through-out the rest of the system.

I'll attach a next-pass patch imminently for deeper scrutiny.

#3 @rinkuyadav999
7 years ago

Hi @johnjamesjacoby

Yes, that's why i used wp_check_password() in 42183.1.patch

@johnjamesjacoby
7 years ago

Uses wp_check_password() and renames a variable to avoid keeping the plaintext password floating around in another variable

#4 @johnjamesjacoby
7 years ago

Yes, that's why i used wp_check_password() in 42183.1.patch

I didn't see the second email come through. Sorry about that. Glad we're on the same page!

#5 @yudge
6 years ago

  • Keywords needs-refresh added

Any status update on implementing this into the core?

Last edited 6 years ago by yudge (previous) (diff)

#6 @yudge
6 years ago

Any status update on implementing this into the core?

#7 @SergeyBiryukov
6 years ago

  • Milestone changed from Awaiting Review to 5.1
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#8 @pento
6 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from 5.1 to Future Release

Patch needs review and testing.

Note: See TracTickets for help on using tickets.