WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 6 weeks ago

#20276 new task (blessed)

Tie nonces and cookies to expirable sessions

Reported by: ryan Owned by:
Milestone: 4.0 Priority: normal
Severity: normal Version:
Component: Security Keywords: has-patch commit dev-feedback
Focuses: Cc:

Description (last modified by duck_)

Authentication cookies are re-usable even after a user decides to explicitly logout. Cookies should be tied to an expirable session that can also be deleted upon logout.

Also, nonce security can be improved by associating them with the same session information. Owasp specifies that "the synchronizer token pattern requires the generating of random challenge tokens that are associated with the user's current session." Our nonces have a timeout, but that timeout can span cookie sessions. Instead, nonces should be somehow tied to the current auth cookie and invalidate whenever the cookie invalidates.

https://www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)_Prevention_Cheat_Sheet

Attachments (11)

20276.diff (6.2 KB) - added by duck_ 11 months ago.
20276.2.diff (7.5 KB) - added by duck_ 11 months ago.
20276.3.diff (7.6 KB) - added by duck_ 11 months ago.
20276.backpress.diff (6.0 KB) - added by duck_ 9 months ago.
20276.backpress.2.diff (6.1 KB) - added by duck_ 9 months ago.
20276.4.diff (7.9 KB) - added by nacin 7 weeks ago.
Refreshed. Would like to make it easy to replace session storage, such as spinning it off to a separate table for sites with already crazy usermeta tables like .com or .org. Either well-placed filters or could be ripe for an interface/object.
20276.5.diff (10.4 KB) - added by nacin 6 weeks ago.
sessions-table.php (2.8 KB) - added by nacin 6 weeks ago.
more-session-info.php (197 bytes) - added by nacin 6 weeks ago.
20276.6.diff (11.7 KB) - added by nacin 6 weeks ago.
20276-potential-sessions-api.php (6.2 KB) - added by mdawaffe 6 weeks ago.

Download all attachments as: .zip

Change History (36)

comment:1 nacin2 years ago

  • Type changed from defect (bug) to enhancement

I imagine we can take a piece of the auth cookie and include it in the hash. We'll need to include an identifier at a consistent location in the nonce in order to make note of which cookie was used, as we are going to want to leverage the SSL cookie if possible, other times we may need to use the logged_in cookie (say, the logout nonce).

If we generate a nonce in the backend with an admin cookie, but try to use the nonce on the frontend, the nonce will fail. So perhaps we need to stick to logged_in cookie for now.

comment:2 scribu2 years ago

  • Cc scribu added

comment:3 johnbillion2 years ago

  • Cc johnbillion added

comment:4 juliobox18 months ago

  • Cc juliobosk@… added

duck_11 months ago

comment:5 duck_11 months ago

  • Keywords has-patch added
  • Milestone changed from Future Release to 3.7
  • Summary changed from Tie nonces to the current session to Tie nonces and cookies to expirable sessions

20276.diff is a first pass at implementing expirable sessions. This patch aims to make auth cookies unforgeable with read-only access to filesystem and/or database, and invalidate auth cookies upon explicit logout.

On login a long random string, r, is generated. r is included in the user's cookie and H(r) is stored in the database. On future requests, r is extracted from the cookie and H(r) is compared to the value in the database. Storing the hash of r means that read-only SQL injection does not allow an attacker to create cookies since they cannot reverse the value to find a valid r. Each "session token" is also associated with an expiry time, so they can only be used for a limited time.

This information is stored in the database as a piece of user meta named "session_tokens" that is an array mapping the random strings to expiry time. Expired tokens are cleared upon login and logout. The current token, as found in the cookie, is also removed from the array on logout.

Questions to answer:

  • Should there be a limit on the number of session tokens?
  • How should the new $token parameter be added to wp_generate_auth_cookie()? Currently it's just added in the middle. This is nicer, but not backwards compatible.
  • Could documentation about these "sessions" be confused with native PHP sessions?

