Closed Bug 1602843 (CVE-2019-17022) Opened 5 years ago Closed 5 years ago

Mutation XSS via copy-and-paste in WYSIWYG editors

Categories

(Core :: DOM: Serializers, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla73
Tracking Status
firefox-esr68 72+ verified
firefox71 --- wontfix
firefox72 --- verified
firefox73 --- verified

People

(Reporter: michal.bentkowski, Assigned: emilio)

References

Details

(Keywords: reporter-external, sec-moderate, Whiteboard: [reporter-external] [client-bounty-form] [verif?][adv-main72+][adv-esr68.4+])

Attachments

(4 files, 1 obsolete file)

I've been recently analyzing copying and pasting in browsers and found a bug similar to bug 1599181, but this one allows a mutation XSS on pasting. The webpage needs to follow some specific pattern to be vulnerable but I could find some quite easily.

The root of the issue is exactly the same as it was in bug 1599181, that is serializing of CSS. To recap, if pasted content contains <style> tag, Firefox decides to sanitize it. The stylesheet is pasted verbatim if it doesn't contain any "dangerous" rules (like @import), but it is rewritten if it does. Consider the following example:

<style>
@import''; 
@font-face { font-family: 'ab<\/style><img src onerror=alert(1)>'}
</style>

Because of @import, after pasting, the code would be sanitized to:

<style>
@font-face { font-family: "ab</style><img src onerror=alert(1)>"; }
</style>

Please note how <\/style> was transformed to </style>. This doesn't introduce any issue by itself because in the case above, the text node of the stylesheet is modified directly, hence the </style> inside doesn't close the tag. However, if the application does something akin to:

textEditor.innerHTML = clipboardData.innerHTML;

then it is vulnerable to XSS, because the <img> tag would leave the <style> tag. It was my assumption that certain applications might do that and by quick googling I found two WYSIWYG editors who do exactly this. So perhaps there are even more.

Proof of Concept

  1. Go to https://jsbin.com/xivapasere/1/edit?html,output.
  2. Press copy me.
  3. Go to https://rawgit.com/alohaeditor/Aloha-Editor/hotfix/src/demo/boilerplate/ (one of the editors that's vulnerable to this mutation XSS).
  4. Give Aloha Editor a while to load...
  5. Go to any contenteditable content.
  6. Paste from clipboard.
  7. Alert fires!

I don't consider it a bug in Aloha Editor, more of a bug in Firefox itself. To understand why alert fired, have a look at https://rawgit.com/alohaeditor/Aloha-Editor/hotfix/src/plugins/common/paste/lib/paste-plugin.js - line 274. What the editor does is that it paste the content in an invisible <div> (called $clipboard or $CLIPBOARD in the code).

Then, using jQuery, the code is parsed as HTML:

			if (typeof content === 'string') {
				$content = $('<div>' + content + '</div>');
			} else if (content instanceof $) {
				$content = $('<div>').append(content);
			}

In the PoC content is equal to "<style>@font-face { font-family: \"ab</style><img src onerror=alert(1)>\"; }</style>", leading to an obvious XSS issue.

I think that perhaps Firefox sanitizer could always escape < to \3c in CSS strings to get rid of the issue?

For the record, I've also reported certain copy-and-paste bugs to Chromium and they decided to go a different way altogether and they just delete <style> tag from pasted content by serializing it into inline style attributes. I think that it significantly reduces the risk. Safari also does the same.

Here's the approach pasted from Chromium patch:

This patch follows the same approach as in WebKit [1]:
- First create a dummy document to insert the markup
- Then computes style and layout in the dummy document
- Re-serialize the dummy document as the markup to be inserted. This
  reuses the code path that we serialize a selection range into
  clipboard, where we need to serialize element computed style into
  inline styles so that the element styles are preserved.
- Make sure all style elements are removed before inserting markup
  into document
Flags: sec-bounty?

Sorry, it seems that incorrect code was saved in JSBin. Check the following link when reproducing the PoC:

Emilio, given you've looked at bug 1599181, want to take a look here?

Group: firefox-core-security → layout-core-security
Type: task → defect
Component: Security → CSS Parsing and Computation
Flags: needinfo?(emilio)
Product: Firefox → Core
See Also: → CVE-2019-17016

Hmm, in this case the string is perfectly valid CSS, so I think the bug here is in the DOM, which doesn't escape the resulting serialization as HTML... Or something of that sort.

I'm happy to take a look though.

Assignee: nobody → emilio

This is indeed a DOM issue in certain way. Here's a snippet from HTML5 spec:

This can enable cross-site scripting attacks. An example of this would be a page that lets the user enter some font family names that are then inserted into a CSS style block via the DOM and which then uses the innerHTML IDL attribute to get the HTML serialization of that style element: if the user enters "</style><script>attack</script>" as a font family name, innerHTML will return markup that, if parsed in a different context, would contain a script node, even though no script node existed in the original DOM.

I think that one of two solutions could be employed here:

  1. Change < to \3c. That wouldn't change anything in the content of the string, while would protect against the attack.
  2. Detect that <style> contains </style> in its textContent and react somehow (like delete that element).

Change < to \3c. That wouldn't change anything in the content of the string, while would protect against the attack.

Hmm, this is a bit tricky as we can do this only on strings, I suspect...

Detect that <style> contains </style> in its textContent and react somehow (like delete that element).

Hmm, that can work. it's a bit ugly because there are probably various edge cases involved (thought I don't know all the HTML tokenization rules).

