1666300 - (CVE-2020-26956) XSS via paste and Clipboard API and SVG + image onerror handler

Status: VERIFIED FIXED

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

Description

[Irvan Kurniawan [:sourc7]]

When fuzzing Firefox clipboard API content sanitization on pasting, I found sanitizer bypass with payload <svg><style><image href=1 onerror=alert(document.domain)>. When user copy from crafted malicious website, then paste the content, XSS will execute. It leads to universal XSS on latest stable release of Firefox, Firefox 78.3.0esr (64-bit), and Firefox Nightly 82.0a1 (2020-09-20).

Steps to reproduce:

1. Go to https://sourc7.appspot.com/bugzilla/0447f4d26c48bd8d0526dce173f57b3ac5235b7b.html
2. Click Copy Text
3. Go to https://output.jsbin.com/gocizet/ (TinyMCE Rich Editor form)
4. Paste with Ctrl+V or Right click -> Paste into TinyMCE form
5. XSS will execute [pop alert(document.domain)]

Comment 1

[:Gijs (he/him)]

:saschanaz, could you take a look given your work on the other related bug in this area of the code?

Comment 2

[Irvan Kurniawan [:sourc7]]

Attachment: 0447f4d26c48bd8d0526dce173f57b3ac5235b7b.html

Payload HTML

Comment 3

[Kagami Rosylight [:saschanaz] (they/them)]

A relevant call stack info:

