Page MenuHomePhabricator

Harden VisualEditor CE/UI against malicious data in DM
Open, Needs TriagePublic

Description

VisualEditor content-editable surface and user interface currently assume that the data model doesn't contain anything malicious, and display DOM nodes (either stored directly or generated from the data) without any sanitization or validation.

This is normally fine, because we sanitize or validate before we put anything in the data model – for example, <script> nodes are removed from pasted HTML, javascript: URLs are disallowed when adding external links, etc.

However, it's possible to put malicious things in the model using low-level APIs (accessible to gadgets etc., or from a browser console), for example:

var data = { 'type': 'link/mwExternal', attributes: { href: 'javascript:alert()' } };
ve.init.target.surface.model.getFragment().insertContent( 'test' ).annotateContent( 'set', 'link/mwExternal', data );

…and have it rendered by the CE and UI:

image.png (2×3 px, 546 KB)

This is not a security problem yet (it's just a roundabout way of doing self-XSS), but our ideas for collaborative editing involve synchronizing the data model between the users sharing an edit session, which would make anyone joining it vulnerable to XSS attacks.

Current code has sanitization for DOM nodes stored directly in the DM (see ve.dm.Change.static.deserializeValue), but not for plain data like in the example above.

We discussed this and it seems more practical to make CE/UI sanitize everything they display than to add validation at the DM level.

Event Timeline

If you actually try to click the link, in Chrome it takes you to about:blank#blocked and in Firefox it refuses to render the link, so it looks like browsers are already doing something clever.

Change 858596 had a related patch set uploaded (by Esanders; author: Esanders):

[VisualEditor/VisualEditor@master] Sanitize href attribute in LinkContextItem and LinkAnnotation

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

The following CE nodes set hrefs from the model and should be protected:

  • ve.ce.LinkAnnotation (see patch above)
  • ve.ce.MWBlockImageNode
  • ve.ce.MWInlineImageNode
  • ve.ce.MWMagicLinkNode
  • ve.ce.MWNumberedExternalLinkNode

Change 858596 merged by jenkins-bot:

[VisualEditor/VisualEditor@master] Sanitize href attribute in LinkContextItem and LinkAnnotation

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

Change 860550 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (099b95023)

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

Change 860559 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/extensions/VisualEditor@master] Always sanitize href attribute in CE nodes

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

In the UI

  • ve.ui.LinkContextItem (see patch above)
  • ve.ui.MWInternalLinkContextItem (might be safe because it only generates internal links, but doesn't hurt to add a guard)
  • ve.ui.MWMediaContextItem (as above)
  • ve.ui.MWGalleryDialog (as above)
  • ve.ui.MWMediaInfoFieldWidget (I think the values come from the API, but again better to be safe) <a> tag is created for extracting plain text, but never rendered.

Change 860550 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (099b95023)

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

Change 860561 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/extensions/VisualEditor@master] Always sanitize href attribute in UI

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

Change 860559 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Always sanitize href attribute in CE nodes

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

Change 860561 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Always sanitize href attribute in UI

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

Change #1193185 had a related patch set uploaded (by Jforrester; author: Esanders):

[VisualEditor/VisualEditor@REL1_39] Sanitize href attribute in LinkContextItem and LinkAnnotation

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

Change #1193186 had a related patch set uploaded (by Jforrester; author: Esanders):

[mediawiki/extensions/VisualEditor@REL1_39] Always sanitize href attribute in UI

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

Change #1193187 had a related patch set uploaded (by Jforrester; author: Esanders):

[mediawiki/extensions/VisualEditor@REL1_39] Always sanitize href attribute in CE nodes

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

Change #1193185 merged by jenkins-bot:

[VisualEditor/VisualEditor@REL1_39] Sanitize href attribute in LinkContextItem and LinkAnnotation

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

Change #1193191 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/extensions/VisualEditor@REL1_39] Update VE core submodule to REL1_39

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

Change #1193191 merged by Jforrester:

[mediawiki/extensions/VisualEditor@REL1_39] Update VE core submodule to REL1_39

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

Change #1193187 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@REL1_39] Always sanitize href attribute in CE nodes

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

Change #1193186 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@REL1_39] Always sanitize href attribute in UI

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

Change #1193384 had a related patch set uploaded (by Esanders; author: Esanders):

[VisualEditor/VisualEditor@master] Create local linting rule for setting user input href's

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

Change #1193392 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/extensions/VisualEditor@master] Enforce sanitized href

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

Change #1193384 merged by jenkins-bot:

[VisualEditor/VisualEditor@master] Create local linting rule for setting user input href's

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

Change #1197667 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (6caeeb2e4)

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

Change #1197667 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (bd2578ef5)

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

Change #1193392 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Enforce sanitized href

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