Page MenuHomePhabricator

CVE-2021-44857, CVE-2021-44858: Unauthorized users can undo edits on any protected page and view contents of private wikis using mcrundo
Closed, ResolvedPublicSecurity

Description

The mcrundo action seems to do no checks at all for if users have permission to perform edits on that page. This means any user can undo edits on any protected page, such as the main page. E.g. this URL: https://commons.wikimedia.org/w/index.php?title=Main_Page&action=mcrundo&undo=603639572&undoafter=600544238 will allow you to undo an edit on the main page.

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

For visibility, I don't believe it's a good idea to be pushing out a security patch (as a tarball release etc) on late Thursday (UTC) or on Friday at all. Especially as we want to give people a pre-release announcement so they know it is coming, rather than just dropping a fairly major security patch and being like lol, glhf.

With December being a short month, I was planning on doing the security release next week anyway (last full week for a lot of people), and will get it out early next week (Wednesday latest).

If this had come up in mid September/October (for example), I would definitely agree that sitting on this till "now" wouldn't have been a good idea. Even if we ended up doing two releases in a quarter, sometimes I think that is the right thing to do. It's been a while since we've had an issue quite of this magnitude.

If people want to help craft the wording around both the pre-release (T292229) and actual release announcements (T292230), as well as what needs to go into the CVE request, that would be appreciated.

These are the action items I dumped in IRC yesterday:

  • stable backports for 1.35, 1.36, 1.37 are needed
  • PHP regression tests for the newly added functionality
  • Bypassing EditFilterMergedContent, as quoted above
  • Auditing remaining actions for similar exploits, I only did a quick skim
  • Disable MCR modules by default if no MCR-related stuff is being run. AFAIK the only wiki that needs it is Commons, most MediaWiki wikis really don't.

Wanted to capture @Majavah's suggestion in _security here, just so it wasn't lost:

majavah> about the mcr incident: what if we removed the ability to read the main page and common.css/js and instead encouraged people to override MediaWiki:Loginreqpagetext to explain the wiki as well

This is a great idea. It would also fix T34716: $wgWhitelistRead leaks username data (Because it allows viewing ?action=history).

<legoktm>	these actions still bypass the EditFilterMergedContent hook, which means they're bypassing SpamBlacklist and AbuseFilter. But someone more familiar with MCR than me will need to implement it. I would rate this as high but not UBN priority

I am thinking that the undo will restore to a version of the page that already passed AbuseFilter and SpamBlacklist. So it doesn't need to be checked again.

Not really, undo (potentially) merges the the revisions together, so you could end up with a brand new revision that's unique. Also I think it would be poor behavior if a vandal edits a page, you revert it and then add an AF/SB rule to block those edits in the future, and then they're able to just reinstate the edit via undo.

Pinging @Grunny who has access to this ticket already but for the sake of visibility

Thanks @Ladsgroup! We'll patch it on our side privately.

Wanted to capture @Majavah's suggestion in _security here, just so it wasn't lost:

majavah> about the mcr incident: what if we removed the ability to read the main page and common.css/js and instead encouraged people to override MediaWiki:Loginreqpagetext to explain the wiki as well

This is a great idea. It would also fix T34716: $wgWhitelistRead leaks username data (Because it allows viewing ?action=history).

Filed this separately (T297416: Restrict access to most actions on $wgWhitelistRead pages on private wikis).

Legoktm raised the priority of this task from High to Unbreak Now!.Dec 9 2021, 6:29 PM

In the short term to re-enable the actions I think we need to add:

  • A check that the revids belong to the given title (shuts down infoleak)

Normal action=edit&undo= is also lacking this check, which allows for the same private wiki read exploit. https://checkuser.wikimedia.org/w/index.php?title=Main_Page&action=edit&undo=39108&undoafter=32677

+2, same logic as what was used for the mcrundo/mcrrestore actions, and that's working fine. Can you deploy or should @Reedy or I?