xul.dll!mozilla::dom::SVGImageElement::LoadSVGImage(bool aForce, bool aNotify) Line 131 (c:\Users\Kagami\Documents\GitHub\gecko-dev\dom\svg\SVGImageElement.cpp:131)
xul.dll!mozilla::dom::SVGImageElement::AfterSetAttr(int aNamespaceID, nsAtom * aName, const nsAttrValue * aValue, const nsAttrValue * aOldValue, nsIPrincipal * aSubjectPrincipal, bool aNotify) Line 184 (c:\Users\Kagami\Documents\GitHub\gecko-dev\dom\svg\SVGImageElement.cpp:184)
xul.dll!mozilla::dom::Element::SetAttrAndNotify(int aNamespaceID, nsAtom * aName, nsAtom * aPrefix, const nsAttrValue * aOldValue, nsAttrValue & aParsedValue, nsIPrincipal * aSubjectPrincipal, unsigned char aModType, bool aFireMutation, bool aNotify, bool aCallAfterSetAttr, mozilla::dom::Document * aComposedDocument, const mozAutoDocUpdate &) Line 2407 (c:\Users\Kagami\Documents\GitHub\gecko-dev\dom\base\Element.cpp:2407)
xul.dll!mozilla::dom::Element::SetAttr(int aNamespaceID, nsAtom * aName, nsAtom * aPrefix, const nsTSubstring<char16_t> & aValue, nsIPrincipal * aSubjectPrincipal, bool aNotify) Line 2264 (c:\Users\Kagami\Documents\GitHub\gecko-dev\dom\base\Element.cpp:2264)
xul.dll!mozilla::dom::Element::SetAttr(int aNameSpaceID, nsAtom * aName, nsAtom * aPrefix, const nsTSubstring<char16_t> & aValue, bool aNotify) Line 890 (c:\Users\Kagami\Documents\GitHub\gecko-dev\obj-x86_64-pc-mingw32\dist\include\mozilla\dom\Element.h:890)
xul.dll!nsHtml5TreeOperation::CreateSVGElement(nsAtom * aName, nsHtml5HtmlAttributes * aAttributes, mozilla::dom::FromParser aFromParser, nsNodeInfoManager * aNodeInfoManager, nsHtml5DocumentBuilder * aBuilder, nsresult(*)(nsIContent * *, already_AddRefed<mozilla::dom::NodeInfo> &&, mozilla::dom::FromParser) aCreator) Line 554 (c:\Users\Kagami\Documents\GitHub\gecko-dev\parser\html\nsHtml5TreeOperation.cpp:554)
xul.dll!nsHtml5TreeBuilder::createElement(int aNamespace, nsAtom * aName, nsHtml5HtmlAttributes * aAttributes, void * aIntendedParent, nsHtml5ContentCreatorFunction aCreator) Line 124 (c:\Users\Kagami\Documents\GitHub\gecko-dev\parser\html\nsHtml5TreeBuilderCppSupplement.h:124)
xul.dll!nsHtml5TreeBuilder::appendToCurrentNodeAndPushElementMayFosterSVG(nsHtml5ElementName * elementName, nsHtml5HtmlAttributes * attributes) Line 4287 (c:\Users\Kagami\Documents\GitHub\gecko-dev\parser\html\nsHtml5TreeBuilder.cpp:4287)
xul.dll!nsHtml5TreeBuilder::startTag(nsHtml5ElementName * elementName, nsHtml5HtmlAttributes * attributes, bool selfClosing) Line 744 (c:\Users\Kagami\Documents\GitHub\gecko-dev\parser\html\nsHtml5TreeBuilder.cpp:744)
xul.dll!nsHtml5Tokenizer::emitCurrentTagToken(bool selfClosing, int pos) Line 344 (c:\Users\Kagami\Documents\GitHub\gecko-dev\parser\html\nsHtml5Tokenizer.cpp:344)
xul.dll!nsHtml5Tokenizer::stateLoop<nsHtml5SilentPolicy>(int state, char16_t c, int pos, char16_t * buf, bool reconsume, int returnState, int endPos) Line 1069 (c:\Users\Kagami\Documents\GitHub\gecko-dev\parser\html\nsHtml5Tokenizer.cpp:1069)
xul.dll!nsHtml5Tokenizer::tokenizeBuffer(nsHtml5UTF16Buffer * buffer) Line 443 (c:\Users\Kagami\Documents\GitHub\gecko-dev\parser\html\nsHtml5Tokenizer.cpp:443)
xul.dll!nsHtml5StringParser::Tokenize(const nsTSubstring<char16_t> & aSourceBuffer, mozilla::dom::Document * aDocument, bool aScriptingEnabledForNoscriptParsing) Line 101 (c:\Users\Kagami\Documents\GitHub\gecko-dev\parser\html\nsHtml5StringParser.cpp:101)
xul.dll!nsHtml5StringParser::ParseFragment(const nsTSubstring<char16_t> & aSourceBuffer, nsIContent * aTargetNode, nsAtom * aContextLocalName, int aContextNamespace, bool aQuirks, bool aPreventScriptExecution) Line 55 (c:\Users\Kagami\Documents\GitHub\gecko-dev\parser\html\nsHtml5StringParser.cpp:55)
xul.dll!nsContentUtils::ParseFragmentHTML(const nsTSubstring<char16_t> & aSourceBuffer, nsIContent * aTargetNode, nsAtom * aContextLocalName, int aContextNamespace, bool aQuirks, bool aPreventScriptExecution, int aFlags) Line 4857 (c:\Users\Kagami\Documents\GitHub\gecko-dev\dom\base\nsContentUtils.cpp:4857)
xul.dll!mozilla::HTMLEditor::HTMLWithContextInserter::FragmentParser::ParseFragment(const nsTSubstring<char16_t> & aFragStr, nsAtom * aContextLocalName, const mozilla::dom::Document * aTargetDocument, mozilla::dom::DocumentFragment * * aFragment, bool aTrustedInput) Line 3502 (c:\Users\Kagami\Documents\GitHub\gecko-dev\editor\libeditor\HTMLEditorDataTransfer.cpp:3502)
xul.dll!mozilla::HTMLEditor::HTMLWithContextInserter::FragmentParser::ParsePastedHTML(const nsTSubstring<char16_t> & aInputString, nsAtom * aContextLocalNameAtom, mozilla::dom::DocumentFragment * * aFragment) Line 3191 (c:\Users\Kagami\Documents\GitHub\gecko-dev\editor\libeditor\HTMLEditorDataTransfer.cpp:3191)
xul.dll!mozilla::HTMLEditor::HTMLWithContextInserter::FragmentFromPasteCreator::CreateDocumentFragmentAndGetParentOfPastedHTMLInContext(const mozilla::dom::Document & aDocument, const nsTSubstring<char16_t> & aInputString, const nsTSubstring<char16_t> & aContextStr, bool aTrustedInput, nsCOMPtr<nsINode> & aParentNodeOfPastedHTMLInContext, RefPtr<mozilla::dom::DocumentFragment> & aDocumentFragmentToInsert) Line 3366 (c:\Users\Kagami\Documents\GitHub\gecko-dev\editor\libeditor\HTMLEditorDataTransfer.cpp:3366)
xul.dll!mozilla::HTMLEditor::HTMLWithContextInserter::FragmentFromPasteCreator::Run(const mozilla::dom::Document & aDocument, const nsTSubstring<char16_t> & aInputString, const nsTSubstring<char16_t> & aContextStr, const nsTSubstring<char16_t> & aInfoStr, nsCOMPtr<nsINode> * aOutFragNode, nsCOMPtr<nsINode> * aOutStartNode, nsCOMPtr<nsINode> * aOutEndNode, bool aTrustedInput) Line 3421 (c:\Users\Kagami\Documents\GitHub\gecko-dev\editor\libeditor\HTMLEditorDataTransfer.cpp:3421)
xul.dll!mozilla::HTMLEditor::HTMLWithContextInserter::CreateDOMFragmentFromPaste(const nsTSubstring<char16_t> & aInputString, const nsTSubstring<char16_t> & aContextStr, const nsTSubstring<char16_t> & aInfoStr, nsCOMPtr<nsINode> * aOutFragNode, nsCOMPtr<nsINode> * aOutStartNode, nsCOMPtr<nsINode> * aOutEndNode, int * aOutStartOffset, int * aOutEndOffset, bool aTrustedInput) Line 3214 (c:\Users\Kagami\Documents\GitHub\gecko-dev\editor\libeditor\HTMLEditorDataTransfer.cpp:3214)

Not sure what should be done here (Edit: because it seems the handler is not directly called by tree builder). Pinging Henri who suggested the previous solution:

Comment 4

[Frederik Braun [:freddy]]

mozregression points to this pushlog, which makes me believe we have been incorrect since we first implemented <image href> for SVG.

Comment 5

[Henri Sivonen (:hsivonen)]

Without testing, my guess is that we remove element children of SVG style but since disconnected image nodes load their src for compat with Netscape <= 4.x, we end up loading processing the loading for the empty-string src and running the onerror handler.

Perhaps we need to change node removal in the sanitizer to iterate over the subtree rooted at the node being removed and remove all attributes in it.

Comment 6

[Jens Stutte [:jstutte]]

Setting P1 to reflect security classification of bug 1666777 (as of comment 13 there).

Comment 7

[Anne (:annevk)]

HTML img element loading (at least per the HTML standard) does an active document check. It seems we should do a similar check for SVG image elements. It seems that only fixing this at the sanitizer level would still make us execute script as part of XMLHttpRequest or DOMParser or some such, right?

(Also, would fixing this at the sanitizer level prevent all network activity?)

Comment 8

[Kagami Rosylight [:saschanaz] (they/them)]

Somehow this does not call onerror:

new DOMParser().parseFromString("<svg><style><image href=1 onerror=alert(document.domain)>", "text/html")

Comment 9

[Henri Sivonen (:hsivonen)]

I'll make an attempt at fixing this.

Comment 10

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1666300 part 1 - Remove attributes from descendants when setting sanitized style.

Comment 11

[Henri Sivonen (:hsivonen)]

The attached patch so far implements only the attribute removal hack, not Anne's suggestion of a document marked as "loaded as data".

