Make WordPress Core

Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#33904 closed defect (bug) (fixed)

user_activation_key is too short causing password reset process to break when using bcrypt

Reported by: tomdxw's profile tomdxw Owned by: ocean90's profile ocean90
Milestone: 4.4 Priority: normal
Severity: normal Version: 4.3
Component: Database Keywords: good-first-bug has-patch
Focuses: Cc:

Description

The field for storing the hash of the password reset token (user_activation_key in wp_users) is a varchar(60).

In 4.3, WordPress very wisely started including a timestamp field in user_activation_key. This poses no problem when using the default "portable" hashing algorithm which takes up 34 bytes in string form.

The timestamp plus the colon plus the hash takes up just 45 bytes. Plenty of space left over.

However, at my company we use a plugin which replaces the default $wp_hasher instance with something that produces slightly harder-to-crack hashes (i.e. bcrypt):

$wp_hasher = new PasswordHash(12, false);

bcrypt produces password hashes which are 60 bytes long. So the timestamped hash would be 71 bytes long.

And this means that the password reset mechanism breaks entirely. WordPress attempts to store the new value in the database, but MySQL complains because it's trying to store 71 bytes in a varchar(60).

But it still sends the email, and the user is left wondering why they can't reset their password.

Attachments (1)

33904.patch (1.6 KB) - added by grvrulz 8 years ago.
Change default length of user_pass and user_activation_key to 255 in schema

Download all attachments as: .zip

Change History (12)

#1 @shazahm1@…
8 years ago

Ha! I wish I would have been able to find this sooner! I spent 5 hours trying to track down why password resets were no longer working for users. I too use bcrypt which was causing the hash to be 71 characters long which would cause the db update to fail for user_activation_key to fail. changing this to 100 fixed it for me but core should allow longer hashed to be stored by default for a bit more breathing room.

#2 @Otto42
8 years ago

The size of the password field should probably be increased to 255 for future proofing.

Ref the notes on PASSWORD_DEFAULT here: https://secure.php.net/manual/en/function.password-hash.php

#3 @SergeyBiryukov
8 years ago

  • Component changed from Users to Database
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.4

#4 @pento
8 years ago

  • Keywords good-first-bug added

I'd be fine for extending user_pass and user_activation_key to 255.

If someone feels like picking this up, note that you'll need to change the definition for both the single site and multisite user tables.

@grvrulz
8 years ago

Change default length of user_pass and user_activation_key to 255 in schema

#5 @swissspidy
8 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

#6 @pento
8 years ago

Testing note: dbDelta() shouldn't upgrade these tables when DO_NOT_UPGRADE_GLOBAL_TABLES is defined, in both single site and multisite.

#7 follow-up: @mattheweppelsheimer
8 years ago

@tomdxw is your the bcrypt hasher plugin available to link to, to assist in testing the patch?

#8 in reply to: ↑ 7 @tomdxw
8 years ago

Replying to mattheweppelsheimer:

@tomdxw is your the bcrypt hasher plugin available to link to, to assist in testing the patch?

Yes: https://wordpress.org/plugins/wp-bcrypt/

#9 @Miglosh
8 years ago

  • Keywords needs-testing removed

Tested 33904.patch.

For update of database to be triggered, in wp-includes/version.php, I incremented $wp_db_version plus one for testing.

Single site:
When setting DO_NOT_UPGRADE_GLOBAL_TABLES, the tables remain unchanged.
Otherwise, tables are updated according to the new version of schema.php.

Multisite:
When setting DO_NOT_UPGRADE_GLOBAL_TABLES, the tables remain unchanged.
Otherwise, tables are updated according to the new version of schema.php.

#10 @ocean90
8 years ago

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

In 35638:

Schema: Increase length of user_pass and user_activation_key fields to 255.

Props grvrulz and Miglosh for testing.
Fixes #33904.

#11 @skithund
8 years ago

#34820 was marked as a duplicate.

Note: See TracTickets for help on using tickets.