CR+2, with the note that this will likely break some workflows and possibly be noticed.

T297322#7560977 is now deployed (required -3 on wmf.9), though it wasn't exploitable since @Majavah disabled $wgWhitelistRead a few minutes earlier.

The downcast should not be necessary, you should be able to do:

$this->mTitle->isSamePageAs( $undorev->getPage() ) && $this->mTitle->isSamePageAs( $oldrev->getPage() )

isSamePageAs() is marked as @since 1.36, and we need to backport this to at least 1.35, which has castFromLinkTarget.

(also please keep the code review on the task instead of the file, just so it stays centralized)

The downcast should not be necessary, you should be able to do:

$this->mTitle->isSamePageAs( $undorev->getPage() ) && $this->mTitle->isSamePageAs( $oldrev->getPage() )

isSamePageAs() is marked as @since 1.36, and we need to backport this to at least 1.35, which has castFromLinkTarget.

I just discovered RevisionLookup::getRevisionByTitle() which does the correct check at the database layer \o/ - I'll post some updated patches later today or tomorrow.

  • Bypassing EditFilterMergedContent, as quoted above

Going to request 2 different CVE's for this issue, as there are two different issues underneath, even though they both happen via action=mcrundo

CVE 1:
It is possible to use action=mcrundo to view hidden pages on private wiki that have page titles set in $wgWhitelistRead.

CVE 2:
It is possible to use action=mcrundo to replace the content of any arbitary page (that the user doesn't have edit rights for) on a wiki. This affects all wikis.

  • Bypassing EditFilterMergedContent, as quoted above

Updated patch:

Going to request 2 different CVE's for this issue, as there are two different issues underneath, even though they both happen via action=mcrundo

CVE 1:
It is possible to use action=mcrundo to view hidden pages on private wiki that have page titles set in $wgWhitelistRead.

I think this one also happens on action=undo as well, Correct?

Going to request 2 different CVE's for this issue, as there are two different issues underneath, even though they both happen via action=mcrundo

Tweaked:

CVE 1:

It is possible to use action=edit&undo=, action=mcrundo, action=mcrrestore to view private pages on a private wiki that has at least one page set in $wgWhitelistRead. MediaWiki 1.23+

CVE 2:

It is possible to use action=mcrundo, action=mcrrestore to replace the content of any arbitrary page (that the user doesn't have edit rights for) on a wiki if it is public, or is private and has at least one page set in $wgWhitelistRead. MediaWiki 1.32+

It is possible to use action=mcrundo, action=mcrrestore to replace the content of any arbitrary page (that the user doesn't have edit rights for) on a wiki if it is public, or is private and has at least one page set in $wgWhitelistRead. MediaWiki 1.32+

I think this only applies to pages which the user can see, so for private wikis only the pages in $wgWhitelistRead are affected for logged-out users.

Going to request 2 different CVE's for this issue, as there are two different issues underneath, even though they both happen via action=mcrundo

Does the EditFilterMergedContent issue need its own CVE too or is it covered by CVE 2?

Updated patch:

You're awesome. Code-Review +1 just because I'm a bit tired right now. Would be nice if someone more familiar with modern AbuseFilter could also look, but logic seems in line with T271037#7178772, which is also coming out with this release.

If you could double check that all the classes/functions referenced are available in 1.35 that would be appreciated too.

Does the EditFilterMergedContent issue need its own CVE too or is it covered by CVE 2?

I think it's covered by CVE 2, which is generally about not checking editing permissions.

Updated patch:

You're awesome. Code-Review +1 just because I'm a bit tired right now. Would be nice if someone more familiar with modern AbuseFilter could also look, but logic seems in line with T271037#7178772, which is also coming out with this release.

If you could double check that all the classes/functions referenced are available in 1.35 that would be appreciated too.

Ping @Daimona :)

Updated patch:

You're awesome. Code-Review +1 just because I'm a bit tired right now. Would be nice if someone more familiar with modern AbuseFilter could also look, but logic seems in line with T271037#7178772, which is also coming out with this release.

If you could double check that all the classes/functions referenced are available in 1.35 that would be appreciated too.

I can't test that it works properly since I don't have a 1.35 database handy, but all relevant classes and method seem to be present. First two patches applied to REL1_35 (with -3), but the EditFilterMergedContent one needed a little adjustment:

Going to request 2 different CVE's for this issue, as there are two different issues underneath, even though they both happen via action=mcrundo

Tweaked:

CVE 1:

It is possible to use action=edit&undo=, action=mcrundo, action=mcrrestore to view private pages on a private wiki that has at least one page set in $wgWhitelistRead. MediaWiki 1.23+

CVE 2:

It is possible to use action=mcrundo, action=mcrrestore to replace the content of any arbitrary page (that the user doesn't have edit rights for) on a wiki if it is public, or is private and has at least one page set in $wgWhitelistRead. MediaWiki 1.32+

In CVE 1, I would change "view private pages" to "view private revisions" to be more specific, although the exploit will not work when the revision is part of a deleted page or revision deleted as far as I can tell, not sure if that needs to be emphasized. In CVE 2 you need to gain edit rights in some form to replace pages with arbitrary content, without edit rights you can't replace Common.js with malicious JS for example, without first creating a revision to restore (which you can't do without edit rights). Thus without already having edit rights on the wiki, the risk of CVE 2 is decreased (eliminates arbitrary JavaScript execution) and the exploit is limited to replacing page content with revisions that already exist.

CVE request has gone in ;). It can be updated at a later date :)

This comment was removed by Dylsss.

@Majavah noted that this was still exploitable via the k8s-experimental setting on the X-Wikimedia-Debug header/browser extension. It seems the k8s deployment may not be getting security patches reliably; we're investigating, but in the meantime https://gerrit.wikimedia.org/r/c/operations/puppet/+/745963 fully disabled the k8s-experimental option, so that issue is remediated.

Once we get the k8s build issues sorted out, we should revert that patch (e.g. @Ladsgroup's https://gerrit.wikimedia.org/r/745873) so as not to inconvenience MW-on-k8s development unnecessarily.

@Majavah noted that this was still exploitable via the k8s-experimental setting on the X-Wikimedia-Debug header/browser extension. It seems the k8s deployment may not be getting security patches reliably; we're investigating, but in the meantime https://gerrit.wikimedia.org/r/c/operations/puppet/+/745963 fully disabled the k8s-experimental option, so that issue is remediated.

Once we get the k8s build issues sorted out, we should revert that patch (e.g. @Ladsgroup's https://gerrit.wikimedia.org/r/745873) so as not to inconvenience MW-on-k8s development unnecessarily.

This was my bad, I didn't commit the patches to /srv/patches. Did that and kicked off a new build. Verified that k8s-experimental is fixed now with curl -k -H 'Host: checkuser.wikimedia.org' 'https://mwdebug.discovery.wmnet:4444/w/index.php?title=Main_Page&action=mcrundo&undo=39108&undoafter=32678' | less.

The k8s install is now patched and the Varnish revert is merged, to be rolled out everywhere over the next 30 minutes.

In CVE 1, I would change "view private pages" to "view private revisions" to be more specific, although the exploit will not work when the revision is part of a deleted page or revision deleted as far as I can tell, not sure if that needs to be emphasized. In CVE 2 you need to gain edit rights in some form to replace pages with arbitrary content, without edit rights you can't replace Common.js with malicious JS for example, without first creating a revision to restore (which you can't do without edit rights). Thus without already having edit rights on the wiki, the risk of CVE 2 is decreased (eliminates arbitrary JavaScript execution) and the exploit is limited to replacing page content with revisions that already exist.

@Dylsss, I cc'd you on T297541 where we have a draft FAQ for this bug, please feel free to edit it with what you describe here. CVE descriptions are usually short summaries, the nuance you are suggesting would fit better in the FAQ.

I squashed the two patches together, and switched to the much simpler getRevisionByTitle() approach.

master/REL1_37 (needs -3):

REL1_36/REL1_35:

And for Debian only, REL1_31/REL1_27 (needs -3):

I squashed the two patches together, and switched to the much simpler getRevisionByTitle() approach.

Thank you! Thoughts on the third patch (EditFilterMergedContent)? I fear that mentioning that in this task will block making this one public in time if we don't get it reviewed / deployed in time :-/

Thank you! Thoughts on the third patch (EditFilterMergedContent)? I fear that mentioning that in this task will block making this one public in time if we don't get it reviewed / deployed in time :-/

It looks good to me, Code-Review +2, though a review from Daimona would still be appreciated if he has time. And then let's plan to deploy it Monday morning, along with the updated patches.

I hereby suggest as a follow up, we fully and irreversibly deprecate $wgWhitelistRead and whole concept of "partially private wiki" (I know it's being done in T297416: Restrict access to most actions on $wgWhitelistRead pages on private wikis but I want to emphasize this shouldn't be temporary)

I want to thank everyone here very much for finding and fixing this. The bit where users can use mcrundo to change pages they don't have edit rights for is very likely my fault. I'll investigate in Monday how that happened. Getting permission checks right was definitely a consideration at the time, but we obviously messed it up...

Reedy changed the task status from Open to In Progress.Dec 13 2021, 3:00 AM
Reedy renamed this task from Unauthorized users can undo edits on any protected page and view contents of private wikis using mcrundo to CVE-2021-44857, CVE-2021-44858:Unauthorized users can undo edits on any protected page and view contents of private wikis using mcrundo .Dec 13 2021, 3:05 AM
Reedy renamed this task from CVE-2021-44857, CVE-2021-44858:Unauthorized users can undo edits on any protected page and view contents of private wikis using mcrundo to CVE-2021-44857, CVE-2021-44858: Unauthorized users can undo edits on any protected page and view contents of private wikis using mcrundo .Dec 13 2021, 3:17 AM

Thank you! Thoughts on the third patch (EditFilterMergedContent)? I fear that mentioning that in this task will block making this one public in time if we don't get it reviewed / deployed in time :-/

It looks good to me, Code-Review +2, though a review from Daimona would still be appreciated if he has time. And then let's plan to deploy it Monday morning, along with the updated patches.

I just tried to deploy this (deploy1002:/home/taavi/T297322-EditFilterMergedContent.patch), it did not work while testing on mwdebug1001. Not sure what's going on as it worked fine locally.

Thank you! Thoughts on the third patch (EditFilterMergedContent)? I fear that mentioning that in this task will block making this one public in time if we don't get it reviewed / deployed in time :-/

It looks good to me, Code-Review +2, though a review from Daimona would still be appreciated if he has time. And then let's plan to deploy it Monday morning, along with the updated patches.

I just tried to deploy this (deploy1002:/home/taavi/T297322-EditFilterMergedContent.patch), it did not work while testing on mwdebug1001. Not sure what's going on as it worked fine locally.

Deployed the following patch:

Thanks for the deploy, @Majavah. SAL. Also updated T276237 with the three patches from this bug and the one related patch from T297574.

Deployed the following patch:

Having double checked, I don't think we got the hook return value right. The issue outlined in T271037 is that if the hook does not abort so $hookResult is true, we still need to check whether the status has an error. So it should look like:

				if ( !$hookResult ) {
					if ( $status->isGood() ) {
						$status->error( 'hookaborted' );
					}

					return $status;
				} elseif ( !$status->isOK() ) {
					if ( !$status->getErrors() ) {
						$status->error( 'hookaborted' );
					}
					return $status;
				}

I think that should match EditFilterMergedContentHookConstraint. I'll post a new set of squashed patches.

Squashed my patches from T297322#7564452 and @Majavah's EditFilterMergedContent one with the fix I suggested above.

The REL1_36 patch applies to REL1_35 too.

And for Debian only:

Squashed my patches from T297322#7564452 and @Majavah's EditFilterMergedContent one with the fix I suggested above.

The EditFilterMergedContent fix looks good, thank you!

I replaced the currently deployed patches with the squashed ones from T297322#7568466.

The CVEs were flipped in the above set of squashed patches. Here is hopefully the final set of patches:

Just for Debian! and worried that if I don't upload it here I'm going to lose it in this mess of patches...

I was more wondering where the REL1_35 one was ;)

