1755081 - (CVE-2022-29909) Cross-origin embeds/objects can obtain permissions of the top-level origin

Status: RESOLVED FIXED

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

Description

[Armin Ebert [:arminius]]

Documents in nested cross-origin browsing contexts can obtain permissions granted to the top-level origin. This could e.g. be a 3rd-party <iframe> accessing the webcam after permission had been given to the parent site, or prompting for geolocation access in the parent's name.

The attack is based on exploiting faulty FeaturePolicy inheritance by inserting a non-<iframe> embedder (e.g. <embed>) in the ancestor chain.

Attack scenario

• The user has granted a permanent microphone permission to voicechat.example.
• voicechat.example embeds a cross-origin advertisement via <iframe src="https://advertiser.example">.
• Once loaded, advertiser.example can access the microphone without user interaction.

If permission had not been granted yet, advertiser.example may also prompt for it. The dialog will then still appear as a request by voicechat.example and not the attacker. (It's by design that the prompt always requests as the top-level origin.)

Proof of concept

(These are identical to the attached poc.zip.)

https://site.example/site.html:

<iframe src="https://attacker.example/attacker_parent.html"></iframe>

https://attacker.example/attacker_parent.html:

<embed src="attacker_child.html"></embed>

https://attacker.example/attacker_child.html:

<button id="ask">access microphone</button>
<div id="info"></div>
<script>
  ask.onclick = async function() {
    const stream = await navigator.mediaDevices.getUserMedia({audio: true});
    info.textContent = `origin ${origin} got ${stream}`;
  }
</script>

A click on "access microphone" will give attacker.example access based on the current permissions for site.example. However, it should have been neither promted for nor granted, because the request originates from a cross-origin frame without special permissions.

I'll follow up with more details.

Comment 1

[Armin Ebert [:arminius]]

Attachment: poc.zip

Comment 2

[Armin Ebert [:arminius]]

Here's more background on the root cause.

For a number of permission types, the delegation to child frames is governed by the Feature Policy.

PermissionDelegateHandler.cpp:

    {"geolocation", u"geolocation", DelegatePolicy::eDelegateUseFeaturePolicy},
    ...
    {"camera", u"camera", DelegatePolicy::eDelegateUseFeaturePolicy},
    {"microphone", u"microphone", DelegatePolicy::eDelegateUseFeaturePolicy},
    {"screen", u"display-capture", DelegatePolicy::eDelegateUseFeaturePolicy},

PermissionDelegateHandler.h:

    /* Permission is delegated using Feature Policy. Permission is denied by
     * default in cross origin iframe and the iframe only could get/set
     * permission if there's allow attribute set in iframe. e.g allow =
     * "geolocation" */
    eDelegateUseFeaturePolicy,

A nested cross-origin frame can only make use of these features if the permission was delegated from their parent. So when a document is initialized, it tries to inherit the parent policy in Document::InitFeaturePolicy():

nsresult Document::InitFeaturePolicy(nsIChannel* aChannel) {
  MOZ_ASSERT(mFeaturePolicy, "we should only call init once");

  mFeaturePolicy->ResetDeclaredPolicy();

  mFeaturePolicy->SetDefaultOrigin(NodePrincipal());

  RefPtr<mozilla::dom::FeaturePolicy> parentPolicy = GetParentFeaturePolicy();
  if (parentPolicy) {
    // Let's inherit the policy from the parent HTMLIFrameElement if it exists.
    mFeaturePolicy->InheritPolicy(parentPolicy);
    mFeaturePolicy->SetSrcOrigin(parentPolicy->GetSrcOrigin());
  }
...

However, if GetParentFeaturePolicy() returns null, the policy remains at its origin's default. This can happen when the embedder is not an HTMLIFrameElement (e.g. with HTMLEmbedElement).

already_AddRefed<dom::FeaturePolicy> Document::GetParentFeaturePolicy() {
  BrowsingContext* browsingContext = GetBrowsingContext();
  if (!browsingContext) {
    return nullptr;
  }
  if (!browsingContext->IsContentSubframe()) {
    return nullptr;
  }

  HTMLIFrameElement* iframe =
      HTMLIFrameElement::FromNodeOrNull(browsingContext->GetEmbedderElement());
  if (iframe) {
    return do_AddRef(iframe->FeaturePolicy());
  }

  if (XRE_IsParentProcess()) {
    return do_AddRef(browsingContext->Canonical()->GetContainerFeaturePolicy());
  }

So a cross-origin non-<iframe> document would keep its default feature policy instead of inheriting a limited one. And since the origin check to ultimately grant (or prompt the user for) access to the feature (e.g. when geolocation.getCurrentLocation() is called) is going to be performed against the top-level origin, the document now acts with the top-level context's permission state.

Comment 3

[Armin Ebert [:arminius]]

Attachment: Proof of concept: load this HTML file in a cross-origin <iframe>

Comment 4

[Armin Ebert [:arminius]]

Attachment: poc_testcase_mochitest.patch

Here's the PoC in the guise of a mochitest.

Comment 5

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Andreas (:farre) said he will look into a bit and see what's up around FeaturePolicy code.

Comment 6

[Daniel Veditz [:dveditz]]

Andreas, please see comment 5

Comment 7

[Andreas Farre [:farre]]

Yeah, this is missing functionality. The spec states that container policies[1] is inherited, but we only consider iframes, not all containers. So, HTMLEmbedElement and HTMLObjectElement doesn't have a FeaturePolicy-getter as in HTMLIFrameElement[2]. I'm moving this to DOM: Security, since that's where the Permissions Policy feature lives. Is that ok with you Christoph?

Fixing this would need adding a FeaturePolicy-member, getter and initialization to HTMLEmbedElement and HTMLObjectElement, as well as adding those cases to Document::GetParentFeaturePolicy().

[1] https://w3c.github.io/webappsec-permissions-policy/#container-policies
[2] https://searchfox.org/mozilla-central/source/dom/html/HTMLIFrameElement.h#159

Comment 8

[Frederik Braun [:freddy]]

I'll give it a shot.

Comment 9

[Andreas Farre [:farre]]

(In reply to Andreas Farre [:farre] from comment #7)

Fixing this would need adding a FeaturePolicy-member, getter and initialization to HTMLEmbedElement and HTMLObjectElement, as well as adding those cases to Document::GetParentFeaturePolicy().

I guess some kind of common base class would do as well, I guess. A bit cleaner, and would only need one case in GetParentFeaturePolicy if the base-class is convertible from the node.

Comment 10

[Frederik Braun [:freddy]]

My initial understanding was based on the assumption that there are just missing functions that can be copied over, but that's not entirely it.
I currently encounter some assumptions from the implementation that I don't fully get:
AFAIU, they are more deeply rooted into the fact that the HTMLIframeElement is a nsGenericHTMLFrameElement but the HTMLObjectElement is reusing code for... image loads? E.g., there is no mFrameLoader and querying the DocShell and BrowsingContext is arcane magic to me at this point.

Given how much legacy we are facing with object & embed elements, I also wonder if it would make sense to hack around this and just disallow all permissioned-APIs within object & embed elements instead?

Anyway, I'll upload a WIP patch with some comments and questions and would appreciate working with you on this, Andreas.

Comment 11

[Frederik Braun [:freddy]]

Attachment: WIP: Bug 1755081 - featurepolicy for object/embed r?farre

Comment 12

[Anne (:annevk)]

So, we shouldn't honor the allow attribute on embed/object elements (and note that embed uses src, but object uses data, and neither has srcdoc). So indeed anything permission-related that has to be delegated will not work from a cross-origin document embedded through embed or object. Same-origin should still work though.

Comment 13

[Frederik Braun [:freddy]]

Attachment: WIP: Bug 1755081 - test case r=farre

Comment 14

[Frederik Braun [:freddy]]

Yes, I fully agree and do not plan to introduce new attributes on those elements.
I may have left some of that in, accidentally.

Given how things currently work, we do need to pass some info down to the embedded Document to tell it, that's currently subject to a policy. (In the future, we'd probably like this to be properly Site-Isolated and perform the check by asking the parent instead).

Comment 15

[Armin Ebert [:arminius]]

Attachment: Mochitest: FeaturePolicy delegation to nested BCs

I've attached another mochitest that might be more convenient for patch development. This one directly checks the feature policy delegation on different types of embedders and nesting levels.

(The earlier testcase (attachment 9263642 [details] [diff] [review]) is a wrapper for the entire PoC, i.e. demonstrates a practical scenario where the attack is launched from a cross-origin <iframe>.)

Comment 16

[Frederik Braun [:freddy]]

Attachment: Bug 1755081 - Add tests for more containers. r=smaug!

Comment 17

[Donal Meehan [:dmeehan]]

:freddy does this need to go out with 99 or can it wait until 100?
We are the final week of beta for 99,

Comment 18

[Frederik Braun [:freddy]]

I'm making some progress, but it's unlikely this will make it. I was on sick leave last week and have to take my PTO before it expires this & next week. I'll try to nudge things along, but there seem to be some issues...

Comment 19

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

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

For more information, please visit auto_nag documentation.

Comment 20

[Andreas Farre [:farre]]

Attachment: Bug 1755081 - Allow more containers to participate in FeaturePolicy r=smaug!,ckerschb!

Depends on D140817

Comment 21

[Andreas Farre [:farre]]

Comment on attachment 9269672 [details]
Bug 1755081 - Allow more containers to participate in FeaturePolicy r=smaug!,ckerschb!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: There is a follow-up testcase in https://phabricator.services.mozilla.com/D140817 that tests the bug, so pretty easy.

The test does more or less describe exactly how to exploit this issue though.

• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• 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?: Yes
• If not, how different, hard to create, and risky will they be?: Patches apply cleanly on beta and esr91 and the test in D140817 passes as well. Haven't tested release, but I'm assuming that same goes for that branch.
• How likely is this patch to cause regressions; how much testing does it need?: The code added to nsObjectLoadingContent is fairly similar to what nsHTMLIFrame already has, and the extra steps in Document::GetParentFeaturePolicy() are hopefully trivial enough.

Comment 22

[Tom Ritter [:tjr]]

Comment on attachment 9269672 [details]
Bug 1755081 - Allow more containers to participate in FeaturePolicy r=smaug!,ckerschb!

Approved to land and request uplift

Comment 23

[Tom Ritter [:tjr]]

Comment on attachment 9267341 [details]
Bug 1755081 - Add tests for more containers. r=smaug!

Clearing sec-approval, we can land the tests on or after May 16

Comment 24

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

Allow more containers to participate in FeaturePolicy r=smaug,ckerschb
https://hg.mozilla.org/integration/autoland/rev/ca23a4fb182d5a4b20e54898ce76b9495c23ce89
https://hg.mozilla.org/mozilla-central/rev/ca23a4fb182d

Comment 25

[Andreas Farre [:farre]]

Comment on attachment 9269672 [details]
Bug 1755081 - Allow more containers to participate in FeaturePolicy r=smaug!,ckerschb!

Beta/Release Uplift Approval Request

• User impact if declined: Possible use of permission policy protected features
• Is this code covered by automated tests?: No
• 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): Behavior is similar to how iframe does this. Automated test is pending.
• String changes made/needed:

Comment 26

[Dianna Smith [:diannaS]]

Comment on attachment 9269672 [details]
Bug 1755081 - Allow more containers to participate in FeaturePolicy r=smaug!,ckerschb!

Approved for 100.0b6

Comment 27

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1d40eef1b07e

Comment 28

[Daniel Veditz [:dveditz]]

Documenting that ESR-91 is affected: this feature was enabled in Release for Firefox 74 in bug 1617219

Comment 29

[Andreas Farre [:farre]]

Need info for myself to remember landing the test.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Is this ready for an ESR91 approval request? We need to build the RC build this week. It grafts cleanly as-landed.

Comment 31

[Andreas Farre [:farre]]

Comment on attachment 9269672 [details]
Bug 1755081 - Allow more containers to participate in FeaturePolicy r=smaug!,ckerschb!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Possible use of permission policy protected features
• Fix Landed on Version: 101
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Behavior is similar to how iframe does this. Automated test is pending.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9269672 [details]
Bug 1755081 - Allow more containers to participate in FeaturePolicy r=smaug!,ckerschb!

Approved for 91.9esr.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

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

Comment 34

[Frederik Braun [:freddy]]

Attachment: advisory.txt

Comment 35

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

Test landed: https://hg.mozilla.org/integration/autoland/rev/138b555d219f584ecb2dbf09bbee4713674d53a8

Backed out for causing mochitest failures in test_nested.html:
https://hg.mozilla.org/integration/autoland/rev/41cd304ad950ac66f7496ff2995467df09acb8b3

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=R12FHO08Q320cZZzijNDzg.0&searchStr=linux%2C18.04%2Cx64%2Cwebrender%2Cdebug%2Cmochitests%2Cwith%2Ccross-origin%2Cand%2Cfission%2Cenabled%2Ctest-linux1804-64-qr%2Fdebug-mochitest-plain-xorig%2C9&revision=abb9371221360a90228fa480693e5d5cdba60312
Failure log link: https://treeherder.mozilla.org/logviewer?job_id=384082302&repo=autoland
Failure line: TEST-UNEXPECTED-FAIL | dom/security/featurepolicy/test/mochitest/test_nested.html | permission delegation to <iframe src="https://example.org/tests/dom/security/featurepolicy/test/mochitest/empty.html"></iframe> - false == true - got false, expected true (operator ==)

Comment 36

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

7 months ago, Tom Ritter [:tjr] placed a reminder on the bug using the whiteboard tag [reminder-test 2022-05-16] .

farre, please refer to the original comment to better understand the reason for the reminder.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8de460cdc4ed
Add tests for more containers. r=smaug

Comment 38

[pstanciu]

Backed out for causing permision failures in test_nested.html

• Backout link
• Push with failures
• Failure Log

Comment 39

[Andreas Farre [:farre]]

This test will land alongside the tests in bug 1890748.

Comment 40

[Daniel Holbert [:dholbert]]

(In reply to Andreas Farre [:farre] from comment #39)

This test will land alongside the tests in bug 1890748.

It looks like bug 1890748 landed -- maybe/probably this test can re-land now, too?

(It sounds like maybe you intended to re-land this as part of the same stack, but it doesn't look like that happened, unless the test was moved/renamed.)

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a91356b00527
Add tests for more containers. r=smaug

Comment 42

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/a91356b00527