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.

In T297322#7560492, @Legoktm wrote:

In T297322#7559979, @sbassett wrote:

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 old revisions and diffs).

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

In T297322#7560977, @Legoktm wrote:

0001-SECURITY-Restore-check-that-undo-revisions-are-on-th.patch1 KBDownload

+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?

In T297322#7560977, @Legoktm wrote:

0001-SECURITY-Restore-check-that-undo-revisions-are-on-th.patch1 KBDownload

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.

In F34871493#2767, @daniel wrote:

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)

In T297322#7561433, @Legoktm wrote:

In F34871493#2767, @daniel wrote:

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.

In T297322#7560492, @Legoktm wrote:

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

In T297322#7563704, @Majavah wrote:

In T297322#7560492, @Legoktm wrote:

• Bypassing EditFilterMergedContent, as quoted above

0001-SECURITY-Call-EditFilterMergedContent-in-McrUndo.patch2 KBDownload

Updated patch:

In T297322#7563756, @Reedy wrote:

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?

In T297322#7563756, @Reedy wrote:

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 T297322#7563759, @Legoktm wrote:

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.

In T297322#7563756, @Reedy wrote:

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?

In T297322#7563757, @Majavah wrote:

Updated patch:

0001-SECURITY-Call-EditFilterMergedContent-in-McrUndo.patch3 KBDownload

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.

In T297322#7563767, @Majavah wrote:

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.

In T297322#7563774, @Legoktm wrote:

In T297322#7563757, @Majavah wrote:

Updated patch:

0001-SECURITY-Call-EditFilterMergedContent-in-McrUndo.patch3 KBDownload

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

In T297322#7563774, @Legoktm wrote:

In T297322#7563757, @Majavah wrote:

Updated patch:

0001-SECURITY-Call-EditFilterMergedContent-in-McrUndo.patch3 KBDownload

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:

In T297322#7563759, @Legoktm wrote:

In T297322#7563756, @Reedy wrote:

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

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

In T297322#7564232, @RLazarus wrote:

@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 T297322#7563863, @Dylsss wrote:

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

In T297322#7564452, @Legoktm wrote:

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 :-/

In T297322#7564459, @Majavah wrote:

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

In T297322#7564460, @Legoktm wrote:

In T297322#7564459, @Majavah wrote:

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.

In T297322#7565912, @Majavah wrote:

In T297322#7564460, @Legoktm wrote:

In T297322#7564459, @Majavah wrote:

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.

In T297322#7567524, @Majavah wrote:

Deployed the following patch:

T297322-EditFilterMergedContent-rev3.patch1 KBDownload

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:

In T297322#7568466, @Legoktm wrote:

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:

REL1_31? :)

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

In T297322#7571412, @Reedy wrote:

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.

In T297322#7565234, @daniel wrote:

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.

In T297322#7571490, @Tgr wrote:

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

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