I was more wondering where the REL1_35 one was ;)

Oh oops. The REL1_36 one directly applies to REL1_35 as well. It all should be correct in the table on T292227.

Thanks all for discovering and fixing these issues! I am very sorry to have caused some of them.

Given the severity of these issues (I don't think there was a comparable vulnerability in MediaWiki before), and that most wikis probably aren't equipped to patch things quickly, would it make sense to share a LocalSettings snippet and wait a bit with sharing the patches (and thus the exact vulnerabilities)? For the write permission issues I don't think that's possible without making the bug easy to guess, but for the read issues just asking people to disable $wgWhitelistRead wouldn't give away much.

The bit where users can use mcrundo to change pages they don't have edit rights for is very likely my fault.

It isn't; it was introduced in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/447099.

Given the severity of these issues (I don't think there was a comparable vulnerability in MediaWiki before), and that most wikis probably aren't equipped to patch things quickly, would it make sense to share a LocalSettings snippet and wait a bit with sharing the patches (and thus the exact vulnerabilities)? For the write permission issues I don't think that's possible without making the bug easy to guess, but for the read issues just asking people to disable $wgWhitelistRead wouldn't give away much.

I think this was discussed but was decided against in that even this solution was considered too risky to divulge within any kind of pre-release announcement, hence the pre-release announcement that was sent. The full announcement does offer a "I don't have time to patch, how do I disable this?" LocalSettings.php solution as seen in the draft at T297541.

Change 747571 had a related patch set uploaded (by Reedy; author: Legoktm):

[mediawiki/core@REL1_35] SECURITY: Fix permissions checks in undo actions

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

Change 747578 had a related patch set uploaded (by Reedy; author: Legoktm):

[mediawiki/core@REL1_36] SECURITY: Fix permissions checks in undo actions

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

Change 747585 had a related patch set uploaded (by Reedy; author: Legoktm):

[mediawiki/core@REL1_37] SECURITY: Fix permissions checks in undo actions

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

Change 747596 had a related patch set uploaded (by Reedy; author: Legoktm):

[mediawiki/core@master] SECURITY: Fix permissions checks in undo actions

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

Change 747571 merged by jenkins-bot:

[mediawiki/core@REL1_35] SECURITY: Fix permissions checks in undo actions

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

Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".
Reedy changed the edit policy from "Custom Policy" to "All Users".

Change 747585 merged by jenkins-bot:

[mediawiki/core@REL1_37] SECURITY: Fix permissions checks in undo actions

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

Change 747578 merged by jenkins-bot:

[mediawiki/core@REL1_36] SECURITY: Fix permissions checks in undo actions

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

Change 747596 merged by jenkins-bot:

[mediawiki/core@master] SECURITY: Fix permissions checks in undo actions

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

Change 748706 had a related patch set uploaded (by Daniel Kinzler; author: Daniel Kinzler):

[mediawiki/core@master] McrUndoAction: use authorizeWrite for permission checks.

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

Change 748706 merged by jenkins-bot:

[mediawiki/core@master] McrUndoAction: use authorizeWrite for permission checks.

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