Maniphest T201418

XSS in WikibaseLexeme form values
Closed, ResolvedPublic
Actions

Description

The WikibaseLexeme extension introduces a datatype for forms, allowing you to state, for example, that the third word of the English phrase “every dog has its day” is derived from the form “has” of the lexeme “have”. The visual rendering of a form consists of all its representations, separated by an interface message defaulting to a slash.

Unfortunately, we don’t HTML-escape the representations.

		$representationString = implode(
			$representationSeparator,
			$representations->toTextArray()
		);

		return Html::rawElement(
			'a',
			[
				'href' => $title->isLocal() ? $title->getLinkURL() : $title->getFullURL(),
			],
			$representationString
		);

Therefore, when you link to a form which has HTML in the representation:

Screenshot from 2018-08-07 15-57-18.png (318×959 px, 15 KB)

The fix is super trivial – use Html::element instead of Html::rawElement – but I’m not sure what the procedure is to deploy that fix without publishing in on Gerrit (which effectively makes the vulnerability public).

Suggested tags: Wikidata, Wikidata Lexicographical data.

Details

	Project	Branch	Lines +/-	Subject
	mediawiki/extensions/WikibaseLexeme	master	+28 -1	SECURITY: HTML-escape form representations

Customize query in gerrit

Related Objects

Mentioned In: rEWLE35d0a70c53f7: SECURITY: HTML-escape form representations

Event Timeline

Lucas_Werkmeister_WMDE created this task.Aug 7 2018, 2:05 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 7 2018, 2:05 PM

https://wikitech.wikimedia.org/wiki/How_to_perform_security_fixes

If you prepare the patch, I can look into deploying this (unless @Bawolff wants to).

Patch:

From 841609dd7263793e3ec4b765aab7468b11b3de5e Mon Sep 17 00:00:00 2001
From: Lucas Werkmeister <lucas.werkmeister@wikimedia.de>
Date: Tue, 7 Aug 2018 17:42:25 +0200
Subject: [PATCH] SECURITY: HTML-escape form representations

Form representations are nearly arbitrary text and can contain valid
HTML, so we must escape them when rendering form IDs to HTML.

Bug: T201418
Change-Id: I6ae427979f32fc49bbc8c8fcff1a5f382fea22d1
---
 src/PropertyType/FormIdHtmlFormatter.php      |  2 +-
 .../PropertyType/FormIdHtmlFormatterTest.php  | 27 +++++++++++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/src/PropertyType/FormIdHtmlFormatter.php b/src/PropertyType/FormIdHtmlFormatter.php
index 66dc212..e193e27 100644
--- a/src/PropertyType/FormIdHtmlFormatter.php
+++ b/src/PropertyType/FormIdHtmlFormatter.php
@@ -78,7 +78,7 @@ public function formatEntityId( EntityId $formId ) {
 			$representations->toTextArray()
 		);
 
