WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#32429 closed defect (bug) (fixed)

Password reset links should expire

Reported by: markjaquith Owned by: markjaquith
Milestone: 4.3 Priority: normal
Severity: normal Version:
Component: Security Keywords: has-patch
Focuses: Cc:

Description

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

Attachments (9)

32429.diff (2.6 KB) - added by MikeHansenMe 3 years ago.
Refresh of the non-cron patch by dllh on #21314
32429.2.diff (3.0 KB) - added by voldemortensen 3 years ago.
32429.3.diff (1.9 KB) - added by voldemortensen 3 years ago.
32429.4.diff (2.2 KB) - added by markjaquith 3 years ago.
32429.tests.diff (4.6 KB) - added by johnbillion 3 years ago.
32429.5.diff (2.2 KB) - added by voldemortensen 3 years ago.
32429.6.diff (2.2 KB) - added by MikeHansenMe 3 years ago.
32429.7.diff (5.5 KB) - added by dd32 3 years ago.
32429.8.diff (5.7 KB) - added by dd32 3 years ago.

Download all attachments as: .zip

Change History (43)

This ticket was mentioned in Slack in #core-passwords by mark. View the logs.


3 years ago

@MikeHansenMe
3 years ago

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

This ticket was mentioned in Slack in #core by mark. View the logs.


3 years ago

#4 @nacin
3 years ago

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

#5 @voldemortensen
3 years ago

  • Keywords has-patch added

32429.2.diff adds an expiration time to user_activation_key.

This ticket was mentioned in Slack in #core-passwords by voldemortensen. View the logs.


3 years ago

#7 @nacin
3 years ago

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().

#8 follow-up: @markjaquith
3 years ago

@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.

This ticket was mentioned in Slack in #core by mark. View the logs.


3 years ago

#10 in reply to: ↑ 8 @nacin
3 years ago

  • Milestone changed from Awaiting Review to 4.3

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
3 years ago

#11 @markjaquith
3 years ago

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

This ticket was mentioned in Slack in #core-passwords by mark. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by mark. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by obenland. View the logs.


3 years ago

#15 @obenland
3 years ago

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?

This ticket was mentioned in Slack in #core by mark. View the logs.


3 years ago

#17 @MikeHansenMe
3 years ago

  • Keywords needs-unit-tests added

#18 @johnbillion
3 years ago

  • Owner set to johnbillion
  • Status changed from new to reviewing

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

#19 @johnbillion
3 years ago

  • Keywords needs-unit-tests removed
  • Owner changed from johnbillion to markjaquith

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.

#20 @dd32
3 years ago

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.

#21 @voldemortensen
3 years ago

@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.

#22 @dd32
3 years ago

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.

#23 @voldemortensen
3 years ago

Updated patch to use DAY_IN_SECONDS.

@MikeHansenMe
3 years ago

#24 @MikeHansenMe
3 years ago

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
3 years ago

#25 @dd32
3 years ago

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
3 years ago

#26 @dd32
3 years ago

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.

Last edited 3 years ago by dd32 (previous) (diff)

#27 @dd32
3 years ago

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.

#28 @dd32
3 years ago

#21314 was marked as a duplicate.

#29 @dd32
3 years ago

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

#30 @markjaquith
3 years ago

In 33034:

Fix small typo from [33019].

see #32429

#31 @skithund
3 years ago

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.

#32 @johnbillion
3 years ago

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.

This ticket was mentioned in Slack in #core-passwords by adamsilverstein. View the logs.


3 years ago

#34 @obenland
3 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.