Page MenuHomePhabricator

CVE-2024-40605: Foreground skin: stored XSS via MediaWiki:Sidebar
Closed, ResolvedPublicSecurity

Description

Top-level menu entries from MediaWiki:Sidebar are not properly escaped in the MediaWiki-skins-Foreground skin, resulting in classic stored XSS. The output of Sanitizer::escapeIdForAttribute is not HTML-safe, but the skin incorrectly assumes it is.

Minimal test case:

  1. As an (interface) administrator or other similarly privileged (editinterface) user, create a top level menu entry "named" '><script>alert('XSS')</script><' in MediaWiki:Sidebar (note that the payload is slightly different than the ones in T361448, T361449, T361450 given that Foreground uses single quotes for the element attributes, not double)
  2. Load a page with the Foreground skin

Expected result:
No alert.

Actual result:
The alertruns.

Proposed & tested patch:

diff --git a/includes/ForegroundTemplate.php b/includes/ForegroundTemplate.php
index 429f362..5eac8c6 100644
--- a/includes/ForegroundTemplate.php
+++ b/includes/ForegroundTemplate.php
@@ -106,7 +106,7 @@ class ForegroundTemplate extends BaseTemplate {
                        <ul id="top-bar-left" class="left">
                                <li class="divider show-for-small"></li>
                                <?php foreach ( $this->getSidebar() as $boxName => $box ) { if ( ( $box['header'] != wfMessage( 'toolbox' )->text() ) ) { ?>
-                                       <li class="has-dropdown active"  id='<?php echo Sanitizer::escapeIdForAttribute( $box['id'] ) ?>'<?php echo Linker::tooltip( $box['id'] ) ?>>
+                                       <li class="has-dropdown active"  id='<?php echo htmlspecialchars( Sanitizer::escapeIdForAttribute( $box['id'] ), ENT_QUOTES ) ?>'<?php echo Linker::tooltip( $box['id'] ) ?>>
                                                <a href="#"><?php echo htmlspecialchars( $box['header'] ); ?></a>
                                                <?php if ( is_array( $box['content'] ) ) { ?>
                                                        <ul class="dropdown">

Details

Risk Rating
Medium
Author Affiliation
Wikimedia Communities
Related Changes in Gerrit:

Event Timeline

ashley added a subscriber: Samwilson.

CC'ing @Samwilson since you've recently committed to this repository with significant contributions.

Thanks!

I've made a patch: https://gerrit.wikimedia.org/r/c/mediawiki/skins/Foreground/+/1015658

Anyone with write access to MediaWiki:Sidebar also can do what they want with MediaWiki:Common.js so it looks like this bug is not very easy to exploit.

sbassett subscribed.

Since this skin isn't deployed or bundled, the vulnerability (and hopefully merged patch) will be (re)announced via the next supplemental security release: T361321.

Anyone with write access to MediaWiki:Sidebar also can do what they want with MediaWiki:Common.js so it looks like this bug is not very easy to exploit.

You should need different rights to edit the sidebar then common.js (In WMF config, sysop vs interface-admin)

Bawolff changed the visibility from "Custom Policy" to "Public (No Login Required)".Apr 3 2024, 9:12 PM
Bawolff changed the edit policy from "Custom Policy" to "All Users".
sbassett triaged this task as Medium priority.Apr 3 2024, 9:16 PM
sbassett changed Author Affiliation from N/A to Wikimedia Communities.
sbassett changed Risk Rating from N/A to Medium.

You should need different rights to edit the sidebar then common.js (In WMF config, sysop vs interface-admin)

True, but that's not the case by default.

Also, why is escapeIdForAttribute() "not guaranteed to be HTML safe"? What other ID attribute is it intended for, that needs to be able to contain angle brackets etc.? Is it because some XML dialects permit more characters in IDs than HTML does? It looks like a bunch of skins are doing similar things to Foreground here, so it does seem a confusingly named function.

I guess the better fix here is for Foreground to not construct HTML directly, but use the Html class.

Also, why is escapeIdForAttribute() "not guaranteed to be HTML safe"? What other ID attribute is it intended for, that needs to be able to contain angle brackets etc.? Is it because some XML dialects permit more characters in IDs than HTML does? It looks like a bunch of skins are doing similar things to Foreground here, so it does seem a confusingly named function.

Looking at escapeIdInternal() within Sanitizer.php, it seems like it's purely focused on what the various html specs allow which, sadly, might not perfectly align with security best-practices. For example, it's not doing anything to prevent DOM-clobbering or filter url schemes or anything else that would likely be a security best-practice for html attributes.

I guess the better fix here is for Foreground to not construct HTML directly, but use the Html class.

Yes. Building html manually by constructing large, dynamic strings is always pretty dangerous, even for very experienced developers.

Change #1051779 had a related patch set uploaded (by Mmartorana; author: Samwilson):

[mediawiki/skins/Foreground@REL1_41] Escape id attribute in sidebar headers

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

Change #1051779 merged by jenkins-bot:

[mediawiki/skins/Foreground@REL1_41] Escape id attribute in sidebar headers

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

Also, why is escapeIdForAttribute() "not guaranteed to be HTML safe"? What other ID attribute is it intended for, that needs to be able to contain angle brackets etc.? Is it because some XML dialects permit more characters in IDs than HTML does? It looks like a bunch of skins are doing similar things to Foreground here, so it does seem a confusingly named function.

I think the intention is to remove characters not allowed after the # in a url (or equivalently not allowed to be an id per spec)

Historically this was pretty restrictive. HTML5 is much less strict so it is less of an issue now. The definition in html4 ( https://www.w3.org/TR/html4/types.html#h-6.2 ), XML/XHTML ( https://www.w3.org/TR/xml/#NT-NameStartChar ) and html5 are all different ( https://html.spec.whatwg.org/multipage/dom.html#global-attributes:the-id-attribute-2 )

Samwilson claimed this task.

That makes sense.

I started looking into other skins that might have this same issue, and will try to raise tasks for them (although it looks like some are unmaintained and it's probably not worth it).

mmartorana renamed this task from Foreground skin: stored XSS via MediaWiki:Sidebar to CVE-2024-40605: Foreground skin: stored XSS via MediaWiki:Sidebar.Jul 8 2024, 5:36 PM