-		return Html::rawElement(
+		return Html::element(
 			'a',
 			[
 				'href' => $title->isLocal() ? $title->getLinkURL() : $title->getFullURL(),
diff --git a/tests/phpunit/mediawiki/PropertyType/FormIdHtmlFormatterTest.php b/tests/phpunit/mediawiki/PropertyType/FormIdHtmlFormatterTest.php
index 43b9f91..635fbfa 100644
--- a/tests/phpunit/mediawiki/PropertyType/FormIdHtmlFormatterTest.php
+++ b/tests/phpunit/mediawiki/PropertyType/FormIdHtmlFormatterTest.php
@@ -155,4 +155,31 @@ public function testFormatId_multipleRepresentations() {
 		);
 	}
 
+	public function testFormatId_htmlEscapesRepresentation() {
+		$formId = new FormId( 'L999-F666' );
+
+		$formRevision = new EntityRevision( new Form( $formId, new TermList( [
+			new Term( 'pt', '<script>alert("hi")</script>' ),
+		] ), [] ) );
+
+		/** @var EntityRevisionLookup|MockObject $revisionLookup */
+		$revisionLookup = $this->getMock( EntityRevisionLookup::class );
+		$revisionLookup->method( 'getEntityRevision' )
+			->with( $this->equalTo( $formId ) )
+			->willReturn( $formRevision );
+
+		$titleLookup = $this->getTitleLookupReturningMainPage( $formId );
+
+		$formatter = new FormIdHtmlFormatter(
+			$revisionLookup,
+			$titleLookup,
+			$this->getMockTextProvider()
+		);
+		$result = $formatter->formatEntityId( $formId );
+		$this->assertSame(
+			'<a href="LOCAL-URL#FORM">&lt;script>alert("hi")&lt;/script></a>',
+			$result
+		);
+	}
+
 }
-- 
2.17.1

Addshore added projects: Wikidata Lexicographical data, Wikidata.Aug 7 2018, 3:55 PM

Addshore added a subscriber: Addshore.

Deployed the above patch and result looks good: https://test.wikidata.org/w/api.php?action=wbformatvalue&format=json&datavalue={%22value%22%3A{%22id%22%3A%22L99-F2%22}%2C%22type%22%3A%22wikibase-entityid%22}&options={%22lang%22%3A%22en%22}&generate=text%2Fhtml%3B%20disposition%3Dverbose&property=P75109 (the < in asldflkd<sdfsfjsdlfkjsdf is correctly escaped).

@Bawolff I didn't commit the change to the /srv/patches git on deploy1001 as there is a lot of uncommitted stuff and I didn't quite know what to do with it.

Thanks, should I rebase the patch against master and upload it on Gerrit as a regular change? (I assume we don’t need to bother with a pre-release announcement or anything for this extension.)

In T201418#4485482, @Lucas_Werkmeister_WMDE wrote:

Thanks, should I rebase the patch against master and upload it on Gerrit as a regular change? (I assume we don’t need to bother with a pre-release announcement or anything for this extension.)

IMO there's no need to wait, given the extension is probably not used outside of WMF (it's very specific and has only recently been introduced). But I would like to wait for a final opinion from @Bawolff or @JBennett.

@Bawolff I didn't commit the change to the /srv/patches git on deploy1001 as there is a lot of uncommitted stuff and I didn't quite know what to do with it.

Some people seem to commit stuff to the patches directory, some don't. Don't worry about that.

So next steps generally for a non-bundled extension:

Commit patch to gerrit, and immediately +2 it (Since the patch is already reviewed, and we want to minimize time where its public but not committed).
[optional but recommended] Increment version number of extension if extension is versioned.
[recommended but ultimately optional] backport to any REL_XX branches that correspond to supported versions of MediaWiki
Send an email to mediawiki-l advising anybody using the extension that a security fix for the extension is available (We've been using mediawiki-l for security announcements for non-bundled extensions. example).

hoo added a subscriber: Lydia_Pintscher.Aug 8 2018, 8:34 AM

hoo mentioned this in rEWLE35d0a70c53f7: SECURITY: HTML-escape form representations.Aug 8 2018, 8:39 AM

The respective patch has been merged into master.

Per @Lydia_Pintscher there's no need to do any further backports or announcements as this extension is not used outside of WMF.

If wanted, we can IMO make this public now.

I’d prefer if we could make this public, yes. I don’t think there’s any need to keep it private (the public commit makes the issue fairly clear anyways).

hoo changed the visibility from "Custom Policy" to "Public (No Login Required)".Aug 14 2018, 1:34 PM

• chasemp added a project: Security.Feb 10 2020, 10:51 PM

• chasemp removed a project: acl*security.Feb 20 2020, 8:12 PM

XSS in WikibaseLexeme form valuesClosed, ResolvedPublicActions

Description

Details

Related Objects

Event Timeline

XSS in WikibaseLexeme form values
Closed, ResolvedPublic
Actions