Page MenuHomePhabricator

wikitext in flow titles is parsed (HTML tags like <tt> and templates) on echo notifications
Closed, ResolvedPublic

Description

I'm preemptively adding this to security since I won't try a possible XSS on a public wiki, in case someone else notices that.

A <tt> tag from the topic title was leaked in an echo notification of this flow post (a thanks action).

tt is allowed in wikitext. I'm not sure if something like a <script> could be leaked too, which could be disastrous. Confirmed: It's not an issue.

It also parses templates, see T123543

Earlier testing by @Etonkovidova. All of them should be plain text:

Tested on test.wikipedia.org

dataFlow topic titlesEcho flyoutNotification page
<code></code>plain textas code textas code text
<script>alert(1)</script>plain textplain textplain text
<tt>plain textis not displayedmonospace; changes all text to monospace
{{release}}plain textred linkred link
<!--T:1-->plain textplain textplain text
<translate>plain textplain textplain text

Details

Related Gerrit Patches:
mediawiki/extensions/Thanks : masterUse plaintextParams for topic title escaping
mediawiki/extensions/Flow : masterFix issues with incorrect HTML->plaintext and double escaping

Event Timeline

Maniphest changed the visibility from "Public (No Login Required)" to "Custom Policy".Dec 3 2015, 9:55 PM
Maniphest changed the edit policy from "All Users" to "Custom Policy".
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 3 2015, 9:55 PM
Ciencia_Al_Poder updated the task description. (Show Details)
Ciencia_Al_Poder changed Security from None to Software security bug.
Ciencia_Al_Poder edited subscribers, added: Ciencia_Al_Poder; removed: Aklapper.
Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptDec 3 2015, 9:55 PM
csteipp added a subscriber: csteipp.

@mattflaschen / @Legoktm, I don't have flow and echo setup in a dev environment. Are one of you able to confirm this?

I wasn't able to trigger an XSS in the Echo flyout, but I was in Flow...

Note that I'm not using Parsoid locally, which might affect this?

The code flow for Echo is $msg->params( $topicTitle )->parse(), so <tt> is accepted, but <script> should not be.

Catrope claimed this task.Dec 3 2015, 11:29 PM
Catrope added a subscriber: Catrope.

I wasn't able to trigger an XSS in the Echo flyout, but I was in Flow...

Chris wasn't able to reproduce this on testwiki, but I was able to reproduce it locally and on beta. That means it's a regression in master after wmf7. I'll bisect.

Catrope changed Security from Software security bug to None.
Catrope changed the edit policy from "Custom Policy" to "All Users".
Catrope changed the visibility from "Custom Policy" to "Public (No Login Required)".Dec 3 2015, 11:52 PM
Restricted Application added a subscriber: StudiesWorld. · View Herald TranscriptDec 3 2015, 11:52 PM

I've split off the XSS bug into T120324: XSS in Flow topic titles. This bug isn't a security bug, and it's nontrivial to fix. Echo messages are parsed as wikitext because some of them do use wikitext (we are getting rid of links in notifications, but some of them still use bold), while we also inject page titles, topic titles and summaries etc into these messages as parameters. But we don't really have a good way to escape them. I suppose we could use wfEscapeWikitext(), or otherwise make Echo messages plain text only.

Catrope triaged this task as Medium priority.Dec 4 2015, 12:00 AM
Legoktm renamed this task from HTML tags in flow titles are leaked on echo notifications to wikitext in flow titles is parsed (e.g. <tt>) on echo notifications.Dec 4 2015, 12:07 AM

I'm not against downgrading Notification text to plaintext-only.

Ciencia_Al_Poder renamed this task from wikitext in flow titles is parsed (e.g. <tt>) on echo notifications to wikitext in flow titles is parsed (HTML tags like <tt> and templates) on echo notifications.Jan 15 2016, 8:05 PM
Ciencia_Al_Poder updated the task description. (Show Details)

I've split off the XSS bug into T120324: XSS in Flow topic titles. This bug isn't a security bug, and it's nontrivial to fix. Echo messages are parsed as wikitext because some of them do use wikitext (we are getting rid of links in notifications, but some of them still use bold), while we also inject page titles, topic titles and summaries etc into these messages as parameters. But we don't really have a good way to escape them. I suppose we could use wfEscapeWikitext(), or otherwise make Echo messages plain text only.

I think this is exactly why Erik added plaintextParams.

In conjunction with this, I'm planning to implement topic-title-plaintext to reduce redundant boilerplate code here.

It will be equivalent to htmlToPlaintext on topic-title-html.

Change 269171 had a related patch set uploaded (by Mattflaschen):
Fix issues with incorrect HTML->plaintext and double escaping

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

Change 269172 had a related patch set uploaded (by Mattflaschen):
Use plaintextParams for topic title escaping

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

Change 269171 merged by jenkins-bot:
Fix issues with incorrect HTML->plaintext and double escaping

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

Change 269172 merged by jenkins-bot:
Use plaintextParams for topic title escaping

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

Can this be the same problem but for the excerpt: T127079 ?

Can this be the same problem but for the excerpt: T127079 ?

Yes, that's probably a similar issue. A <pre> could arise from a wikitext line starting with a space.

Re-checked in betalabs after the fix - the cases for <tt>, {{release}} , and <pre>(per T127079: Ugly unexpected <pre> in Notifications), a single quotation mark (per T127145: A straight single quotation mark in the title was percent-encoded upon saving) :

Flow topic titlesEcho flyoutNotification page
<code></code>plain textplain textplain text
<tt>plain textplain textplain text
{{release}}plain textplain textplain text
<pre>plain textplain textplain text
single straight quotation markplain textplain textplain text

Also checked all cases mentioned in this ticket for text excerpts for T128062: Keep notification excerpts compact and meaningful.

jmatazzoni closed this task as Resolved.Mar 3 2016, 7:07 PM