CVE-2024-40604: Nimbus skin: stored XSS via MediaWiki:Nimbus-sidebar
Closed, ResolvedPublicSecurity
Actions

Assigned To

Authored By

	ashley
	Mar 31 2024, 7:42 PM

Description

Like T361448 and T361449, just for a different skin and using a different source message.

Works for both the "main" (displayed) navigation menu entry as well as for submenu entries, e.g. this code in MediaWiki:Nimbus-sidebar results in two alerts being run:

* "><script>alert('XSS')</script>
** "><script>alert('even more XSS')</script>

Proposed & tested patch (it took me a while to understand this awful code again and figure out the best place for the escaping):

diff --git a/includes/NimbusTemplate.php b/includes/NimbusTemplate.php
index 042f72e..6efab4b 100644
--- a/includes/NimbusTemplate.php
+++ b/includes/NimbusTemplate.php
@@ -429,7 +438,7 @@ class NimbusTemplate extends BaseTemplate {
                // Determine what to show as the human-readable link description
                if ( $this->skin->msg( $line )->isDisabled() ) {
                        // It's *not* the name of a MediaWiki message, so display it as-is
-                       $text = $line;
+                       $text = htmlspecialchars( $line, ENT_QUOTES );
                } else {
                        // Guess what -- it /is/ a MediaWiki message!
                        $text = $this->skin->msg( $line )->escaped();
@@ -437,7 +446,7 @@ class NimbusTemplate extends BaseTemplate {
 
                if ( $link != null ) {
                        if ( $this->skin->msg( $line_temp[0] )->isDisabled() ) {
-                               $link = $line_temp[0];
+                               $link = htmlspecialchars( $line_temp[0], ENT_QUOTES );
                        }
                        if ( preg_match( '/^(?:' . wfUrlProtocols() . ')/', $link ) ) {
                                $href = $link;

(Line numbers in the patch might be a bit off due to an unrelated chunk of unsubmitted code not being included here.)

Details

Subject	Repo	Branch	Lines +/-
[SECURITY] Avoid stored XSS via MediaWiki:Nimbus-sidebar	mediawiki/skins/Nimbus	REL1_42	+6 -2
[SECURITY] Avoid stored XSS via MediaWiki:Nimbus-sidebar	mediawiki/skins/Nimbus	REL1_41	+6 -2
[SECURITY] Avoid stored XSS via MediaWiki:Nimbus-sidebar	mediawiki/skins/Nimbus	REL1_40	+6 -2
[SECURITY] Avoid stored XSS via MediaWiki:Nimbus-sidebar	mediawiki/skins/Nimbus	master	+6 -2

Customize query in gerrit

Related Objects

Mentioned In: T361321: Write and send supplementary release announcement for extensions and skins with security patches (1.39.8/1.40.4/1.41.2/1.42.0)
T361452: CVE-2024-40605: Foreground skin: stored XSS via MediaWiki:Sidebar
T361451: CVE-2024-40602: Tempo skin: stored XSS via MediaWiki:Sidebar
Mentioned Here: T361452: CVE-2024-40605: Foreground skin: stored XSS via MediaWiki:Sidebar
T361321: Write and send supplementary release announcement for extensions and skins with security patches (1.39.8/1.40.4/1.41.2/1.42.0)
T361448: CVE-2024-40599: GuMaxDD skin: stored XSS via MediaWiki:Sidebar
T361449: CVE-2024-40600: Metrolook skin: stored XSS via MediaWiki:Sidebar

Event Timeline

ashley created this task.Mar 31 2024, 7:42 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 31 2024, 7:42 PM

ashley claimed this task.Mar 31 2024, 7:42 PM

ashley added projects: Vuln-XSS, Nimbus.

ashley mentioned this in T361451: CVE-2024-40602: Tempo skin: stored XSS via MediaWiki:Sidebar.Mar 31 2024, 8:08 PM

ashley mentioned this in T361452: CVE-2024-40605: Foreground skin: stored XSS via MediaWiki:Sidebar.Mar 31 2024, 9:25 PM

sbassett mentioned this in T361321: Write and send supplementary release announcement for extensions and skins with security patches (1.39.8/1.40.4/1.41.2/1.42.0).Apr 2 2024, 5:56 PM

Since this skin isn't deployed or bundled, the proposed patch can go through gerrit at any time. It will be (re)announced via the next supplemental security release: T361321.

sbassett edited projects, added SecTeam-Processed; removed Security-Team.Apr 2 2024, 5:59 PM

sbassett added a project: security-bug.Apr 2 2024, 7:00 PM

Maybe instead of htmlspecialchars the $link, it would be better to htmlspecialchars the href in printMenu() ? Since other things that make links might need to be escaped too (e.g. I see some stuff with QuizGame that is generating urls based on $wgQuizID)

In T361450#9686148, @Bawolff wrote:

Maybe instead of htmlspecialchars the $link, it would be better to htmlspecialchars the href in printMenu() ? Since other things that make links might need to be escaped too (e.g. I see some stuff with QuizGame that is generating urls based on $wgQuizID)

Good point. I looked into this and some of the things in there definitely made me feel a bit icky (not the $wgQuizID/$wgPictureGameID stuff tho, that should be decently safe and those should be settable only by QuizGame and PictureGame, respectively). At a quick glance I think those are content actions bits which get escaped around the super duper fugly code appropriately marked with the comment @todo FIXME: this code deserves to burn in hell.

That said...in printMenu, escaping the href doesn't seem to fix anything (?!) but escaping text fixes both cases, like so:

diff --git a/includes/NimbusTemplate.php b/includes/NimbusTemplate.php
index 042f72e..80cf141 100644
--- a/includes/NimbusTemplate.php
+++ b/includes/NimbusTemplate.php
@@ -509,7 +518,7 @@ class NimbusTemplate extends BaseTemplate {
                                        ( $level ? $last_count . '_' : '_' ) . $count . '" href="' .
                                        ( !empty( $this->navmenu[$child]['href'] ) ? $this->navmenu[$child]['href'] : '#' ) . '">';
 
-                               $menu_output .= $this->navmenu[$child]['text'];
+                               $menu_output .= htmlspecialchars( $this->navmenu[$child]['text'], ENT_QUOTES );
                                // If a menu item has submenus, show an arrow so that the user
                                // knows that there are submenus available
                                if (

All I honestly know at this point is that Nimbus uses one of the most awful implementations of the nested/"V3-style" menu parsing code, also used by Monaco and a few other skins (Mask, MediaWiki-skins-WebPlatform at least); somehow the others manage to not go full raw HTML whereas Nimbus is like "let's just stitch together a bunch of HTML on our own, what could possibly go wrong?".

Should skin:Nimbus just be retired/archived then? It's not used by Wikimedia AFAIK though I'm not sure how widely-used it is. And WikiApiary is down again.

In T361450#9689361, @sbassett wrote:

Should skin:Nimbus just be retired/archived then? It's not used by Wikimedia AFAIK though I'm not sure how widely-used it is. And WikiApiary is down again.

No.

Frankly, I'm not sure what is it with the eagerness to archive usable, maintained things. This is slightly offtopic, but since we're at here and I presume the task'll be made public once the fix is in gerrit & merged...

To me, the WMF infra like svn.wikimedia.org and its successors (gerrit.wikimedia.org, gitlab.wikimedia.org, etc.) were "official MediaWiki development resources hosted by the WMF", as opposed to "WMF code repositories that might be usable by third parties". Back in the day I championed extensively to get third-parties to open up their source code repositories and to start collaborating with upstream. In retrospect I was probably too naive and eager, but nevertheless, I'd argue that some great things did come out of those efforts, even if sites/organizations like Wikia or wikiHow never directly contributed to upstream MediaWiki. The Social-Tools family of extensions, which I maintain (together with a few other things), for example, technically speaking originated off-Wikia, but Wikia bought the technology, used them for a while and essentially dropped support for them; I picked them up again and fixed them to be compatible with a standard, supported vanilla (LTS) MediaWiki.

Now, though, it seems that there's a massive eagerness to "archive" things, especially if said things were once used by WMF sites but no longer are, e.g. ArticleFeedbackv5 (for better or for worse, I also maintain that as it's used on Brickimedia, a ShoutWiki site). I cannot in good faith agree with this. Not only is it very rude and also alienating, it harms the wider MediaWiki community in the long run by reducing the amount of features and customization options available to them. MediaWiki was supposed to be "a free and open source wiki engine written in PHP and used by Wikipedia and other Wikimedia Foundation web properties", not just "that Wikipedia engine that you might be able to use elsewhere, or then not". There's a huge difference between those two, at least in the way of thinking: whose contributions are valued, and how non-WMF employees and their code are viewed, as valued equals or as some customer-adjacent annoying pests that have to be sadly dealt with on a daily basis.

Back on topic...

As noted in T361452, this issue ("the output of Sanitizer::escapeIdForAttribute() isn't properly escaped in <some skin>) isn't the most exploitable in the world, and as you can see based on the related tickets, many of which I self-assigned in what, to me, was an obvious signaling of "I got this", this isn't that hard to fix either. It's just a matter of wrapping one or a couple lines in htmlspecialchars. It does not require significant refactoring of backend code or anything.

That said, I'm not even going to pretend that Nimbus' code is the greatest in the world, because it's far from that. But it's usable and, hopefully, after this patch, also secure enough to be used on public-facing websites out there. (If you want pretty and amazingly resistent code, look at some of Isarra's skins, like Anisa, GreyStuff, HasSomeColours, Mask, Splash, Timeless, WoOgLeShades ...I'm always amazed at how robust her work is compared to mine, and I've not yet managed to break 'em, no matter how hard I try. :)

In T361450#9695720, @ashley wrote:

Frankly, I'm not sure what is it with the eagerness to archive usable, maintained things. This is slightly offtopic, but since we're at here and I presume the task'll be made public once the fix is in gerrit & merged...

I think there's a reasonable debate to be had around overall usage of a given component and what, exactly, are "usable, maintained things", but the big reason for me and many others to ask this question is Technical-Debt. We have an absolutely outrageous amount of it at the Foundation and within the Community and it causes a multitude of problems. The Foundation, for example, currently has a dedicated working group focusing on code ownership and maintenance since it has become such an enormous problem. So if the opportunity to retire or archive a component is at all apparent, it's entirely fair to ask if it should be archived or not. If the answer is no, great. If it's not, then we should start a software component deprecation process to reduce maintenance efforts and reduce various attack surfaces.

In T361450#9686408, @ashley wrote:
In T361450#9686148, @Bawolff wrote:

Maybe instead of htmlspecialchars the $link, it would be better to htmlspecialchars the href in printMenu() ? Since other things that make links might need to be escaped too (e.g. I see some stuff with QuizGame that is generating urls based on $wgQuizID)

Good point. I looked into this and some of the things in there definitely made me feel a bit icky (not the $wgQuizID/$wgPictureGameID stuff tho, that should be decently safe and those should be settable only by QuizGame and PictureGame, respectively). At a quick glance I think those are content actions bits which get escaped around the super duper fugly code appropriately marked with the comment @todo FIXME: this code deserves to burn in hell.

That said...in printMenu, escaping the href doesn't seem to fix anything (?!) but escaping text fixes both cases, like so:
diff --git a/includes/NimbusTemplate.php b/includes/NimbusTemplate.php
index 042f72e..80cf141 100644
--- a/includes/NimbusTemplate.php
+++ b/includes/NimbusTemplate.php
@@ -509,7 +518,7 @@ class NimbusTemplate extends BaseTemplate {
                                        ( $level ? $last_count . '_' : '_' ) . $count . '" href="' .
                                        ( !empty( $this->navmenu[$child]['href'] ) ? $this->navmenu[$child]['href'] : '#' ) . '">';
 
-                               $menu_output .= $this->navmenu[$child]['text'];
+                               $menu_output .= htmlspecialchars( $this->navmenu[$child]['text'], ENT_QUOTES );
                                // If a menu item has submenus, show an arrow so that the user
                                // knows that there are submenus available
                                if (
All I honestly know at this point is that Nimbus uses one of the most awful implementations of the nested/"V3-style" menu parsing code, also used by Monaco and a few other skins (Mask, MediaWiki-skins-WebPlatform at least); somehow the others manage to not go full raw HTML whereas Nimbus is like "let's just stitch together a bunch of HTML on our own, what could possibly go wrong?".

Probably both href and text should be escaped. Exploiting href is hard since there is a lot of overlap between url escaping and html escaping, so the href part might not actually be exploitable, but its probably a good idea just in case as different url escaping libraries can differ. The main risk is with the fragment part of the url, and it might be hard for an attacker to control the fragment of any of these titles. However its better to just escape instead of trying to figure out whether or not the user can set a fragment.

In T361450#9705727, @Bawolff wrote:

Probably both href and text should be escaped. Exploiting href is hard since there is a lot of overlap between url escaping and html escaping, so the href part might not actually be exploitable, but its probably a good idea just in case as different url escaping libraries can differ. The main risk is with the fragment part of the url, and it might be hard for an attacker to control the fragment of any of these titles. However its better to just escape instead of trying to figure out whether or not the user can set a fragment.

Hm, good point!

diff --git a/includes/NimbusTemplate.php b/includes/NimbusTemplate.php
index 042f72e..8f63910 100644
--- a/includes/NimbusTemplate.php
+++ b/includes/NimbusTemplate.php
@@ -507,9 +516,13 @@ class NimbusTemplate extends BaseTemplate {
                                                ( $level ? $last_count . '_' : '_' ) . $count . '">';
                                $menu_output .= "\n\t\t\t\t\t" . '<a id="' . ( $level ? 'a-sub-' : 'a-' ) . 'menu-item' .
                                        ( $level ? $last_count . '_' : '_' ) . $count . '" href="' .
-                                       ( !empty( $this->navmenu[$child]['href'] ) ? $this->navmenu[$child]['href'] : '#' ) . '">';
+                                       // Escaping this is just additional paranoia, see T361450
+                                       ( !empty( $this->navmenu[$child]['href'] ) ? htmlspecialchars( $this->navmenu[$child]['href'], ENT_QUOTES ) : '#' ) . '">';
+
+                               // Escaping this is very much necessary and not doing that results in
+                               // somewhat trivially exploitable XSS; see T361450
+                               $menu_output .= htmlspecialchars( $this->navmenu[$child]['text'], ENT_QUOTES );
 
-                               $menu_output .= $this->navmenu[$child]['text'];
                                // If a menu item has submenus, show an arrow so that the user
                                // knows that there are submenus available
                                if (

lgtm

The patch got merged a while ago (but I do need to look into the phan-reported double escaping issues, but that's out of scope for this patch; the security problem is fixed).

Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".May 20 2024, 11:33 AM

Reedy changed the edit policy from "Custom Policy" to "All Users".

Change #1051773 had a related patch set uploaded (by Mmartorana; author: Jack Phoenix):

[mediawiki/skins/Nimbus@REL1_42] [SECURITY] Avoid stored XSS via MediaWiki:Nimbus-sidebar

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

Change #1051774 had a related patch set uploaded (by Mmartorana; author: Jack Phoenix):

[mediawiki/skins/Nimbus@REL1_41] [SECURITY] Avoid stored XSS via MediaWiki:Nimbus-sidebar

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

Change #1051775 had a related patch set uploaded (by Mmartorana; author: Jack Phoenix):

[mediawiki/skins/Nimbus@REL1_40] [SECURITY] Avoid stored XSS via MediaWiki:Nimbus-sidebar

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

Change #1051775 abandoned by Umherirrender:

[mediawiki/skins/Nimbus@REL1_40] [SECURITY] Avoid stored XSS via MediaWiki:Nimbus-sidebar

Reason:

REL1_40 is end of life

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

Change #1051774 merged by jenkins-bot:

[mediawiki/skins/Nimbus@REL1_41] [SECURITY] Avoid stored XSS via MediaWiki:Nimbus-sidebar

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

Change #1051773 merged by jenkins-bot: