The two possible mitigations I can think of:

• When resetting the session ID (ie. on a Session::resetId() call), do not delete the old session. (Ie. remove SessionBackend.php#L299.) This goes against best practice (e.g. CWE-384 says standard mitigation is "Invalidate any existing session identifiers prior to authorizing a new user session") but seems harmless for login and account creation. I'm not sure about credentials change - not sure how a session fixation attack would work at all in that situation. Maybe we can replace the old session with an anonymous one, just to be sure.
• When validating the session fails, treat it as an anonymous session but do not try to update session cookies. (The unpersisting happens in SessionManager.php#L566, I think. I'm quite not sure where/why the persisting happens.) I'm not sure if there is a security motive behind the current behavior, or the motivation is just "the session seems to be in an invalid state; let's move it to a known valid state".

A trivial testcase (this is on vagrant, with CentralAuth):

$ curl -Is 'http://dev.wiki.local.wmftest.net:22100/wiki/' -H 'Cookie: wiki_session=1234567890abcdefghijklmnopqrstxyz' | grep Set-Cookie
Set-Cookie: wiki_session=lb7pm1hnfjkgaq20p65t34nnm3ltfc1v; path=/; HttpOnly

With "remember me", session survives: (all cookies other than wiki_session are valid)

$ curl -Is 'http://dev.wiki.local.wmftest.net:22100/wiki/' -H 'Cookie: wikiUserName=Admin; centralauth_User=Admin; centralauth_Token=443a90ae188ce20803b86563a8a15823; wikiUserID=1; wiki_session=1234567890abcdefghijklmnopqrstxyz; centralauth_Session=e75a794edfa0a1f94a0875c49ac46a46' | grep Set-Cookie
Set-Cookie: wiki_session=5avih7ikpf2qritljdehkc0g3rpcog3p; path=/; HttpOnly
Set-Cookie: wikiUserID=1; expires=Wed, 13-Jul-2022 07:44:59 GMT; Max-Age=15552000; path=/; HttpOnly
Set-Cookie: wikiUserName=Admin; expires=Wed, 13-Jul-2022 07:44:59 GMT; Max-Age=15552000; path=/; HttpOnly
Set-Cookie: centralauth_User=Admin; expires=Wed, 13-Jul-2022 07:44:59 GMT; Max-Age=15552000; path=/; HttpOnly
Set-Cookie: centralauth_Token=443a90ae188ce20803b86563a8a15823; expires=Wed, 13-Jul-2022 07:44:59 GMT; Max-Age=15552000; path=/; HttpOnly
Set-Cookie: centralauth_Session=32fce4bc9240f35f2c2eee0da5ce5da2; path=/; HttpOnly

With central session but no "remember me", session still survives: (all cookies other than wiki_session are valid)

$ curl -Is 'http://dev.wiki.local.wmftest.net:22100/wiki/' -H 'Cookie: wiki_session=1234567890abcdefghijklmnopqrstxyz; wikiUserID=42; wikiUserName=Test; centralauth_User=Test; centralauth_Session=c762b52fb34cb14d719b8ba9a37485a8' | grep Set-Cookie
Set-Cookie: wiki_session=nput4bfhj1dmmntmlen8hmlg9ua4tt0h; path=/; HttpOnly
Set-Cookie: wikiUserID=42; expires=Sun, 13-Feb-2022 07:49:16 GMT; Max-Age=2592000; path=/; HttpOnly
Set-Cookie: wikiUserName=Test; expires=Sun, 13-Feb-2022 07:49:16 GMT; Max-Age=2592000; path=/; HttpOnly
Set-Cookie: centralauth_User=Test; expires=Sun, 13-Feb-2022 07:49:16 GMT; Max-Age=2592000; path=/; HttpOnly
Set-Cookie: centralauth_Session=3114c6667e881c11e1d64a158368f65c; path=/; HttpOnly

So while this is definitely an issue on a wiki with CookieSessionProvider, it might not be related to the issues we seen on CentralAuth wikis, or the relationship is more complex than a simple race condition.

In T299193#7621624, @Tgr wrote:

• When validating the session fails, treat it as an anonymous session but do not try to update session cookies. (The unpersisting happens in SessionManager.php#L566, I think. I'm quite not sure where/why the persisting happens.)

Actually not. Persisting/unpersisting happens because CookieSessionProvider returns a SessionInfo with a non-existent ID, so the SessionBackend constructor can't load the session data and sets the dirty flag, and that makes the delayed save in SessionManager::getSessionFromInfo() either persist or unpersist (depending on whether the SessionInfo had its persisted flag set). Can we make it not unpersist? I think so - the one place where security relies on unpersisting a valid session is logout, which makes an explicit unpersist() call. It does seem like a scary change though.

Can we make it not unpersist?

On a closer look, not easily. Session::unpersist() just sets the persisted flag to false and saves the session. The session does not remember past values of the persisted flag. So unpersisting when the session gets saved with the flag set to false is important.

Maybe there should be a tombstone mechanism where logically deleted session store objects are set to a special value instead of getting physically deleted, and that value is mostly handled the same as missing, except SessionProvider unpersist calls are skipped.

Tagging Security-Team for awareness, and if you all have any suggestions on the proposed mitigations (T299193#7621624), please let us know.

In T299193#7635625, @Tgr wrote:

Maybe there should be a tombstone mechanism where logically deleted session store objects are set to a special value instead of getting physically deleted, and that value is mostly handled the same as missing, except SessionProvider unpersist calls are skipped.

Speaking more for myself and not the entire Security-Team (if other folks would like to chime in, feel free) - while the above approach seems a bit nasty, I'm not sure there's a better way to deal with these weird edge cases. The other approaches, as described above, all seem like they might have potentially negative consequences, some of which we might be unaware of, whereas introducing a tombstone state should theoretically only add a bit more complexity. I don't believe @Anomie has been very active as a volunteer lately, but I'd imagine they would have some relevant guidance to offer on this matter.

I don't believe @Anomie has been very active as a volunteer lately, but I'd imagine they would have some relevant guidance to offer on this matter.

I've been around enwiki, but after certain terrible managers did some terrible managing I really haven't felt like giving unpaid volunteer coding effort to the WMF.

Change 758418 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/core@master] [WIP] Tombstone the old session on SessionBackend::resetId()

https://gerrit.wikimedia.org/r/758418

In T299193#7661040, @Anomie wrote:

I've been around enwiki, but after certain terrible managers did some terrible managing I really haven't felt like giving unpaid volunteer coding effort to the WMF.

Ok, no problem, and completely understandable.

Tagging #platform-engineering in case there are any folks there available to look at this patch.

I think the tombstone mechanism is feasibly correct to solve this problem but the only query I have in mind with this mechanism is the complexity it introduces and amount of extra overheads involved. I don't want to worry much about the extra memory we'll use the store the tomestone session data (maybe we want to invalidate it after a period of time?).

@Tgr, the patch set you've submitted says it's a WIP, maybe because tests are still on the way? What is needed for us to move this forward? Thanks!

It's WIP because the tests don't pass, but I don't really like the patch as written now. It adds a lot of complexity to SessionManager for something that's conceptually fairly simple, and there is no way to accurately simulate the provider metadata (since the provider thinks this is an authenticated session when it is creating the metadata, but we override it into a non-authenticated one without having a way to let the provider know about that). It's very unlikely, but in theory the provider could put something security-sensitive in there, like declaring the user as privileged in the metadata and then using that in SessionProvider::getAllowedUserRights().

I'm thinking of using the tombstone to force a special session load failure (without unpersisting) in SessionManager::getSessionInfoForRequest() instead, that's a much smaller change and doesn't do things not expected by the provider.

Change 791814 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/core@master] Tombstone the old session on SessionBackend::resetId()

https://gerrit.wikimedia.org/r/791814

Change 758418 abandoned by Gergő Tisza:

[mediawiki/core@master] [WIP] Tombstone the old session on SessionBackend::resetId()

Reason:

in favor of I3a76b67aa51159ebf0195db15cf7c34e00a64a2e

https://gerrit.wikimedia.org/r/758418

Change 791814 merged by jenkins-bot:

[mediawiki/core@master] Tombstone the old session on SessionBackend::resetId()

https://gerrit.wikimedia.org/r/791814

In T299193#7952697, @gerritbot wrote:

Change 791814 merged by jenkins-bot:

[mediawiki/core@master] Tombstone the old session on SessionBackend::resetId()

https://gerrit.wikimedia.org/r/791814

@Tgr I believe you mentioned you wanted to backport this, so that any potential fallout would be easier to spot separately from the train rollout. Is that right? If so, should we plan to backport it today or tomorrow?

In T299193#7952764, @kostajh wrote:

@Tgr I believe you mentioned you wanted to backport this, so that any potential fallout would be easier to spot separately from the train rollout. Is that right? If so, should we plan to backport it today or tomorrow?

Discussed elsewhere but for the record, the plan is to do that on Monday. Quoting myself:

That way 1) unexpected side effects are easy to attribute even if they are infrequent, as there are no code changes on Monday (on Wednesday there are code changes on loginwiki and on Thursday there are code changes on Wikipedias), 2) we can test it on mwdebug first, 3) I'll be around for a few more days in case something unexpected happens (will be on sabbatical starting next week).

