[wp-trac] [WordPress Trac] #32429: Password reset links should expire

[WordPress Trac]

#32429: Password reset links should expire
--------------------------+------------------
 Reporter:  markjaquith   |       Owner:
     Type:  defect (bug)  |      Status:  new
 Priority:  normal        |   Milestone:  4.3
Component:  Security      |     Version:
 Severity:  normal        |  Resolution:
 Keywords:  has-patch     |     Focuses:
--------------------------+------------------
Changes (by nacin):

 * milestone:  Awaiting Review => 4.3


Comment:

 Replying to [comment:8 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.

--
Ticket URL: <https://core.trac.wordpress.org/ticket/32429#comment:10>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform