Changing security config should cancel existing "remember me" tokens and sessions

[robbieaverill]

Acceptance Criteria

• Authentication tokens can be represented by "remember me" (database state) or PHP sessions
• All authentication tokens are expired when a password is changed or reset (retaining the current browser session)
• All authentication tokens are expired when a second factor is added or removed (retaining the current browser session)
• If the system is configured for multiple tokens, they're all expired

From Olivier:

Changing security configuration (like adding/removing 2nd factor or changing/resetting password) should result in cancelling all existing sessions. rational: users change their password because they think it might have been compromised.

---

Originally from @chillu in https://github.com/Firesphere/silverstripe-bootstrapmfa/issues/52

[ScopeyNZ]

There's two ways that this is done AFAICT:

• Reset all other sessions when resetting a password
• Give the option to reset other sessions (usually on the same screen as resetting a password while logged on)

Having option 2 means you don't need to do option 1, although it requires the user to perform the extra step if they are changing their password when they're worried about having been compromised. But it also means that the (probably more common) use case of changing your password for any other reason doesn't result in you having to log in again from all your other devices.

[chillu]

I've updated the scope here to cover both "remember me" tokens and PHP sessions, acknowledging that sessions will be harder to fix.

Option 1: Track multiple sessions

• We could track session identifiers as a has_many against the member record
• When member explicitly logs out (or otherwise requests destruction of session), we can purge that database record
• Sticky sessions in multi server setups mean that a single webserver doesn't have access to all sessions (or could alter their state). We'd need to add that caveat, and allow devs to disable "log me out everywhere" buttons in their setup
• Needs to be resilient against database restores (not logging out all users when restoring a different database state)

Option 2: Use single session based on user identifier across multiple devices

• Create session identifier through session_id()
• Assumes that any stateful UI edge cases already exist (multiple browser tabs in single session and single device), so no change when using same session across devices

Option 3: Is the Option 1 decoupled from the database storage

If we don't use has_many to keep the sessions in the database attached to the user record, but rather have a dynamic storage backend for that info, devops might be able to configure it more flexibly. This might be especially useful on multi-server setups.

Notes

• Could be an opportunity to add a "log me out everywhere" button
• Limitations of PHP filesystem session storage: You can't list sessions, can only access by identifier
• Can't require changes to database session storage (can still support PHP filesystem, dynamodb, or database storage if configured)
• @kinglozzer has already built Option 1 into a module: https://github.com/kinglozzer/silverstripe-session-manager

[dhensby]

