Page MenuHomePhabricator
Create Task
Maniphest T151910

Globally blocked users can still use Thanks
Closed, ResolvedPublic
Actions

Description

Globally blocked users (using Special:GlobalBlock, provided by extension GlobalBlocking) can still use Thanks. The extension only checks for local blocks, with $user->isBlocked().

The simplest solution is to also check for $user->isBlockedGlobally(). That method is documented as: "Do not use for actual edit permission checks! This is intended for quick UI checks.", but it looks like it runs the complete checks.

Alternatively, we could introduce a new permission like 'thank', granted to all users – checks with Title::userCan() etc. will all fail if the user is globally blocked.

Details

SubjectRepoBranchLines +/-
Security: Disable thank when the user is globally blockedmediawiki/extensions/ThanksREL1_31+4 -1
Security: Disable thank when the user is globally blockedmediawiki/extensions/ThanksREL1_27+3 -0
Security: Disable thank when the user is globally blockedmediawiki/extensions/ThanksREL1_30+3 -0
Security: Disable thank when the user is globally blockedmediawiki/extensions/ThanksREL1_29+3 -0
Security: Disable thank when the user is globally blockedmediawiki/extensions/Thanksmaster+4 -1
Customize query in gerrit

Related Objects

Event Timeline

matmarex created this task.Nov 29 2016, 6:14 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 29 2016, 6:14 PM
matmarex added a project: Thanks.Nov 29 2016, 6:17 PM
matmarex added a subscriber: • DragonflySixtyseven.

@DragonflySixtyseven pointed out that this could allow for cross-wiki harassment of users active in multiple projects (especially since notifications are cross-wiki now). I guess offending accounts can be also locked, though, which prevents them from logging in (this is from extension CentralAuth), and logged out users can't send thanks.

Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptNov 29 2016, 6:17 PM
Anomie subscribed.Nov 29 2016, 6:17 PM

Why do we have a $user->isBlocked() method that doesn't fully indicate that the user is blocked and a $user->isBlockedGlobally() method that isn't supposed to actually be called?

matmarex added a comment.Nov 29 2016, 6:22 PM

