Closed Bug 1768537 (CVE-2022-34468) Opened 2 years ago Closed 2 years ago

CSP sandbox header without `allow-scripts` can be bypassed via retargeted javascript: URI

Categories

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

defect

Tracking

()

VERIFIED FIXED
103 Branch
Tracking Status
firefox-esr91 102+ verified
firefox101 --- wontfix
firefox102 + verified
firefox103 + verified

People

(Reporter: arminius, Assigned: pbz)

References

Details

(Keywords: csectype-sop, sec-high, Whiteboard: [domsecurity-active][adv-main102+][adv-esr91.11+])

Attachments

(3 files, 1 obsolete file)

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().)

Flags: sec-bounty?
Group: core-security → dom-core-security

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

Flags: needinfo?(pbz)

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

Flags: needinfo?(pbz) → needinfo?(fbraun)
Flags: needinfo?(fbraun)

(Done)

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))

Flags: needinfo?(dveditz)
Flags: needinfo?(pbz)

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

Flags: needinfo?(pbz) → needinfo?(nika)

(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

Flags: needinfo?(nika)
Assignee: nobody → pbz
Status: NEW → ASSIGNED
Flags: needinfo?(dveditz)
See Also: → CVE-2022-29911
Attached file Bug 1768537, r=smaug!
Attached file Bug 1768537 - tests, r=smaug! (obsolete) —

Depends on D146914

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
Attachment #9277488 - Flags: sec-approval?
Attachment #9277489 - Flags: sec-approval?

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(ckerschb)
Severity: -- → S3
Flags: needinfo?(ckerschb)
Priority: -- → P1
Whiteboard: [domsecurity-active]

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.

Flags: needinfo?(pbz)
Flags: needinfo?(pbz) → needinfo?(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.

Severity: S3 → S2
Flags: needinfo?(ckerschb)

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

Approved to land and uplift

Attachment #9277488 - Flags: sec-approval? → sec-approval+

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

Test approved to land Aug 15 or later

Attachment #9277489 - Flags: sec-approval?
Flags: in-testsuite?
Blocks: 1772290

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

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

Attachment #9277489 - Attachment is obsolete: true
Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 103 Branch

Please nominate this for Beta & ESR approval.

Attached file 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 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.
  1. 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.
Flags: needinfo?(pbz)
Attachment #9277488 - Flags: approval-mozilla-esr91?
Attachment #9277488 - Flags: approval-mozilla-esr102?
Attachment #9277488 - Flags: approval-mozilla-beta?
Flags: qe-verify+

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

QA Whiteboard: [qa-triaged]

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Attachment #9277488 - Flags: approval-mozilla-esr102?

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

Approved for 102 beta 5, thanks.

Attachment #9277488 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

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

Approved for 91.11esr.

Attachment #9277488 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+
Flags: sec-bounty? → sec-bounty+
Whiteboard: [domsecurity-active] → [domsecurity-active][adv-main102+]

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

Whiteboard: [domsecurity-active][adv-main102+] → [domsecurity-active][adv-main102+][adv-esr91.11+]
Alias: CVE-2022-34468
Flags: in-testsuite? → in-testsuite+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: