Page MenuHomePhabricator

CVE-2024-: XSS in edit summary parser
Closed, ResolvedPublicSecurity

Description

Steps to reproduce:

  • Create a page named PageThatExists
  • On some other page (not PageThatExists), create an edit with the following edit summary: [[Special:RecentChanges#%1b0000000|link1]] [[PageThatExists#/autofocus/onfocus=alert("xss\n"+document.domain)//|link2]]
  • View the history of that page. [This assumes that the edit you just made was the most recent edit to the page]
  • An alert box pops up announcing the XSS.

Suggested fix:

  • Change marker syntax in MediaWiki\CommentFormatter\CommentParser to have " and ' in it like Parser does instead of being just \x1b

Explanation:

This is similar to T110143

During parsing, page titles get url decoded (It should be noted, that this doesn't work in all spots, because some things decode \x1b as the unicode replacement character, particularly entity decoders). This allows the attacker to insert an \x1B character after they were all stripped, and hence allows the attacker to insert a link marker. If a link marker is inserted inside an attribute.

Whether the link is directly inserted or if it uses a strip marker depends on if the link is in link cache. Additionally non existing links generally don't have a fragment (The actual page name is not allowed to have 0x1B in it). So we want the first link to always be in link cache, and the second link to not be in link cache but be an existing page. It should be noted, that this cache, as well as the marker numbering, is shared across all the edit summaries on the page (including one more than is shown, so limit=1 would actually have 2 edit summaries parsed). Special pages always exist so they are treated as if they are always in link cache regardless of what is there. Then we just have to hope that the other page we use is not, but as long as it isn't mentioned anywhere else on the page, we should be good.

The end result is - The special recentchanges link is substituted immediately. It has a marker. When markers are replaced there is one in the attribute that is replaced, this breaks out of the html.

So after first step we get: (pretend \x1B are a literal U+001B character)

<a href="/w/index.php/Special:RecentChanges#\x1B000000" title="Special:RecentChanges">link1</a> \x1B0000000

Then we do finalize to replace link markers which gives us:

<a href="/w/index.php/Special:RecentChanges#<a href="/w/index.php/Test#/autofocus/onfocus=alert(&quot;xss\n&quot;+document.domain)//" title="Test">link2</a>" title="Special:RecentChanges">link1</a> <a href="/w/index.php/Test#/autofocus/onfocus=alert(&quot;xss\n&quot;+document.domain)//" title="Test">link2</a>

HTML parses that as if it is:

<a href="/w/index.php/Special:RecentChanges#<a href=" w="" index.php="" Test#="" autofocus onfocus="alert(&quot;xss\n&quot;+document.domain)//&quot;" title="Test">link2</a>" title="Special:RecentChanges"&gt;link1</a> <a href="/w/index.php/Test#/autofocus/onfocus=alert(&quot;xss\n&quot;+document.domain)//" title="Test">link2</a>

Browser sees autofocus attribute so focuses the link on page load. Then the onfocus event is triggered, so the JS runs.

Event Timeline

For cross reference purposes, it seems like this was introduced when support for LinkCache was added in f7f84dddb33ee4bde12608115a6015ae9463b8d6 (T285917), so MediaWiki 1.38.0 and later are vulnerable.

An alternative patch would be the following:

diff --git a/includes/CommentFormatter/CommentParser.php b/includes/CommentFormatter/CommentParser.php
index 455d714f89f..f2c53dd09e1 100644
--- a/includes/CommentFormatter/CommentParser.php
+++ b/includes/CommentFormatter/CommentParser.php
@@ -362,7 +362,7 @@ class CommentParser {
                                if ( strpos( $match[1], '%' ) !== false ) {
                                        $match[1] = strtr(
                                                rawurldecode( $match[1] ),
-                                               [ '<' => '&lt;', '>' => '&gt;' ]
+                                               [ '<' => '&lt;', '>' => '&gt;', "\x1b" => "\u{FFFD}" ]
                                        );
                                }

However, I think changing the marker syntax is much more robust and I trust it much more to be complete. I suppose we could maybe do both,

sbassett triaged this task as Medium priority.Jan 22 2024, 5:18 PM
sbassett moved this task from Incoming to Security Patch To Deploy on the Security-Team board.
sbassett added a project: SecTeam-Processed.
sbassett changed Risk Rating from N/A to Medium.

Proposed patch:

Tested via MediaWiki-Docker and can confirm both the issue and that this patch seems to fix it. Not sure if there will be caching issues here - I applied, un-applied and re-applied the patch a few different times while testing a few different pages and the old rendered edit summaries (with the xss) seemed to still exist until I cleared my local browser cache, though that may have just been Chrome.

Anyhow, very interesting find! Hopefully we can get this deployed today during the security window and then tracked for the next core release under T353895.

Just FYI, there was a minor issue with the deploy, bug filed here: T355622. Looks like SRE forgot to remove a re-purposed mw host from scap's config, but that appears to be fixed now.

SecurityPatchBot raised the priority of this task from Medium to Unbreak Now!.Mar 8 2024, 9:49 AM

Patch 01-T355538.patch is currently failing to apply for the most recent code in the mainline branch of core. This is blocking MediaWiki release 1.42.0-wmf.22(T354440)

If the patch needs to be rebased

To unblock the release, a new version of the patch can be placed at the right location in the deployment server with the following Scap command:

REVISED_PATCH=<path_to_revised_patch>
scap update-patch --message-body 'Rebase to solve merge conflicts with mainline code' /srv/jenkins-agent/workspace/Branch cut pretest patches/work/patches/1.42.0-wmf.22/core/01-T355538.patch "$REVISED_PATCH"

If the patch has been made public

To unblock the release, the patch can be removed for the right version from the deployment server with the following Scap command:

scap remove-patch --message-body 'Remove patch already made public' /srv/jenkins-agent/workspace/Branch cut pretest patches/work/patches/1.42.0-wmf.22/core/01-T355538.patch

(Note that if patches for the version don't exist yet, they will be created and the patch you specified removed)

The patch paths mentioned in the scap commands above are wrong. They should read: /srv/patches/1.42.0-wmf.22/core/01-T355538.patch.

Sry about that, working on fixing it in the notifier.

jnuche lowered the priority of this task from Unbreak Now! to Medium.Mar 8 2024, 10:43 AM

Testing message fix. Changing priority manually to allow for another notification to get through.

SecurityPatchBot raised the priority of this task from Medium to Unbreak Now!.Mar 8 2024, 10:44 AM

Patch 01-T355538.patch is currently failing to apply for the most recent code in the mainline branch of core. This is blocking MediaWiki release 1.42.0-wmf.22(T354440)

If the patch needs to be rebased

To unblock the release, a new version of the patch can be placed at the right location in the deployment server with the following Scap command:

REVISED_PATCH=<path_to_revised_patch>
scap update-patch --message-body 'Rebase to solve merge conflicts with mainline code' /srv/patches/1.42.0-wmf.22/core/01-T355538.patch "$REVISED_PATCH"

If the patch has been made public

To unblock the release, the patch can be removed for the right version from the deployment server with the following Scap command:

scap remove-patch --message-body 'Remove patch already made public' /srv/patches/1.42.0-wmf.22/core/01-T355538.patch

(Note that if patches for the version don't exist yet, they will be created and the patch you specified removed)

Hm, the patch from T355538#9479104, which was deployed and should be the exact same patch as /srv/patches/1.42.0-wmf.22/core/01-T355538.patch seems to apply just fine to master and wmf/1.42.0-wmf.21 for me. Since we're currently on wmf/1.42.0-wmf.21, is there somewhere we could get access to whatever test version of 1.42.0-wmf.22 @SecurityPatchBot is looking at? Since wmf/1.42.0-wmf.22 won't be cut for core until next Monday and I don't see any obvious test instances of it on the deployment hosts.

jnuche lowered the priority of this task from Unbreak Now! to Medium.Mar 11 2024, 10:09 AM

Looked into it and this was a false positive.

The git workspace where we run the checks somehow got an untracked change. That led to a failure for the first patched being applied, which happened to be 01-T355538.patch:

02:02:01 Applying patch /srv/jenkins-agent/workspace/Branch cut pretest patches/work/patches/branch_cut_pretest/core/01-T355538.patch in /srv/jenkins-agent/workspace/Branch cut pretest patches/work/mediawiki/php-branch_cut_pretest
02:02:01 ERROR: git is not clean: /srv/jenkins-agent/workspace/Branch cut pretest patches/work/mediawiki/php-branch_cut_pretest/.
[...]
02:02:01 [FAILED] /srv/jenkins-agent/workspace/Branch cut pretest patches/work/patches/branch_cut_pretest/core/01-T355538.patch

I cleaned the workspace and the patch now applies successfully. Sorry about the noise!

Reedy renamed this task from XSS in edit summary parser to CVE-2024-: XSS in edit summary parser.Mar 26 2024, 2:53 PM

Change #1015409 had a related patch set uploaded (by Reedy; author: Brian Wolff):

[mediawiki/core@REL1_39] SECURITY: Ensure CommentParser link processing does not lead to XSS

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

Change #1015414 had a related patch set uploaded (by Reedy; author: Brian Wolff):

[mediawiki/core@REL1_40] SECURITY: Ensure CommentParser link processing does not lead to XSS

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

Change #1015418 had a related patch set uploaded (by Reedy; author: Brian Wolff):

[mediawiki/core@REL1_41] SECURITY: Ensure CommentParser link processing does not lead to XSS

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

Change #1015422 had a related patch set uploaded (by Reedy; author: Brian Wolff):

[mediawiki/core@master] SECURITY: Ensure CommentParser link processing does not lead to XSS

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

Change #1015414 merged by jenkins-bot:

[mediawiki/core@REL1_40] SECURITY: Ensure CommentParser link processing does not lead to XSS

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

Change #1015418 merged by jenkins-bot:

[mediawiki/core@REL1_41] SECURITY: Ensure CommentParser link processing does not lead to XSS

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

Change #1015409 merged by jenkins-bot:

[mediawiki/core@REL1_39] SECURITY: Ensure CommentParser link processing does not lead to XSS

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

Reedy claimed this task.
Reedy subscribed.

While rMWf7f84dddb33e: Introduce CommentFormatter (T285917: Batching of Linker::formatComment()) has a lot of refactoring, it seems to have been introduced in that patch, which was in MW >= 1.38.0.

Change #1015422 merged by jenkins-bot:

[mediawiki/core@master] SECURITY: Ensure CommentParser link processing does not lead to XSS

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

Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".Mar 28 2024, 11:31 PM
Reedy changed the edit policy from "Custom Policy" to "All Users".