No idea. They're ancient. If we wanted to change User::isBlocked() to call User::isBlockedGlobally(), we'd first have to audit all uses of the former to see whether they want to check for permissions (where the change is OK), or for presence of a block (where it may not be OK, e.g. if we're checking to see whether we should apply another block).

matmarex added a comment.Nov 29 2016, 6:24 PM

And User::isBlockedGlobally() is definitely supposed to be called, but for UI checks (e.g. whether we should show an error message before the user spends an hour writing an article, only to find out they can't save), rather than for permissions checks (e.g. whether we should allow the user to save said article).

Legoktm subscribed.Nov 29 2016, 7:30 PM

This seems similar to T150796.

Bawolff triaged this task as High priority.Dec 13 2016, 10:38 PM
Bawolff subscribed.

This seems very misleading. We should have isBlocked() mean any type of block, and then we could have more specific isBlockedGlobally() and isBlockedLocally()

Ladsgroup moved this task from Backlog / Other to Patch pending review on the acl*security board.Jul 12 2018, 1:52 PM
Ladsgroup subscribed.

I think fixing "isBlocked" method would fix this issue too but can we tackle this high-priority security bug for now?
I made this quick fix for thanks extension:

0001-Security-Disable-thank-when-the-user-is-globally-blo.patch1 KBDownload

Restricted Application added a project: Growth-Team. · View Herald TranscriptJul 12 2018, 1:52 PM
Catrope added subscribers: Reedy, Catrope.Aug 21 2018, 1:58 AM
In T151910#4419826, @Ladsgroup wrote:

I think fixing "isBlocked" method would fix this issue too but can we tackle this high-priority security bug for now?
I made this quick fix for thanks extension:

0001-Security-Disable-thank-when-the-user-is-globally-blo.patch1 KBDownload

I approve of this patch.

@Bawolff / @Reedy et al, what's the protocol for getting this patch onto the cluster and into the repo?

Catrope removed a project: Collaboration-Team-Triage.Aug 21 2018, 1:59 AM
Catrope moved this task from Untriaged to Blocked / Awaiting info on the Collaboration-Team-Triage board.
Catrope moved this task from Inbox to Blocked on the Growth-Team board.
Reedy added a comment.Aug 21 2018, 2:17 AM
In T151910#4517500, @Catrope wrote:

@Bawolff / @Reedy et al, what's the protocol for getting this patch onto the cluster and into the repo?

So we don't bundle this in the tarballs, so the process is easier.

Do we still agree this is a High priority? I guess we probably want to avoid BEANS because it's an easy to use abuse vector, so probably don't want to have it in the open.

So... It needs deploying on all active WMF branches (just missed this weeks security window, but feel free to deploy yourselves if you want), and then it can go into gerrit into master. Then cherry picked to any supported MW branches too. No real need to cleanup the wmf branches in use particularly (ie no need to cherry pick the patch to the branches on gerrit). It'll ride the train in newer versions

Optionally you can send a message mediawiki-announce (et al) after it's released.

Catrope mentioned this in T199993: Indef blocked users can get information exposed per mail.Aug 22 2018, 10:14 PM

Thanks @Reedy! I'll deploy it myself once I get my patch on T199993 (another non-bundled extension security issue) reviewed.

Jalexander subscribed.Aug 23 2018, 4:58 AM

Just for documentation purposes on here because I know some task readers were confused (others are not, it's just not clear in the writing :) ): Global blocks are IP based instead of user based so there are no "globally blocked users" in this case. This particular bug comes up when someone is editing on an (unblocked) local account but connecting via a globally blocked IP that is "hard" blocked (does not allow editing even when logged in unless you have the ip block exemption user right). Still should be fixed (and Roan and I chatted so i know he was fixing it with this in mind) just clarifying for the audience :).

Catrope changed the visibility from "Custom Policy" to "Public (No Login Required)".Aug 23 2018, 6:37 PM
gerritbot subscribed.Aug 23 2018, 6:37 PM

Change 454884 had a related patch set uploaded (by Catrope; owner: Amir Sarabadani):
[mediawiki/extensions/Thanks@master] Security: Disable thank when the user is globally blocked

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

gerritbot added a project: Patch-For-Review.Aug 23 2018, 6:37 PM
gerritbot added a comment.Aug 23 2018, 6:54 PM

Change 454884 merged by jenkins-bot:
[mediawiki/extensions/Thanks@master] Security: Disable thank when the user is globally blocked

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

ReleaseTaggerBot added a project: MW-1.32-notes (WMF-deploy-2018-08-28 (1.32.0-wmf.19)).Aug 23 2018, 7:01 PM
Catrope closed this task as Resolved.Aug 23 2018, 7:32 PM
Catrope claimed this task.
gerritbot added a comment.Aug 23 2018, 8:27 PM

Change 454911 had a related patch set uploaded (by Reedy; owner: Amir Sarabadani):
[mediawiki/extensions/Thanks@REL1_31] Security: Disable thank when the user is globally blocked

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

gerritbot added a comment.Aug 23 2018, 8:35 PM

Change 454916 had a related patch set uploaded (by Reedy; owner: Amir Sarabadani):
[mediawiki/extensions/Thanks@REL1_30] Security: Disable thank when the user is globally blocked

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

gerritbot added a comment.Aug 23 2018, 8:36 PM

Change 454917 had a related patch set uploaded (by Reedy; owner: Amir Sarabadani):
[mediawiki/extensions/Thanks@REL1_29] Security: Disable thank when the user is globally blocked

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

gerritbot added a comment.Aug 23 2018, 8:39 PM

Change 454918 had a related patch set uploaded (by Reedy; owner: Amir Sarabadani):
[mediawiki/extensions/Thanks@REL1_27] Security: Disable thank when the user is globally blocked

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

alaa subscribed.Sep 24 2018, 7:12 AM
gerritbot added a comment.Oct 9 2018, 5:17 PM

Change 454917 abandoned by Reedy:
Security: Disable thank when the user is globally blocked

Reason:
No one cares about REL1_29 anymore

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

gerritbot added a comment.Oct 30 2018, 9:11 PM

Change 454916 merged by Umherirrender:
[mediawiki/extensions/Thanks@REL1_30] Security: Disable thank when the user is globally blocked

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

gerritbot added a comment.Oct 30 2018, 9:12 PM

Change 454918 merged by Umherirrender:
[mediawiki/extensions/Thanks@REL1_27] Security: Disable thank when the user is globally blocked

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

gerritbot added a comment.Oct 30 2018, 9:15 PM

Change 454911 merged by Umherirrender:
[mediawiki/extensions/Thanks@REL1_31] Security: Disable thank when the user is globally blocked

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

sbassett moved this task from Patch pending review to Done on the acl*security board.Sep 26 2019, 2:42 PM
Maintenance_bot removed a project: Patch-For-Review.Sep 26 2019, 3:10 PM
• chasemp added a project: Security.Feb 10 2020, 10:55 PM
• chasemp removed a project: acl*security.Feb 20 2020, 8:15 PM
Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:40 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL