Password reset links should expire

[markjaquith]

These links shouldn't sit in e-mail boxes for years, being valid.

[skithund]

Related #21314

[MikeHansenMe]

Refresh of the non-cron patch by dllh on #21314

[nacin]

I feel like we can probably bake this into the user_activation_key field. Might be a bit nicer. :-)

[voldemortensen]

32429.2.diff adds an expiration time to user_activation_key.

[nacin]

Our naming convention for filters in this area is all over the place:

• auth_cookie_expiration
• nonce_life
• wp_feed_cache_transient_lifetime
• oembed_ttl
• post_password_expires

I would suggest, instead of time_to_expire_password_keys, something like password_reset_expiration.

I don't think $time needs to appear in the link itself, nor should it be set into the cookie. It's not user input; it's for validation on the backend. We should be able to contain the logic in check_password_reset_key().

[markjaquith]

@nacin, the reason I suggested keeping the time in the link itself was for 100% backwards compat for anyone who is generating those links themselves. But it’s not a huge concern.

[nacin]

Replying to markjaquith:

@nacin, the reason I suggested keeping the time in the link itself was for 100% backwards compat for anyone who is generating those links themselves. But it’s not a huge concern.

I'm trying to figure out how that would help. If they're generating the link themselves, then they're bypassing retrieve_password() which generates the key, hashes the key, stores the key in the DB, and sends an email with the key. (There's a reason this function wasn't moved to wp-includes/user.php like some others — it does too much to be an API. But it also does everything in one place. That means that if you're generating the link yourself, then you're sending the email yourself, and you're storing something in the DB yourself.

Including the time in the link would only help if we wanted to expire the link via the time as supplied, but that's easily spoofed, thus offering no security and only frustrating the user.

If we wanted, we could expire all activation keys on upgrade (we've done it before), then allow new hashes to enter the DB without a time, as if it were manually entered. Then check_password_reset_key() could choose to only validate the time if it exists in the DB.

Alternatively, we ignore manually entered hashes, thus breaking all old hashes, and forcing new hashes to go through retrieve_password() and for the keys (and internal time) to be confirmed with check_password_reset_key().

Additionally:

• As I said, we've expired all activation keys before. See [25696]. In the process, we changed how keys were actually stored in the DB, and no one complained AFAIK, so I don't think there's much of a compatibility issue here. (This also wasn't that long ago.)

• We can always put a filter in place to allow non-timeboxed hashes to continue to work, exactly how we did it in [25696]. In fact, I would follow that model to a T.

Or did I miss something obvious?

Other things:

• If we're going to change the string for expiredkey (and it looks like a good change), we should change it for invalidkey too. We should look at how other systems handle this message, just to ensure we're using the most mainstream/common language around expired keys. (This string change is perhaps thus better handled in a distinct ticket.)

• password_reset_expiration should be passed DAY_IN_SECONDS.

• Returning the string Invalid key from check_password_reset_key() should be fine. We do it repeatedly already, and wp-login.php overrides the string based on the WP_Error code.

• The code should account for the fact that a :time might not exist in the old hash. It can just check for a : first before exploding. Otherwise that list() will generate a notice.

• Can a colon appear in a phpass hash? Could it appear in a future or alternative hash? e.g. password_hash() or crypt(). As far as I know, no, not for the moment. However, perhaps we should consider reversing the fields, thus doing list( $time, $hash ) = explode( ':' $value, 2 ). Just a thought.

[markjaquith]

Made some changes based on Nacin's feedback, and a few things I had noticed. Needs testing.

[obenland]

Works for me. I reset the expiration time to 1 second to test and it gave me the error. What other ways should we be testing?

[johnbillion]

Tests coming up. Looks like current-style activation keys are being treated as expired.

[johnbillion]

32429.tests.diff​ introduces tests for:

• Valid, invalid (including truncated), and empty keys.
• Valid, invalid, and empty keys when a user has a legacy user_activation_key.
• Valid, invalid, and empty keys when a user has a non-hashed user_activation_key.
• Invalid and empty keys when a user has no user_activation_key.

The tests currently fail because legacy keys are being rejected as expired (using 32429.4.diff​). As Nacin mentioned above, we could actually invalidate these, otherwise a years old key that exists prior to 4.3 will remain valid until a new one is generated.

[dd32]

I feel that breaking existing reset keys should be the expected behaviour here.

Anyone with a now-expired link will just have to request them again - not a major hassle IMHO, and the added security outweighs the risks.

[voldemortensen]

@dd32 said what I've been trying to come up with. The goal here is to make links expire, not sit around in inboxes for someone to have a one shot chance to gain control.

[dd32]

The only thing I can see with this patch is that password_reset_expiration still passes 24 hours when DAY_IN_SECONDS is far more the-wordpress-way. I see no need for it to be a human-readable string.

[voldemortensen]

Updated patch to use DAY_IN_SECONDS.

[MikeHansenMe]

strtotime always returns false if the value passed in is not Human Readable. 32429.6.diff​ corrects this and makes the variables more descriptive.

[dd32]

32429.7.diff​ builds off that to make the password_reset_key_expired filter continue to work.

In previous patches it was impossible for that filter to ever run, for the plain-text keys or for hashed keys without an expiration, now it runs for both.
This is mostly untested, but I think works as expected.

[dd32]

32429.8.diff​ only runs password_reset_key_expired when an old-style key is presented (plain-text, or no $expiration specified)
32429.7.diff​ runs password_reset_key_expired when the key has expired (ie. $expiration < time(), in addition to plain-text or no $expiration)

I'm unsure which is the correct angle to take here, the filter name suggests the latter (7), but the docs and parameters suggest the former (8).
EDIT: Due to the way the filter works, 32429.8.diff needs to be used. The filter only runs when it's a valid old-key but expired.

[dd32]

Additionally, the new string Your password reset token has expired. is never used by core, so the cases of that in check_password_reset_key() should probably be changed back to Invalid key.

[dd32]

#21314 was marked as a duplicate.

[dd32]

In 33019:

Expire password reset links after 24 hours (by default). This causes existing password reset links to become invalid.

Props markjaquith, voldemortensen, johnbillion, MikeHansenMe, dd32
See #32429

[markjaquith]

In 33034:

Fix small typo from [33019].

see #32429

[skithund]

Not to sound ungrateful, but fixing a bug I've reported/patched three years ago (#21314), marking it as duplicate and no props?
The initial patch for this bug was based on a version of my patch.

[johnbillion]

Unfortunately it's not always easy to keep track of who should get props on a given ticket. skithund and dllh definitely should have gotten props here too.

[audrasjb]

In 58333:

Login and Registration: Flush user_activation_key after successfully login.

This changeset ensures the user_activation_key is flushed after successful login, so reset password links can not be used anymore after the user successfully log into their dashboard.

Props nsinelnikov, rajinsharwar, Rahmohn, oglekler, hellofromTonya.
Fixes #58901.
See #32429