Page MenuHomePhabricator

Logout does not work when the loginwiki account is unattached
Closed, ResolvedPublic

Description

Steps to reproduce:

  • create a user on vagrant with the centralauth role enabled; that should create a local user on devwiki and loginwiki
  • unattach the loginwiki account

The user will be able to log in, but not able to log out. I think what happens is:

  • at login, the user is sent to loginwiki:Special:CentralLogin/start which never checks whether the account is connected, and logs the user in on loginwiki
  • at logout, the user is logged out locally, and the global token is reset, but that won't affect the unconnected loginwiki account. Not being logged in triggers an autologin sequence; again, loginwiki:Special:CentralLogin/checkLoggedIn never checks if the user is connected, so they get logged in again.

In theory unconnected accounts on loginwiki should not exist. In practice, I have several on my test machine. I initially though that's the side effect of me doing strange things while debugging, but today I realized that new ones are still being created when I do normal testing via the web UI. I'm not sure if the same thing happens in production (and haven't yet found a query which would find such accounts in a reasonable time), but I'd rather err on the side of caution.

Event Timeline

Tgr created this task.Jun 10 2016, 1:16 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 10 2016, 1:16 PM
Tgr added a comment.Jun 10 2016, 1:34 PM

This should do it:

Please double check that account creation isn't broken.

Tgr added a comment.Jun 10 2016, 2:00 PM
mysql:research@analytics-store.eqiad.wmnet [loginwiki]> select log_user_text from logging left join centralauth.localuser on lu_name = log_user_text and lu_wiki = 'loginwiki' where log_type = 'newusers' and log_timestamp > '20160601' and lu_name is null;
Empty set (5 min 41.99 sec)

Unless I messed up that query, Wikimedia sites are not affected, so this bug can be un-securitized.

@Anomie, @Tgr. Thanks for being on this. It looks like @Anomie's patch supersedes @Tgr. Are one of you going to apply this soon?

dpatrick triaged this task as Normal priority.Jun 14 2016, 8:14 PM

Since it's my patch, I'd rather not be the one effectively +2ing it.

Tgr added a comment.Jul 22 2016, 11:56 PM

Apologies for dropping this. CR+2, I will test it live on Monday (I don't really trust vagrant when it's about CentralAuth) and deploy if nothing goes wrong.

Tgr added a comment.EditedJul 23 2016, 2:14 AM

Re: T141160, ran

mwscript invalidateUserSessions.php --wiki=loginwiki --file=T141160-unattached-users-3.txt | tee T141160-reset.log

This was done after the accounts have reattached, so users should be safely logged out.

Tgr added a comment.Jul 25 2016, 9:53 PM

Deployed. (Looks like Stashbot is not working for private tasks...)

Should this task either be a subtask (=block) T140591: MediaWiki 1.28.1/1.27.2/1.23.16 security release or should this task be closed as it's been deployed?

Should this task either be a subtask (=block) T140591: MediaWiki 1.28.1/1.27.2/1.23.16 security release or should this task be closed as it's been deployed?

It doesn't need to be a subtask since it's not in core and is not in a bundled extension, however it should be committed to Gerrit and announced (if that's what we do with the CentralAuth extension).

Tgr added a comment.Aug 2 2016, 8:15 PM

Given that this is more of a layered defense than a bugfix (the only way the bug fixed here can be exposed is when there is some other bug creating unattached accounts) I'm not sure it's even worth announcing.

demon added a subscriber: demon.Sep 6 2016, 4:32 PM

Given that this is more of a layered defense than a bugfix (the only way the bug fixed here can be exposed is when there is some other bug creating unattached accounts) I'm not sure it's even worth announcing.

It's always the right thing to at least provide a cursory mention to wikitech-l/mediawiki-l :)

If there's no objections, let's go ahead and make this public today.

Tgr added a comment.Oct 27 2016, 10:02 PM

If there's no objections, let's go ahead and make this public today.

I'll do that now; no one objected and this blocks something I am working on.

Tgr changed the visibility from "Custom Policy" to "Public (No Login Required)".Oct 27 2016, 10:16 PM
Tgr changed Security from Software security bug to None.

Change 318443 had a related patch set uploaded (by Gergő Tisza):
SECURITY: Check that the loginwiki account is attached when logging in

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

Change 318443 merged by jenkins-bot:
SECURITY: Check that the loginwiki account is attached when logging in

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

Tgr closed this task as Resolved.Oct 27 2016, 10:52 PM
Tgr assigned this task to Anomie.

Change 318461 had a related patch set uploaded (by Gergő Tisza):
SECURITY: Check that the loginwiki account is attached when logging in

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

Change 318461 merged by jenkins-bot:
SECURITY: Check that the loginwiki account is attached when logging in

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

Change 318464 had a related patch set uploaded (by Gergő Tisza):
SECURITY: Check that the loginwiki account is attached when logging in

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

Change 318465 had a related patch set uploaded (by Gergő Tisza):
SECURITY: Check that the loginwiki account is attached when logging in

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

Change 318464 merged by Gergő Tisza:
SECURITY: Check that the loginwiki account is attached when logging in

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

Change 318465 merged by Gergő Tisza:
SECURITY: Check that the loginwiki account is attached when logging in

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