Make WordPress Core

Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#24973 closed defect (bug) (fixed)

Impossible to login with passwords that contain trailing or leading spaces

Reported by: rpattillo's profile rpattillo Owned by: nacin's profile nacin
Milestone: 3.7 Priority: normal
Severity: normal Version: 3.6
Component: Users Keywords: has-patch
Focuses: Cc:

Description

It is possible to set a user's password to a string that includes leading or trailing spaces. The spaces will be hashed with the password as it is saved. During login attempts, the spaces will be trimmed before hashing, making it impossible to log in again.

To reproduce:

1] Log in.
2] Navigate to /wp-admin/profile.php to edit your profile.
3] In the new password and confirmation fields, enter any password followed by trailing spaces.
4] Logout
5] Try to log in using the new password with trailing spaces and without.

What is expected:
The password without trailing spaces should work. This would be in line with what happens if the password is changed via the lost password system, and the new password has trailing or leading spaces.

What actually happens:
Neither the password without trailing spaces nor the password with the same trailing spaces will work. It will be impossible for the user to login until the password is reset again.

This is similar to ticket #23494. The resolution there was to trim the password in wp_set_password() before passing it to wp_hash_password(). However not all methods of changing the password go through wp_set_password().

Attachments (3)

trim-passwords.diff (811 bytes) - added by rpattillo 11 years ago.
SVN diff file, adding trim() to wp_hash_password() and removing it from wp_set_password()
24973.diff (1.5 KB) - added by jeremyfelt 11 years ago.
trim-password-2.diff (2.1 KB) - added by joehoyle 11 years ago.

Download all attachments as: .zip

Change History (11)

@rpattillo
11 years ago

SVN diff file, adding trim() to wp_hash_password() and removing it from wp_set_password()

#1 @SergeyBiryukov
11 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.7

#2 @nacin
11 years ago

  • Keywords needs-unit-tests added

trim-passwords.diff could very much benefit from unit tests.

@jeremyfelt
11 years ago

#3 @jeremyfelt
11 years ago

  • Keywords needs-unit-tests removed

24973.diff refreshes the current patch against develop.svn and adds unit tests for testing a trim of the password in wp_hash_password().

#4 @joehoyle
11 years ago

@jeremyfelt: looks like to beat me to it! Though mine tests new lines and vertical tabs :P

#5 @jeremyfelt
11 years ago

Better tests indeed! It looks like some leftovers from another ticket made their way into trim-password-2.diff. Nothing big, only some docs.

#6 @joehoyle
11 years ago

I cleaned the patch by overwriting it, are you maybe you were looking at it before I updated the patch?

#7 @nacin
11 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 25709:

Move the trim() from wp_set_password() to inside wp_hash_password().

props rpattillo, joehoyle.
fixes #24973. see #23494.

#8 @dave1010
11 years ago

WordPress is trim()ing passwords to make a better UX. Going down the same route, should WordPress also strtolower() passwords all the time, in case users accidentally have caps lock on? Should WordPress remove duplicate consecutive characters, in case the user held a key down too long?

Both trim() and strtolower() sacrifice password entropy for UX. You could easily argue that the UX gains from this ticket are worth the reduction in security, but the scary thing is that the security implications don't seem to have been considered! (At least in this ticket and from a quick Google, apologies if this was discussed elsewhere).

Is WordPress' modification of users' passwords documented anywhere?

Note: See TracTickets for help on using tickets.