WordPress.org

Make WordPress Core

Opened 19 months ago

Closed 17 months ago

Last modified 16 months ago

#24973 closed defect (bug) (fixed)

Impossible to login with passwords that contain trailing or leading spaces

Reported by: rpattillo Owned by: 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 19 months 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 17 months ago.
trim-password-2.diff (2.1 KB) - added by joehoyle 17 months ago.

Download all attachments as: .zip

Change History (11)

@rpattillo19 months ago

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

comment:1 @SergeyBiryukov18 months ago

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

comment:2 @nacin17 months ago

  • Keywords needs-unit-tests added

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

@jeremyfelt17 months ago

comment:3 @jeremyfelt17 months 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().

comment:4 @joehoyle17 months ago

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

@joehoyle17 months ago

comment:5 @jeremyfelt17 months 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.

comment:6 @joehoyle17 months ago

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

comment:7 @nacin17 months 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.

comment:8 @dave101016 months 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.