Closed Bug 1646140 (CVE-2020-15676) Opened 4 years ago Closed 4 years ago

Bypass of copy+paste leading to paste XSS (using svg onload inside a style tag)

Categories

(Core :: DOM: Editor, defect, P2)

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
82 Branch
Tracking Flags:
Tracking Status
firefox-esr68 --- wontfix
firefox-esr78 81+ verified
firefox77 --- wontfix
firefox78 --- wontfix
firefox79 --- wontfix
firefox80 --- wontfix
firefox81 + verified
firefox82 --- verified

People

(Reporter: daniel.frojdendahl, Assigned: saschanaz)

References

(Regressed 1 open bug,
URL
)

Details

(Keywords: sec-moderate, Whiteboard: [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main81+][adv-esr78.3+])

Attachments

(3 files, 3 obsolete files)

20200616_210102.jpg
2.63 MB, image/jpeg
Details
Bug 1646140 - Fire SVG onload only when scripting is enabled r=hsivonen
47 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-beta+
RyanVM
: approval-mozilla-esr78+
Details | Review
advisory.txt
305 bytes, text/plain
Details
Attached image 20200616_210102.jpg —

After reading Michał Bentkowski Work on the Copy+Paste XSS (https://research.securitum.com/the-curious-case-of-copy-paste/) I began to test the differencies in browsers, Firefox and Chrome browser. It seems like I stumbled upon a bypass for the paste XSS, Im using 68.9.0esr (64-bit) but I did message Michal and asked for instructions and he told me to report it here and that I could use his 'playground.html' as part of the PoC

Steps to reproduce:

  1. Go to: https://cdn.sekurak.pl/copy-paste/playground.html
  2. In the first box (HTML input) use this payload: <svg><style><svg onload=alert(document.domain)>
  3. Click 'Copy as HTML'
  4. Paste it in the 'Paste target'
  5. A wild Alert box appears

Just as a good measure (Im a 100% Firefox user) I compared this to Chrome with the exact same payload, and no alert box is present.

Please do let me know if you need anything else! I do feel like this report is a bit lackluster in description, but Im just basing my work on Michal Bentkowski's research.
I'll attach an image as PoC as well :)

Flags: sec-bounty?

Thanks for the report. This is interesting. It doesn't work without the initial svg and style tag. Emilio/Henri, do you know where this is going wrong? HTML parser, css parser, sanitizer, something else?

Group: firefox-core-security → dom-core-security
Status: UNCONFIRMED → NEW
Type: task → defect
status-firefox77: --- → wontfix
status-firefox78: --- → affected
status-firefox79: --- → affected
status-firefox-esr68: --- → affected
status-firefox-esr78: --- → affected
Component: Security → DOM: Editor
Ever confirmed: true
Flags: needinfo?(hsivonen)
Flags: needinfo?(emilio)
Product: Firefox → Core

Waiting for Pernosco.

When we parse the input, we post a runnable that will fire the load event at the second svg node (also another for the first one). When we sanitize the style element, we replace its content with an empty string, which removes the second svg node before we get to sanitizing the attributes of the second svg node.

When the runnable runs, it fires a load event on the detached svg node that still has the onload event handler in place, because we detached the node before sanitizing its attributes.

The SVG load event is such a bad idea in the first place.

Do we ever actually want the SVG load event to fire when parsing from a source other than a network stream or document.write?

Firing dispatching the load event in a disconnected node seems a bit odd... I guess we do the same for <img>? If so the best alternative may be to sanitize the stylesheet after sanitizing the descendants...

Flags: needinfo?(emilio)

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

If so the best alternative may be to sanitize the stylesheet after sanitizing the descendants...

Forgive my ignorance as I only know a tiny bit about the sanitizer, and practically nothing about the other code here, but this seems like a solution focused on this specific case. Could we more generically avoid this by noticing (perhaps via a mutation observer on the node into which we're parsing or similar?) the nodes that get removed from the fragment while parsing/sanitizing, and then sanitizing attributes on those removed items still?

Also curious if we could avoid firing the events at all during the sanitization pass (by some extra arg passed by the sanitizer or something) - presumably we fire them (again?) when the sanitized content is appended to the "real" DOM, for any content that's left at this point (which wouldn't include the nested SVG node in this case) ?

So far, it remains unclear to me what mechanism in Blink prevents the event handler from running. We should probably avoid making up something different.

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

Do we ever actually want the SVG load event to fire when parsing from a source other than a network stream or document.write?

Yes, document.body.innerHTML = "<svg onload='alert(1)'></svg>"; alerts in Chrome.

<!DOCTYPE html>
<body>
<script>
var p = (new DOMParser()).parseFromString("<svg onload='alert(1)'></svg>", "text/html");
document.body.appendChild(p.body);
</script>

Alerts in Firefox but not in Chrome.

annevk, is there a piece of spec text somewhere that says not to fire the SVG load event or says not to compile its handler if scripting is disabled? Alternatively, is there similar text for the scenario when the node belongs to a document that does not have a browsing context?

Flags: needinfo?(hsivonen) → needinfo?(annevk)

Scripting disabled is covered by https://html.spec.whatwg.org/#getting-the-current-value-of-the-event-handler I think, though SVG probably doesn't properly integrate with that. I don't really know beyond that. (SVG not being maintained is a continuous source of minor issues, perhaps at some point we should attempt to take it on.)

Flags: needinfo?(annevk)

Thanks. So the right way to fix this would be to refuse to compile the onload handler, because scripting is disabled for DOMParser-created documents.

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

Thanks. So the right way to fix this would be to refuse to compile the onload handler, because scripting is disabled for DOMParser-created documents.

...For the DOMParser case. We need some other check for the paste case.

The severity field is not set for this bug.
:hsinyi, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(htsai)

We should probably do this:

Create an RAII type that increments and decrements a "sanitizing parses running" counter in nsContentUtils. Wrap any invocation of the parser and the sanitizer in a scope where that type is instantiated on the stack.

In https://searchfox.org/mozilla-central/source/parser/html/nsHtml5TreeOperation.cpp#731 return early if the node's document has scripting disabled, doesn't have a docshell, or the above-mentioned counter is non-zero.

Severity: -- → S3
Flags: needinfo?(htsai)
Priority: -- → P2

Hello again Mozilla team!
I just wanted to ping for a little update :) I know perhaps this is not the perfect timing with things going on in mozilla!
Anyway; I've updated the PoC a little bit to make it even easier to triage / share:

  1. Enter data:text/html,<div style="border: black solid;" role="textbox" contenteditable="true"></div> in the url
  2. Go to 911.tf/alertPoC.html -> Click to copy the payload
  3. Paste the result into the text field in firefox.

