Page MenuHomePhabricator

Remove or limit edituserjs and similar rights
Open, Needs TriagePublic

Description

The (somewhat misleadingly named) edituserjs/editusercss/edituserjson permissions allow a user to edit another user's personal JS/CSS/JSON files. There is little practical use for this (it might be used to help users who messed up their personal scripts but just telling them what to change would work just as well) and it's a convenient attack vector. Either it should be removed or limited so that users cannot edit the scripts/styles of a user who has more permission (e.g. an admin should not be able to edit a bureaucrat's personal scripts; a bureaucrat should not be able to edit a steward's).

(This issue has been discussed in various places, but a dedicated task did not exist.)

Event Timeline

Tgr created this task.Jun 13 2018, 10:23 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 13 2018, 10:23 AM
Izno added subscribers: TheDJ, Izno.Jun 13 2018, 4:24 PM

@TheDJ has done quite a bit of work maintaining user scripts (I guess he would prefer not to do it, but I do know he currently provides his time in this fashion) that less-experienced users would probably not be willing (read: "sure-enough") or able to do. Removing the permissions is not sensible from this dimension.

I would guess with T190015: Create separate user group for editing sitewide CSS/JavaScript that does not include administrators by default he would still be a member of the technical administrator group. Were we to limit what tech admin can edit, I don't think a reliance on other rights packages makes sense in this context.

Nirmos added a subscriber: Nirmos.Jun 14 2018, 2:12 AM
Tgr added a comment.Jun 15 2018, 4:46 PM

@Izno do less-experienced users involve admins, stewards etc? The main concern here is that someone compromising a technical administrator account can use this ability to take over something more powerful, like a steward account. (That could be done by editing non-personal Javascript as well, but that's much easier to notice - there are only so many JS pages that get loaded and a large number of people are bound to be watching them. Creating User:Foo/vector.js is much more likely to avoid notice.)

An option to reduce the attack vector, while allowing other users to "clean" broken code, would be the ability to only blank or delete the page, instead of modifying it. Blanking the page wouldn't compromise anything.

I'd prefer blanking over deleting, because the user can recover or revert it on its own, while deleting the page will remove that possibility except for users with the right to see deleted revisions. However, I don't know how this could be implemented in the UI

Tgr added a comment.Jun 15 2018, 5:30 PM

I'd prefer blanking over deleting, because the user can recover or revert it on its own, while deleting the page will remove that possibility except for users with the right to see deleted revisions. However, I don't know how this could be implemented in the UI

Not easily; performance checks happen well before you decide what to save. It could be done with a special page, but that seems a lot of hassle for something like this.

Comparing the permissions of the page owner to the permissions of the editor and refusing if they are higher still seems like the easiest approach to me (CentralAuth makes it a bit more complicated but probably still doable).

Do you plan to decide based on the number of user rights/permissions of each user? permissionCount(userA) > permissionCount(userB)

Izno added a comment.Jun 15 2018, 5:55 PM

@Izno do less-experienced users involve admins, stewards etc?

As an example, many users not intimately familiar with how Javascript and CSS pages work are (appropriately) afraid of the banner that says "this page can be used to execute malicious code". This set of users does indeed include those holding advanced permissions. So in this context, absolutely. (Hence the reason these rights are being removed from the administrator package.)

There is also the ease of it--TheDJ would have to fork the page in question, make the changes, then ask the user to copy his changes, and then nuke whatever copy he had made, if he could not edit the pages directly. This ease is valuable (for the same reason that it is a dangerous vector).

The main concern here is that someone compromising a technical administrator account can use this ability to take over something more powerful, like a steward account.

I understood this to be the case from the description. However, the tech admin group is already going to have mandatory 2FA or some similar requirement, which significantly reduces the probability of an account compromise (maybe even to negligible if not zero). This is one dimension of your classical risk cube.

As for the other dimension of the risk cube (consequence), I want to avoid elaborating too much on the possible consequences of taking an advanced permission holder's account, but either a) those consequences would be logged for review and subsequent alerting or b) are so massive that everyone will understand what has happened. In both cases, Someone will be contacted to remove the rights from the compromised account.

Tgr added a comment.Jun 15 2018, 7:39 PM

However, the tech admin group is already going to have mandatory 2FA or some similar requirement, which significantly reduces the probability of an account compromise (maybe even to negligible if not zero).

It is reduced, but definitely not negligible. The recent round of compromises had nothing to do with passwords, for example.

I want to avoid elaborating too much on the possible consequences of taking an advanced permission holder's account, but either a) those consequences would be logged for review and subsequent alerting or b) are so massive that everyone will understand what has happened.

The way a typical attack would go, someone compromises a (tech)admin account on eowiktionary, use that to take over an account that's admin on enwiki, then put viagra ads on the enwiki main page or whatever. The time between the two account compromises is the window for preventing the attack; there is decent chance that someone would notice the attack quickly if it's done via MediaWiki:Common.js, much less if it's done via the user js page of some user whom the denizens of that wiki don't even recognize.

I see your point of how this would obstruct some people's work, though. Maybe there is a better way, e.g. flagging such edits as dangerous and having the owner approve them somehow.

Do you plan to decide based on the number of user rights/permissions of each user? permissionCount(userA) > permissionCount(userB)

Obviously not. Probably have a precedence list of very powerful permissions (steward > bureaucrat > admin); if on some wiki userA has a higher-precedence permission than userB then userB editing the JS of userA (on any wiki) should be considered a dangerous edit.

TheDJ added a comment.Jun 15 2018, 9:26 PM

There is also the ease of it--TheDJ would have to fork the page in question, make the changes, then ask the user to copy his changes, and then nuke whatever copy he had made, if he could not edit the pages directly. This ease is valuable (for the same reason that it is a dangerous vector).

Yeah not doing that ;)
I drag people out of a tough spot, but I spent like 5 minutes doing so most of the time. The more work it is, the less I can justify doing these often 'in-between' things. Besides, quite often these people can't even copy paste the scripts without accidentally introducing syntax errors. Many people just see symbols, not javascript when looking at these pages. Which is again, why in modern days this feature would never have been added of course.

