Page MenuHomePhabricator

XSS on Special:UserRights
Closed, ResolvedPublic

Description

Follow up to T222261#5606910, there is an XSS on Special:UserRights and anywhere that an UserGroupMembership is passed into UserGroupMembership::getLink().

The problem happens when a user's groups are loaded, and one of them has an expiration.

Reproduction Steps

  1. Change the group-membership-link-with-expiry message in your language to include an XSS like <script>alert('hello!');</script>
  2. Go to Special:UserRights and load a user, change the user to have a right with an expiration (if they don't already).

Event Timeline

dbarratt created this task.Oct 25 2019, 6:34 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 25 2019, 6:34 PM
dbarratt updated the task description. (Show Details)Oct 25 2019, 6:35 PM
sbassett triaged this task as High priority.Oct 28 2019, 2:56 PM
sbassett added a project: Vuln-XSS.
sbassett added a subscriber: sbassett.
sbassett added a project: Security-Team.
sbassett moved this task from Incoming to Watching on the Security-Team board.

@sbassett Is the Security-Team not planning to take on this task?

@Niharika - Not at this time, no. I assumed since @dbarratt had commented on the previous task and created this one, they might have interest in picking this up. If not, the Security-Team will have to attempt a patch soon or perhaps get this on the radar of Core Platform Team, as this seems to be more an issue with mw core than originally thought.

@Niharika - Not at this time, no. I assumed since @dbarratt had commented on the previous task and created this one, they might have interest in picking this up.

No interest from me, just wanted to make sure it was documented. :)

The previous task was investigated by our team because of our work on CheckUser, but from that investigation it appears to be outside of that scope (nor does their appear to be an issue with CheckUser itself).

Anomie added a subscriber: Anomie.EditedOct 28 2019, 7:30 PM

As a general issue, this would be a duplicate of T2212: Some MediaWiki: messages not safe in HTML (tracking) or one of its subtasks. As a specific issue, everything passing 'html' as the $format to UserGroupMembership::getLink() should probably be checked; it might be that changing that method to use ->parse() rather than ->text() for that case would be the solution.

Or have it return a Message rather than a string and have callers do the right thing to it (which most probably don't at the moment).

sbassett claimed this task.EditedOct 28 2019, 8:20 PM
sbassett moved this task from Watching to In Progress on the Security-Team board.

As a general issue, this would be a duplicate of T2212: Some MediaWiki: messages not safe in HTML (tracking) or one of its subtasks. As a specific issue, everything passing 'html' as the $format to UserGroupMembership::getLink() should probably be checked; it might be that changing that method to use ->parse() rather than ->text() for that case would be the solution.
Or have it return a Message rather than a string and have callers do the right thing to it (which most probably don't at the moment).

Yeah, I think I suggested this approach, kinda, within the previous task (T222261#5151355, T222261#5156337). So I suppose it's just working through all of the occurrences, writing security patches here and then deploying them. I'll move this on our team board and claim it for now.

Simple patch to fix the larger issue within UserGroupMembership::getLink():

Mitigates the issue when testing locally on master. The unit tests also run fine, in particular UserGroupMembershipTest:testGetLink:

Using PHP 7.3.6
PHPUnit 8.5.2 by Sebastian Bergmann and contributors.

.........                                                           9 / 9 (100%)

You should really speed up these slow tests (>50ms)...
 1. 321ms to run UserGroupMembershipTest:testGetLink
 2. 320ms to run UserGroupMembershipTest:testAddAndRemoveGroups
 3. 203ms to run UserGroupMembershipTest:testGetMembershipsForUser
 4. 156ms to run UserGroupMembershipTest:testGetMembership
 5. 130ms to run UserGroupMembershipTest:testValidCovers with data set #1
 6. 130ms to run UserGroupMembershipTest:testValidCovers with data set #2
 7. 127ms to run UserGroupMembershipTest:testValidCovers with data set #0
 8. 127ms to run UserGroupMembershipTest:testValidCovers with data set #4
 9. 125ms to run UserGroupMembershipTest:testValidCovers with data set #3


Time: 9.59 seconds, Memory: 30.00 MB

OK (9 tests, 43 assertions)

And I'm not seeing any instances where group-membership-link-with-expiry messages would result in anything strange happening by now running them through parse(). I suppose I could check a few more instances of getLink() just to be safe, but this should be ready for deployment as a security patch, and tracked for the next release (T240392).

Looks like I re-discovered and fixed this issue with change https://gerrit.wikimedia.org/r/c/mediawiki/core/+/582909. I was not aware it was filed as a security bug.

sbassett closed this task as Resolved.Thu, Mar 26, 3:56 AM
sbassett reassigned this task from sbassett to matmarex.

Looks like I re-discovered and fixed this issue with change https://gerrit.wikimedia.org/r/c/mediawiki/core/+/582909. I was not aware it was filed as a security bug.

Sounds good, thanks!

sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".Thu, Mar 26, 3:56 AM
sbassett moved this task from In Progress to Our Part Is Done on the Security-Team board.
sbassett moved this task from Backlog to Done on the user-sbassett board.
sbassett reopened this task as Open.Thu, Mar 26, 3:59 AM

Actually, re-opening to attempt some backports to release branches. And obviously this won't be held for a security release since the patch went through gerrit.

Change 583571 had a related patch set uploaded (by Reedy; owner: Bartosz Dziewoński):
[mediawiki/core@REL1_33] SECURITY: UserGroupMembership: Fix HTML escaping in #getLink

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

Change 583574 had a related patch set uploaded (by Reedy; owner: Bartosz Dziewoński):
[mediawiki/core@REL1_31] SECURITY: UserGroupMembership: Fix HTML escaping in #getLink

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

Change 583574 merged by jenkins-bot:
[mediawiki/core@REL1_31] SECURITY: UserGroupMembership: Fix HTML escaping in #getLink

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

Change 583571 merged by jenkins-bot:
[mediawiki/core@REL1_33] SECURITY: UserGroupMembership: Fix HTML escaping in #getLink

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

sbassett closed this task as Resolved.Thu, Mar 26, 7:19 PM
sbassett added subscribers: Bawolff, Reedy.

Thanks @Reedy @Bawolff and @matmarex for taking care of 1.31 and 1.33!