Blocked accounts on BlockDisablesLogin wikis aren't logged out
Closed, ResolvedPublic

Description

About a month ago I decided to quit OTRS. I had my account removed.

When I try to login I get this nice message, so far so good:

Login error 
 This account has been closed, most likely due to inactivity. Inactive accounts are routinely closed to maintain the security of the OTRS system/wiki and to make sure that only those who need access have it. If you require further information, or would like to discuss having your account re-activated, contact an OTRS administrator through their individual talk pages/e-mails or by writing an e-mail to volunteers-otrs [at] wikimedia [dot] org.

But I still had a logged in session and in that session I can still see all the private information on https://otrs-wiki.wikimedia.org . This is an information leak. Closed down accounts shouldn't be able to see anything anymore.

Judging from https://otrs-wiki.wikimedia.org/wiki/Special:ListGroupRights the group "Inactive users" was created for this, but if you compare https://otrs-wiki.wikimedia.org/w/index.php?title=Special:ListUsers&group=inactive to https://otrs-wiki.wikimedia.org/wiki/List_of_accounts/closed than you'll notice that lot's of users are not in there. https://otrs-wiki.wikimedia.org/w/index.php?title=Special:ListUsers&offset=&limit=5000&username=&group= gives an even better overview.

My guess is to fix this, all blocked accounts should be placed in the inactive user group. Probably a regular check should be performed to see if the blocked and inactive users are still in sync.

This problem might exist on other private wiki's.

Patches

There are a very large number of changes, so older changes are hidden. Show Older Changes

This problem might exist on other private wiki's.

We should assume it does until proven otherwise. I don't think otrs-wiki has a custom setup in this aspect

Not an OTRS bug, this is for MediaWiki

Also, I think this is an issue with wgBlockDisablesLogin and session handling code - the inactive group was from the DisableAccount extension IIRC

It directly affects the security of our OTRS system so I would keep the OTRS project on it.

Not an OTRS bug, this is for MediaWiki

Also, I think this is an issue with wgBlockDisablesLogin and session handling code - the inactive group was from the DisableAccount extension IIRC

The whole blocking system is all about preventing people to edit, not about preventing people to login or read. If I block someone on say Wikidata, I still want that person to be able to login so they can respond on their user talk page. Using blocks to prevent people from reading feels flawed to me, the system was never designed for that.

Oh, why did you completely change the scope of this ticket @Krenair ? This is a specific security incident that needs a specific fix to prevent a leak of private data. This is not a hypothetical issue, see for example https://commons.wikimedia.org/w/index.php?title=Commons%3AAdministrators%2FRequests%2FThe_Photographer&type=revision&diff=189005015&oldid=188843954

Multichill changed the title from "Blocked accounts on BlockDisablesLogin wikis aren't logged out" to "Closed accounts on otrs-wiki aren't logged out and can still see private information".Mar 13 2016, 12:22 AM
Multichill added a subscriber: csteipp.

There are two ways to handle this:

  • check blocks on every request - expensive
  • log the user out (change the token) when blocked - does not work for IP blocks, probably worse UX-wise (blocking and then unblocking a user leaves them logged out), must be reimplemented in auth extensions which might be nontrivial (e.g. on a SUL wiki the user gets logged out of all other wikis as well).

Probably the first is the way to go, with some sort of caching.

Not an OTRS bug, this is for MediaWiki

Also, I think this is an issue with wgBlockDisablesLogin and session handling code - the inactive group was from the DisableAccount extension IIRC

It directly affects the security of our OTRS system so I would keep the OTRS project on it.

It's not a ticket.wikimedia.org issue so it's not OTRS (see the project description). otrs-wiki is not actually OTRS itself, and AFAICT there is no suggestion that this issue is specific to otrs-wiki - actually, your description says "This problem might exist on other private wiki's."
The only OTRS link I can find here is that the wiki you discovered it on is called otrs-wiki.

The whole blocking system is all about preventing people to edit, not about preventing people to login or read. If I block someone on say Wikidata, I still want that person to be able to login so they can respond on their user talk page. Using blocks to prevent people from reading feels flawed to me, the system was never designed for that.

It is about that as well actually. Read the documentation: https://www.mediawiki.org/wiki/Manual:$wgBlockDisablesLogin.

Oh, why did you completely change the scope of this ticket @Krenair ? This is a specific security incident that needs a specific fix to prevent a leak of private data. This is not a hypothetical issue, see for example https://commons.wikimedia.org/w/index.php?title=Commons%3AAdministrators%2FRequests%2FThe_Photographer&type=revision&diff=189005015&oldid=188843954

I didn't completely change the scope of this ticket. I did not move it out of security. I am not claiming it is hypothetical.

Krenair changed the title from "Closed accounts on otrs-wiki aren't logged out and can still see private information" to "Blocked accounts on BlockDisablesLogin wikis aren't logged out".Mar 13 2016, 12:39 AM
MaxSem added a subscriber: MaxSem.Mar 13 2016, 1:01 AM
  • log the user out (change the token) when blocked - does not work for IP blocks, probably worse UX-wise (blocking and then unblocking a user leaves them logged out), must be reimplemented in auth extensions which might be nontrivial (e.g. on a SUL wiki the user gets logged out of all other wikis as well).

None of the BlockDisablesLogin wikis are SUL-enabled on WMF, and this generally makes no sense for third parties, too. So in order to both fix this issue and preserve UX, we only need to log out when blocking on such wikis.

Tgr added a comment.Mar 13 2016, 2:24 AM

None of the BlockDisablesLogin wikis are SUL-enabled on WMF, and this generally makes no sense for third parties, too.

Well, using CentralAuth generally makes no sense for third parties (although it still happens). But apart from that, having a private wikifarm with single sign-on is perfectly sensible.

csteipp triaged this task as "High" priority.Mar 15 2016, 7:08 PM
  • log the user out (change the token) when blocked - does not work for IP blocks, probably worse UX-wise (blocking and then unblocking a user leaves them logged out), must be reimplemented in auth extensions which might be nontrivial (e.g. on a SUL wiki the user gets logged out of all other wikis as well).

I think this is the right way to go, despite the complexity.

  • log the user out (change the token) when blocked - does not work for IP blocks, probably worse UX-wise (blocking and then unblocking a user leaves them logged out), must be reimplemented in auth extensions which might be nontrivial (e.g. on a SUL wiki the user gets logged out of all other wikis as well).

I think this is the right way to go, despite the complexity.

Also, in the private wiki case, we're mostly concerned about direct blocks. I don't think it really matters in this case for IP blocks, range blocks, etc.

My guess is to fix this, all blocked accounts should be placed in the inactive user group. Probably a regular check should be performed to see if the blocked and inactive users are still in sync.

We actually do have APCOND_BLOCKED for $wgAutopromote in order to make an implicit group for blocked users, if we wanted to go that route.

OTOH: Having the user be logged out at time of block seems a bit more fragile. In addition to various auth plugins, its also essentially doing a permission check at time of permission change, instead of at time of action.

Maybe we should do both approaches? Then it will also work with IP based blocks too.


The above patch logs users out upon block (For vanilla MW, and only for direct blocks)


This patch extends block handling, so that if $wgBlockDisableLogin is set, Title::userCan behaves as if the user is an anon. In many ways this seems safer, since the block checking is always done at time of action. And it can't be affected by authentication extensions that do their own loading from session.

hoo added a subscriber: DC.May 16 2016, 3:14 PM
Anomie merged a task: Restricted Task.Jun 3 2016, 5:26 PM
Anomie added subscribers: Glaisher, Natuur12, Zppix, Sjoerddebruin.

We retain advanced rights on our arbcomwiki after blocking (don't ask me why, i'm new), users are also being able to remove their own block and reset the session so they can read the wiki for 30 days again. Could we please make a decision?

Regarding what Sjoerd said, I indeed managed to unblock my account at a closed wiki as a test... I wonder why it takes this long to fix an important security issue.

Krenair added a comment.EditedJun 3 2016, 11:00 PM

I wonder why it takes this long to fix an important security issue.

Would you like to rank it against all the other open security issues?

@Krenair: 9 on a scale of 10. Me (term ended) and the other user left in good faith but I cannot imagine the damage that could have been done if someone who would have left in bad faith wanted to do some real damage. I don't know how the wiki's that contain even more private data than an Arbcomwiki work but I figure they use the same extension. Can you just imagine a rogue user with access to private data wanting to use it do damage without anyone knowing about it?

