Page MenuHomePhabricator

mw.message.parse() accepts javascript: protocol in wikilinks (CVE-2020-25814)
Closed, ResolvedPublic

Description

Not sure if this is a bug or intentional, but it is fairly counterintuitive, given that parse() sanitizes scripts in general.

Steps to reproduce:

  • create a message with [javascript:alert(document.cookie) xss]
  • turn it into a jQuery object with mw.message().parse()

Expected result: the jQuery object does not contain an <a> tag (or it does not have a href attribute, or it's empty etc)

Actual result: the object contains an <a href ="javascript... which executes when clicked.

Noticed by @m4tx.

Event Timeline

Tgr raised the priority of this task from to Needs Triage.
Tgr updated the task description. (Show Details)
Tgr changed Security from None to Software security bug.
Tgr subscribed.
Restricted Application changed the visibility from "Public (No Login Required)" to "Custom Policy". · View Herald TranscriptJan 14 2015, 6:46 AM
Restricted Application changed the edit policy from "All Users" to "Custom Policy". · View Herald Transcript
Restricted Application added a project: acl*security. · View Herald Transcript
dpatrick added a project: Vuln-XSS.
dpatrick added a project: Security-Team.

Lowering priority to normal, since not directly exploitable by itself.


It also doesn't verify the style tag is not evil

Here's a patch. However, I don't normally do much js stuff, and I don't have a super good grasp on how jqueryMsg actually works, so it should be reviewed somewhat carefully. Also I couldn't figure out how to get qunit tests to run locally.

matmarex lowered the priority of this task from High to Medium.Oct 27 2015, 2:29 PM
matmarex changed the visibility from "Custom Policy" to "Custom Policy".
matmarex changed the edit policy from "Custom Policy" to "Custom Policy".
matmarex added a subscriber: m4tx.

That patch should work (although JSHint will whine about double quotes, should use single for all strings in JS for consistency), but I think it would be more elegant to solve it in the parsing code rather than in the emitting code.

			function extlink() {
				var result, parsedResult, target;
				result = null;
				parsedResult = sequence( [
					openExtlink,
					nOrMore( 1, nonWhitespaceExpression ),
					whitespace,
					nOrMore( 1, expression ),
					closeExtlink
				] );

The nOrMore( 1, nonWhitespaceExpression ) should be replaced with something that only matches the valid protocols, or the parameter syntax ($1, $2 etc.).

Note that this would still allow javascript: URLs to be passed as parameters (mw.messages.set( { foo: '[$1 Click me]' } ); $( 'body' ).append( mw.message( 'foo', 'javascript:alert()' ).parse() )). I'm not sure if that's something we should defend against, or a feature of the library (we allow passing JS functions as parameters, after all…).

Note that this would still allow javascript: URLs to be passed as parameters (mw.messages.set( { foo: '[$1 Click me]' } ); $( 'body' ).append( mw.message( 'foo', 'javascript:alert()' ).parse() )). I'm not sure if that's something we should defend against, or a feature of the library (we allow passing JS functions as parameters, after all…).

Passing a function or jQuery object to a parameter seems like something that you won't do accidentally. On the other hand, passing a text url as a parameter that's under the control of the user seems like it could happen, and it is extremely non-obvious that this is dangerous. Especially because this is totally safe in the php version of the message object.

For example, mwe-upwiz-api-warning-exists is rather close to doing something like that (Its not quite, since the url isn't entirely user controlled, as it is prefixed with $wgServer). But if we can get that close to inserting a user controlled url in existing code, I feel like eventually someone is going to accidentally go all the way, and allow a totally user-controlled url to be inserted.

Fair point.


Updated patch:

  • Rebased to master (some recent changes conflicted)
  • Tweaked code style to pass all our assorted linters (hopefully)
  • Added a test for passing 'javascript:' URL as message parameter
  • Changed to remove <span> tag from output and preserve text formatting when URL is rejected


Two more thoughts:

  • The patch above disallows relative URLs too, which is likely to break a lot of users. We can't allow any, since it's difficult to tell whether a string is a relative URL or fully-qualified one with funky protocol, but it should be okay to allow everything starting with /? MediaWiki's functions generally produce URLs in that form.
  • Could we just disallow the 'style' attribute altogether? Unlike the Parser, this code is not required to parse everything, only the parts of wikitext that commonly appear in localisation messages.

Since we happened to notice the same issue in the Wikidata-Bridge team, here’s a rebased patch:

Rebased patch with SECURITY message:

sbassett subscribed.

Deployed:

21:14 sbassett@deploy1001: Synchronized php-1.36.0-wmf.2/resources/src/mediawiki.jqueryMsg/mediawiki.jqueryMsg.js: (no justification provided) (duration: 01m 00s)

Forgot --no-log-message, though the default message that was generated should be vague enough. Let's hold this, keep the task protected and wait on the backports for the next security release (T256335).

The patch for this bug unexpectedly broke parsing of relative URLs in wiki links:

mw.message('testmsg').params( [ 'https://example.org' ] ).parse()
"Hello <a href="https://example.org">world</a>"
mw.message('testmsg').params( [ '//example.org' ] ).parse()
"Hello <a href="//example.org">world</a>"
mw.message('testmsg').params( [ '/wiki/Foo' ] ).parse()
"Hello [/wiki/Foo world]"

The latter works in master (where this patch is not applied) but breaks in wmf.2, causing T259575: [regression -wmf.2] Homepage - SE filter "Create a new article" description displays url-encoded text not a link

  • The patch above disallows relative URLs too, which is likely to break a lot of users. We can't allow any, since it's difficult to tell whether a string is a relative URL or fully-qualified one with funky protocol, but it should be okay to allow everything starting with /? MediaWiki's functions generally produce URLs in that form.

The most common pattern this breaks is mw.message( 'foo' ).params( [ mw.util.getUrl( 'Help:Bar' ) ] ). I like your suggestion of allowing URLs that start with /.

I certainly wouldn't mind deprecating the [/wiki/.... Foo] syntax, if we can provide a good alternative to the getUrl pattern (maybe a helper function to emit a protocol-relative URL), since the "real" wikitext syntax doesn't allow relative URLs here. Things get a bit confusing when the 'fake wikitext' parsed by the message parser diverges too much from 'real wikitext', as evidenced by the confusion in T259565. It would be nicer if the message parser accepted a strict subset of 'real wikitext'.

(And of course the "real wikitext" parser has robust sanitization.)

Given the amount of breakage this has surfaced I think we either need a revert or a fixed patch rather soon. And then publicly we could discuss deprecating the problematic relative syntax (or at least give people some advance notice that it will break).

Modified version of the patch that allows URLs that start with a slash (/), and also fixes eslint errors in the qunit tests.

Diff relative to the previous patch:

diff --git a/resources/src/mediawiki.jqueryMsg/mediawiki.jqueryMsg.js b/resources/src/mediawiki.jqueryMsg/mediawiki.jqueryMsg.js
index 26a1d93b9d..0d51e83c08 100644
--- a/resources/src/mediawiki.jqueryMsg/mediawiki.jqueryMsg.js
+++ b/resources/src/mediawiki.jqueryMsg/mediawiki.jqueryMsg.js
@@ -1256,7 +1256,7 @@ mw.jqueryMsg.HtmlEmitter.prototype = {
                                } );
                        } else {
                                target = textify( arg );
-                               if ( target.search( new RegExp( '^(' + mw.config.get( 'wgUrlProtocols' ) + ')' ) ) !== -1 ) {
+                               if ( target.search( new RegExp( '^(/|' + mw.config.get( 'wgUrlProtocols' ) + ')' ) ) !== -1 ) {
                                        $el.attr( 'href', target );
                                } else {
                                        mw.log( 'External link in message had illegal target ' + target );
diff --git a/tests/qunit/suites/resources/mediawiki/mediawiki.jqueryMsg.test.js b/tests/qunit/suites/resources/mediawiki/mediawiki.jqueryMsg.test.js
index 51e32b5bc9..bc49b67dae 100644
--- a/tests/qunit/suites/resources/mediawiki/mediawiki.jqueryMsg.test.js
+++ b/tests/qunit/suites/resources/mediawiki/mediawiki.jqueryMsg.test.js
@@ -1179,16 +1179,15 @@
 
                this.suppressWarnings();
 
-               assert.equal(
+               assert.strictEqual(
                        formatParse( 'illegal-url' ),
                        '[javascript:alert(1) foo]',
                        'illegal-url: \'parse\' format'
                );
 
-               assert.equal(
-                       /* jshint scripturl: true */
+               assert.strictEqual(
+                       // eslint-disable-next-line no-script-url
                        formatParse( 'illegal-url-param', 'javascript:alert(1)' ),
-                       /* jshint scripturl: false */
                        '[javascript:alert(1) foo]',
                        'illegal-url-param: \'parse\' format'
                );
@@ -1199,7 +1198,7 @@
 
                this.suppressWarnings();
 
-               assert.equal(
+               assert.strictEqual(
                        formatParse( 'illegal-style' ),
                        '&lt;span style="background-image:url( http://example.com )"&gt;bar&lt;/span&gt;',
                        'illegal-style: \'parse\' format'

Modified version of the patch that allows URLs that start with a slash (/), and also fixes eslint errors in the qunit tests.

Code-Review+1

It would be good to have a test case that ensures the [/wiki/ ...] syntax works as well.

Modified version of the patch that adds tests for /wiki/ and //example.com (protocol-relative URLs weren’t explicitly mentioned as a requirement, but since the current regex allows them, I thought it’s best to test them too so that when if break them in future, it’s intentional, not accidental). Also added @Catrope and myself as Co-Authored-By.

Diff relative to the previous patch:

diff --git a/tests/qunit/suites/resources/mediawiki/mediawiki.jqueryMsg.test.js b/tests/qunit/suites/resources/mediawiki/mediawiki.jqueryMsg.test.js
index bc49b67dae..7f214211ab 100644
--- a/tests/qunit/suites/resources/mediawiki/mediawiki.jqueryMsg.test.js
+++ b/tests/qunit/suites/resources/mediawiki/mediawiki.jqueryMsg.test.js
@@ -620,6 +620,16 @@
             'Foo <a href="http://example.com">bar</a>',
             'External link message processed when format is \'parse\''
         );
+        assert.htmlEqual(
+            formatParse( 'external-link-replace', '/wiki/Special:Version' ),
+            'Foo <a href="/wiki/Special:Version">bar</a>',
+            'External link message allows relative URL when processed'
+        );
+        assert.htmlEqual(
+            formatParse( 'external-link-replace', '//example.com' ),
+            'Foo <a href="//example.com">bar</a>',
+            'External link message allows protocol-relative URL when processed'
+        );
         assert.htmlEqual(
             formatParse( 'external-link-replace', $( '<i>' ) ),
             'Foo <i>bar</i>',

Code-Review+1 for the non-test parts from me as well.

I deployed that security patch:

Mentioned in SAL (#wikimedia-operations) [2020-08-04T11:12:39Z] <Lucas_WMDE> Deployed patch for T86738 / T259565

T259565 seems to be fixed, and [javascript:alert(document.cookie) xss] still doesn’t parse as a JavaScript link. I also committed the updated patch under /srv/patches.

The issue from T259565#6358811 / T259565#6358812 still seems to exist.

Wiki文本<a title="mw:Special:MyLanguage/Help:Formatting" href="/wiki/mw:Special:MyLanguage/Help:Formatting" target="_blank">使用标记语法</a>,当然您也可以随时<a href="#" class="flow-ui-editorWidget-label-preview">预览结果</a>。

It might be due to the <a href="#", maybe that’s [# something] in wikitext? (But then I’m not sure where the class="flow-ui-editorWidget-label-preview"> would come from.)

I deployed that security patch:

Mentioned in SAL (#wikimedia-operations) [2020-08-04T11:12:39Z] <Lucas_WMDE> Deployed patch for T86738 / T259565

T259565 seems to be fixed, and [javascript:alert(document.cookie) xss] still doesn’t parse as a JavaScript link. I also committed the updated patch under /srv/patches.

Thanks all for cleaning this up.

The issue from T259565#6358811 / T259565#6358812 still seems to exist.

Wiki文本<a title="mw:Special:MyLanguage/Help:Formatting" href="/wiki/mw:Special:MyLanguage/Help:Formatting" target="_blank">使用标记语法</a>,当然您也可以随时<a href="#" class="flow-ui-editorWidget-label-preview">预览结果</a>。

It might be due to the <a href="#", maybe that’s [# something] in wikitext? (But then I’m not sure where the class="flow-ui-editorWidget-label-preview"> would come from.)

This turns out to be due to another change, see T115888#6359981.

@Legoktm added @ssastry and me (@cscott) to this task from Parsing-Team--ARCHIVED and I guess there is some incidental parsing-related issue here, although more in the realm of "wikitext is so complex there are dozens of different parsers for it, all with their own incompatibilities (and in some cases sanitization and security issues), including at least four within WMF production alone". I don't have access to T115888 so I don't know exactly what the "other" issue is, since folks on the public side in T259565 are still reporting problems. Tag someone from parsing team over in T115888 if you need us, otherwise we'll check out of this and assume you folks have it under control.

I don't have access

(hope no one minds that I have fixed this)

I don't have access

(hope no one minds that I have fixed this)

Completely fine. @cscott - if you want general Security access in Phab, just let us know.

REL1_31 patch (will upload a file later)... Mostly reworked due to a file rename

From 334a6b807d388c8b1ad3f313038a59db356601e3 Mon Sep 17 00:00:00 2001
From: Brian Wolff <bawolff+wn@gmail.com>
Date: Tue, 27 Oct 2015 01:39:34 -0600
Subject: [PATCH] SECURITY: mediawiki.jqueryMsg: Sanitize URLs and 'style'
 attribute

Previously you could leverage the style attribute, and external
links to execute javascript.

Bug: T86738
Change-Id: I6f15ece1db136369e06dfeee34d1a0c5bc03e32b
Co-Authored-By: Roan Kattouw <roan.kattouw@gmail.com>
Co-Authored-By: Lucas Werkmeister <lucas.werkmeister@wikimedia.de>
---
 .../src/mediawiki/mediawiki.jqueryMsg.js      | 22 ++++++++--
 .../mediawiki/mediawiki.jqueryMsg.test.js     | 42 +++++++++++++++++++
 2 files changed, 61 insertions(+), 3 deletions(-)

diff --git a/resources/src/mediawiki/mediawiki.jqueryMsg.js b/resources/src/mediawiki/mediawiki.jqueryMsg.js
index 67d6e2c5f5..93623d8c27 100644
--- a/resources/src/mediawiki/mediawiki.jqueryMsg.js
+++ b/resources/src/mediawiki/mediawiki.jqueryMsg.js
@@ -695,7 +695,7 @@
 			 * @return {boolean} true if this is HTML is allowed, false otherwise
 			 */
 			function isAllowedHtml( startTagName, endTagName, attributes ) {
-				var i, len, attributeName;
+				var i, len, attributeName, badStyle;
 
 				startTagName = startTagName.toLowerCase();
 				endTagName = endTagName.toLowerCase();
@@ -703,12 +703,18 @@
 					return false;
 				}
 
+				badStyle = /[\000-\010\013\016-\037\177]|expression|filter\s*:|accelerator\s*:|-o-link\s*:|-o-link-source\s*:|-o-replace\s*:|url\s*\(|image\s*\(|image-set\s*\(/i;
+
 				for ( i = 0, len = attributes.length; i < len; i += 2 ) {
 					attributeName = attributes[ i ];
 					if ( settings.allowedHtmlCommonAttributes.indexOf( attributeName ) === -1 &&
 						( settings.allowedHtmlAttributesByElement[ startTagName ] || [] ).indexOf( attributeName ) === -1 ) {
 						return false;
 					}
+					if ( attributeName === 'style' && attributes[ i + 1 ].search( badStyle ) !== -1 ) {
+						mw.log( 'HTML tag not parsed due to dangerous style attribute' );
+						return false;
+					}
 				}
 
 				return true;
@@ -1138,7 +1144,8 @@
 		extlink: function ( nodes ) {
 			var $el,
 				arg = nodes[ 0 ],
-				contents = nodes[ 1 ];
+				contents = nodes[ 1 ],
+				target;
 			if ( arg instanceof jQuery && !arg.hasClass( 'mediaWiki_htmlEmitter' ) ) {
 				$el = arg;
 			} else {
@@ -1156,7 +1163,16 @@
 						}
 					} );
 				} else {
-					$el.attr( 'href', textify( arg ) );
+				target = textify( arg );
+					if ( target.search( new RegExp( '^(/|' + mw.config.get( 'wgUrlProtocols' ) + ')' ) ) !== -1 ) {
+						$el.attr( 'href', target );
+					} else {
+						mw.log( 'External link in message had illegal target ' + target );
+						return appendWithoutParsing(
+							$( '<span>' ),
+							[ '[' + target + ' ' ].concat( contents ).concat( ']' )
+						).contents();
+					}
 				}
 			}
 			return appendWithoutParsing( $el.empty(), contents );
diff --git a/tests/qunit/suites/resources/mediawiki/mediawiki.jqueryMsg.test.js b/tests/qunit/suites/resources/mediawiki/mediawiki.jqueryMsg.test.js
index 0653dfd3d0..7e3348326a 100644
--- a/tests/qunit/suites/resources/mediawiki/mediawiki.jqueryMsg.test.js
+++ b/tests/qunit/suites/resources/mediawiki/mediawiki.jqueryMsg.test.js
@@ -657,6 +657,16 @@
 			'Foo <a href="http://example.com">bar</a>',
 			'External link message processed when format is \'parse\''
 		);
+		assert.htmlEqual(
+			formatParse( 'external-link-replace', '/wiki/Special:Version' ),
+			'Foo <a href="/wiki/Special:Version">bar</a>',
+			'External link message allows relative URL when processed'
+		);
+		assert.htmlEqual(
+			formatParse( 'external-link-replace', '//example.com' ),
+			'Foo <a href="//example.com">bar</a>',
+			'External link message allows protocol-relative URL when processed'
+		);
 		assert.htmlEqual(
 			formatParse( 'external-link-replace', $( '<i>' ) ),
 			'Foo <i>bar</i>',
@@ -1153,6 +1163,38 @@
 		assert.equal( logSpy.callCount, 2, 'mw.log.warn calls' );
 	} );
 
+	QUnit.test( 'Do not allow javascript: urls', function ( assert ) {
+		mw.messages.set( 'illegal-url', '[javascript:alert(1) foo]' );
+		mw.messages.set( 'illegal-url-param', '[$1 foo]' );
+
+		this.suppressWarnings();
+
+		assert.strictEqual(
+			formatParse( 'illegal-url' ),
+			'[javascript:alert(1) foo]',
+			'illegal-url: \'parse\' format'
+		);
+
+		assert.strictEqual(
+			// eslint-disable-next-line no-script-url
+			formatParse( 'illegal-url-param', 'javascript:alert(1)' ),
+			'[javascript:alert(1) foo]',
+			'illegal-url-param: \'parse\' format'
+		);
+	} );
+
+	QUnit.test( 'Do not allow arbitrary style', function ( assert ) {
+		mw.messages.set( 'illegal-style', '<span style="background-image:url( http://example.com )">bar</span>' );
+
+		this.suppressWarnings();
+
+		assert.strictEqual(
+			formatParse( 'illegal-style' ),
+			'&lt;span style="background-image:url( http://example.com )"&gt;bar&lt;/span&gt;',
+			'illegal-style: \'parse\' format'
+		);
+	} );
+
 	QUnit.test( 'Integration', function ( assert ) {
 		var expected, logSpy, msg;
 
-- 
2.25.1

Resolving for ease of tracking in parent

Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".Sep 24 2020, 11:15 PM
Reedy changed the edit policy from "Custom Policy" to "All Users".
Legoktm renamed this task from mw.message.parse() accepts javascript: protocol in wikilinks to mw.message.parse() accepts javascript: protocol in wikilinks (CVE-2020-25814).Sep 27 2020, 11:50 AM