Vvjjkkii renamed this task from Remove or limit edituserjs and similar rights to o4aaaaaaaa.Jul 1 2018, 1:04 AM
Vvjjkkii triaged this task as High priority.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed a subscriber: Aklapper.
IKhitron renamed this task from o4aaaaaaaa to Remove or limit edituserjs and similar rights.Jul 1 2018, 3:29 PM
IKhitron raised the priority of this task from High to Needs Triage.
IKhitron updated the task description. (Show Details)
IKhitron added a subscriber: Aklapper.
IKhitron added a subscriber: IKhitron.
He7d3r added a subscriber: He7d3r.Jul 12 2018, 3:22 PM

I do think techadmin right is enough. There should always some users that can control the whole websites - if nobody have edituserjs rights it will be impossible to manage libelous and non-public personal information someone may placed on their JS or CSS page.

MBH added a subscriber: MBH.Jul 20 2018, 8:55 AM

I think it will be enough to make impossible to edit personal js/css files of users, who can edit sitewide js/css. So, ordinary admins should can edit personal js/css of ordinary users, but not techadmins (maybe admins also). This will prevent the spread of malicious code throughout the site.

It is rare (though valid) that personal js/css can be edited by others, so I think techadmin right is enough. MediaWiki technically does not have a hierachical system of user groups - admins can block any users, including stewards.

edituserjs/editusercss/edituserjson permissions have been revoked from sysops in rMWdb888bc5ad

With this being removed from sysop, is it actually also being ADDED to techadmin - or will it be orphaned?

Izno added a comment.Jul 27 2018, 9:15 PM

With this being removed from sysop, is it actually also being ADDED to techadmin - or will it be orphaned?

According to the linked patch, it is being added to tech admin at this time. I think the jury is out as to the necessity of this ticket at this time...

RP88 added a subscriber: RP88.Jul 30 2018, 4:57 PM
1997kB added a subscriber: 1997kB.Jul 31 2018, 8:02 AM
1997kB removed a subscriber: 1997kB.Jan 25 2019, 3:10 PM