Page MenuHomePhabricator

Self-XSS in VisualEditor link context menu
Closed, ResolvedPublic

Description

Steps to reproduce:

  1. Open VisualEditor
  2. Create an internal link with "Javascript:alert('XSS!')" as the target
  3. Place cursor inside the link
  4. Click on the link preview in the context menu

Note that this issue does not occur when the link already exists on the page when you open it for editing. Such link correctly links to a page titled "Javascript:alert('XSS!')". It only occurs for links you place yourself while editing. As far as I can tell, there is no "real" XSS here, but possibly it could be abused by a creative attacker.

Note also that this is an obvious extension of this publicly reported issue: T206231: Link in link popup has incomplete target URL if page contains namespace.

Event Timeline

matmarex created this task.Oct 5 2018, 7:09 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 5 2018, 7:10 PM
Bawolff added a subscriber: Bawolff.EditedOct 5 2018, 7:13 PM

What about in flow where posts are saved as html, instead of serializing between wikitext & back again?

Edit: I tested, flow has same issue, but is not a stored XSS

I'm not sure what you mean. I don't think that exists yet? When such a thing exists, I would expect it to have a sanitization step which would catch this.

This patch fixes the issue for me, and it doesn't seem to cause any extra spurious './' in resulting wikitext:

In general, the way we handle 'hrefPrefix' is confusing to me. If I remember correctly, extra href prefixes used to be used by Parsoid in the HTML of subpages, so that a link to "C" on a page called "A/B" would actually link to "C" (../C) rather than "A/C" (./C). But it seems that today's Parsoid always outputs links prefixed with ./ and instead uses a <base href="http://…/wiki/"> for links on subpages to point to the right place. Possibly we can remove a lot of that code to make it easier to spot issues like this?

matmarex added subscribers: Esanders, DLynch.

(Somehow it seems that Editing engineers who could review this patch are not in the Security group, so CC-ing some folks)

We use the DOMPurify library in situations where HTML might be shared between clients without being converted to wikitext, although at the moment that is only the experimental collaborative editor.

Note that DomPurify by itself isnt generally sufficient as we also have the goal of preventing leaking IP addresses so anything that could trigger an http request (e.g. images) needs to be filtered too

Patch looks good BTW

@Bawolff how would we go about filtering external images in the client?

matmarex merged a task: Restricted Task.Oct 9 2018, 7:30 PM
matmarex added subscribers: Tgr, GWicke, Deskana.

In general, the way we handle 'hrefPrefix' is confusing to me. If I remember correctly, extra href prefixes used to be used by Parsoid in the HTML of subpages, so that a link to "C" on a page called "A/B" would actually link to "C" (../C) rather than "A/C" (./C). But it seems that today's Parsoid always outputs links prefixed with ./ and instead uses a <base href="http://…/wiki/"> for links on subpages to point to the right place. Possibly we can remove a lot of that code to make it easier to spot issues like this?

For reference, Parsoid stopped generating them in 2014 as a result of T72743, and it ignored them in input as well. We should remove everything dealing with hrefPrefix and hardcode ./ everywhere.

Esanders edited subscribers, added: ssastry; removed: GWicke.Oct 10 2018, 11:35 AM
matmarex claimed this task.Jan 27 2019, 4:04 AM

Yet another duplicate has been filed publicly: T214610.

Unless someone has a plan to deploy this patch, I'm going to submit it publicly to Gerrit after All Hands and let it ride the train.

This has been merged and deployed (and fixed) for a long time now, but it has been lingering on the VisualEditor workboard. I think this is because some folks on the Editing team don't have the access to view security tasks (they should get it, but never asked for it) and so no one pushed it along. I'm going to make it public so that we can close it per our usual process.

matmarex changed the visibility from "Custom Policy" to "Public (No Login Required)".Mar 13 2019, 8:08 PM

Change 501040 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] Remove support for page title "hrefPrefix" other than './'

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

Change 501041 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/Cite@master] Remove "hrefPrefix" from tests

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

This is the followup/hardening change I proposed here earlier.

Change 501041 merged by Jforrester:
[mediawiki/extensions/Cite@master] Remove "hrefPrefix" from tests

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

Change 501040 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Remove support for page title "hrefPrefix" other than './'

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

ppelberg closed this task as Resolved.Apr 11 2019, 2:12 AM

Change 517907 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@REL1_33] Remove support for page title "hrefPrefix" other than './'

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

Change 517907 merged by Krinkle:
[mediawiki/extensions/VisualEditor@REL1_33] Remove support for page title "hrefPrefix" other than './'

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