Page MenuHomePhabricator
Create Task
Maniphest T120291

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

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

ProjectBranchLines +/-Subject
mediawiki/extensions/Thanksmaster+5 -2Use plaintextParams for topic title escaping
mediawiki/extensions/Flowmaster+12 -11Fix issues with incorrect HTML->plaintext and double escaping
Customize query in gerrit

Related Objects
Search...

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 created this task.Dec 3 2015, 9:55 PM
Ciencia_Al_Poder updated the task description. (Show Details)
Ciencia_Al_Poder added projects: StructuredDiscussions, Notifications, Thanks, acl*security.
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 subscribers: • Mattflaschen-WMF, Legoktm.Dec 3 2015, 10:06 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?

Legoktm added a comment.Dec 3 2015, 11:01 PM

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

Legoktm added a comment.Dec 3 2015, 11:04 PM

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.
In T120291#1850816, @Legoktm wrote:

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 edited projects, added Collaboration-Team-Archive-2015-2016; removed Collaboration-Team-Triage.Dec 3 2015, 11:50 PM
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
Catrope mentioned this in T120324: XSS in Flow topic titles.Dec 3 2015, 11:58 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
Catrope edited projects, added Collaboration-Team-Triage; removed Collaboration-Team-Archive-2015-2016, acl*security.
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
Jdforrester-WMF added a subscriber: Jdforrester-WMF.Dec 4 2015, 12:09 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)
• Mattflaschen-WMF added a comment.Jan 22 2016, 9:38 PM
In T120291#1850995, @Catrope wrote:

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.

• Mattflaschen-WMF added a comment.Jan 28 2016, 7:46 PM

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.

gerritbot added a subscriber: gerritbot.Feb 8 2016, 5:48 PM

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

gerritbot added a project: Patch-For-Review.Feb 8 2016, 5:48 PM

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

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

gerritbot added a comment.Feb 10 2016, 12:21 AM

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

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

gerritbot added a comment.Feb 13 2016, 1:18 AM

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

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

SBisson added a comment.Feb 16 2016, 4:25 PM

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

Catrope added a comment.Feb 16 2016, 5:57 PM
In T120291#2031756, @SBisson wrote:

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.

Etonkovidova mentioned this in T128062: Keep notification excerpts compact and meaningful.Mar 2 2016, 11:42 PM

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
Content licensed under Creative Commons Attribution-ShareAlike 3.0 (CC-BY-SA) unless otherwise noted; code licensed under GNU General Public License (GPL) or other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL