Page MenuHomePhabricator

Non-jqueryMsg version of mw.message(…).parse() doesn't escape HTML
Closed, ResolvedPublic

Description

The non-jqueryMsg version of mw.message( … ).parse() doesn't escape HTML. This affects both message contents (which are generally safe) and the parameters (which can be based on user input). (When jqueryMsg *is* loaded, it correctly accepts only whitelisted tags in message contents, and escapes all parameters.)

There are two cases when it's used:

  • When the mediawiki.jqueryMsg ResourceLoader module is not loaded. This is unlikely to be an issue in practice because:
    • Tons of things load jqueryMsg these days, it's probably included on almost all pages on any serious wiki. I managed to reproduce the issue on Special:SpecialPages on a wiki with no extensions installed.
    • Calling .parse() only makes sense if you have loaded jqueryMsg, so unless other modules have missing dependencies, this never happens.
  • When jqueryMsg thinks the message is "simple" and falls back to the basic version to avoid expensive parsing. This is done for messages that do not contain {{, [, <, >, &.
    • Since that check ensures that these messages don't contain HTML themselves, this leaves the problem of unescaped parameters.
// This message contains '<', so it will always go through jqueryMsg if it's loaded
mw.messages.set( 'foo1', '<img onerror="alert()" src="foo">' );
$( 'body' ).append( mw.message( 'foo1' ).parse() );
// This message also contains '<', so it will always go through jqueryMsg if it's loaded
mw.messages.set( 'foo2', '<b>$1</b>' );
$( 'body' ).append( mw.message( 'foo2', '<img onerror="alert()" src="foo">' ).parse() );
// This message will never go through jqueryMsg
mw.messages.set( 'foo3', '$1' );
$( 'body' ).append( mw.message( 'foo3', '<img onerror="alert()" src="foo">' ).parse() );

The non-jqueryMsg implementation of .parse() should probably escape the output. Basically, it should be equivalent to .escaped() and not .text(). (Take care not to accidentally double-escape in the jqueryMsg case.)

Event Timeline

matmarex created this task.Oct 19 2015, 4:55 PM
matmarex raised the priority of this task from to Needs Triage.
matmarex updated the task description. (Show Details)
matmarex changed the visibility from "Public (No Login Required)" to "Custom Policy".
matmarex changed the edit policy from "All Users" to "Custom Policy".
matmarex changed Security from None to Software security bug.
matmarex added a subscriber: matmarex.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 19 2015, 4:55 PM

I noticed the issue when working on mediawiki.Upload.BookletLayout, which in some cases echoes API output back to the user, passing it as a parameter to a "simple" message:

			message = mw.message( 'api-error-' + error.code );
			if ( !message.exists() ) {
				message = mw.message( 'api-error-unknownerror', JSON.stringify( stateDetails ) );
			}
			return new OO.ui.Error(
				$( '<p>' ).html(
					message.parse()
				),
				{ recoverable: false }
			);

That particular instance will be accidentally fixed by https://gerrit.wikimedia.org/r/#/c/246241/ (which I submitted for review before noticing this bug).

csteipp triaged this task as Medium priority.Oct 20 2015, 9:44 PM
csteipp added a subscriber: csteipp.

The non-jqueryMsg implementation of .parse() should probably escape the output. Basically, it should be equivalent to .escaped() and not .text(). (Take care not to accidentally double-escape in the jqueryMsg case.)

That sounds like the correct behavior to me

matmarex added a project: Patch-For-Review.
matmarex removed matmarex as the assignee of this task.Dec 21 2015, 6:29 PM

(Unassigning myself because this task is cluttering my https://phabricator.wikimedia.org/maniphest/query/assigned/ page, and doesn't seem to be moving forward.)

Since I just happened to notice the same issue, here’s a rebased patch with added test:

Is there any particular reason not to fix this? I understand it could in theory break some code that happens to use mw.message().parse() without having jqueryMsg loaded, and relies on it not escaping HTML, but are we aware of any actual instances of this? (I assume the fix would usually be to declare a dependency on mediawiki.jqueryMsg.)

Is there any particular reason not to fix this? I understand it could in theory break some code that happens to use mw.message().parse() without having jqueryMsg loaded, and relies on it not escaping HTML, but are we aware of any actual instances of this? (I assume the fix would usually be to declare a dependency on mediawiki.jqueryMsg.)

Does this just need a security deploy? If so, I'm happy to do that. Rebased patch seems sane and still applies to master. Minor nit: we should preface the subject with SECURITY: prior to deploy and backporting.

It’s probably okay to security-deploy this, and just accept the (potential) minor breakage that might result, yeah. (It would’ve been nice to get this into 1.35, but it’s probably a bit late for that… so I guess the change just stays security-deployed in production until it’s time to make MW 1.35.1 due to other issues.) Rebased patch with SECURITY message:

Note that T86738 is another issue affecting jqueryMsg and could probably be fixed together with it. (I’d forgotten about it until I noticed that I had a security2 branch in my core.git ^^)

Ok, thanks. I'll plan to deploy both this one and T86738 during the security window today and keep an eye on logstash.

Deployed:

21:35 sbassett: Deployed mitigations for T115888

Let's hold this, keep the task protected and wait on the backports for the next security release (T256335).

sbassett lowered the priority of this task from Medium to Low.Aug 3 2020, 9:41 PM
Catrope added a subscriber: Catrope.Aug 4 2020, 2:27 AM
This comment was removed by Catrope.
This comment was removed by Catrope.
sbassett added a comment.EditedAug 4 2020, 2:12 PM

Just to confirm - this particular patch has not caused any issues in production, correct? It's a much simpler patch than the one for T86738 and I see it's still applied on wmf.2, so I assume we're ok.

Actually, the following issue turns out to be due to this fix, not the fix for T86738:

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.)

Flow really does inject raw HTML into the message arguments and expect it to be passed through:

mw.flow.ui.EditorWidget.js
mw.message( 'flow-wikitext-editor-help-and-preview' ).params( [
	// Link to help page
	$( '<div>' )
		.html( mw.message( 'flow-wikitext-editor-help-uses-wikitext' ).parse() )
		.find( 'a' )
		.attr( 'target', '_blank' )
		.end()
		.html(),
	// Preview link
	$( '<a>' )
		.attr( 'href', '#' )
		.addClass( 'flow-ui-editorWidget-label-preview' )
		.text( mw.message( 'flow-wikitext-editor-help-preview-the-result' ).text() )
		.get( 0 ).outerHTML
] ).parse() )

Because jqueryMsg already supports passing jQuery elements through the message, the fix isn’t too complicated: stop turning the jQuery elements into HTML, by removing the .html() and .get( 0 ).outerHTML parts at the end of both arguments. (And change the first <div> to a <span> so it’s not a block element.)

diff --git a/modules/flow/ui/widgets/editor/mw.flow.ui.EditorWidget.js b/modules/flow/ui/widgets/editor/mw.flow.ui.EditorWidget.js
index b06a4a3e..71676a0b 100644
--- a/modules/flow/ui/widgets/editor/mw.flow.ui.EditorWidget.js
+++ b/modules/flow/ui/widgets/editor/mw.flow.ui.EditorWidget.js
@@ -60,18 +60,16 @@
 			label: $( '<span>' ).append(
 				mw.message( 'flow-wikitext-editor-help-and-preview' ).params( [
 					// Link to help page
-					$( '<div>' )
+					$( '<span>' )
 						.html( mw.message( 'flow-wikitext-editor-help-uses-wikitext' ).parse() )
 						.find( 'a' )
 						.attr( 'target', '_blank' )
-						.end()
-						.html(),
+						.end(),
 					// Preview link
 					$( '<a>' )
 						.attr( 'href', '#' )
 						.addClass( 'flow-ui-editorWidget-label-preview' )
 						.text( mw.message( 'flow-wikitext-editor-help-preview-the-result' ).text() )
-						.get( 0 ).outerHTML
 				] ).parse() )
 				.find( '.flow-ui-editorWidget-label-preview' )
 				.on( 'click', this.onPreviewLinkClick.bind( this ) )

Should that be a security patch, or can it just go on Gerrit? I don’t think it’s security-sensitive in itself, but I suppose it makes it to some extent more visible that jqueryMsg used to pass through unescaped HTML.

Because jqueryMsg already supports passing jQuery elements through the message, the fix isn’t too complicated: stop turning the jQuery elements into HTML, by removing the .html() and .get( 0 ).outerHTML parts at the end of both arguments. (And change the first <div> to a <span> so it’s not a block element.)

...

Should that be a security patch, or can it just go on Gerrit? I don’t think it’s security-sensitive in itself, but I suppose it makes it to some extent more visible that jqueryMsg used to pass through unescaped HTML.

+1 to the patch, makes sense. IMO, this is likely fine to go through gerrit with a benign commit subject and message where it can have more eyes from both developers and tests. Especially if we can review it somewhat quickly near the train or a backport window.

Base added a subscriber: cscott.Aug 5 2020, 3:38 PM
Base added a subscriber: Base.

And also https://gerrit.wikimedia.org/r/c/mediawiki/extensions/ContentTranslation/+/618508, for practically the same change in ContentTranslation, which should fix T259565#6361410.

This was merged and I just backported it to wmf.3 (but not wmf.2, I figure it’s enough to let the fix ride the train). The Flow fix is still open.

This was merged and I just backported it to wmf.3 (but not wmf.2, I figure it’s enough to let the fix ride the train). The Flow fix is still open.
Also backported to wmf.3 now.

Yep, should be fine if we're just talking about the Flow and CT patch sets.

To sum up:

  1. Both the supplemental patches https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Flow/+/618507 and https://gerrit.wikimedia.org/r/c/mediawiki/extensions/ContentTranslation/+/618508 were merged to their respective master and wmf.3 branches and are in production.
  2. The security patch from T115888#6356714 is still applied in production on wmf.3.
  3. The security patch from T115888#6356714 is being tracked for the next release (T256335) and this task should remain private until said release.

I'd guess the related public bug (T259565) could also be resolved for now.

Reedy added a subscriber: Reedy.EditedMon, Sep 21, 11:49 PM

https://github.com/wikimedia/mediawiki/blob/REL1_31/resources/src/mediawiki/mediawiki.js#L299-L301

		parser: function () {
			return mw.format.apply( null, [ this.map.get( this.key ) ].concat( this.parameters ) );
		},

https://github.com/wikimedia/mediawiki/blob/REL1_31/resources/src/mediawiki/mediawiki.jqueryMsg.js#L1360-L1374

	mw.Message.prototype.parser = function () {
		if ( this.format === 'plain' || !/\{\{|[<>[&]/.test( this.map.get( this.key ) ) ) {
			// Fall back to mw.msg's simple parser
			return oldParser.apply( this );
		}

		if ( !this.map.hasOwnProperty( this.format ) ) {
			this.map[ this.format ] = mw.jqueryMsg.getMessageFunction( {
				messages: this.map,
				// For format 'escaped', escaping part is handled by mediawiki.js
				format: this.format
			} );
		}
		return this.map[ this.format ]( this.key, this.parameters );
	};

The code for MW-1.31-release is quite different... The patch for master applies cleanly on REL1_35 and REL1_34.

Can anyone more familiar with this code advise whether this is applicable to 1.31? And if so, help write a new patch?

Test code seems to apply fine


ALSO... Do these need backporting? They're not bundled in the tarball, so no major rush.. But if we have relevant fixes, we should be backporting where it's possible and makes sense.

They do backport cleanly to REL1_34/REL1_35.

The Flow patch backports cleanly to REL1_31.

The CT one doesn't... but conflicts are trivial looking at https://gerrit.wikimedia.org/r/c/mediawiki/extensions/ContentTranslation/+/628992

And also https://gerrit.wikimedia.org/r/c/mediawiki/extensions/ContentTranslation/+/618508, for practically the same change in ContentTranslation, which should fix T259565#6361410.

The issue seems to be applicable to 1.31, I ran the foo3 part of the code in the task description on a 1.31.1 wiki and an alert() was fired.

The Flow and ContentTranslation patches should be backported, yes, since otherwise those extensions will show some escaped HTML when MediaWiki core receives the fix.

Reedy added a comment.Tue, Sep 22, 5:17 PM

The issue seems to be applicable to 1.31, I ran the foo3 part of the code in the task description on a 1.31.1 wiki and an alert() was fired.

Thanks. Any chance I could get you to backport the patch please?

The Flow and ContentTranslation patches should be backported, yes, since otherwise those extensions will show some escaped HTML when MediaWiki core receives the fix.

I've backported these (at least in gerrit)

https://gerrit.wikimedia.org/r/q/I46fc3002d6934996216b821eb38eae786a26768f
https://gerrit.wikimedia.org/r/q/I16757cec32f0fe638bc37c1ef55555e62f6c38c1

REL1_31 needed some extra backporting for CT (as usual as these things change) - T263509: The module 'wikibase.api.RepoApi' required by 'ext.cx.wikibase.link' must exist

CI is v+2-ing. I've not C+2'd it as I was waiting for a confirmation here :)

The issue seems to be applicable to 1.31, I ran the foo3 part of the code in the task description on a 1.31.1 wiki and an alert() was fired.

Thanks. Any chance I could get you to backport the patch please?

Turned out to be not quite as bad as I feared:

Reedy closed this task as Resolved.Wed, Sep 23, 5:05 PM
Reedy assigned this task to Lucas_Werkmeister_WMDE.

Perfect, thanks!

Closing for tracking purposes as we have a full complement of backports

Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".Thu, Sep 24, 11:16 PM
Reedy changed the edit policy from "Custom Policy" to "All Users".