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.

In T129738#2115180, @Multichill wrote:

In T129738#2114985, @Krenair wrote:

Not an Znuny 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 Znuny project on it.

It's not a ticket.wikimedia.org issue so it's not Znuny (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.

In T129738#2115180, @Multichill wrote:

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.

In T129738#2115182, @Multichill wrote:

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.

In T129738#2115185, @Tgr wrote:

• 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.

In T129738#2115191, @MaxSem wrote:

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.

In T129738#2123780, @csteipp wrote:

• 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.

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.

In T129738#2354690, @Natuur12 wrote:

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.

In T129738#2218016, @Bawolff wrote:

Make Blocks log out user in question if $wgBlockDisablesLogin is set1 KBDownload

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.

Change permissions to make blocked users like anon when BlockDisablesLogin3 KBDownload

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.

In T129738#2355213, @Multichill wrote:

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.

In T129738#2363011, @Bawolff wrote:

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.

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

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

In T129738#2378214, @Krenair wrote:

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)

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.

In T129738#2380811, @Multichill wrote:

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.

In T129738#2387658, @Natuur12 wrote:

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.

In T129738#2416302, @Bawolff wrote:

[v2] Remove rights from blocked users if block disables login4 KBDownload

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() ?

In T129738#2424928, @Bawolff wrote:

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.

In T129738#2424927, @Bawolff wrote:

[v3] Make wgBlockDisablesLogin restrict logged in users rights to anon4 KBDownload

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

Split to T139670

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