Comment 12

[Henri Sivonen (:hsivonen)]

Per https://searchfox.org/mozilla-central/rev/c2e3ac11be4837718c2604e58085fbc8252b69dd/editor/libeditor/HTMLEditorDataTransfer.cpp#3241 , we parse using the document that is being edited as the owner document for the nodes during the node creation. Other than performance, would there be downsides to parsing with a distinct throw-away document marked as "loaded as data"?

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

Somehow this does not call onerror:

new DOMParser().parseFromString("<svg><style><image href=1 onerror=alert(document.domain)>", "text/html")

That's because DOMParser parses into a document marked as "loaded as data".

The attached patch is sufficient for fixing the reported problem from comment 0, though.

Comment 13

[Henri Sivonen (:hsivonen)]

Hmm. While a "loaded as data" document might be nice as a defense-in-depth mechanism, it won't help innerHTML in chrome, unless we also de-optimize innerHTML in chrome not to parser directly into the target document.

Comment 14

[Henri Sivonen (:hsivonen)]

Clearing needinfo that was answered by voice. I'll write a patch for using a throw-away loaded-as-data document.

Comment 15

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1666300 part 2 - Parse into an inert document.

Comment 16

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1666300 test - Test SVG style sanitization on paste.

Comment 17

[Henri Sivonen (:hsivonen)]

Comment on attachment 9181014 [details]
Bug 1666300 part 1 - Remove attributes from descendants when setting sanitized style.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Very easily from part 1; less so from part 2. (If we really wanted to, we could defer part 1.) Note that while this bug is sec-moderate, this is relates to bug 1667113, which is sec-high and should land together. However, I consider this one more severe than the other one, because the other one is itself about a defense-in-depth mechanism and is already mitigated by another defense-in-depth mechanism.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Either the same patch applies or it will be a mechanical rebase
• How likely is this patch to cause regressions; how much testing does it need?: Not very likely: Regressions are likely to break the entire mochitest test harness (and did during development), so now that the harness works again, I expect things to be OK.

Comment 18

[Tom Ritter [:tjr]]

Comment on attachment 9181014 [details]
Bug 1666300 part 1 - Remove attributes from descendants when setting sanitized style.

Approved to land and uplift. Test approved to land after 12/15

Comment 19

[Tom Ritter [:tjr]]

Comment on attachment 9181514 [details]
Bug 1666300 part 2 - Parse into an inert document.

Approved to land and uplift. Test approved to land after 12/15

Comment 20

[Julien Cristau [:jcristau]]

Landed:
https://hg.mozilla.org/integration/autoland/rev/9b0c50269ef788e4b1b8ed3a0662b184e322d04c
https://hg.mozilla.org/integration/autoland/rev/0f0fc78ad52193bff03fe070e688b04c71efe994

And backed out for perma failures on browser_parsable_css.js:
https://hg.mozilla.org/integration/autoland/rev/104bf955a8130a41673f410b7c139edcd85ec0a8

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=319351800&repo=autoland&lineNumber=1586

Comment 21

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/integration/autoland/rev/3476387362fb15c82f133f390afef719ad36de0a
https://hg.mozilla.org/integration/autoland/rev/b067b0d3670b37daad95505b87bddca6bb113d11

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3476387362fb
https://hg.mozilla.org/mozilla-central/rev/b067b0d3670b

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a3ce9140020a
https://hg.mozilla.org/releases/mozilla-beta/rev/279f3b9d4d17

Comment 24

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/112b19f43ac9
https://hg.mozilla.org/releases/mozilla-esr78/rev/667553745b48

Comment 25

[Frederik Braun [:freddy]]

Attachment: advisory.txt

Comment 26

[Henri Sivonen (:hsivonen)]

https://hg.mozilla.org/mozilla-central/rev/782446e715644da3ca8226d0c3413e3fafb69d6f

This caused bug 1682950, which was unexpected.