WordPress.org

Make WordPress Core

Opened 21 months ago

Closed 19 months ago

Last modified 18 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 21 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 19 months ago.
trim-password-2.diff (2.1 KB) - added by joehoyle 19 months ago.

Download all attachments as: .zip

Change History (11)

@rpattillo21 months ago

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

comment:1 @SergeyBiryukov20 months ago

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

comment:2 @nacin19 months ago

  • Keywords needs-unit-tests added

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

@jeremyfelt19 months ago

comment:3 @jeremyfelt19 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 @joehoyle19 months ago

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

@joehoyle19 months ago

comment:5 @jeremyfelt19 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 @joehoyle19 months ago

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

comment:7 @nacin19 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 @dave101018 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.