Furthermore, I've noticed that SOME sites are not vulnerable due to CSP blocking this with script-src untrusted.
I hope everyone is feeling well in these strange pandemic times - Stay safe!
With best wishes from Daniel Fröjdendahl :)

Jens, can we find someone to fix this based on Henri's suggestions?

Flags: needinfo?(jstutte)
Flags: needinfo?(jstutte)

Hi, can you take a look, please?

Flags: needinfo?(krosylight)

I added scriptingEnabled && !mPreventScriptExecution to nsHtml5Parser::elementPopped SvgLoad caller part and it seems it works for the cases mentioned here. Henri, would this be sufficient or could you provide an example where the sanitizer check is required?

I'm about to send my commit to try server, but since this is a security issue, are there special rules I should follow?

Flags: needinfo?(krosylight) → needinfo?(hsivonen)

(In reply to Kagami :saschanaz from comment #19)

I added scriptingEnabled && !mPreventScriptExecution to nsHtml5Parser::elementPopped SvgLoad caller part and it seems it works for the cases mentioned here. Henri, would this be sufficient or could you provide an example where the sanitizer check is required?

If that works, I don't have concrete reasons why it wouldn't be sufficient. I'd be more convinced that there aren't edge cases to worry about if there was also the counter explicitly tied to sanitizer runs.

I'm about to send my commit to try server, but since this is a security issue, are there special rules I should follow?

Please don't use the tryserver. Once the patch has approval to land, it lands, and if it bounces, then let's fix it at that point.

Flags: needinfo?(hsivonen)
Assignee: nobody → krosylight
Status: NEW → ASSIGNED

Depends on D89218

https://hg.mozilla.org/integration/autoland/rev/67c242c00ce3e796dafb7e98bbbf025e29cc1c67
https://hg.mozilla.org/mozilla-central/rev/67c242c00ce3

Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox82: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch
Flags: qe-verify+
Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage]