Test plan for making sure nothing is broken:

test manually (signup, login with and without "remember me" flag, centralauth edge login, centralauth autologin, bot password login, OAuth request, password change, request with manually altered session cookie, request after manually deleting the data from the session store - I think that covers all code paths), check the session and auth logstash channels for unexpected errors, check the authevents grafana board after a few hours just in case.

Test plan for seeing if T267273: [arwiki] Submitting a POST on a form redirected to immediately after account creation sometimes logs user out was fixed, per Kosta:

re-run https://gitlab.wikimedia.org/kharlan/T267273-welcomesurvey-logout/-/blob/main/welcomesurveylogout.ipynb on Tuesday, to see what the data looks like after 24 hours of the patch being live.

In T299193#7954643, @Tgr wrote:

Test plan for seeing if T267273: [arwiki] Submitting a POST on a form redirected to immediately after account creation sometimes logs user out was fixed, per Kosta:

re-run https://gitlab.wikimedia.org/kharlan/T267273-welcomesurvey-logout/-/blob/main/welcomesurveylogout.ipynb on Tuesday, to see what the data looks like after 24 hours of the patch being live.

I updated the notebook and added a query for a single day across all wikis.

I also re-ran the notebook for April and it looks like the logout rate is about 12.7% for arwiki.

Change 799388 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/core@wmf/1.39.0-wmf.13] Tombstone the old session on SessionBackend::resetId()