Note that this is a regression from the 3.x line (https://github.com/silverstripe/silverstripe-framework/blob/3.7.3/security/Member.php#L982) - see https://www.silverstripe.org/download/security-releases/ss-2016-008/

[dnsl48]

Renew the Session ID After Any Privilege Level Change is the OWASP recommendation.

It is recommended to invalidate tokens not only when password changes, but privileges as well. Another part of the authentication is login (email in our case?). Changing it should also invalidate authenticated sessions (in case it can be changed). A complementary measure is to use a different session ID pre and post authentication (might have been implemented already, I didn't check).

[chillu]

We've got more drivers for this now at Silverstripe Ltd, the ability for users to invalidate their sessions across devices (and for admins to do the same across all users) has been flagged as an important security feature.

FWIW I've just written up a summary on how https://github.com/kinglozzer/silverstripe-session-manager works. Assuming the goal is to support every session persistence supported by PHP and Silverstripe (file-based, DynamoDB, hybridsessions), I can't see a way around mirroring sessions in the database. And consequently, an additional query updating the expiry date (UPDATE LoginSession in Loz' module) is unavoidable as well.

I'm a bit worried about the complexity of this (randomised garbage collection or cron job, more complex session behaviour). Given that Loz has proven this can be written as a module, I'm in favour that any implementation discussed here should stay as a module. This would allow us to iterate on the API. Like silverstripe/login-forms, we could still choose to bundle it with a default recipe.

[kinglozzer]

As you can probably tell from the commit history, I’m not actively working on that module 😅 so if Silverstripe wants to take on building a module for this, feel free to fork or just copy anything you want from there 👍

[chillu]

Thanks for writing this module @kinglozzer, gave me more confidence that the concept is actually workable. And I'm sure it's a great piece of code, with a bit of a spring clean, tests, etc :) Just surveying our options at the moment, but as a developer I'd rather not reinvent that particular wheel if we can find a way to work together or take more ownership of the module ourselves.

[Cheddam]

@kinglozzer Just a heads up - we're planning to use your module as a base for an implementation in the silverstripe namespace. We'll perform a silent fork to avoid confusion and problems with default PR targets, but will put full credit in the README 😃

[sminnee]

If Loz is up for it, you could also move the repo. Biggest advantage of that is that any links to Loz's repo would redirect, if Loz doesn't want to keep maintaining his version.

Downside would be his work isn't credited as clearly, but you could solve that by an explanatory/thanks header in the readme.

[chillu]

Another reason a move to github.com/silverstripe might be easier is to ensure sufficient build allocation in Travis - they've gone super stingy since Dec last year, 10k build minutes until you have to beg them for more. On github.com/silverstripe we're running a paid account now. @Cheddam If Loz agrees, is there any other reason for a "silent fork" over a repo move? We'd credit Loz and presumably retain his maintainer access.

[kinglozzer]

I’m happy to just transfer the repo if that’s easier all round. Don’t worry about credit, I’m just happy the code might actually see some real world use 😝

[Cheddam]

Great idea! We'll be kicking off work on this in the next day or two, so @kinglozzer if you could make the transfer when you get a chance that'd be much appreciated 😄

[kinglozzer]

I’ve transferred it to silverstripe, and granted access to:

• Core team
• Contributing committers
• Triage

[Cheddam]

Thanks @kinglozzer ❤️

[maxime-rainville]

Just spent some time investigating this. Here's some semi-random observation.

Things that are working as they should

There's a method a Member::encryptWithUserSettings. This method uses the Member's salt to hash a string. When a user's password is changed, that also changes the salt. Which will alter the output encryptWithUserSettings

encryptWithUserSettings is used when generating the RememberLoginHash. So changing your password will invalidate remember me tokens.

Things that are NOT working as they should

Other changes to the user settings, do NOT alter the salt. This means, we do not currently meet this requirement:

All authentication tokens are expired when a second factor is added or removed

LoginSession does not use encryptWithUserSettings at all. So changing your password or your MFA methods, won't terminate any of your active session.

Approach to fixing it

Here's a couple things I think we could/should do.

Making encryptWithUserSettings more resilient

We can't change the Salt when you add a MFA method, because that would invalidate the password hash in the DB. We wouldn't be able to generate a new hash since we wouldn't have the user's password at that time.

We should update encryptWithUserSettings so we can concatenate other string to the salt when hashing the input (e.g.: the Email address). This should be a hook so other modules can add their own bits of string to the salt.

MFA should use that hook to so the output of encryptWithUserSettings changes when you change your MFA settings.

Central API for when a user setting change

Maybe we need to have a method on Member that some modules calls when a substantial change to user settings occur. Other modules could hook into that method to be notified when something has changed.

For example, MFA would call this method when a user reset their MFA settings. This could notify session manager that a substantial change to the Member profile has occurred which would allow it to invalidate all the LoginSession objects.

Add an hash to LoginSession object in session manager

Here's the info we store in the LoginSession currently:

private static $db = [
    'LastAccessed' => 'DBDatetime',
    'IPAddress' => 'Varchar(45)',
    'UserAgent' => 'Text',
    'Persistent' => 'Boolean'
];

We could add an hash that would essentially take all the other values and pass them to encryptWithUserSettings.

When LoginSessionMiddleware validates that your LoginSession is still valid, it would also validate that your hash is still valid.

We might need to move the LoginSession write call after $delegate($request). Otherwise, if you made a substantiate change to your user settings on that request, you'll be logged out.

What should we do

"Making encryptWithUserSettings more resilient" in definitively needed.

We could get away with doing only one of "Central API for when a user setting change" or "Add an hash to LoginSession object in session manager". Both approaches could work.

I'm leaning towards "Add an hash to LoginSession object in session manager" because in seems generally more elegant and hook into an existing method.

There could be a case for doing both.

[brettt89]

@maxime-rainville

Would this approach destroy existing PHP sessions from users who have already logged in? Or only impacts people attempting to login/autologin? (E.g. does it call session_destroy() on all the users PHP sessions?)

We can't change the Salt when you add a MFA method, because that would invalidate the password hash in the DB. We wouldn't be able to generate a new hash since we didn't have the user's password at that time.

Could we not have the user input their password when enabling MFA, we could then use the provided password to confirm it matches the current salted password, then re-salt it with MFA manipulation if successful?

[maxime-rainville]

@brettt89 As of now, encryptWithUserSettings is only used by the RememberLoginHash. So if you change your password, your Remember Me cookie is invalidated, but not any active PHP sessions ... you can keep using the site, but if you restart your computer, you will have to re-enter your credentials.

Ideally, if you change your password and you have session manager installed, all you active PHP session should be invalidated as well ... except the one you use to change your password.

Could we not have the user input their password when enabling MFA, we could then use the provided password to confirm it matches the current salted password, then re-salt it with MFA manipulation if successful?

We could, but that would only narrowly address the situation where the user is updating MFA. They are other pieces of data on a Member profile that arguably should invalidate session when changed (e.g.: username/email).

Then again, one could argue that if those other aspects are so important, we should ask the Member for their password before updating them.

Either way, that wouldn't address the scenario where an admin makes a significant change on someone else's behalf. e.g.: An admin changes another user's email address without knowing that other user's password.

[brettt89]

A good discussion about Session Management and password changing within Applications. https://security.stackexchange.com/questions/63256/on-password-change-in-a-web-application-should-it-log-out-all-other-sessions/178669

Some good insights into how this impacts users / attackers / etc and what decisions people might want to make in different scenarios.

[brynwhyman]

We had a talk about this internally, feels like focusing on this could be a bit of an epic in itself. Some thoughts below

• We should finalise the list of what actions should expire sessions, taking reference from OWASP and refining to our own user journeys

  • Also considering what happens when a user updates their details, vs. and administrator updating their details

• Thinking about this as an epic, we could split this into a bunch of issues

  • Update the encrypt user settings functionality as noted by Maxime + update the session manager module to conform with this
  • Force you to enter your password when you change you email
  • Notification emails when your password is changed, email your old email address when your email is changed
  • Consider what opt-out functionality might be required here.

• Noted separately that there's no requirement to re-add your password when changing your email address in the profile page. Might be worth looking at this at the same time.

  • Maybe we use Sudo Mode for this?

• Noting that while looking at all of this stuff, it might be worth finally getting security-extensions module into the core recipe.