Honestly, I had the possibility to stay on a closed wiki for a long time using data from that wiki to manipulate an Arbcomcase regarding a block I placed. I would never do such a thing because I care for giving even the worst trolls a fair treatment but still. It shouldn't be possible.

So this has been open for several months and nothing happened. Do we have to go whistle blower here? Maybe a nice post to Wikimedia-l so our fans at Wikipediocracy have another topic to ramble about?

To patch up this issue you can just have a daily cronjob that goes over the private wiki's and logs out everyone that is in the blocked list. That's probably just a join of https://www.mediawiki.org/wiki/Manual:Ipblocks_table and https://www.mediawiki.org/wiki/Manual:User_table where user_token IS NOT NULL and ipb_expiry='infinity' and than setting the user_token to NULL. Based on https://nl.wikipedia.org/wiki/Speciaal:WebsiteMatrix that's only 30 wiki's.

That's not a very elegant solution, but it does prevent this from happening until you find a more permanent solution. Waiting for the more permanent solution could take months.

Anomie added a comment.Jun 6 2016, 2:13 PM


The above patch logs users out upon block (For vanilla MW, and only for direct blocks)

This one looks good to me, but I haven't tested. I might add a comment that it's specifically logging out only on the local wiki, since being blocked on one SUL wiki probably shouldn't force a logout everywhere.

This one mostly looks good to me too, but I haven't tested.

I note the code doesn't quite match your statement here that it "behaves as if the user is anon": If anons can take an action but there's a group that loses the ability thanks to $wgRevokePermissions, User::isEveryoneAllowed() will return false.

So this has been open for several months and nothing happened. Do we have to go whistle blower here?

Sorry for all of the delays, but please don't. From what I understand, @Bawolff is taking a look at his April patches to make sure they work with AuthManager. Ping @dpatrick , @Bawolff, and/or myself on IRC if this still looks stalled to you and you need to take action, particularly after 2016W23 has gone by.

I can confirm my patch from April still works post-auth manager and applies cleanly to master.

I can confirm my patch from April still works post-auth manager and applies cleanly to master.

\o/ excellent!

@greg or @demon , do you have advice for what the next step should be?

Next step would be deploy the patch (if its considered reviewed sufficiently). Previously that would have been csteipp, now i guess that is dpatrick (?)

Any deployer with security access should be able to deploy it... Assuming it's been adequately reviewed. Actually, why don't you have deployment access @Bawolff?

@Bawolff, thanks for verifying. I plan on deploying this today or tomorrow after testing the patch.

dpatrick added a comment.EditedJun 9 2016, 8:36 PM

This has been tested. Because AuthManager is being enabled on public wikis this week, with the final set happening within a few hours, I'm delaying applying @Bawolff's patches until next week, to allow some time for potentials problems with AuthManager to manifest.

@Multichill Thanks for your patience as we work on scheduling this, and apologies for the delay in getting the issue fixed.

These patches have been deployed:

22:41 dapatrick: Deployed patches for T129738 to wmf5
Bawolff closed this task as "Resolved".Jun 14 2016, 2:34 AM
Bawolff claimed this task.

Shouldn't this stay open until the patch is released to mediawiki master?

Shouldn't this stay open until the patch is released to mediawiki master?

Other security bugs seem to have been closed once a fix is deployed, and made to block the next release tracking bug (T133070). Then when next security release is happening, bug gets applied to master and made public.

However, I don't particularly feel attached to that methodology (Although it is nice to have the feeling of bugs getting checked off once the main part is done)

DC added a subscriber: Krd.Jun 14 2016, 9:22 AM
Krd added a subscriber: Rjd0060.Jun 14 2016, 9:32 AM

For now it seem to have worked. Still not logged out but I cannot see anything I should not see. I added a screenshot.

Re @Natuur12 : so going forward, when anyone new is blocked, they should be logged out. However that doesnt include any of the currently blocked people. The existing blocked people will now just have the blocked notice on all pages.

Multichill reopened this task as "Open".Jun 14 2016, 7:29 PM

Re @Natuur12 : so going forward, when anyone new is blocked, they should be logged out. However that doesnt include any of the currently blocked people. The existing blocked people will now just have the blocked notice on all pages.