Another thing that may work to prevent this kind of attack is just doing something like re-sanitizing until we reach a point where we don't need to sanitize. That seems less expensive than creating a new document and doing layout on it, as most of the time we'll stop real soon and do no extra work...

Henri, do you have any preference? You have way more background than me on this kind of stuff.

Flags: needinfo?(hsivonen)

Oh, of course you could always go the Chrome and Safari way and delete <style> tags and re-insert them as inline style attributes. I feel that from purely security perspective this is the best approach (as it also protects against data exfiltration attacks).

(In reply to Michał Bentkowski from comment #6)

Oh, of course you could always go the Chrome and Safari way and delete <style> tags and re-insert them as inline style attributes. I feel that from purely security perspective this is the best approach (as it also protects against data exfiltration attacks).

It is also a regression in functionality though, as there's stuff you can't just stash in a style attribute, like @media, or what not... But sure, we could consider that as well.

Anyhow the bug is clearly not in the style system itself, but in how editor and the sanitizer uses it...

In fairnes, I think this is mostly an upstream library bug, but I'm simpathetic to the argument in comment 0... The fact that innerHTML doesn't produce output that roundtrips is quite nasty IMO, but probably too late to change that?

Component: CSS Parsing and Computation → DOM: Editor

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

(In reply to Michał Bentkowski from comment #6)

Oh, of course you could always go the Chrome and Safari way and delete <style> tags and re-insert them as inline style attributes. I feel that from purely security perspective this is the best approach (as it also protects against data exfiltration attacks).

It is also a regression in functionality though, as there's stuff you can't just stash in a style attribute, like @media, or what not... But sure, we could consider that as well.

The question is whether we want to let users put @media in pasted content at all. Security issues aside, if you put the following HTML in the clipboard:

<style>
  * { background: red}
</style>

Then the whole page gets a red background, not only the pasted content, which I don't think is desirable.

Henri, do you have any preference?

My preference would be to escape < in CSS string literals in the CSS serializer if we change and reserialize the CSS.

Flags: needinfo?(hsivonen)

I know this is a lot of work, but if we fix these whack-a-mole style, Michał will just continue reporting more of these :-)
It just sounds like some of the things that will be able to cause problems when websites (for example gmail :)) start relying on Chrome's behavior.

We should really consider alignment and interop with other browsers here. Not just for security's sake.

We should really consider alignment and interop with other browsers here. Not just for security's sake.

Well, I don't know. We do have some leeway here, but "just align with other browsers" is not particularly trivial.

Just from a quick look, Chrome uses a 800x600 thing for media queries, and they have various really hacky fixups like this...

Their whole serialization strategy seems pretty different from ours.

(In reply to Henri Sivonen (:hsivonen) from comment #10)

Henri, do you have any preference?

My preference would be to escape < in CSS string literals in the CSS serializer if we change and reserialize the CSS.

This is not particularly easy because I'd need to propagate the "is this for the sanitizer" bit to every CSS string...

I think I can push stylesheet sanitization further down and preserve the input on a per-CSS-rule basis without too much trouble.

Also, I think right now we may trigger @import stylesheet loads in the target page which is pretty bad. We should reject them while parsing.

This should work, but I need to fix the mOnlyConditionalCSS mode which was just
introduced for Thunderbird... :/

(In reply to Emilio Cobos Álvarez (:emilio) from comment #15)

This should work, but I need to fix the mOnlyConditionalCSS mode which was just
introduced for Thunderbird... :/

(And which is totally broken in various ways...)

Flags: needinfo?(emilio)
Flags: needinfo?(emilio)

So, ok... I think for now I'll go with this approach after fixing that other sanitizer feature, and I'll file a follow-up bug to investigate removing all non-inline styles from the markup for paste (probably on Nightly-only, for now).

I agree it's a bit bizarre to allow arbitrary styles that can affect the whole page...

WebKit and Blink seem to have this sort of complex system to try to preserve copy&paste fidelity, which involves poking at the computed style of an element and trying to extract some of its properties and setting them as inline styles.

There's no fancy thing like that in Gecko, but I'm on the fence between horrifying (after having looked at WebKit's code) and neat.

Masayuki / Makoto, do you know if we have a bug tracking copy&paste fidelity improvements?

Anyhow, I think that should be a sensible plan for the time being, wdyt Freddy?

Flags: needinfo?(masayuki)
Flags: needinfo?(m_kato)
Flags: needinfo?(fbraun)
Attachment #9115174 - Attachment description: Bug 1602843 - WIP: Preserve rule input exactly after sanitization. → Bug 1602843 - Preserve CSS input exactly during sanitization. r=hsivonen

Ok, I sent a patch which I think is what we should take for now. Then if the plan above sounds good I'll work on adding the extra mode to the sanitizer to drop <style> altogether.

Flags: needinfo?(emilio)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #17)

Masayuki / Makoto, do you know if we have a bug tracking copy&paste fidelity improvements?

I don't know. Is DOM:serialize better bug? You know, Editor uses nsTreeSanitizer...

Flags: needinfo?(m_kato)

Yeah, perhaps...

Tom, can I have a sec-rating for this? I'm not sure it meets the bar for sec-high but your call.

Component: DOM: Editor → DOM: Serializers
Flags: needinfo?(tom)

This does seem to fall somewhere between a moderate and a high. I'm inclined to round down right now, while there are certainly many affected sites, it does not seem like Aloha Editor is very pervasive or high-value like the targets in the prior bug....

Flags: needinfo?(tom)
Keywords: sec-moderate

Comment on attachment 9115174 [details]
Bug 1602843 - Preserve CSS input exactly during sanitization. r=hsivonen

Beta/Release Uplift Approval Request

  • User impact if declined: See comment 0.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: comment 0 (but note the right jsfiddle url in the comments below)
  • List of other uplifts needed: none
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Relatively low. Rewrites the CSS Sanitization to preserve input and prevent the mutation attack.
  • String changes made/needed: none

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: relatively-safe sec-moderate to address.
  • User impact if declined: see above
  • Fix Landed on Version:
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Relatively low as per above, but I understand the bar for ESR may be higher. Your call whether this is worth uplifting to ESR.
  • String or UUID changes made by this patch: none
Attachment #9115174 - Flags: approval-mozilla-esr68?
Attachment #9115174 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Group: layout-core-security → core-security-release
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73

Oh, sorry, I forgot to reply this.

As far as I know, there is no meta bug tracking copy-paste of HTML styles, but I found bug 406488. I don't like the Blink/WebKit's behavior which creating a lot of <span> elements and setting style attribute to a lot of inline styles, but I guess it fixes the bug.

Flags: needinfo?(masayuki)

Comment on attachment 9115174 [details]
Bug 1602843 - Preserve CSS input exactly during sanitization. r=hsivonen

approved for 72.0b8

Attachment #9115174 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

This needs a rebased patch for Beta and ESR68.

Flags: needinfo?(emilio)

Beta patch.

Flags: needinfo?(emilio)
QA Whiteboard: [qa-triaged]
Attachment #9116163 - Attachment filename: 0001-Bug-1602843-Preserve-CSS-input-exactly-during-saniti.patch → beta.patch
Attached patch esr68.patch (obsolete) — Splinter Review

Two notes:

  • I had to keep the old code-path for when XBL is enabled (so xbl-in-content pref or chrome docs).
  • I had to update cssparser. So we'd be uplifting bug 1563892 too, but that seems fine.
Attached file esr68.patch

(Missed an if (mLogRemovals) around the LogMessage call.)

Attachment #9116382 - Attachment is obsolete: true

I have reproduced the issue by copying the code with the "copy me" button from this (https://jsbin.com/xivapasere/2/edit?html,output) test page and then paste it anywhere on this (https://rawgit.com/alohaeditor/Aloha-Editor/hotfix/src/demo/boilerplate/) test page. A notification with the text "1" is displayed. Reproduced on Nightly v7.0a1 from 2019-12-12 and Beta v72.0b7.

Verified in Nightly v73.0a1 from 2019-12-16. Waiting for Beta build to verify.

Flags: needinfo?(daniel.bodea)
Comment on attachment 9116383 [details] esr68.patch Approved for 68.4esr.
Attachment #9116383 - Flags: approval-mozilla-esr68+
Attachment #9115174 - Flags: approval-mozilla-esr68?

This fix has also been verified on the Beta v72.0b10 and ESR v68.4.0esr. Thank you.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(daniel.bodea)
Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?][adv-main72+]
Whiteboard: [reporter-external] [client-bounty-form] [verif?][adv-main72+] → [reporter-external] [client-bounty-form] [verif?][adv-main72+][adv-esr68.4+]
Attached file advisory.txt
Flags: sec-bounty? → sec-bounty+
Alias: CVE-2019-17022

(In reply to Emilio Cobos Álvarez (:emilio) from comment #17)

Anyhow, I think that should be a sensible plan for the time being, wdyt Freddy?

Good plan. Thank you ;)

Flags: needinfo?(fbraun)
Group: core-security-release
See Also: → CVE-2020-15676
Regressions: CVE-2020-26973
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: