Mutation XSS via copy-and-paste in WYSIWYG editors
Categories
(Core :: DOM: Serializers, defect)
Tracking
()
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)
47 bytes,
text/x-phabricator-request
|
jcristau
:
approval-mozilla-beta+
|
Details | Review |
18.21 KB,
patch
|
Details | Diff | Splinter Review | |
36.41 KB,
text/plain
|
RyanVM
:
approval-mozilla-esr68+
|
Details |
572 bytes,
text/plain
|
Details |
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
- Go to https://jsbin.com/xivapasere/1/edit?html,output.
- Press
copy me
. - Go to https://rawgit.com/alohaeditor/Aloha-Editor/hotfix/src/demo/boilerplate/ (one of the editors that's vulnerable to this mutation XSS).
- Give Aloha Editor a while to load...
- Go to any
contenteditable
content. - Paste from clipboard.
- 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
Reporter | ||
Comment 1•5 years ago
|
||
Sorry, it seems that incorrect code was saved in JSBin. Check the following link when reproducing the PoC:
Comment 2•5 years ago
|
||
Emilio, given you've looked at bug 1599181, want to take a look here?
Assignee | ||
Comment 3•5 years ago
|
||
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.
Reporter | ||
Comment 4•5 years ago
|
||
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:
- Change
<
to\3c
. That wouldn't change anything in the content of the string, while would protect against the attack. - Detect that
<style>
contains</style>
in its textContent and react somehow (like delete that element).
Assignee | ||
Comment 5•5 years ago
|
||
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.
Reporter | ||
Comment 6•5 years ago
|
||
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).
Assignee | ||
Comment 7•5 years ago
|
||
(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 inlinestyle
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.
Assignee | ||
Comment 8•5 years ago
|
||
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?
Reporter | ||
Comment 9•5 years ago
|
||
(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 inlinestyle
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.
Comment 10•5 years ago
|
||
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.
Comment 11•5 years ago
|
||
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.
Assignee | ||
Comment 12•5 years ago
|
||
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.
Assignee | ||
Comment 13•5 years ago
|
||
(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...
Assignee | ||
Comment 14•5 years ago
|
||
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.
Assignee | ||
Comment 15•5 years ago
|
||
This should work, but I need to fix the mOnlyConditionalCSS mode which was just
introduced for Thunderbird... :/
Assignee | ||
Comment 16•5 years ago
|
||
(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...)
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 17•5 years ago
|
||
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?
Updated•5 years ago
|
Assignee | ||
Comment 18•5 years ago
|
||
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.
Comment 19•5 years ago
|
||
(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
...
Assignee | ||
Comment 20•5 years ago
|
||
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.
Comment 21•5 years ago
|
||
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....
Assignee | ||
Comment 22•5 years ago
|
||
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
Assignee | ||
Updated•5 years ago
|
![]() |
||
Comment 23•5 years ago
|
||
https://hg.mozilla.org/integration/autoland/rev/79e508e5f428fee324b08d942c679a32af085f45
https://hg.mozilla.org/mozilla-central/rev/79e508e5f428
Comment 24•5 years ago
|
||
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.
Updated•5 years ago
|
Comment 25•5 years ago
|
||
Comment on attachment 9115174 [details]
Bug 1602843 - Preserve CSS input exactly during sanitization. r=hsivonen
approved for 72.0b8
![]() |
||
Comment 28•5 years ago
|
||
uplift |
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 29•5 years ago
|
||
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.
Assignee | ||
Comment 30•5 years ago
|
||
(Missed an if (mLogRemovals)
around the LogMessage
call.)
Comment 31•5 years ago
|
||
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.
Comment 32•5 years ago
|
||
Updated•5 years ago
|
Comment 33•5 years ago
|
||
uplift |
Comment 34•5 years ago
|
||
This fix has also been verified on the Beta v72.0b10 and ESR v68.4.0esr. Thank you.
Updated•5 years ago
|
Updated•5 years ago
|
Comment 35•5 years ago
|
||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 36•5 years ago
|
||
(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 ;)
Updated•5 years ago
|
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Updated•9 months ago
|
Description
•