@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 Platform Engineering, as this seems to be more an issue with mw core than originally thought.

In T236509#5612918, @sbassett wrote:

@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).

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

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.

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