https://gerrit.wikimedia.org/r/799388

Change 800082 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/core@master] SessionManager: Add regression test for logout bug

https://gerrit.wikimedia.org/r/800082

I finally put together a regression test, and unfortunately I think the test error is genuine. The tombstone prevents SessionManager::loadSessionInfoFromStore() from unpersisting, but after that SessionManager ends up with no established session and goes into getEmptySession(), which saves the session (or more specifically it calls getSessionFromInfo() which does so), and saving an empty session makes the session provider unpersist :(

The ways I can think of to handle this:

1. Go back to the earlier version of the patch which, instead of discarding a tombstoned session, kept it but then ignored save & similar commands for it. This is probably the most complex approach.
2. Make an unpersisted SessionBackend start in a non-dirty metadata state (so there is no unpersisting), maybe in a fully non-dirty state. This seems logical but I'm not sure I fully grasp the potential consequences of the change.
3. Use a flag to make getSessionFromInfo() skip saving the session when it is called from getEmptySessionInternal().
4. Pass the tombstone flag all the way to getSessionFromInfo() so it can skip saving only in a tombstoned request.

Options 2 and 3 will prevent saving the session when it is empty and freshly created. Saving the session has two effects: the unpersisting (invalid cookies are cleaned up) and saving the session data in in-process cache (but not the real storage). Skipping the unpersisting might mean invalid (e.g. expired) session cookies stick around and prevent the user from being served from edge cache. Skipping the in-process cache probably means an extra session store read on unauthenticated requests. Neither of those seem completely inacceptable but aren't ideal either. I don't think either will actually happen, but we should be watching for them (check the Kask lookup metrics, check the appserver request volume).

Change 800860 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/core@master] SessionManage: Do not save when creating empty session

https://gerrit.wikimedia.org/r/800860

Change 800860 merged by jenkins-bot:

[mediawiki/core@master] SessionManage: Do not save when creating empty session

https://gerrit.wikimedia.org/r/800860

Test plan for backporting rMW7dba98b69fea: Tombstone the old session on SessionBackend::resetId() and rMW1682de5e839b: SessionManage: Do not save when creating empty session:

• try user signup on some random wiki (maybe arwiki since we started all this to fix T267273: [arwiki] Submitting a POST on a form redirected to immediately after account creation sometimes logs user out )
• try user login without "remember me" flag (which does a session ID reset)
  • try a curl request with the old request credentials afterwards (should fail but not purge) does not work
• try user login with "remember me" flag
  • try a curl request with the old request credentials afterwards (should fail but not purge) does not work
