1738418 - (CVE-2021-43543) <embed/object> frames bypass parent's CSP `sandbox` header directive

Status: RESOLVED FIXED

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

Description

[Armin Ebert [:arminius]]

A given Content-Security-Policy: sandbox ...; HTTP header directive is not correctly enforced for <embed> and <object> child frames during load, allowing to bypass the set restrictions.

Test case

parent.html:

Content-Security-Policy: sandbox

<iframe src="child.html"></iframe>
<embed src="child.html"></embed>
<object data="child.html"></object>

child.html:

<script>prompt(document.domain)</script>

Expected result

Per the blanket sandbox directive, all three child frames should be treated as cross-origin loads and be denied script execution.

Actual result

The <embed> and <object> children are able to execute same-origin script code.

CSP3 spec reference:

6.2.2. sandbox
The sandbox directive specifies an HTML sandbox policy which the user agent will apply to a resource, just as though it had been included in an iframe with a sandbox property.

I will follow up with additional details to the root cause.

Comment 1

[Armin Ebert [:arminius]]

From what I understand, an object/embed load has to be handled different from an iframe load in a few places because it's not necessarily going to load a document. So for an iframe document load, the LoadInfo is created in DocumentLoadListener::OpenDocument() via

  RefPtr<CanonicalBrowsingContext> browsingContext =
      GetDocumentBrowsingContext();
...
  RefPtr<LoadInfo> loadInfo =
      CreateDocumentLoadInfo(browsingContext, aLoadState);

and

static auto CreateDocumentLoadInfo(CanonicalBrowsingContext* aBrowsingContext,
                                   nsDocShellLoadState* aLoadState)
    -> already_AddRefed<LoadInfo> {
  uint32_t sandboxFlags = aBrowsingContext->GetSandboxFlags();
  RefPtr<LoadInfo> loadInfo;

  auto securityFlags = SecurityFlagsForLoadInfo(aLoadState);

  if (aBrowsingContext->GetParent()) {
    loadInfo = LoadInfo::CreateForFrame(aBrowsingContext,
                                        aLoadState->TriggeringPrincipal(),
                                        securityFlags, sandboxFlags);
...

Here, the sandboxFlags that scripts will be checked against are read from the BC for the embedded subdocument. So in the test case above, the iframe is loaded with the right flags.

Now for object loads, the LoadInfo is constructed via DocumentLoadListener::OpenObject():

  auto sandboxFlags = GetLoadingBrowsingContext()->GetSandboxFlags();

  RefPtr<LoadInfo> loadInfo = CreateObjectLoadInfo(
      aLoadState, aInnerWindowId, aContentPolicyType, sandboxFlags);
...

This takes the loading BC's sandboxFlags. In the test case, that would be the top-level BC which doesn't have any sandbox flags set. It seems that's how the restrictions get dropped here.

Comment 2

[Armin Ebert [:arminius]]

Attachment: testcase_mochitest.patch

Here's an additional mochitest to demonstrate the bug.

0:02.94 TEST_START: dom/security/test/csp/test_bug1738418.html
 0:04.00 FAIL document in <object> should have sandboxed origin - got "mochi.test", expected ""
    SimpleTest.is@SimpleTest/SimpleTest.js:500:14
    @dom/security/test/csp/test_bug1738418.html:17:5
 0:04.00 FAIL document in <embed> should have sandboxed origin - got "mochi.test", expected ""
    SimpleTest.is@SimpleTest/SimpleTest.js:500:14
    @dom/security/test/csp/test_bug1738418.html:17:5
 0:04.00 PASS document in <iframe> should have sandboxed origin
 0:04.01 TEST_END: Test OK. Subtests passed 1/3. Unexpected 2

Comment 3

[:Gijs (out for now; he/him)]

Christoph, can you make sure this ends up in front of the right people?

Comment 4

[Daniel Veditz [:dveditz]]

Freddy: can you take this one on?

Comment 5

[Frederik Braun [:freddy]]

Wow, thanks so much for the detailed description and testcase, that's super useful!
I'm taking a look.

Comment 6

[Frederik Braun [:freddy]]

Attachment: Bug 1738418 - Testcase r?ckerschb

Comment 7

[Frederik Braun [:freddy]]

Attachment: Bug 1738418 - set sandboxflags for object/embed loads correctly r?ckerschb,smaug

Depends on D130958

Comment 8

[Frederik Braun [:freddy]]

A likely regression is bug 164689. But with all previously involved people (patch author and reviewers) having left Mozilla, I'm taking a wild guess for reviews.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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

A likely regression is bug 164689.

That looks very unlikely. Does the bug number perhaps miss one digit?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

bug 1646899 might be the right one.

Comment 11

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

set sandboxflags for object/embed loads correctly r=ckerschb,smaug,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/4e15ae576cf571aff74d534396a8550ce0f1aae3
https://hg.mozilla.org/mozilla-central/rev/4e15ae576cf5

Comment 12

[Frederik Braun [:freddy]]

test needs landing too. do we need to wait?

Comment 13

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

The patch landed in nightly and beta is affected.
:freddy, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Test should wait, yes. Please move ahead with the Beta & ESR91 requests.

Comment 15

[Pascal Chevrel:pascalc]

Our last beta builds in a few hours, do we want to uplift that in 95 beta and ESR91 this cycle?

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 9250343 [details]
Bug 1738418 - set sandboxflags for object/embed loads correctly r?ckerschb,smaug

Beta/Release Uplift Approval Request

• User impact if declined: Our Content Security implementation correctly applies provided sandbox attribute to loads of type iframe but not to loads of type embed and object which in turn grants JS code in embed and object to run with same-origin privileges.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• 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): Risk is low since we are only propagating the missed sandbox flags to loads of type embed and object. (Every page not providing a CSP sandbox attribute is unaffected).
• String changes made/needed: no

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: As above.
• User impact if declined:
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String or UUID changes made by this patch:

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 9248526 [details] [diff] [review]
testcase_mochitest.patch

This file does not need to be uplifted.

Comment 18

[Pascal Chevrel:pascalc]

Comment on attachment 9250343 [details]
Bug 1738418 - set sandboxflags for object/embed loads correctly r?ckerschb,smaug

Approved for landing on the beta95 and esr91.4 branches, thanks!

Comment 19

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-esr91/rev/0e7140b8eb95

Comment 20

[Pascal Chevrel:pascalc]

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

Comment 21

[Pascal Chevrel:pascalc]

I am leaving the uplifting of the testcase to next cycle, once it has also landed on mozilla-central.

Comment 22

[Frederik Braun [:freddy]]

Hey, thank you for handling the patch uplift in my absence.

Comment 23

[Frederik Braun [:freddy]]

Attachment: advisory.txt

Comment 24

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

Testcase r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/77ff2a35889db18f7df08a474094cc95f090344f
https://hg.mozilla.org/mozilla-central/rev/77ff2a35889d

Comment 25

[Frederik Braun [:freddy]]

Looks like I landed the test and never set the testsuite fag to +.