[wp-trac] [WordPress Trac] #20276: Tie nonces and cookies to expirable sessions

[WordPress Trac]

#20276: Tie nonces and cookies to expirable sessions
-------------------------------------------+------------------
 Reporter:  ryan                           |       Owner:
     Type:  task (blessed)                 |      Status:  new
 Priority:  normal                         |   Milestone:  4.0
Component:  Security                       |     Version:
 Severity:  normal                         |  Resolution:
 Keywords:  has-patch commit dev-feedback  |     Focuses:
-------------------------------------------+------------------

Comment (by mdawaffe):

 attachment:20276.6.diff looks cool.

 A few thoughts of unknown validity about the scheme:

 1. 62^40^ < 2^256^.  We'd need to do `wp_generate_password( 43 )` to
 saturate SHA-256.  I have no idea if that's important.
 2. The patch uses SHA-256. It also breaks all previously generated
 cookies.  Should we use it as an excuse to move from `hash_hmac( 'md5' )`
 to `hash_hmac( 'sha256' )`?  HMAC-MD5 isn't broken, so I don't know if
 matters.
 3. In the [http://www.cse.msu.edu/~alexliu/publications/Cookie/cookie.pdf
 paper the current implementation is based on], the HMAC key is generated
 by doing `key = HMAC( user_name | expiration_time, server_secret )`.  The
 reason it's not just `key = server_secret` is to protect against possible
 future volume attacks on HMAC: each new cookie is signed with a unique
 key.  If that's the only reason, adding the token to the key generation
 isn't necessary. It's possible it hurts since it's not necessarily secret.

 One thought about the API:

 1. The `::update_sessions( $sessions = null )` API is racier than an
 `::update_session( $verifier, $session = null )` API would be

 If one user logs in at nearly the same time from two different clients,
 one client's credentials will be invalidated.

 That doesn't matter for the user_meta implementation, when it's always
 going to be racy, but it does matter for other implementations, which
 would otherwise be able to avoid the race condition.

 Also, other than expirations, there's only one situation in which multiple
 writes are required: `::destroy_other_sessions()`.  Everything else only
 acts on individual sessions, so it seems like an `::update_session()`
 method would be more natural.

 And one last con: requiring `::update_sessions()` reduces the flexibility
 of storage implementations.  With an API based on `::update_session()`,
 only displaying a list of sessions to the user,
 `::destroy_all_sessions()`, and `::destroy_other_sessions()` require
 `::get_sessions()`.  Those events are all rare, so a session storage is
 free to shard in some advantageous way.  For example, a site might want to
 shard based on network "geography".  With an API based on
 `::update_sessions()` I think sites are forced to shard on user_id.

 Here's what an `::update_session()` API might look like (the base class is
 abstract just to make it clear about what parts are the responsibility of
 the storage engine).  It's not as simple: there's more things to
 implement.

 attachment:20276-potential-sessions-api.php

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