Page MenuHomePhabricator

Use custom language code to find i18n XSS issues
Closed, ResolvedPublic3 Estimated Story PointsSecurity

Description

This is not a security vulnerability in itself, but I think the task should not be public at the moment. It can probably be made public later.

After finding an i18n XSS vulnerability (i.e., an issue where an i18n message wasn’t properly escaped, resulting in HTML injection via the message JSON files or the MediaWiki: namespace) in a recent Wikibase change (already fixed), I thought that to investigate this and other issues without having to edit the message files and rebuild the l10n cache, it would be nice to have a custom language code instead. I managed to put together this LocalSettings.php snippet:

$wgHooks['MessagesPreLoad'][] = function ( $title, &$message, $code ) {
	if ( $code !== 'xss' ) {
		return true;
	}

	$key = lcfirst( preg_replace( '|/xss$|', '', $title ) );
	$rawMessages = \MediaWiki\MediaWikiServices::getInstance()
		->getMainConfig()
		->get( \MediaWiki\MainConfigNames::RawHtmlMessages );
	if ( in_array( $key, $rawMessages, true ) ) {
		return true;
	}

	$xssViaInnerHtml = "<script>alert('$key')</script>";
	$xssViaAttribute = '">' . $xssViaInnerHtml . '<span data-rest="';
	$message = $xssViaInnerHtml . $xssViaAttribute;
	return false;
};

It sets every i18n message to a <script>alert('message-key')</script>-like HTML snippet (with a second copy to also catch <span title="unescapedMessage"> issues, in fact), and is activated via ?uselang=xss in the URL. When I tested it, I immediately found another XSS vulnerability: T340200: CVE-2023-45365: i18n XSS in Citoid Wikibase module

I’m not sure where to go from here… this feels like a useful technique that I want to share with others, but it probably shouldn’t be public yet while it’s so easy to find real vulnerabilities with it?

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Lucas_Werkmeister_WMDE changed Author Affiliation from N/A to Wikimedia Deutschland.Jun 23 2023, 2:47 PM

When I tested it, I immediately found another XSS vulnerability: T340200: CVE-2023-45365: i18n XSS in Citoid Wikibase module

Also {T340217} a few minutes later. (I found nothing in Special:RecentChanges and VisualEditor, then thought to try another skin with more JS features.)

Also {T340220} (slightly different but also in Vector)

I think this is very cool. I'd recommend making it into a proper feature in MediaWiki, behind a config setting to be enabled in developer environments only, similar to UsePigLatinVariant.

You might want to skip alerting for messages listed in RawHtmlMessages.

Thanks, agreed that this would be nice to have as a toggleable option via DevelopmentSettings + config value. We could also use in CI with Selenium to check if an alert is open.

I think this is very cool. I'd recommend making it into a proper feature in MediaWiki, behind a config setting to be enabled in developer environments only, similar to UsePigLatinVariant.

Sounds good to me.

You might want to skip alerting for messages listed in RawHtmlMessages.

I forgot about that, good idea.

(cc @jhsoby, who has been recently working on preventing potential i18n XSS issues in WikiEditor [https://gerrit.wikimedia.org/r/q/topic:remove-autoMsg], and who might enjoy using this for testing)

This is very useful and clever!

One nitpick: xss is a valid and assigned ISO 639 code. It's for a dead language with very little written record, so we will never have that as an interface language, but it could theoretically be used for something in Wikidata, for instance. So it might be better to maybe use a private use BCP47-compliant tag, like maybe x-xss? But it might not matter too much if, as described, this is only available behind a config switch – and xss is definitely easier to remember than any other thing I can think of.

Ah, I was hoping xss wouldn’t be assigned :/ something in the qaa..qtz reserved range would be “safer”, but also harder to remember.

I should have checked the source directly instead of enwiki: xss is deprecated, and therefore shouldn't be used for any actual application – meaning that we could safely use it for this without risking interfering with any real-world use of the code even if it's not 100% kosher.

sbassett changed the task status from Open to In Progress.Jul 10 2023, 4:29 PM
sbassett triaged this task as Low priority.
sbassett changed Risk Rating from N/A to Low.

I’m not sure where to go from here… this feels like a useful technique that I want to share with others, but it probably shouldn’t be public yet while it’s so easy to find real vulnerabilities with it?

Very cool, indeed, and we can keep this task private for some time. I'm not really sure there's much else actionable for this specific task though. It seems like practical applications with findings should have their own bugs filed (as has been done already) or be used for open tasks like T200997.

T340220, T340217 and T340200 should all be fixed in production now; once the tasks have been made public, I think we can publish this one as well. And then someone™ can work on reimplementing it as a MediaWiki feature with a config setting to enable it (like the pig latin variant), rather than a hook for LocalSettings.php. (Probably in a way similar to how the qqx language code is implemented?) Not sure if I’ll have time for that myself, though.

And then someone™ can work on reimplementing it as a MediaWiki feature with a config setting to enable it (like the pig latin variant), rather than a hook for LocalSettings.php. (Probably in a way similar to how the qqx language code is implemented?) Not sure if I’ll have time for that myself, though.

Eh, found some time for it after all :D

Can go on Gerrit once this task is public, and then be reviewed as usual. (In the meantime, for anyone interested in using the feature now, the LocalSettings.php snippet in the task description is probably easier than constantly rebasing this patch locally.)

Can go on Gerrit once this task is public, and then be reviewed as usual. (In the meantime, for anyone interested in using the feature now, the LocalSettings.php snippet in the task description is probably easier than constantly rebasing this patch locally.)

I think that might able to happen now, as all of the following have patches deployed within Wikimedia production:

  1. T340200: CVE-2023-45365: i18n XSS in Citoid Wikibase module - SAL
  2. {T340217} - SAL
  3. {T340220} - SAL

Were there any others at this time?

Not as far as I remember – I was assuming this would have to wait until the next regular security release in three months or so, but I’m also okay with doing it sooner.

Not as far as I remember – I was assuming this would have to wait until the next regular security release in three months or so, but I’m also okay with doing it sooner.

I think I'd consider either approach low risk at this point - either opening this up or keeping it protected until the security releases at the end of September. We're fully patched in Wikimedia production and the various related tasks above can be kept private until the September security releases (certainly the Vector ones will be). So I don't think much would be disclosed here. And even if this is a handy tool to put in front of malicious actors, the means of exploit is still non-trivial IMO since they'd have to compromise an int-admin account or bypass various twn checks in some novel way.

Hm, I think I agree the risk is low, but I’m still not sure it’s useful to publish this already. T340200: CVE-2023-45365: i18n XSS in Citoid Wikibase module is pretty obscure, but I think there’s a good chance that people trying out the feature would rediscover {T340220} and/or {T340217}. That’s not a huge problem for the reason you described, but it’s… weird?

But I wonder if we can use this as a “demonstration” of the feature later. If we publish this task (and the $wgEnableXssLanguage feature), say, one week before the next security release, then we could perhaps say something in the announcement like: “The next MediaWiki security release, scheduled for X, fixes a few issues that were discovered using this feature. If you manage to find an unescaped message, consider waiting until after the release to check whether the issue was already fixed or not.”

But I wonder if we can use this as a “demonstration” of the feature later. If we publish this task (and the $wgEnableXssLanguage feature), say, one week before the next security release, then we could perhaps say something in the announcement like: “The next MediaWiki security release, scheduled for X, fixes a few issues that were discovered using this feature. If you manage to find an unescaped message, consider waiting until after the release to check whether the issue was already fixed or not.”

This sounds like a good plan to me. I've made a note to remind us on T340865.

I drafted a wikitech-l email that I can send a week before the security release – feel free to comment or suggest changes: https://docs.google.com/document/d/1pTzClyWWdqB3RlwQ2YYyCiyBJxvbXodIvfqMdP-D1eI/edit?usp=sharing

I drafted a wikitech-l email that I can send a week before the security release – feel free to comment or suggest changes: https://docs.google.com/document/d/1pTzClyWWdqB3RlwQ2YYyCiyBJxvbXodIvfqMdP-D1eI/edit?usp=sharing

LGTM. This will, of course, now provide potential malicious actors a very easy and thorough way of finding such exploits, but I think the tradeoff - the hope that security researchers and other developers find them first - is worthwhile. Thanks again for all of your work on this.

Alright, patches are up:

The first one should be merged soon, the second one should wait until after the upcoming security release (which I assume is planned for the end of this month).

I think really quite strongly that we should not violate language standards and re-use the language code, even if it's currently deprecated. We've been working for years to make MW comply with standards, please don't break that work.

This won't catch anything using an explicit language or inContentLanguage, right?

Good point – I don’t think it will, no.

All the changes for this feature are now merged (including @Fomafixfollow-up for message parameters); I think we can close this task and make it public now. (I’ll try to add the feature to some relevant mw.o pages.)

sbassett assigned this task to Lucas_Werkmeister_WMDE.
sbassett moved this task from Watching to Our Part Is Done on the Security-Team board.
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
sbassett changed the edit policy from "Custom Policy" to "All Users".

One thing to consider - instead of script tag you might want to consider doing <img src=x onerror="alert('xss')"> instead, because it works in more places (namely when setting innerHTML)

One thing to consider - instead of script tag you might want to consider doing <img src=x onerror="alert('xss')"> instead, because it works in more places (namely when setting innerHTML)

It might be nice to have a configurable xssPayload variable or similar.

This also seems to be really demonstrating the value of phan taint check. So far it seems like most of the found issues are in some corner that phan-taint-check can't analyze

  • mustache templates (+ some skin stuff which I didn't look too closely at. Not sure if this is purely due to mustache or if there are additional complications)
  • LogFormatter subclasses
  • Pager class hierarchy.

[There's some other one-off things I didn't mention that i think would be more straight forward to add to phan-taint-check]

Maybe we should think about how to either refactor these code patterns so we can use phan-taint-check on them, or think of ways of making phan taint check better.

  • mustache templates

Yup, that would be T199397. I'm not sure how feasible that would be.

Maybe we should think about how to either refactor these code patterns so we can use phan-taint-check on them, or think of ways of making phan taint check better.

+1, always happy to hear suggestions on how to make taint-check more useful.

Creating T347787 to brainstorm ideas about the Pager class hierarchy. I didn't create a brainstorming one for LogFormatter, as that case seems pretty hopeless.

I'm curious how we can track issues found by this...

Just xref this task in the description?

I'm curious how we can track issues found by this...

Just xref this task in the description?

We could subtask them under this task. Or sure, cross-ref this task within any new bug, and maybe still subtask them under T2212?