Page MenuHomePhabricator

XSS in MinervaNeue skin (CVE-2019-19910)
Closed, ResolvedPublic

Description

  1. Create a talk page with this content:
== foo<img src="http://eff.org/we_have_your_ip.jpg" onmouseover='alert("XSS")'> ==

bar
  1. Switch to the mobile view
  2. Click on the section heading, now listed under "Active discussions"
  3. Note, in your browser console, the request to eff.org.
  4. Now mouse-over the little broken image next to "foo" at the top.

Details

Related Gerrit Patches:

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 11 2019, 6:39 PM

User-agent Mozilla/5.0 (X11; Linux x86_64; rv:71.0) Gecko/20100101 Firefox/71.0
MediaWiki 1.35.0-alpha (fdfa0e9)
MobileFrontend 2.1.0 (a812ef8)

This comment was removed by suffusion_of_yellow.
Urbanecm triaged this task as High priority.Dec 11 2019, 9:45 PM
Urbanecm added a project: Vuln-XSS.
Urbanecm added a subscriber: Florian.
nray added a comment.Dec 13 2019, 1:18 AM

Thanks for brining this up. I believe this has impacted 3rd party wikis only as our wmf-config has the following config:

// T152540
'wgFragmentMode' => [
	'default' => [ 'html5', 'legacy' ],
]

which when set, prevents the Overlay from even opening when the section is clicked. Regardless, I'm making a fix for it

nray added a comment.Dec 13 2019, 1:49 AM

I spoke too soon. I think this could impact production on certain browsers where the overlay is able to open. There is currently a bug T238364 that prevents the overlay from opening in certain browsers with certain characters. Given that the characters > and < are relevant here, that ticket shows that safari 13.0.3 and iOS Safari 12.0 (probably others) are able to open the overlay and could be impacted.

How do I find out if an XSS impacts production? I don't want to save anything like that on testwiki, even if I delete it one minute later.
Anyhoo, this page works for me even with $wgFragmentMode = [ 'html5', 'legacy' ];:

<h2><span class="section-heading"><span class="mw-headline"><img src="blah.jpg" onmouseover="alert('XSS')"></span></span><span class="mf-section-1">blah</span></h2>

In fact, there's no need to click on the section header anymore. Just open the talk page in MF, and the section will open on its own.

nray added subscribers: sbassett, Reedy.EditedDec 13 2019, 6:33 PM

@Reedy @sbassett I think this does unfortunately have an impact on production. I think it occurs because of the following line which unescapes escaped html:

https://github.com/wikimedia/mediawiki-skins-MinervaNeue/blob/master/resources/skins.minerva.scripts/talk.js#L65

				line: $headline.text()

That unescaped html is then passed to the Section class which outputs the raw (unescaped) html when the overlay is open. It's my code and my fault.

I'm eager to get this fixed, but am wondering what the next steps are?

<h2><span class="section-heading"><span class="mw-headline"><img src="blah.jpg" onmouseover="alert('XSS')"></span></span><span class="mf-section-1">blah</span></h2>

This one definitely works on production wikis. The first one (from the task description) I had trouble reproducing. Not sure if I'm missing steps or anything but I was never able to see a broken image as I did with the second one, testing in Chrome and Firefox. Regardless, there is an XSS here which needs to be fixed.

How do I find out if an XSS impacts production? I don't want to save anything like that on testwiki, even if I delete it one minute later.

I'm not sure it's possible to avoid this scenario sometimes, unless one can build out a development environment that closely mirrors Wikimedia production, which from what I've seen is fairly difficult to do. The current MW dev vagrant and dockers aren't really a good mirror of production AIUI, and building out one's own development environment can be tedious. It's a good default to try to find and test vulnerabilities like this locally, but again, sometimes that's just incredibly inconvenient or even impossible and so discreet testing somewhere like the testwikis becomes the only viable option to fixing these vulnerabilities.

