Tie nonces and cookies to expirable sessions

[ryan]

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

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.

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.

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.

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

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

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

Let's get this in first thing in 3.8.

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

#27126 was marked as a duplicate.

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

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

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.

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.

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.

attachment:20276.6.diff​ looks cool.

A few thoughts of unknown validity about the scheme:

1. 62⁴⁰ < 2²⁵⁶. 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​

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

Replying to mdawaffe:

A few thoughts of unknown validity about the scheme:

1. 62⁴⁰ < 2²⁵⁶. 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 2¹²⁸ possibilities as is, so it's safe.

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.

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

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.

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.

but why not.

take away for simplicity

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

In [29221]:

Tie cookies and nonces to user sessions so they may be invalidated upon logout.

Sessions are stored in usermeta via WP_User_Meta_Session_Tokens, which extends the abstract WP_Session_Tokens class. Extending WP_Session_Tokens can allow for alternative storage, such as a separate table or Redis.

Introduces some simple APIs for session listing and destruction, such as wp_get_all_sessions() and wp_destroy_all_sessions().

This invalidates all existing authentication cookies, as a new segment (the session token) has been added to them.

props duck_, nacin, mdawaffe.
see #20276.

---

Note the original commit message said wp_get_active_sessions() — it is actually wp_get_all_sessions().

I would like to see some additional review on the API in [29221]. It's feeling really good, though.

Here's what it looks like:

function wp_get_session_token() {
function wp_get_all_sessions() {
function wp_destroy_current_session() {
function wp_destroy_other_sessions() {
function wp_destroy_all_sessions() {

abstract class WP_Session_Tokens {
	protected function __construct( $user_id ) {
	final public static function get_instance( $user_id ) {
	final private function hash_token( $token ) {
	final public function verify_token( $token ) {
	final public function create_token( $expiration ) {
	final public function update_token( $token, $session ) {
	final public function destroy_token( $token ) {
	final public function destroy_other_tokens( $token_to_keep ) {
	final protected function is_still_valid( $session ) {
	final public function destroy_all_tokens() {
	final public static function destroy_all_tokens_for_all_users() {
	final public function get_all_sessions() {

	abstract protected function get_sessions();
	abstract protected function get_session( $verifier );
	abstract protected function update_session( $verifier, $session = null );
	abstract protected function destroy_other_sessions( $verifier );
	abstract protected function destroy_all_sessions();
	abstract public static function drop_sessions();

class WP_User_Meta_Session_Tokens extends WP_Session_Tokens {
	. . . abstract methods are implemented, plus:
	protected function prepare_session( $session ) {
	protected function update_sessions( $sessions ) {

Unsure we can do {{{abstract public static function drop_sessions}}, see a related ticket #28942. I'm unsure about this because of PHP 5.2. I believe doing abstract and static for a method call is always late static binding. Which is only supported from PHP 5.3 and up.

Replying to markoheijnen:

Unsure we can do {{{abstract public static function drop_sessions}}.

Indeed, generates ​https://cloudup.com/ia9tzSPMyno as of r29221

<b>Strict Standards</b>:  Static function WP_Session_Tokens::drop_sessions() should not be abstract
in <b>/srv/www/wordpress-develop/src/wp-includes/session.php</b> on line <b>250</b><br />

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

Replying to stephdau:

generates ​https://cloudup.com/ia9tzSPMyno as of r29221

And that's on the current version of VVV (PHP 5.5.9):

vagrant@vvv:/vagrant/www/wordpress-develop$ php --version
PHP 5.5.9-1ubuntu4.3 (cli) (built: Jul  7 2014 16:36:58) 

FWIW, you can specify static methods on an interface, so that might work better here.

What does the upgrade process from <4.0 to 4.0 look like if we're invalidating the current session during the upgrade?

I was dumped to w-login.php, with this warning at the top, after a GUU (grand unified updater) initiated upgrade from yesterday's nightly to a forced nightly refresh containing the work since last night. So, nightly to nightly upgrade was interrupted with log in.

I get a parse error on PHP 5.2.17:

Parse error: syntax error, unexpected T_PAAMAYIM_NEKUDOTAYIM in wp-includes/session.php on line 184

Needs a call_user_func for 5.2 compatability.

see: ​http://stackoverflow.com/questions/3787957/cant-call-static-method-from-class-as-variable-name

I believe 20276.call_user_func.diff should fix it, Sergey, can you confirm? I don't have a 5.2 environment.

In 29222:

Fix parse error in PHP 5.2.

props georgestephanis.
see #20276.

Beta testers still get an ugly notice. Can we clear that and force a nightly?

​https://cloudup.com/cCgJWXXNQ9R

Replying to johnbillion:

What does the upgrade process from <4.0 to 4.0 look like if we're invalidating the current session during the upgrade?

One of two things will happen:

• For a user not performing an update, they'll get a login screen, probably as a result of the auth check JS. Easy peasy.

• For a user performing the update, they'll be dumped to wp-login.php on the way to about.php. There is some code in wp-login.php to detect when about.php is the redirect_to. It then displays a message: "You have successfully updated WordPress! Please log back in to experience the awesomeness." We did this back when we were changing how salts were auto-generated.

Either way, not a problem.

In 29224:

WP_Session_Tokens: Can't abstract a static method.

Implementations can choose to implement the drop_sessions() method on their own.

see #20276.

Replying to nacin:

In 29224:

WP_Session_Tokens: Can't abstract a static method.

Implementations can choose to implement the drop_sessions() method on their own.

see #20276.

Thanks, that fixed the notice for me.

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

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

Can we call this fixed? Anything left to do here?

"You have successfully updated WordPress! Please log back in to experience the awesomeness".

I don't know about others, but this kind of tone in a message is the last thing I would want a serious client to see. Perhaps its tone comes across differently in your culture, but to me as a Brit, it cries out "built by amateurs!", which of course WP is not.

I'd fear that a client who was serious money for a website, and who saw a message like that, would immediately think "What kind of toy website are these people building for me? Are they children?"

I meant to bring this up myself. I agree with David. "Please log back in to experience the awesomeness" is a horrendous phrase.

+1 for shortening this to "Please log back in."

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

Any string changes are unrelated to this and can be dealt with in new tickets.

In 29635:

Rename the public methods in the session tokens API.

Introduces a new get( $token ) method. get_token() would not have made sense and spurred the overall renaming. Public methods are now get, get_all, verify, create, update, destroy, destroy_others, and destroy_all.

The protected abstract methods designed for alternative implementations remain the same.

props mdawaffe.
see #20276.

After flexing the API as a result of starting development on a plugin and also presenting about it at WordCamp Boston last weekend, I noticed a pretty major hole — there was no way to get session information, given a token. A good use case would be to get the active session and attach new session information to it. You could call update_token() but there was no getter.

get_token( $token ) (returning a session) didn't make sense, and at this point, mdawaffe and I realized that the API — using token for public methods, _session for the abstract protected ones — could benefit from some method renaming. Hence [29635].

The new prototypes are:

protected function __construct( $user_id ) {

final public static function get_instance( $user_id ) {
final public function get( $token ) {
final public function verify( $token ) {
final public function create( $expiration ) {
final public function update( $token, $session ) {
final public function destroy( $token ) {
final public function destroy_others( $token_to_keep ) {
final public function destroy_all() {
final public static function destroy_all_for_all_users() {
final public function get_all() {

final private function hash_token( $token ) {
final protected function is_still_valid( $session ) {

abstract protected function get_sessions();
abstract protected function get_session( $verifier );
abstract protected function update_session( $verifier, $session = null );
abstract protected function destroy_other_sessions( $verifier );
abstract protected function destroy_all_sessions();

public static function drop_sessions() {}