Page MenuHomePhabricator

Possible XSS in SpecialGlobalUsage (CVE-2020-35622)
Closed, ResolvedPublicSecurity

Description

Seen while upgrading taint-check in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GlobalUsage/+/642262:

SpecialGlobalUsage.php line 178
// @var GlobalUsageQuery $query
foreach ( $query->getSingleImageResult() as $wiki => $result ) {
	// ...
	foreach ( $result as $item ) {
		$out->addHtml( "\t<li>" . self::formatItem( $item ) . "</li>\n" );
	}

$query->getSingleImageResult() returns an array of database fields, including page titles. Then we have

public static function formatItem( $item ) {
	if ( !$item['namespace'] ) {
		$page = $item['title'];
	} else {
		$page = "{$item['namespace']}:{$item['title']}";
	}

	$link = WikiMap::makeForeignLink( $item['wiki'], $page,
		str_replace( '_', ' ', $page ) );
	// Return only the title if no link can be constructed
	return $link === false ? $page : $link;
}

So if the page cannot be found (which I guess is a possibility?), it will echo the DB fields verbatim. In theory, this should be safe thanks to $wgLegalTitleChars, but it's still outputting user-controlled data.

The fix is as simple as escaping $page, so formatItem() will always return an escaped value. I'm creating this task as security-protected just in case, but I guess it can also be made public; leaving this up to the Security Team.

Event Timeline

sbassett triaged this task as Medium priority.Nov 23 2020, 4:09 PM
sbassett moved this task from Incoming to Watching on the Security-Team board.
sbassett added a subscriber: sbassett.

Could be public from my point of view

Similar to T268917, let's wait for the train deployments this week and then make this task public.

sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".Dec 21 2020, 8:39 PM
sbassett renamed this task from Possible XSS in SpecialGlobalUsage to Possible XSS in SpecialGlobalUsage (CVE-2020-35622).Dec 22 2020, 8:50 PM

Change 651589 had a related patch set uploaded (by SBassett; owner: Umherirrender):
[mediawiki/extensions/GlobalUsage@REL1_35] Pass escaped html to WikiMap::makeForeignLink for sanity

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

Change 651590 had a related patch set uploaded (by SBassett; owner: Umherirrender):
[mediawiki/extensions/GlobalUsage@REL1_31] Pass escaped html to WikiMap::makeForeignLink for sanity

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

Change 651589 merged by jenkins-bot:
[mediawiki/extensions/GlobalUsage@REL1_35] Pass escaped html to WikiMap::makeForeignLink for sanity

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

Change 651590 merged by jenkins-bot:
[mediawiki/extensions/GlobalUsage@REL1_31] Pass escaped html to WikiMap::makeForeignLink for sanity

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

Caused T274063.

So if the page cannot be found

While it's good to be preemptive, there are already checks to ensure the file exists before constructing the links (this includes querying the database).

Checking this again, I'm not sure that the fix is correct. If WikiMap::makeForeignLink returns false, then we should indeed escape $page before returning it. However, it should not be escaped when passed to makeForeignLink, since then it ends up being passed to Linker::makeExternalLink which escapes $text, which explains the double escape.

@Daimona - so looking at the gerrit change set, we'd just need to remove that first htmlspecialchars in your opinion? Basically change this:

$link = WikiMap::makeForeignLink( $item['wiki'], $page,
str_replace( '_', ' ', htmlspecialchars( $page ) ) );
// Return only the title if no link can be constructed
return $link === false ? htmlspecialchars( $page ) : $link;

to:

$link = WikiMap::makeForeignLink( $item['wiki'], $page,
str_replace( '_', ' ', $page ) );
// Return only the title if no link can be constructed
return $link === false ? htmlspecialchars( $page ) : $link;

Also - if we don't really need that underscore faux-normalization, then we shouldn't need the third arg at all, per the docblock:

@param string|null $text Link's text; optional, default to $page

I'll try to get a patch up soon for review. Never mind, I see there's already a patch up on the other bug: https://gerrit.wikimedia.org/r/692590