So this would be an issue with MinervaNeue, not MobileFrontend then? Or is the usage of text() here pervasive across every skin as executed under MobileFrontend? Anyhow, the next step would be getting a patch written and posted on this task (not gerrit) so that we can deploy it as a security patch to production. If we can get that done today, I can deploy it this afternoon. I'm not sure if it's as simple as changing this line to use .html() as $heading.next() does on the following line or if more advanced sanitization method (using something like [[ https://doc.wikimedia.org/mediawiki-core/master/js/#!/api/mw.html-method-escape | mw.html.escape() ]]) is necessary.

Jdlrobson edited projects, added MinervaNeue; removed MobileFrontend.EditedDec 13 2019, 7:14 PM

@sbassett https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/556843/ can be swatted to fix the issue. Are you able to take care of this?

Note: the patch got accidentally posted to Gerrit yesterday so we'll need to get this to production ASAP

@Jdlrobson - I can sec-deploy it sooner, and probably should. Can we manually V+2 to override that failing mwselenium-quibble-docker test? I'm not really sure why that's complaining or if it's related to this. Also, did you get a chance to test this locally? I'm not 100% certain that using .html() completely fixes this issue...

Well, ok, the mwselenium-quibble-docker tests passed this time and the patch merged. I'll pick to wmf.10 and deploy now.

It's a good default to try to find and test vulnerabilities like this locally, but again, sometimes that's just incredibly inconvenient or even impossible and so discreet testing somewhere like the testwikis becomes the only viable option to fixing these vulnerabilities.

Thanks. I deleted your sandbox on testwiki, to at least hide it as much as possible. Admins can already exploit T240502 anyway.

So this would be an issue with MinervaNeue, not MobileFrontend then?

I think part of the problem is bd1b16ff18bc; before that it looks like section.line was being escaped by mustache.js.

@Jdlrobson - I can sec-deploy it sooner, and probably should. Can we manually V+2 to override that failing mwselenium-quibble-docker test? I'm not really sure why that's complaining or if it's related to this. Also, did you get a chance to test this locally? I'm not 100% certain that using .html() completely fixes this issue...

<div id="Town_development">&lt;foo&gt;</div>

> $element = $('#Town_development');

> $element.html()
"&lt;foo&gt;"

> $element.text()
"<foo>"

The talk overlay expects HTML for overlay headers and treats them as HTML so what ever is passed to it is converted to HTML. So in latter the text is valid HTML.

Basically we were doing

$('<div>').html( $element.text() )

Now

$('<div>').html( $element.html() )

Can you foresee other types of attack?

sbassett assigned this task to nray.EditedDec 13 2019, 8:16 PM

Update: the gerrit patch (557097) was deployed to wmf.10. I tested on testwiki again (deleted & suppressed the edits this time) and the XSS appears to be mitigated. Thanks all for reporting and getting this fixed. The next steps would be to backport to supported release versions, request a CVE and make this task public, though that can all wait until next week. And maybe investigate why mwselenium-quibble-docker appears to be so flaky.

Thanks. I deleted your sandbox on testwiki, to at least hide it as much as possible. Admins can already exploit T240502 anyway.

Thanks.

Basically we were doing

$('<div>').html( $element.text() )

Now

$('<div>').html( $element.html() )

Can you foresee other types of attack?

Not at the moment, no, though I might play around with it some more. Thanks for confirming.

nray added a comment.EditedDec 13 2019, 8:27 PM
<h2><span class="section-heading"><span class="mw-headline"><img src="blah.jpg" onmouseover="alert('XSS')"></span></span><span class="mf-section-1">blah</span></h2>

In regards to the above example where the overlay opens automatically on page load, I think the is due to that .section-heading element which the code interprets as a valid section and since its child .mw-headline element doesn't have a id on it, the router opens the overlay thinking it matches a route without a hash.

Edit: Still working through how to fix that

@nray - If we're just talking about preventing the overlay from automatically loading, I think that could be done publicly through gerrit. At most that sounds like a code-hardening measure, IMO. Probably warrants a separate, public task as well.

nray added a comment.EditedDec 13 2019, 8:50 PM

thanks @sbassett . Yes, I think that's more of a bug than related to the xss vector. I'll open a separate ticket for that. I appreciate all of your help along with @suffusion_of_yellow !

sbassett closed this task as Resolved.Dec 16 2019, 10:42 PM
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".

Ok, so 556843 doesn't want to pick cleanly to any supported release branches in gerrit, so I'll try to manually merge them, if possible.

sbassett moved this task from Backlog to In Progress on the user-sbassett board.
sbassett added a comment.EditedDec 17 2019, 10:02 PM

Ok, looks like the nefarious $headline.text() call in the skin was introduced here, so around November 5th, 2019. Obviously that won't affect anything from before 1.34 and 1.34 doesn't seem to be in a similar enough state as master (164 commits behind?) for the affected file. Just out of curiosity, I did have a glance at a bit of the history of resources/skins.minerva.scripts/talk.js and resources/skins.minerva.talk/init.js and I didn't see anything that looked suspicious within a text() vs html() context, so I think we can call this done with the merge to master and wmf.10.

sbassett moved this task from In Progress to Done on the user-sbassett board.Dec 17 2019, 10:02 PM
sbassett renamed this task from XSS in MobileFrontend to XSS in MinervaNeue skin.Dec 19 2019, 5:53 PM
Restricted Application added a subscriber: Masumrezarock100. · View Herald TranscriptDec 19 2019, 5:53 PM
sbassett renamed this task from XSS in MinervaNeue skin to XSS in MinervaNeue skin (CVE-2019-19910).Dec 19 2019, 6:42 PM