• try logging in from an existing account to another existing account (ID reset from an authenticated session)
  • try a curl request with the old request credentials afterwards (should fail but not purge) does not work
• try password change (another type of session ID reset)
• try logging out
• do a request with a manually altered (invalid) session cookie - the cookie should get deleted
• do a request after manually deleting the data from the session store (ObjectCache::getInstance($wgSessionCacheType)->delete('<wiki>:MWSession:<sessionid>'))
  • without "remember me"
  • with "remember me"
• try some of these on a non-centralauth wiki (e.g. officewiki)
• test centralauth 2nd-level domain login during the above login attempts: go to another Wikipedia, see if you are already logged in (note this was already somewhat unreliable) broken, unrelated
• test centralauth edge login during the above login attempts: open network tab and see if Special:CentralAutoLogin requests seem to go normally; go to some non-Wikipedia (maybe English Wiktionary and German Wikiquote, to test both local and 2nd-level session cookies), see if you are already logged in (note this was already somewhat unreliable) broken, unrelated
• try centralauth autologin (go to outreach wiki, see if you get logged in automatically, check network tab) (note this was already somewhat unreliable, and the UI part is probably broken in Vector 2022)
• try bot password login
• try some OAuth request (e.g. Quarry login)
• check the mwdebug session and auth logstash channels for unexpected errors
• check login/signup stats on the authevents grafana board after a few hours
• check kask grafana board after a few hours
• check varnish stats (pass rate, backend hit rate shouldn't increase)

And finally to check if the bug is fixed:

• re-run https://gitlab.wikimedia.org/kharlan/T267273-welcomesurvey-logout/-/blob/main/welcomesurveylogout.ipynb on Wednesday, to see what the data looks like after 24 hours of the patch being live.

Change 799388 merged by jenkins-bot:

[mediawiki/core@wmf/1.39.0-wmf.13] Tombstone the old session on SessionBackend::resetId()

https://gerrit.wikimedia.org/r/799388

• try user signup on some random wiki (maybe arwiki since we started all this to fix T267273: [arwiki] Submitting a POST on a form redirected to immediately after account creation sometimes logs user out )
• try user login without "remember me" flag (which does a session ID reset)
  • try a curl request with the old request credentials afterwards (should fail but not purge)
• try user login with "remember me" flag
  • try a curl request with the old request credentials afterwards (should fail but not purge)
• try logging in from an existing account to another existing account (ID reset from an authenticated session)
  • try a curl request with the old request credentials afterwards (should fail but not purge)
• try password change (another type of session ID reset)
• try logging out
• do a request with a manually altered (invalid) session cookie - the cookie should get deleted
• do a request after manually deleting the data from the session store (ObjectCache::getInstance($wgSessionCacheType)->delete('<wiki>:MWSession:<sessionid>'))
  • without "remember me"
  • with "remember me"
• try some of these on a non-centralauth wiki (e.g. officewiki)
• test centralauth 2nd-level domain login during the above login attempts: go to another Wikipedia, see if you are already logged in (note this was already somewhat unreliable)
• test centralauth edge login during the above login attempts: open network tab and see if Special:CentralAutoLogin requests seem to go normally; go to some non-Wikipedia (maybe English Wiktionary and German Wikiquote, to test both local and 2nd-level session cookies), see if you are already logged in (note this was already somewhat unreliable)
• try centralauth autologin (go to outreach wiki, see if you get logged in automatically, check network tab) (note this was already somewhat unreliable, and the UI part is probably broken in Vector 2022)
• try bot password login
• try some OAuth request (e.g. Quarry login)
• check the mwdebug session and auth logstash channels for unexpected errors
• check login/signup stats on the authevents grafana board after a few hours
• check kask grafana board after a few hours
• check varnish stats (pass rate, backend hit rate shouldn't increase)

And finally to check if the bug is fixed:

• re-run https://gitlab.wikimedia.org/kharlan/T267273-welcomesurvey-logout/-/blob/main/welcomesurveylogout.ipynb on Wednesday, to see what the data looks like after 24 hours of the patch being live.

Change 801203 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/core@wmf/1.39.0-wmf.12] Tombstone the old session on SessionBackend::resetId()

https://gerrit.wikimedia.org/r/801203

Change 801203 merged by jenkins-bot:

[mediawiki/core@wmf/1.39.0-wmf.12] Tombstone the old session on SessionBackend::resetId()

https://gerrit.wikimedia.org/r/801203

Mentioned in SAL (#wikimedia-operations) [2022-05-31T11:01:41Z] <tgr@deploy1002> Synchronized php-1.39.0-wmf.13/includes/session: Backport: [[gerrit:799388|Tombstone the old session on SessionBackend::resetId() (T299193)]] (duration: 03m 12s)

It does not seem to be working, even in the narrow sense of fixing the bug described in this task. Steps to reproduce:

• in Chrome, open the network tab
• go to the login page
• select the request for the login page, copy as curl, add -v > /dev/null to the end
• log in (which will tombstone the session)
• send the curl request
• observe that the session cookie is deleted

I can confirm the tombstoned key in the session store, so the patch is doing something, but it doesn't prevent unpersisting.
On a CentralAuth wiki this is probably irrelevant as only the core session cookie is deleted, the CentralAuth cookie is not (WebResponse never issues a delete for cookies which aren't present in the request, and the login session is local) and the session can be recovered from that.

In T299193#7969827, @Tgr wrote:

It does not seem to be working, even in the narrow sense of fixing the bug described in this task. Steps to reproduce:

• in Chrome, open the network tab
• go to the login page
• select the request for the login page, copy as curl, add -v > /dev/null to the end
• log in (which will tombstone the session)
• send the curl request
• observe that the session cookie is deleted

I can confirm the tombstoned key in the session store, so the patch is doing something, but it doesn't prevent unpersisting.
On a CentralAuth wiki this is probably irrelevant as only the core session cookie is deleted, the CentralAuth cookie is not (WebResponse never issues a delete for cookies which aren't present in the request, and the login session is local) and the session can be recovered from that.

At least in my local environment, it fixed the issue where an unresolved HTTP request to validate password validity on Special:CreateAccount caused a logout on the next request. Of course, there are many differences between my local environment and what production users are interacting with.

Not an area I'm knowledgable in at all, but while looking at T309611: Logging out automatically, I found T246943: Login failed/disappeared during 2FA which certainly feels similar.. (if only due to the MediaWiki\Session\MetadataMergeException: Key "CentralAuthSource" changed)

Change 801683 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/core@wmf/1.39.0-wmf.13] Revert "Tombstone the old session on SessionBackend::resetId()"

https://gerrit.wikimedia.org/r/801683

Change 801683 merged by jenkins-bot:

[mediawiki/core@wmf/1.39.0-wmf.13] Revert "Tombstone the old session on SessionBackend::resetId()"

https://gerrit.wikimedia.org/r/801683

Change 801748 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/core@wmf/1.39.0-wmf.14] Revert "Tombstone the old session on SessionBackend::resetId()"

https://gerrit.wikimedia.org/r/801748

Change 801749 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/core@master] Revert "Tombstone the old session on SessionBackend::resetId()"

https://gerrit.wikimedia.org/r/801749

Change 801748 merged by jenkins-bot:

[mediawiki/core@wmf/1.39.0-wmf.14] Revert "Tombstone the old session on SessionBackend::resetId()"

https://gerrit.wikimedia.org/r/801748

Caused T309616: Cross-wiki session loss on Wikimedia wikis and had to be reverted. It was disruptive enough that I felt it's better to revert quickly instead of investigating what's going on, but I did notice that the <wiki>Session and `ss0-<wiki>Session cookies had different values some of the time, which I think doesn't normally happen. (The ss0- version is for T252236: Prepare CentralAuth (e.g. login.wikimedia.org) for requirement of SameSite=None cross-site cookies in Chrome.)

Since the patch only changed the behavior of session ID reset, I assume the new bug was somehow related to Special:CentralAutoLogin/setCookies (one of the few things that does a session ID reset, and the only CentralAuth-related one; and something that can happen in the background on random other wikis via edge login). Not sure how, though.

Change 801749 merged by jenkins-bot:

[mediawiki/core@master] Revert "Tombstone the old session on SessionBackend::resetId()"

https://gerrit.wikimedia.org/r/801749

Moved to "needs work" as not planning to work on this right now but should be investigated later.

Change 800082 abandoned by Kosta Harlan:

[mediawiki/core@master] SessionManager: Add regression test for logout bug

Reason:

Abandoning per https://gerrit.wikimedia.org/r/c/mediawiki/core/+/800082/2#message-cd69a4d2444c75c6623f7713195c1b7c496e771a

https://gerrit.wikimedia.org/r/800082

@Tgr I'm moving this off our current sprint. I think we've done what we can, as far as our team's priorities and workload allow for at the moment, anyway.