Page MenuHomePhabricator

XSSs from not escaping i18n messages in recent core patch
Closed, ResolvedPublicSecurity

Description

As part of T280766, a few patches were pushed to replace outdated CSS styling. Some of these also replaced calls to OutputPage::wrapWikiMsg with code like

$out->addHTML( Html::errorBox(
	$out->msg( 'deletedwhileediting' )->plain(),
	'',
	'mw-deleted-while-editing'
) );

which is understandable, since the docblock of wrapWikiMsg says:

*     $wgOut->wrapWikiMsg( "<div class='errorbox'>\n$1\n</div>", 'some-error' );
*
* Is equivalent to:
*
*     $wgOut->addWikiTextAsInterface( "<div class='errorbox'>\n"
*         . wfMessage( 'some-error' )->plain() . "\n</div>" );

but this is not correct. wrapWikiMsg is actually escaping its content, probably by running it through the parser (haven't checked deeply). I spotted this in r682960 for AbuseFilter, but similar code was also introduced in r682915 for MW core.

I believe that escaping should be fixed immediately, and equally important is to update the documentation of wrapWikiMsg, which is basically asking for this mistake to happen again in the future.

(FTR, this wasn't spotted by taint-check because version 3.2.1 of the plugin still can't fully understand that the HTML parameter is passed through by successBox. r681443 is a change that I'm hoping to complete in a few days that would flag this code)

Event Timeline

Daimona added subscribers: Jdlrobson, Mainframe98.

(Also, I can't send a patch right now)

Ugh, I so hate those methods! I had it right the first time.

As far as I understand it, those methods interpret the text they're given as wikitext, which must be parsed, so they're actually equivalent to calling Message::parse.

I didn't update the documentation of OutputPage::wrapWikiMsg, because it is technically correct and I don't want to create even more confusion by attempting to clarify it.

Jdlrobson triaged this task as Unbreak Now! priority.Apr 30 2021, 6:52 PM

Note: The code is not in production yet so marking as a train blocker. Perhaps we could revert the initial patch?

To clarify, is this only exploitable by interface administrators? If so, we should probably just submit the patch to Gerrit, and review and merge it as normal. We've handled those low-severity escaping issue like that in the past (e.g. T278058 looks like the latest example).

To clarify, is this only exploitable by interface administrators?

And possibly translated messages, I'd guess, though there is some automation and intentional code-review to catch anything at that layer.

Note: The code is not in production yet so marking as a train blocker. Perhaps we could revert the initial patch?

If so, we should probably just submit the patch to Gerrit, and review and merge it as normal. We've handled those low-severity escaping issue like that in the past (e.g. T278058 looks like the latest example).

I think this would be low-risk enough to send up through gerrit and have both go out with wmf.4 next week. The only possible weirdness would be external MediaWiki operators pulling down master almost every day and introducing this issue within a production system for some brief time period, which I'm guessing is extremely rare if it happens at all.

Change set for review: https://gerrit.wikimedia.org/r/683982

I wouldn't mind keeping this task private until next week though.

I didn't update the documentation of OutputPage::wrapWikiMsg, because it is technically correct and I don't want to create even more confusion by attempting to clarify it.

Oh right, sorry, I misread it and didn't notice it was using addWikiTextAsInterface, which is the actual parsing method.

To clarify, is this only exploitable by interface administrators?

Depends on the definition of "interface administrators" -- it's exploitable by anybody who can edit interface messages (editinterface), which I think usually means normal sysops, not "interface administrators" as in "can edit site-wide JS". But anyway yes, this is certainly low risk.

This is still marked as a train blocker. Is there any outstanding reason for that?

Removing as train blocker, please re-add if I'm mistaken.

sbassett closed this task as Resolved.EditedMay 5 2021, 3:46 PM
sbassett lowered the priority of this task from Unbreak Now! to Low.

This is still marked as a train blocker. Is there any outstanding reason for that?

No. Both commits from https://gerrit.wikimedia.org/r/682915 and https://gerrit.wikimedia.org/r/683982 are on wmf.4.

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 Watching to Our Part Is Done on the Security-Team board.