Page MenuHomePhabricator

CVE-2023-45366: FederatedPropertiesError shows label as unescaped HTML
Closed, ResolvedPublicSecurity

Description

As far as I can tell, there is no XSS vulnerability here, but I’m filing this as a security task initially so we can make sure it’s fine, and maybe open it up later.

Wikibase’s TemplateFactory::render() must be called with safe / HTML-escaped parameters, but FederatedPropertiesError directly passes a label into it:

if ( $hasLabel ) {
	$labelText = $entity->getLabels()->getByLanguage( $languageCode )->getText();
}

$idInParenthesesHtml = htmlspecialchars( wfMessage( 'parentheses', [ $entityId ] )->parse() );

$html = $templateFactory->render( 'wikibase-title',
	!$hasLabel ? 'wb-empty' : '',
	!$hasLabel ? wfMessage( 'wikibase-label-empty' )->parse() : $labelText,
	$idInParenthesesHtml
);

parent::__construct( new RawMessage( $html ), $errorBody, [] ); // parent = ErrorPageError

If the entity being shown has HTML in its label (an example on Wikidata would be the infamous <script>alert("!Mediengruppe Bitnik");</script>), then some of that HTML will be shown on the error page. Specifically, an item with the label

<b style="color: purple; background: url(https://lucaswerkmeister.de/);"><script>alert('xss')</script><b>

will look like:

image.png (196×998 px, 23 KB)

where the heading’s outer HTML is:

<h1 id="firstHeading" class="firstHeading mw-first-heading"><span class="wikibase-title ">
<span class="wikibase-title-label"><b style="/* insecure input */">&lt;script&gt;alert('xss')&lt;/script&gt;<b></b></b></span><b style="/* insecure input */"><b>
<span class="wikibase-title-id">(Q8)</span>
</b></b></span></h1>

Notice that the <script> was escaped, and the style= was replaced with the harmless /* insecure input */. Apparently, this happens when OutputPage::setPageTitle() uses Sanitizer::removeSomeTags() for the page title. (The HTML <title> further gets Sanitizer::stripAllTags() treatment.)

Still, the fact that you can inject some HTML markup is undesirable, and should be fixed.

Event Timeline

I’m afraid I don’t have good steps to reproduce this. I don’t have a wiki with a proper, working federated properties setup; I hacked something together like this:

LocalSettings.php
wfLoadExtension( 'WikibaseRepo', "$IP/extensions/Wikibase/extension-repo.json" );
require_once "$IP/extensions/Wikibase/repo/ExampleSettings.php";
$wgWBRepoSettings['federatedPropertiesEnabled'] = true;
$wgWBRepoSettings['federatedPropertiesSourceScriptUrl'] = 'http://localhost/wiki1/';
$wgWBRepoSettings['entitySources'] = [
    'local' => [
        'entityNamespaces' => [ 'item' => 120 ],
        'repoDatabase' => false,
        'baseUri' => 'http://localhost/entity/',
        'interwikiPrefix' => '',
        'rdfNodeNamespacePrefix' => 'wd',
        'rdfPredicateNamespacePrefix' => 'wdt',
        'type' => 'db'
    ],
    'fedprops' => [
        'entityNamespaces' => [ 'property' => 122 ],
		'entityTypes' => [ 'property' ],
        'baseUri' => 'http://localhost/entity/',
        'interwikiPrefix' => 'wiki1',
        'rdfNodeNamespacePrefix' => 'fpwd',
        'rdfPredicateNamespacePrefix' => 'fpwd',
        'type' => 'api'
    ],
];
hacky patch to show the error page for unrelated errors
diff --git a/repo/includes/FederatedProperties/FederatedPropertiesUiEntityParserOutputGeneratorDecorator.php b/repo/includes/FederatedProperties/FederatedPropertiesUiEntityParserOutputGeneratorDecorator.php
index 9e0902bd53..82c664ae5c 100644
--- a/repo/includes/FederatedProperties/FederatedPropertiesUiEntityParserOutputGeneratorDecorator.php
+++ b/repo/includes/FederatedProperties/FederatedPropertiesUiEntityParserOutputGeneratorDecorator.php
@@ -9,6 +9,7 @@
 use Wikibase\Lib\Store\EntityRevision;
 use Wikibase\Repo\ParserOutput\EntityParserOutputGenerator;
 use Wikibase\Repo\WikibaseRepo;
