Page MenuHomePhabricator

XSS: MobileFrontend fails to apply escaping to page titles and wikibase labels when using them as fallback for displaytitle
Closed, ResolvedPublic

Description

The displaytitle page prop in MediaWiki core is defined to be HTML, and is used as raw (safe) HTML by MobileFrontend. However, if displaytitle is not set, MF uses the plain page title (or the label from supplied by the wikibase pageterms module) as a fallback. These are however plain text, so eswcaping must be applied when assigning them to a field that is treated as safe HTML.

Event Timeline

Maniphest changed the visibility from "Public (No Login Required)" to "Custom Policy".Nov 26 2015, 12:57 PM
Maniphest changed the edit policy from "All Users" to "Custom Policy".
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 26 2015, 12:57 PM
JanZerebecki added a project: Security.
JanZerebecki changed Security from None to Software security bug.
JanZerebecki edited subscribers, added: JanZerebecki, daniel, Bene; removed: Aklapper.
daniel added a comment.EditedNov 26 2015, 1:00 PM

Steps to reproduce: put <script>alert("oops!")</script> into an item label. Search on Special:Nearby. When that item is found, the script will be executed.

This is probably due to the label being treated as a display title somewhere, without the proper escaping. DisplayTitle is always safe html. Labels are not, they are unsafe plain text.

Tested with the following settings:

$wgMFNearby = true;
$wgMFQueryPropModules[] = 'pageterms';
$wgMFSearchAPIParams['wbptterms'] = array( 'label' );
$wgMFSearchGenerator = array(
	'name' => 'wbsearch',
	'prefix' => 'wbs'
);

It does not seem to matter whether $wgMFSearchGenerator is set or not, though.

daniel triaged this task as Unbreak Now! priority.Nov 26 2015, 1:00 PM
daniel added projects: Wikidata, MobileFrontend.
daniel added a subscriber: Lydia_Pintscher.
JanZerebecki added a subscriber: Jdlrobson.

The problem seems to be the following line in Page.js:

displayTitle = terms && terms.label ? terms.label[0] : pageprops.displaytitle;

pageprops.displaytitle is HTML by definition. terms.label[0] is plain text and needs escaping.

daniel added a subscriber: Florian.Nov 26 2015, 1:49 PM

The issue was apparently introduced by I49cf14db7857b077b8536ac654c04a4af2f4d2ee see https://gerrit.wikimedia.org/r/#/c/246282/12/resources/mobile.startup/Page.js

Steps to reproduce: put <script>alert("oops!")</script> into an item label. Search on Special:Nearby. When that item is found, the script will be executed.
This is probably due to the label being treated as a display title somewhere, without the proper escaping. DisplayTitle is always safe html. Labels are not, they are unsafe plain text.

After a first look, this seems to be the problem. Normally we escape things in our hogan templates (which is the nearest "part" of the code before the html is generated/outputted. If we escape the displayTitle property or Page.js there, we would double escape the displaytitle returned by the api.

Would it be a problem to return safe html for the label inside Wikibase? Otherwise we need to escape it client side (which means in _any_ client, which uses these labels). I assume you already escape the labels in Wikibase frontend code? If you can't return safe html in Wikibase, I'll upload a change to escape the label in MobileFrontend (IIRC, Security related tasks needs to upload a git patch to the task, which will be imported into gerrit, after they're approved, right?).

JanZerebecki added a comment.EditedNov 26 2015, 2:36 PM

copied to tin.eqiad.wmnet:/srv/patches/extensions/MobileFrontend/T119707.patch

The displayTitle also falls back in JS to page title which is also plain text, not only to wikidata label.

Adding a few more wmf people. Jon is out on vacation, iirc.

! In T119707#1833886, @Florian wrote:
Normally we escape things in our hogan templates (which is the nearest "part" of the code before the html is generated/outputted. If we escape the displayTitle property or Page.js there, we would double escape the displaytitle returned by the api.
Would it be a problem to return safe html for the label inside Wikibase?

That would be odd and inconsistent. The API generally returns plain text for everything, unless explicitly stated otherwise. Core's dispalytitle pageprop is a rare exception.

The API caters to all kinds of clients, many of which don't ever need HTML. Forcing them to unescape our labels is not a good idea conceptually, and it would of course be a breaking change to the API.

So yes, that would be a problem.

Otherwise we need to escape it client side (which means in _any_ client, which uses these labels).

Escaping should always be done just before output. So yes, escaping should happen in the clients. All clients. Because the clients know for what target format they need escaping. The API cannot possibly know that.

I assume you already escape the labels in Wikibase frontend code?

Of yourse, just like core escapes page titles when outputting them, etc.

Btw, this problem isn't specific to MF interaction with wikibase. YOu can also use a regular page title for XSS injections: MF makes displaytitle fall back to the plain page title without escaping.

If you can't return safe html in Wikibase, I'll upload a change to escape the label in MobileFrontend (IIRC, Security related tasks needs to upload a git patch to the task, which will be imported into gerrit, after they're approved, right?).

My patch hopefully covers all instances where displaytitle is set to something unsafe in MF. There's actually quite a few places where this happens.

Adding @jernandez and @bmansurov.

daniel renamed this task from XSS: on mobile wikidata has the label as some title which gets rendered as HTML to XSS: MobileFrontend fails to apply escaping to page titles and wikibase labels when using them as fallback for displaytitle.Nov 26 2015, 3:11 PM
daniel updated the task description. (Show Details)
csteipp added a subscriber: MaxSem.Nov 26 2015, 3:24 PM

I can't access the patch for some reason.

@bmansurov: I've added you as a reviewer on the change.

Krenair added a subscriber: Krenair.EditedNov 26 2015, 4:57 PM

I can't access the patch for some reason.

It's a draft, so only those of us in the 'security' group and people explicitly listed as reviewers can see it via gerrit - obviously *anyone* can still see the actual commit itself if they want.

Florian closed this task as Resolved.Nov 27 2015, 3:38 PM
Florian assigned this task to daniel.
Florian removed a project: Patch-For-Review.
Florian changed Security from Software security bug to None.
Krinkle changed the visibility from "Custom Policy" to "All Users".Nov 27 2015, 3:38 PM
Krinkle changed the visibility from "All Users" to "Public (No Login Required)".
Krenair changed the edit policy from "Custom Policy" to "All Users".Nov 27 2015, 5:39 PM
Yedeng reopened this task as Open.Mar 16 2016, 5:49 AM
Yedeng set the point value for this task to 2.
Restricted Application added a subscriber: Malyacko. · View Herald TranscriptMar 16 2016, 5:49 AM
Yedeng changed the point value for this task from 2 to 3.Mar 16 2016, 5:51 AM
Yedeng changed the point value for this task from 3 to 0.Mar 16 2016, 5:54 AM
Yedeng changed the point value for this task from 0 to 1.Mar 16 2016, 5:57 AM
Yedeng changed the point value for this task from 1 to 2.Mar 16 2016, 6:00 AM
Yedeng closed this task as Resolved.Mar 16 2016, 6:05 AM
greg removed the point value for this task.Mar 16 2016, 3:21 PM