Todo:

  • Use the session token as part of nonce generation.
  • Add an action if wp_verify_session_token() fails (?)
  • Better method for expiry. _session_not_expired() calls time() for every array element.

comment:6 duck_11 months ago

  • Description modified (diff)

How should the new $token parameter be added to wp_generate_auth_cookie()? Currently it's just added in the middle. This is nicer, but not backwards compatible.

The best solution to this is probably going to be to have $token added as an optional fourth parameter. If it is not present then a session token will be generated automatically.

comment:7 DrewAPicture11 months ago

  • Cc xoodrew@… added

duck_11 months ago

comment:8 duck_11 months ago

20276.2.diff uses the session token during nonce generation/verification and makes the new $token parameter optional as described above.

comment:9 ethitter11 months ago

  • Cc erick@… added

duck_11 months ago

comment:10 duck_11 months ago

20276.3.diff adds the auth_cookie_bad_session_token action, and switches session expiry to a duplicated foreach loop.

comment:11 nacin11 months ago

This will have OMG BBQ implications for BackPress-driven sites like bbPress 1.x (so, a lot of WordPress.org). Please do not commit until we figure out this piece. (Related in this regard: #24783.)

comment:12 nacin10 months ago

  • Type changed from enhancement to task (blessed)

duck_9 months ago

duck_9 months ago

comment:13 nacin9 months ago

  • Keywords commit added
  • Milestone changed from 3.7 to 3.8

Let's get this in first thing in 3.8.

comment:14 Denis-de-Bernardy8 months ago

  • Cc ddebernardy@… added

comment:15 nacin7 months ago

  • Keywords 3.9-early added
  • Milestone changed from 3.8 to Future Release

Per conversation with duck_ at WordCamp London, we're going to hold on this for another release, in part to weigh costs and benefits.

comment:16 helen5 months ago

#27126 was marked as a duplicate.

nacin7 weeks ago

Refreshed. Would like to make it easy to replace session storage, such as spinning it off to a separate table for sites with already crazy usermeta tables like .com or .org. Either well-placed filters or could be ripe for an interface/object.

nacin6 weeks ago

nacin6 weeks ago

nacin6 weeks ago

comment:17 nacin6 weeks ago

  • Keywords dev-feedback added; 3.9-early removed
  • Milestone changed from Future Release to 4.0

20276.4.diff is a refresh of duck_'s work here.

On a multisite network with a massive amount of users (take, for example, wordpress.com or wordpress.org), usermeta is the largest table in the system. Sessions churn often and it may be desirable to keep these in a separate database table. It's actually not as much for the table as much to get them out of the usermeta cache bucket, which can often get really, really big.

So, 20276.5.diff is an alternate take on how we're managing session storage. It bundles up everything into a WP_Session_Tokens class with public methods create_token(), verify_token(), destroy_session(), and destroy_sessions(). Additionally, it has two methods that can be extended: get_sessions() and update_sessions(). You can specify an alternate class using the session_token_manager filter that implements these methods to push/pull from an alternative data store.

Additionally, the plugin allows for additional data to be stored in a session. By default, we're just storing key/value pairs which are the verifier (hash of the token) and its expiration. But the attach_session_information filter can be used to add additional information, such as the user agent, IP address, and timestamp when the session was created. I actually engineered this to work out of the box, making it so get_sessions() always returns an associative array for each session but only storing the expiration as the value if additional data isn't being used.

In terms of API and what someone could actually do with this: get_sessions() is a public method that could be used to enumerate a user's sessions, such as what Gmail does. destroy_sessions() is essentially a "log me out everywhere" feature. destroy_other_sessions() doesn't exist yet but makes sense as the "log me out everywhere else" feature.

There is not an official way to update a session, but a more complicated data store can handle that in update_sessions(). Use case would be keeping a log of future activity on that session, such as new IPs used by it, a "last seen" time, etc.

Additionally, there's no requirement that an expired session actually be deleted from the data store. So a separate table could, for example, mark a session as destroyed or expired but keep it around for a set period of time for auditing purposes (such as letting someone know someone had logged in from somewhere else, even if they since logged out). To do this in core by default for expired sessions would be easy, either with a filter or by default. For destroyed sessions, we could set "expired" value to -1 or false (when we're storing only minimal information, of course).

