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)

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

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.

@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?

In T240487#5738477, @suffusion_of_yellow wrote:

<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.

In T240487#5740035, @nray wrote:

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

line: $headline.text()

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.

@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.

In T240487#5740204, @sbassett wrote:

@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?

<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.

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 !

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.

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.