Page MenuHomePhabricator

Escaping error in old URL format
Closed, ResolvedPublic

Description

I was checking if T65696: Flow: display, behavior, and API failures when displaying a post revision still applies, and it partly does. I'm extracting this part out because there is leakage of an HTML attribute that could lead to an XSS.

See https://www.mediawiki.org/wiki/Topic:Sjbg86fqz4m00em9?topic_postId=sjbg8hzaneecmqks&topic_revId=sjbg8hzaneecmqks

The actual post is just 'C'. It's also a post, not a topic title. See https://www.mediawiki.org/w/index.php?title=Topic:Sjbg86fqz4m00em9&topic_showPostId=sjbg8hzaneecmqks#flow-post-sjbg8hzaneecmqks

Event Timeline

Maniphest changed the visibility from "Public (No Login Required)" to "Custom Policy".Jun 19 2015, 4:50 AM
Maniphest changed the edit policy from "All Users" to "Custom Policy".
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 19 2015, 4:50 AM
Mattflaschen-WMF triaged this task as Unbreak Now! priority.
Mattflaschen-WMF updated the task description. (Show Details)
Mattflaschen-WMF changed Security from None to Software security bug.
Mattflaschen-WMF edited subscribers, added: EBernhardson, Quiddity, Spage and 2 others; removed: Aklapper.
Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptJun 19 2015, 4:50 AM

Yep, looks like <p data-parsoid='{"dsr":[0,1,0,0]}'>C</p> is being dropped unescaped into data-flow-topic-title. Let's get that fixed asap.

There is a Handlebars helper in Flow called escapeContent, which is dangerously misnamed. It doesn't escape anything; instead, it potentially armors its input string against the escaping that Handlebars performs by default. The implementation is basically return format === 'html' ? SafeString(content) : content; .

This would be appropriate for outputting content that could be either HTML or wikitext; in production it doesn't seem like we will ever call this with a non-HTML format parameter, but I think we might call it with format='wikitext' in standalone installs (I haven't checked). However, this is never appropriate to call inside of an attribute value, which is what was happening here. I audited the repo and found no other cases of escapeContent being called in an attribute value.

We should also rename escapeContent to something less misleading and more appropriately scary-sounding.

Patch that fixes the reported issue. Keeping it secret for now while I figure out if XSS is actually possible.

Patch looks good

@mmodell, can you deploy this and confirm here when it's on the cluster?

@mmodell, actually, Roan is going to upload one for wmf10, so the "binary" part works.

wmf10 patch

csteipp added a subscriber: demon.Jun 22 2015, 10:20 PM

@mmodell, we deployed it for wfm10. We'll need to figure out releasing this and getting it into wmf11.

@demon, should we add this Flow patch to the next security release? Or should we just make it public since Flow is technically beta on https://www.mediawiki.org/wiki/Extension:Flow?

@demon, should we add this Flow patch to the next security release? Or should we just make it public since Flow is technically beta on https://www.mediawiki.org/wiki/Extension:Flow?

It's not bundled and it's beta software, I'd say no.

Catrope closed this task as Resolved.Jun 29 2015, 7:37 PM
Catrope changed Security from Software security bug to None.Jun 29 2015, 8:29 PM
Catrope changed the visibility from "Custom Policy" to "Public (No Login Required)".Jun 29 2015, 9:34 PM
Catrope changed the edit policy from "Custom Policy" to "All Users".
Restricted Application added a subscriber: Luke081515. · View Herald TranscriptJan 28 2016, 5:56 PM