Remember Me functionality fails when renewal is interrupted

[Cheddam]

Module version(s) affected

Tested against 5.2.x but related logic introduced in 4.x

Description

The 'Remember Me' feature stores two additional cookies in the user's browser that enable Silverstripe CMS to renew the user's session when it is destroyed (either through a timeout on the server side or when the user closes/reopens their browser.)

The alc_device cookie is static throughout the lifetime of the login, but the alc_enc cookie value is rotated whenever the session is renewed. This makes some sense from a security perspective, in the same vein as CSRF token rotation, but it relies on the browser receiving the new value, which is not guaranteed - network issues or user behaviour could result in a failure or cancellation of the request prior to headers being returned to the browser.

In any case where the browser does not receive the new cookie value, any subsequent request will transmit the original, which no longer maps to an active RememberLoginHash, triggering a logout.

How to reproduce

1. Log into a Silverstripe CMS instance with the 'Remember Me' box checked
2. Delete the PHPSESSID / SECSESSID cookie to simulate expiry of the session (or wait for it to time out)
3. Trigger a cancelled request from the browser
   • The request must progress far enough to be processed by the server in order to trigger the renewal (i.e. after initial connection and handshake)
   • The most reliable way I've reproduced this is through repeatedly clicking a link in the front-end of the site that loads a slow route (you can pop sleep(10) in the relevant controller method to simulate this)
4. Load an authenticated route, observe kick to login screen

Possible Solution

I see two potential avenues to resolution:

Option 1: Remove/disable rotation logic

As previously mentioned, the rotation does make some sense from a security standpoint, but the risk it mitigates is arguably minimal. If an attacker can steal the alc_enc cookie, they can also steal the SECSESSID cookie, and they will likely exploit it while it is fresh regardless.

Based on this, either removing the rotation step entirely, or allowing it to be disabled in configuration, seems like a viable solution. To be clear, this should only affect rotation during the lifetime of a login - explicit logouts and new logins should still generate a fresh value.

For a quick comparison, I took a look at Laravel's equivalent implementation and it appears that they do not perform rotation of the token during the lifetime of the login.

Option 2: Introduce a grace period

This would be more complex to implement / maintain, and the exact parameters would need to be considered, but in essence we could mitigate this failure by respecting requests with a stale alc_enc within a small window and re-transmitting the renewed value.

Additional Context

No response

Validations

• [X] Check that there isn't already an issue that reports the same bug
• [X] Double check that your reproduction steps work in a fresh installation of silverstripe/installer (with any code examples you've provided)

PRs

• https://github.com/silverstripe/developer-docs/pull/541
• https://github.com/silverstripe/silverstripe-framework/pull/11299
• https://github.com/silverstripe/silverstripe-framework/pull/11355
• https://github.com/silverstripe/silverstripe-session-manager/pull/214
• https://github.com/silverstripe/developer-docs/pull/572

[GuySartorelli]

Can confirm both from the actual src code and the tests of the laravel implementation that they don't cycle the token except to set it during initial login (and if it's missing) and to remove it when logging out.

I'd say that's the way to go.

It does look like Syfony cycles it (if it hadn't already been cycled in the last minute): https://github.com/symfony/symfony/blob/a86c96a85931f98e1ba6275629c3fcc268990527/src/Symfony/Component/Security/Http/RememberMe/AbstractRememberMeHandler.php#L47-L56

https://github.com/symfony/symfony/blob/a86c96a85931f98e1ba6275629c3fcc268990527/src/Symfony/Component/Security/Http/RememberMe/PersistentRememberMeHandler.php#L91-L100

So I'm not sure if they have similar issues to what this issue describes.

[GuySartorelli]

If an attacker can steal the alc_enc cookie, they can also steal the SECSESSID cookie, and they will likely exploit it while it is fresh regardless.

That does depend on whether they have a reliable attack method that they can repeat, or if it's once-off. If it's once-off, then cycling alc_enc will stop them from being able to access your account even if they have the main session cookie. So it is slightly more secure to keep cycling that, I think? I dunno.

Given Laravel doesn't bother with it I'm inclined to accept that it's not doing a lot of good, but I'd like other opinions from @silverstripe/core-team if anyone has one.

[GuySartorelli]

@Cheddam if no one replies in say a week or so, feel free to raise a PR and we can proceed from there.

[madmatt]

Removing the recycling of these would result in many more entries in the RememberLoginHash table, no? Or would you just expect the original hash to just live forever so you'd still only have one per device per Member?

I'm not sure I really see the use case for removing the existing functionality myself - sounds like this was a transient network issue that caused a request to be sent and processed but the HTTP response was not received by the browser so they never found out about the new alc_enc cookie? Without adding your sleep, how reproducible is it? Is it solving any other problem to remove this?

Agree that the rotation itself doesn't really achieve very much - as long as you're running over HTTPS you should be pretty safe from attack and/or the SECSESSID cookie is of more immediate value.

[Cheddam]

With Option 1, there'd still only be one RememberLoginHash entry per autologin. With Option 2, there would be multiple valid hashes, but we'd potentially mark outdated ones and significantly shorten their expiry. I haven't fully explored the implementation details yet, as I'm hoping we can just go with Option 1.

There's a range of potential triggers for the response not getting to the browser - a user double-clicking a link, multiple tabs of the same site rehydrating when a user reopens their browser, a user going through a tunnel and spamming refresh, a SPA firing off multiple asynchronous API requests to update the UI (this one may not trigger a full logout as the new value would still reach the browser, but could still trigger a failure in other requests sent before the new cookies arrived.)

One more wrinkle that we'd need to account for in Option 2 is if multiple requests resolve and successfully relay new cookies to the browser, but the responses are out of order, resulting in now-outdated cookies being set. (I really think Option 1 is going to be the best path forward.)

[Cheddam]

I'm working on a patch based on Option 1, making this configurable and retaining the current behaviour by default.

My gut feeling is that structuring it as configuration constitutes a minor change rather than a patch, but happy to be convinced otherwise - any thoughts @GuySartorelli?

[GuySartorelli]

Yup, new configuration properties are new public API, so should be included in a minor release.

[GuySartorelli]

It's not likely anyone will ever bother applying the change in their projects though - so if we think that the new behaviour is better, we should probably have it be the behaviour in CMS 6.

[Cheddam]

@GuySartorelli Regarding an open question from the merged PR - the Session Manager module depends on the onAfterRenewToken extension point to replace the session identifier with its own value. This logic feels a bit icky, but I suspect even a refactored approach would still require a hook into the ALC process, so the extension point needs to be retained in some form.

I'll work on getting a PR up for the 6 branch to fully drop the renewal logic soon - let me know what you think about shifting / replacing the onAfterRenewToken call.

[GuySartorelli]

the Session Manager module depends on the onAfterRenewToken extension point to replace the session identifier with its own value

But it only does that on renewal, right? It has a separate extension hook for on generation. So if we're not renewing, we don't need to do that? Or is it meant to happen every time we set that cookie?

If it's meant to happen on every authentication, then a onAfterAuthenticate hook might be appropriate, or use the existing memberAutoLoggedIn hook, or if it has to be before the call to Cookie::set(), then onBeforeSetTokenCookie or something along those lines.

Basically, when you do the PR, if an extension hook is needed and an existing one doesn't fit the purpose, just add one with an appropriate name in the appropriate place.

tl;dr I'm sure whatever you do in the PR will be good code 👍

[GuySartorelli]

@Cheddam Are you still intending on raising a CMS 6 PR for this?

[Cheddam]

@GuySartorelli Yep! Should have it ready for you to look at next week.