+use Wikimedia\Assert\PostconditionException;
 
 /**
  * Wraps an EntityParserOutputGenerator and adds Federated Properties UI modules and error handling.
@@ -58,7 +59,7 @@ public function getParserOutput(
 				] );
 			}
 
-		} catch ( FederatedPropertiesException $ex ) {
+		} catch ( FederatedPropertiesException | PostconditionException $ex ) {
 			$entity = $entityRevision->getEntity();
 
 			if ( $entity instanceof LabelsProvider ) {
action=wbeditentity data= payload to create an item in the API sandbox
{"labels":{"en":{"language":"en","value":"<b style=\"color: purple; background: url(https://lucaswerkmeister.de/);\"><script>alert('xss')</script><b>"}},"claims":[{"mainsnak":{"snaktype":"value","property":"http://localhost/entity/P599","datavalue":{"value":"ExampleString","type":"string"}},"type":"statement","rank":"normal"}]}

Anyway, the fix for this is simple enough, I think:

non-test part of the patch for convenience
diff --git a/repo/includes/FederatedProperties/FederatedPropertiesError.php b/repo/includes/FederatedProperties/FederatedPropertiesError.php
index c83ebb4369..f245933d47 100644
--- a/repo/includes/FederatedProperties/FederatedPropertiesError.php
+++ b/repo/includes/FederatedProperties/FederatedPropertiesError.php
@@ -45,5 +45,5 @@ public function __construct( $languageCode, $entity, $msg, $params = [] ) {
 		$html = $templateFactory->render( 'wikibase-title',
 			!$hasLabel ? 'wb-empty' : '',
-			!$hasLabel ? wfMessage( 'wikibase-label-empty' )->parse() : $labelText,
+			!$hasLabel ? wfMessage( 'wikibase-label-empty' )->parse() : htmlspecialchars( $labelText ),
 			$idInParenthesesHtml
 		);

But since Federated Properties aren’t enabled anywhere in production ($wgWBRepoSettings['federatedPropertiesEnabled'] defaults to false and isn’t touched in production config), I’m not sure we even need to deploy this in production. (If it was possible to inject fully arbitrary HTML/JS, I’d still want to do it just to be safe, but with the Sanitizer protection in place, I think we can skip it.) It’s probably enough to just push this to Gerrit, and backport it to supported REL* branches, shortly before T334778: Prepare wmde.12 (1.38.7) minor / security release / T332786: Prepare wmde.13 (1.39.4) minor / security release happens. (CC @darthmon_wmde) I’m also not sure if the commit message needs the “SECURITY” prefix in that case.

Subscribing the other Wikibase Suite Team members, I don’t know how many of them have security access.

But since Federated Properties aren’t enabled anywhere in production ($wgWBRepoSettings['federatedPropertiesEnabled'] defaults to false and isn’t touched in production config), I’m not sure we even need to deploy this in production. (If it was possible to inject fully arbitrary HTML/JS, I’d still want to do it just to be safe, but with the Sanitizer protection in place, I think we can skip it.) It’s probably enough to just push this to Gerrit, and backport it to supported REL* branches, shortly before T334778: Prepare wmde.12 (1.38.7) minor / security release / T332786: Prepare wmde.13 (1.39.4) minor / security release happens. (CC @darthmon_wmde) I’m also not sure if the commit message needs the “SECURITY” prefix in that case.

+1 from the Security-Team for pushing this through gerrit if it's confirmed disabled within Wikimedia production (which it absolutely seems to be). And the fact that Wikimedia/WMF are likely the only orgs even running Wikibase under large, production wikis at this point.

+1 for the patch as well for this specific issue. I typically like using ENT_QUOTES with htmlentities when feasible, even if it isn't technically, contextually needed (it's not here), as long as it doesn't result in odd double-escapes or the unexpected display of html entity codes.

+1 for the patch as well for this specific issue. I typically like using ENT_QUOTES with htmlentities when feasible, even if it isn't technically, contextually needed (it's not here), as long as it doesn't result in odd double-escapes or the unexpected display of html entity codes.

Sure, done:

(That also means fewer changes when we move to PHP 8.1, because ENT_QUOTES is the default there.)

Ok, I think this is fine to push through gerrit and get merged on a Monday before a train cut. Even that likely isn't necessary, just extra-cautious.

But that means third-party Wikibases with Federated Properties enabled would be vulnerable until the next Wikibase Suite Team release comes out. I think the timing of the change on Gerrit should be aligned with the next Wikibase Suite release (T340939, I guess? or T332786?), not with the WMF train.

Suite devs: can you please confirm that you have this issue on your radar?

Suite devs: can you please confirm that you have this issue on your radar?

Yes, this issue is on our radar. Though, we do not have a process defined how to deliver it with the next release.

AFAIK we currently do not have a mechanism to ship patches not on gerrit. Might probably be a good idea to do though. I will take the discussion to the team and report back here.

Our next releases are: T334778, T332786 and T340939.

As just discussed with @Lucas_Werkmeister_WMDE, the issue does not represent an exposed vulnerability and therefore does not need to be handled as a security issue. This task can be opened to the public now.

The patch can just go to gerrit / git and the wikibase suite release pipeline will pick it up from there upon the next release.

Change 935883 had a related patch set uploaded (by SBassett; author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] SECURITY: Escape label in FederatedPropertiesError

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

Change 935883 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] SECURITY: Escape label in FederatedPropertiesError

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

Change 961220 had a related patch set uploaded (by Mmartorana; author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@REL1_35] SECURITY: Escape label in FederatedPropertiesError

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

Change 961222 had a related patch set uploaded (by Mmartorana; author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@REL1_39] SECURITY: Escape label in FederatedPropertiesError

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

Change 961223 had a related patch set uploaded (by Mmartorana; author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@REL1_40] SECURITY: Escape label in FederatedPropertiesError

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

Change 961220 merged by SBassett:

[mediawiki/extensions/Wikibase@REL1_35] SECURITY: Escape label in FederatedPropertiesError

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

Change 961222 merged by SBassett:

[mediawiki/extensions/Wikibase@REL1_39] SECURITY: Escape label in FederatedPropertiesError

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

Change 961223 merged by SBassett:

[mediawiki/extensions/Wikibase@REL1_40] SECURITY: Escape label in FederatedPropertiesError

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

mmartorana triaged this task as Medium priority.
mmartorana changed the visibility from "Custom Policy" to "Public (No Login Required)".
mmartorana renamed this task from FederatedPropertiesError shows label as unescaped HTML to CVE-2023-45366: FederatedPropertiesError shows label as unescaped HTML.Oct 10 2023, 5:28 PM