1768537 - (CVE-2022-34468) CSP sandbox header without `allow-scripts` can be bypassed via retargeted javascript: URI

Status: VERIFIED FIXED

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

Description

[Armin Ebert [:arminius]]

Under a CSP sandbox header directive without allow-scripts, the document shouldn't be able to run scripts. However, this can be bypassed using a javascript: URI with a suitable (i.e. script-capable, same-origin) target document.

Proof of concept

parent.html:

<iframe src="child.html"></iframe>

child.html:

Content-Security-Policy: sandbox allow-same-origin allow-top-navigation

<noscript>no scripts allowed here</noscript>
<a href="javascript:alert(`origin=${origin} location=${location}`)" target="_parent">click me</a>

When the link is clicked, the script runs in the context of parent.html. As per allow-top-navigation, the child is allowed to navigate the parent, but not to execute scripts since its sandbox doesn't allow-scripts.

Details

Since the target (parent.html) is itself allowed to run scripts, the relevant security check here is the verification that the sandbox flags of the triggering context allow scripts, too:

nsJSThunk::EvaluateScript() at nsJSProtocolHandler.cpp:238

  // Sandboxed document check: javascript: URI execution is disabled in a
  // sandboxed document unless 'allow-scripts' was specified.
  if ((targetDoc && !targetDoc->IsScriptEnabled()) ||
      (loadInfo->GetTriggeringSandboxFlags() & SANDBOXED_SCRIPTS)) {
  ...

In the example, loadInfo->GetTriggeringSandboxFlags() == 0, thus the script is allowed.

My understanding is that the triggering sandbox flags are taken from the triggering BC (not the document):

nsDocShell::OnLinkClickSync() at nsDocShell.cpp:13004

  uint32_t triggeringSandboxFlags = 0;
  if (mBrowsingContext) {
    triggeringSandboxFlags = mBrowsingContext->GetSandboxFlags();
  }
  ...
  aLoadState->SetTriggeringSandboxFlags(triggeringSandboxFlags);
  ...
  nsresult rv = InternalLoad(aLoadState);

However, the CSP sandbox header didn't change the sandbox flags of the BC:

(gdb) b nsDocShell.cpp:13100
Thread 1 "Isolated Web Co" hit Breakpoint 1, nsDocShell::OnLinkClickSync (this=0x7f514773d400, aContent=0x7f5142609030, aLoadState=0x7f516e69b480, aNoOpenerImplied=false, aTriggeringPrincipal=0x7f514284d880) at /m-c/docshell/base/nsDocShell.cpp:13100
13100     aLoadState->SetTriggeringSandboxFlags(triggeringSandboxFlags);
(gdb) p GetDocument()->GetSandboxFlags()
$1 = 1048555
(gdb) p mBrowsingContext->GetSandboxFlags()
$2 = (const uint32_t &) @0x7f514a905b64: 0
(gdb) p triggeringSandboxFlags
$3 = 0

Note that if the sandbox isn't set by CSP header, but <iframe sandbox="allow-same-origin allow-top-navigation"> instead, the issue doesn't occur. In that case, the sandbox flags on the document and BC are aligned, as expected:

(gdb) b nsDocShell.cpp:13100
Thread 1 "Isolated Web Co" hit Breakpoint 1, nsDocShell::OnLinkClickSync (this=0x7f5147740000, aContent=0x7f5142609190, aLoadState=0x7f516e69b480, aNoOpenerImplied=false, aTriggeringPrincipal=0x7f514284d1a0) at /m-c/docshell/base/nsDocShell.cpp:13100
13100     aLoadState->SetTriggeringSandboxFlags(triggeringSandboxFlags);
(gdb) p GetDocument()->GetSandboxFlags()
$1 = 1048555
(gdb) p mBrowsingContext->GetSandboxFlags()
$2 = (const uint32_t &) @0x7f516e6d3164: 1048555
(gdb) p triggeringSandboxFlags
$3 = 1048555

(Here, the BC's sandbox flags were updated from nsFrameLoader::ApplySandboxFlags().)

Comment 1

[Frederik Braun [:freddy]]

This looks related to bug 1761981 (or is this a coincidence)?
Paul, can you take a look?

Comment 2

[Paul Zühlcke [:pbz]]

Thanks, I can take a look. I'm not (or no longer) CCed on that bug though, could you add me?

Comment 3

[Frederik Braun [:freddy]]

(Done)

Comment 4

[Nika Layzell [:nika] (ni? for response)]

This should be a fairly straightforward fix: the issue is that in OnLinkClick the code to initialize TriggeringSandboxFlags appears to be wrong, and pulls the sandbox flags from the browsing context instead of the document (https://searchfox.org/mozilla-central/rev/88792eff309001778cb2431f2a0ed92f8f3c258a/docshell/base/nsDocShell.cpp#13002-13005)

If we replace those lines with:

  uint32_t triggeringSandboxFlags = aContent->OwnerDoc()->GetSandboxFlags();

I believe that should fix the main issue.

Looking at other callers of SetTriggeringSandboxFlags it appears as though we also pass flags from the BrowsingContext for LoadErrorPage (https://searchfox.org/mozilla-central/rev/88792eff309001778cb2431f2a0ed92f8f3c258a/docshell/base/nsDocShell.cpp#4094), however that load is actually triggered by the system principal, so I don't think we need to worry about the triggering sandbox flags, as theoretically the trigger is non-sandboxed.

I also see https://searchfox.org/mozilla-central/rev/88792eff309001778cb2431f2a0ed92f8f3c258a/toolkit/components/windowwatcher/nsWindowWatcher.cpp#1214, which pulls the sandbox flags from the parent BC for the load in the new pop-up. This should probably also be using the active document's sandbox flags if they're available, which are conveniently fetched in https://searchfox.org/mozilla-central/rev/88792eff309001778cb2431f2a0ed92f8f3c258a/toolkit/components/windowwatcher/nsWindowWatcher.cpp#843,850-863 (though we probably should fetch them even if we're targeting an existing browsing context by name (i.e. newBC is set))

Comment 5

[Paul Zühlcke [:pbz]]

Thanks for looking into this :nika! Do you already have a patch or draft you can submit? Otherwise I can take the bug.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

(In reply to Paul Zühlcke [:pbz] from comment #5)

Thanks for looking into this :nika! Do you already have a patch or draft you can submit? Otherwise I can take the bug.

I don't have a patch already written for this IIRC, so feel free to take it

Comment 7

[Paul Zühlcke [:pbz]]

Attachment: Bug 1768537, r=smaug!

Comment 8

[Paul Zühlcke [:pbz]]

Attachment: Bug 1768537 - tests, r=smaug!

Depends on D146914

Comment 9

[Paul Zühlcke [:pbz]]

Comment on attachment 9277488 [details]
Bug 1768537, r=smaug!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The concrete attack vector, JavaScript URI, isn't obvious from looking at the patch. However given that the patch only changes one line, the vulnerability is easy to recognize by an attacker familiar with CSP / iframe sandbox.
  The test is in a separate patch and should also land separately, because it's basically the PoC submitted by the reporter.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• 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?: I expect that this patch can be applied to older branches easily, including ESR. The affected code line has not changed for a long time.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause regressions since the change is quite simple.
• Is Android affected?: Yes

Comment 10

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

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

For more information, please visit auto_nag documentation.

Comment 11

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

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:pbz, could you consider increasing the severity of this security bug?

For more information, please visit auto_nag documentation.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Release mgmt bot [:suhaib / :marco/ :calixte] from comment #11)

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:pbz, could you consider increasing the severity of this security bug?

The Bug already had an r+ patch at the time I set the severity, and it's a CSP bug, so S3 seemed accurate at the time. Happy to make it an S2, but then again, we already have a patch ready for it.

Comment 13

[Tom Ritter [:tjr]]

Comment on attachment 9277488 [details]
Bug 1768537, r=smaug!

Approved to land and uplift

Comment 14

[Tom Ritter [:tjr]]

Comment on attachment 9277489 [details]
Bug 1768537 - tests, r=smaug!

Test approved to land Aug 15 or later

Comment 15

[Phabricator Automation]

Comment on attachment 9277489 [details]
Bug 1768537 - tests, r=smaug!

Revision D146915 was moved to bug 1772290. Setting attachment 9277489 [details] to obsolete.

Comment 16

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

r=smaug
https://hg.mozilla.org/integration/autoland/rev/2f02bf038165d4894f0a273a34cfe4773d4e966e
https://hg.mozilla.org/mozilla-central/rev/2f02bf038165

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta & ESR approval.

Comment 18

[Paul Zühlcke [:pbz]]

Attachment: PoC-node.zip

Here is a PoC written for NodeJS. To run it extract the zip, then run the following commands:

npm install
node index.js

The site is served on http://localhost:3000

Comment 19

[Paul Zühlcke [:pbz]]

Comment on attachment 9277488 [details]
Bug 1768537, r=smaug!

Beta/Release Uplift Approval Request

• User impact if declined: Untrusted, sandboxed sites may run scripts.
• 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: 1. Run the server in comment 18 and visit http://localhost:3000.

2. click the link on the page
   Expected result: no alert is shown

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Well understood change, small code change, has automated test coverage.
• String changes made/needed:
• Is Android affected?: Yes

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Untrusted, sandboxed sites may run scripts.
• Fix Landed on Version: 103
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Well understood change, small code change, has automated test coverage.

Comment 20

[Paul Zühlcke [:pbz]]

Verified the fix in today's Nightly on macOS using the PoC code from comment 18.

Comment 21

[Ciprian Georgiu, Desktop QA]

I've reproduced this bug using an affected Nightly build (2022-05-09), on Ubuntu 18.04 x64.

The issue is also verified as fixed on latest Nightly 103.0a1, running Win 10 x64 and Ubuntu 18.04 x64.

Comment 22

[Pascal Chevrel:pascalc]

Comment on attachment 9277488 [details]
Bug 1768537, r=smaug!

Approved for 102 beta 5, thanks.

Comment 23

[Pascal Chevrel:pascalc]

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

Comment 24

[Ciprian Georgiu, Desktop QA]

Verified as fixed on 102.0b5 as well, under Win 10 x64, Ubuntu 18.04 x64 and macOS 11.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9277488 [details]
Bug 1768537, r=smaug!

Approved for 91.11esr.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/58aad8bcc032

Comment 28

[Ciprian Georgiu, Desktop QA]

This is also verified as fixed on esr 91.11.0esr, under Win 10 x64, macOS 11 and Ubuntu 18.04 x64.

Comment 29

[Tom Ritter [:tjr]]

Attachment: advisory.txt