So the actual incident for which I filed this task hasn't been resolved yet. Reopening. Should be quite easy to do a one time invalidation run as described in T129738#2355213

So the actual incident for which I filed this task hasn't been resolved yet.

Are you sure about that? There were two changes made here:

  1. When a user is blocked on these wikis, they will also be logged out. But this can't apply to IP blocks.
  2. Blocked users on these wikis effectively have the same rights as an anonymous user when it comes to title permission checks, i.e. if anons can't read pages, the blocked user can't either.

Item #2 should be taking care of your issue for your existing session, and @Natuur12 has stated it's working in their case. Are you not seeing the same behavior?

No since I can still acces the notification system which could still provide me with info I should not see. Really small change but still.

No since I can still acces the notification system which could still provide me with info I should not see. Really small change but still.

Oh i see. Notifications just checks if you have read rights in general, and not if you have read rights for a specific page.

Alternative patch, that prevents the $user->isAllowed() case, but also still does the per-title check for the better error message.

Restricted Application removed a subscriber: Zppix. · View Herald TranscriptJun 29 2016, 9:51 PM

You should move the limitation to after the UserGetRights hook call, though. Otherwise things like CentralAuth's global rights wouldn't be properly limited.

Nitpicks: At one point you have New User rather than new User, and I don't see the point of the $blkDisablesLogin variable versus just doing $config->get( 'BlockDisablesLogin' ) inside the if.

Thanks.

Modified version:

You should move the limitation to after the UserGetRights hook call, though. Otherwise things like CentralAuth's global rights wouldn't be properly limited.

Wait. Doesn't this mean that CentralAuth global group rights aren't being limitted by $this->getRequest()->getSession()->getAllowedUserRights() ?

Anomie added a comment.Jul 5 2016, 2:07 PM

You should move the limitation to after the UserGetRights hook call, though. Otherwise things like CentralAuth's global rights wouldn't be properly limited.

Wait. Doesn't this mean that CentralAuth global group rights aren't being limitted by $this->getRequest()->getSession()->getAllowedUserRights() ?

... Yes, it does. That should be fixed.

Anomie added a comment.Jul 5 2016, 2:08 PM

Code-Review: +1, but we may want to do something here about the existing issue you identified.

Bawolff edited the task description. (Show Details)Jul 18 2016, 9:27 AM

is a patch to revert in order that we can apply instead.

Bawolff closed this task as "Resolved".Jul 18 2016, 11:02 PM

bawolff !log deploy fix for T129738 to php-1.28.0-wmf.10

So the updated version of the patch should be live, which will make echo and everything else be blocked if the user is blocked but for some reason not logged out on a private wiki.

demon changed the visibility from "Custom Policy" to "Public (No Login Required)".Aug 23 2016, 1:23 AM
demon changed Security from Software security bug to None.

Change 306087 merged by jenkins-bot:
SECURITY: Make blocks log users out if $wgBlockDisablesLogin

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

Change 306115 merged by jenkins-bot:
SECURITY: Make blocks log users out if $wgBlockDisablesLogin

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

Change 306116 merged by jenkins-bot:
SECURITY: Make $wgBlockDisablesLogin also restrict logged in permissions

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

Change 306106 merged by jenkins-bot:
SECURITY: Make blocks log users out if $wgBlockDisablesLogin

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

Change 306125 had a related patch set uploaded (by Ejegg):
Make blocks log users out if $wgBlockDisablesLogin

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

Change 306126 had a related patch set uploaded (by Ejegg):
SECURITY: Make $wgBlockDisablesLogin also restrict logged in permissions

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

Change 306107 merged by jenkins-bot:
SECURITY: Make $wgBlockDisablesLogin also restrict logged in permissions

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

Change 306096 merged by jenkins-bot:
SECURITY: Make blocks log users out if $wgBlockDisablesLogin

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

Change 306088 merged by jenkins-bot:
SECURITY: Make $wgBlockDisablesLogin also restrict logged in permissions

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

Change 306125 merged by jenkins-bot:
Make blocks log users out if $wgBlockDisablesLogin

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

Change 306126 merged by jenkins-bot:
SECURITY: Make $wgBlockDisablesLogin also restrict logged in permissions

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

Change 306097 merged by jenkins-bot:
SECURITY: Make $wgBlockDisablesLogin also restrict logged in permissions

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