I've attached two plugins. more-session-info.php stores user agent, IP address, and the timestamp when the session was created in existing usermeta. sessions-table.php uses a global sessions table (you'll have to add it on your own). The sessions table class works, though it's not ready for production, as it lacks persistent caching. Easy to do.

comment:18 ircbot6 weeks ago

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.

comment:19 follow-up: jeremyfelt6 weeks ago

20276.5.diff is pretty wonderful.

I'm still testing it locally, but I dig the extendability. I like the idea of having a Gmail style "your other sessions" area. Sessions were created for additional browsers as expected. When I invalidated the session in Chrome, the session in Firefox remained valid.

One note so far—if the salt keys in wp-config.php are changed, the session is invalidated as expected. However, the original session is not removed from the DB and the new session piles on. This *could* cause clutter over time.

comment:20 in reply to: ↑ 19 nacin6 weeks ago

Replying to jeremyfelt:

One note so far—if the salt keys in wp-config.php are changed, the session is invalidated as expected. However, the original session is not removed from the DB and the new session piles on. This *could* cause clutter over time.

They'll eventually be removed upon expiration. By default, WordPress allows sessions to last for 48 hours, and "Remember me" extends that to 14 days. So the clutter would not be severe.

However, I'd like to bump "Remember me" to something like 30, 60, 90, or even 365 days once we get this in. That could indeed result in clutter. I'm not terribly concerned about more stuff in the DB, but since these sessions are invalid, then the get_sessions() method would lie when used for presentation purposes.

One option would be to take all keys/salts and hash them into a single DB option, and watch for that hash to change. If it does, simply invalidate all sessions, since that's what is happening anyway. That's achievable via API with delete_metadata( 'user', false, 'session_tokens', '', true ); and would probably be wrapped up into a staticdestroy_sessions_for_all_users() method.

nacin6 weeks ago

comment:21 nacin6 weeks ago

20276.6.diff introduces destroy_all_sessions_for_all_users() as a static method. It also renames ::destroy_sessions() to ::destroy_all_sessions(), and introduces the wrapper wp_destroy_all_sessions(). And, it introduces ::destroy_other_sessions( $token_to_keep ) and introduces the wrapper wp_destroy_other_sessions().

There is no wrapper for ::destroy_all_sessions_for_all_users(), but it can be called as WP_User_Sessions::destroy_all_sessions_for_all_users(). It occurs to me that this doesn't work when the class is replaced with the attach_session_information filter, though. Shouldn't be difficult to come up with something, though we'll need to avoid late static bindings.

comment:22 follow-up: mdawaffe6 weeks ago

attachment:20276.6.diff looks cool.

A few thoughts of unknown validity about the scheme:

  1. 6240 < 2256. 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 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

comment:23 ircbot6 weeks ago

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.

comment:24 in reply to: ↑ 22 duck_6 weeks ago

Replying to mdawaffe:

A few thoughts of unknown validity about the scheme:

  1. 6240 < 2256. We'd need to do wp_generate_password( 43 ) to saturate SHA-256. I have no idea if that's important.

Doesn't matter. It just needs to be unguessable. Way over 2128 possibilities as is, so it's safe.

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

Sure. As you say, it's not broken, but why not.

  1. In the 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.

Indeed, it's not necessary. But doesn't hurt either since both user and expiration are also known. However, I didn't have any reason for adding to key computation, so happy to take away for simplicity.

comment:25 mdawaffe6 weeks ago

but why not.

take away for simplicity

I don't understand the consequences of a decision either way.

Note: See TracTickets for help on using tickets.