You guys sure work fast, Im glad to see that the build passed as well! :)
Two questions from my side:

  1. Will this be assigned a CVE?
  2. Will this report be eligible for a bounty? If yes, is there any other form for me to fill? Perhaps this one: (https://bugzilla.mozilla.org/form.client.bounty) ?

Many thanks and good patching! Please do let me know if you need anything else.

With best regards // Daniel Fröjdendahl

(In reply to daniel fröjdendahl from comment #24)

You guys sure work fast, Im glad to see that the build passed as well! :)
Two questions from my side:

  1. Will this be assigned a CVE?

This normally happens after resolving the bug when it's clear what version things ship in. I expect we'd assign a CVE for this, but :tjr can confirm.

  1. Will this report be eligible for a bounty? If yes, is there any other form for me to fill? Perhaps this one: (https://bugzilla.mozilla.org/form.client.bounty) ?

It's already flagged for a bounty request (sec-bounty flag set to ?), so you don't need to do anything else. The bounty committee will make a decision when they next meet, I think.

:saschanaz, should uplift to 81 beta be considered here?

status-firefox78: affected → wontfix
status-firefox79: affected → wontfix
status-firefox80: --- → affected
status-firefox81: --- → affected
Flags: needinfo?(tom)
Flags: needinfo?(krosylight)

(In reply to :Gijs (he/him) from comment #25)
Ok! Thank you very much for the fast response :)
Will definitely look for more bugs with Firefox considering the very professional response from all you!

  1. Will this be assigned a CVE?

This normally happens after resolving the bug when it's clear what version things ship in. I expect we'd assign a CVE for this, but :tjr can confirm.

Yes, this meets the criteria of a CVE, so when this gets released we will issue a CVE for it.

Flags: needinfo?(tom)

:saschanaz, should uplift to 81 beta be considered here?

I guess yes. I'll wait for 1-2 days to make sure this didn't cause any regression and then file an uplift request.

Flags: needinfo?(krosylight)

We should uplift, especially since it's picked up at least one duplicate which shows researchers are poking in this area.

status-firefox80: affected → wontfix
status-firefox-esr68: affected → wontfix
tracking-firefox81: --- → +
tracking-firefox-esr78: --- → 81+
Flags: sec-bounty? → sec-bounty+
See Also: → CVE-2019-17022

Please nominate this for Beta & ESR78 approval.

Flags: needinfo?(krosylight)

Comment on attachment 9173774 [details]
Bug 1646140 - Fire SVG onload only when scripting is enabled r=hsivonen

Beta/Release Uplift Approval Request

  • User impact if declined: Users will be vulnerable from potential XSS attacks by pasting malicious HTML+SVG+JS code.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: Please see comment #8 and comment #16. Automated tests are also posted but will be landed later separately.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): It only adds a short condition where onload must not fire.
  • String changes made/needed:

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: tracking-firefox-esr78: 81+
  • User impact if declined: Users will be vulnerable from potential XSS attacks by pasting malicious HTML+SVG+JS code.
  • Fix Landed on Version: 82
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): It only adds a short condition where onload must not fire.
  • String or UUID changes made by this patch:
Flags: needinfo?(krosylight)
Attachment #9173774 - Flags: approval-mozilla-esr78?
Attachment #9173774 - Flags: approval-mozilla-beta?
QA Whiteboard: [qa-triaged]

Verified as FIXED on the latest Nightly 82.0a1 on Ubuntu 18.04 x64, Mac OS X 10.15, and Windows 10 x64 - the alert box is no longer displayed when following the scenario from the Description and the ones from Comment 8 and Comment 16.

status-firefox82: fixed → verified

Comment on attachment 9173774 [details]
Bug 1646140 - Fire SVG onload only when scripting is enabled r=hsivonen

Approved for 81.0b9 and 78.3esr.

Attachment #9173774 - Flags: approval-mozilla-esr78?
Attachment #9173774 - Flags: approval-mozilla-esr78+
Attachment #9173774 - Flags: approval-mozilla-beta?
Attachment #9173774 - Flags: approval-mozilla-beta+

Comment on attachment 9174232 [details]
Bug 1646140 - Add tests for SVG onload r=hsivonen

Revision D89383 was moved to bug 1664036. Setting attachment 9174232 [details] to obsolete.

Attachment #9174232 - Attachment is obsolete: true
Blocks: 1664036

Thanks for working on this Kagami & Henri!

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

(In reply to Kagami :saschanaz from comment #19)

I'm about to send my commit to try server, but since this is a security issue, are there special rules I should follow?

Please don't use the tryserver. Once the patch has approval to land, it lands, and if it bounces, then let's fix it at that point.

Drive-By Comment: Please see our documentation on Fixing Security Bugs. It's not a long and I recommend reading it carefully. (The yellow banner at the very top of the page links to our security bug approval docs, which are also interesting, but only apply to sec-high and sec-critical)

Verified as fixed on Firefox 78.3.0 esr (from Treeherder, Build ID: 20200909191438) on Windows 10 x64, Ubuntu 18.04 x64 and, Mac OS X 10.15.

status-firefox-esr78: fixed → verified

Verified as fixed on Firefox 81 beta 9 on Windows 10 x64, Mac OS X 10.15, and Ubuntu 18.04 x64.

QA Whiteboard: [qa-triaged]
Whiteboard: [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage] → [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main81+]
Whiteboard: [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main81+] → [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main81+][adv-esr78.3+]
QA Whiteboard: [qa-triaged]

When queueing runnables for SVG elements, we incorrectly detached an element before sanitizing it, resulting in JavaScript being executed after pasting attacker-controlled data into a contenteditable element.

For clarity with less reference to internal mechanics I suggest:

We sometimes ran the onload handler for SVG elements that the DOM sanitizer decided to remove resulting in JavaScript being executed after pasting attacker-controlled data into a contenteditable element.

Attached file advisory.txt (obsolete) —

Thanks!

Attachment #9176108 - Attachment is obsolete: true

As per comment 38, setting the tracking flag for firefox81 to verified.

QA Whiteboard: [qa-triaged]
status-firefox81: fixed → verified
Flags: qe-verify+
Attached file advisory.txt —
Attachment #9176289 - Attachment is obsolete: true
Alias: CVE-2020-15676
Summary: Bypass of copy+paste leading to paste XSS → Bypass of copy+paste leading to paste XSS (using svg onload inside a style tag)

Am I correct to assume that this is a regression? I can't seem to remember us ever running event handlers before something is in the DOM..

(In reply to Frederik Braun [:freddy] from comment #45)

Am I correct to assume that this is a regression? I can't seem to remember us ever running event handlers before something is in the DOM..

We've always run event handlers on <img> elements before they're inserted in the document. I'm not sure whether that's always extended to event handler attributes, though.

Group: core-security-release
Regressions: 1754727
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: