Page MenuHomePhabricator

CVE-2021-30154: Unescaped messages used in HTML on Special:NewFiles
Closed, ResolvedPublicSecurity

Description

On Special:NewFiles, all the mediastatistics-header-* messages are output in HTML unescaped.

Steps to reproduce:

  1. Edit one of the mediastatistics-header-*messages (e.g. edit MediaWiki:Mediastatistics-header-drawing) and add a simple XSS string like <img src=x onerror=alert(document.domain)>
  2. Visit Special:NewFiles and see the JavaScript executed

NewFilesXSS.png (533×948 px, 78 KB)

This happens because the form options is using the ->text() output format with options, which is not escaped, rather than options-messages.

It's relatively low risk given it's admin-only, but filing as a private issue similar to T256171 and T255918.

Event Timeline

Proposed patch:

From 65b0d900790a2fc8b96007425ac9aa850e9fd6dd Mon Sep 17 00:00:00 2001
From: grunny <mwgrunny@gmail.com>
Date: Sat, 20 Mar 2021 04:46:44 +1000
Subject: [PATCH] Escape mediastatistics-header-* messages on Special:NewFiles

The mediastatistics-header-* messages used on Special:NewFiles currently
allow raw HTML as raw options labels are output as raw HTML in forms. We
could change these to use options-messages with the message keys, but it
looks like this was done so the list could be alphabetised based on the
labels and this wouldn't be in alphabetical order if we sorted on the
message keys.

Change-Id: I5f59ccf4c167756255952cfbf31a8d7891463e92
---
 includes/specials/SpecialNewFiles.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/includes/specials/SpecialNewFiles.php b/includes/specials/SpecialNewFiles.php
index fc589f1..92798e0 100644
--- a/includes/specials/SpecialNewFiles.php
+++ b/includes/specials/SpecialNewFiles.php
@@ -155,7 +155,7 @@ class SpecialNewFiles extends IncludableSpecialPage {
 			// mediastatistics-header-office, mediastatistics-header-text,
 			// mediastatistics-header-executable, mediastatistics-header-archive,
 			// mediastatistics-header-3d,
-			return $this->msg( 'mediastatistics-header-' . strtolower( $type ) )->text();
+			return $this->msg( 'mediastatistics-header-' . strtolower( $type ) )->escaped();
 		}, $this->mediaTypes );
 		$mediaTypesOptions = array_combine( $mediaTypesText, $this->mediaTypes );
 		ksort( $mediaTypesOptions );
-- 
2.7.4

As the commit message says, we could use options-messages, but it seems this was intended to allow the output list to be alphabetical, which this approach maintains.

sbassett changed Author Affiliation from N/A to Wikimedia Communities.
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
sbassett changed the edit policy from "Custom Policy" to "All Users".
sbassett moved this task from Incoming to In Progress on the Security-Team board.
sbassett subscribed.

Thanks again for the report and patch. Making this public and pushing through gerrit as low-risk.

Change 674083 had a related patch set uploaded (by SBassett; owner: Grunny):
[mediawiki/core@master] Escape mediastatistics-header-* messages on Special:NewFiles

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

Change 674083 merged by jenkins-bot:
[mediawiki/core@master] Escape mediastatistics-header-* messages on Special:NewFiles

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

Change 674107 had a related patch set uploaded (by SBassett; owner: Grunny):
[mediawiki/core@REL1_35] Escape mediastatistics-header-* messages on Special:NewFiles

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

Thanks, @sbassett ! Just for my reference on if I find any more of these in core or WMF deployed extensions, is it OK to push straight to Gerrit, and I assume we'd still want a ticket created for reference?

Change 674107 merged by jenkins-bot:
[mediawiki/core@REL1_35] Escape mediastatistics-header-* messages on Special:NewFiles

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

Thanks, @sbassett ! Just for my reference on if I find any more of these in core or WMF deployed extensions, is it OK to push straight to Gerrit, and I assume we'd still want a ticket created for reference?

I would say likely, yes. However it's probably a good idea to continue filing these as private bugs so the Security-Team can review them (our weekly clinic meeting is Monday morning) just to verify they are indeed low-risk and to also time them well for the weekly train deployment, in getting pushed to gerrit.

Thanks, @sbassett ! Just for my reference on if I find any more of these in core or WMF deployed extensions, is it OK to push straight to Gerrit, and I assume we'd still want a ticket created for reference?

I would say likely, yes. However it's probably a good idea to continue filing these as private bugs so the Security-Team can review them (our weekly clinic meeting is Monday morning) just to verify they are indeed low-risk and to also time them well for the weekly train deployment, in getting pushed to gerrit.

Sounds good, thanks!

Reedy assigned this task to Grunny.

Change 676795 had a related patch set uploaded (by Reedy; author: Grunny):

[mediawiki/core@REL1_31] Escape mediastatistics-header-* messages on Special:NewFiles

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

Change 676795 merged by jenkins-bot:

[mediawiki/core@REL1_31] Escape mediastatistics-header-* messages on Special:NewFiles

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

Reedy renamed this task from Unescaped messages used in HTML on Special:NewFiles to CVE-2021-30154: Unescaped messages used in HTML on Special:NewFiles.Apr 6 2021, 7:11 PM