1619338 - Loads triggered by content scripts can be used to bypass X-Frame-Options

Status: VERIFIED FIXED

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

Description

[Rob Wu [:robwu]]

Attachment: xfo-bypass.zip

In bug 1595652, a change was made to allow extensions to load frames in extension pages, even if normally blocked by XFO. An undesired side effect of the change is that the mechanism allows content scripts to bypass XFO.

The check needs to be more strict, to only allow extensions to bypass XFO if the load is inside the extension process, or similarly, if the top-level is the extension's principal.

STR:

1. Visit about:debugging and load the attached add-on.
2. Visit https://example.com
3. The extension loads a frame. Look at the frame's content and optionally the console of the tab's devtools.

Expected:

• The frame should be blank. The console of the tab's devtools should show:

  Load denied by X-Frame-Options: “SAMEORIGIN” from “https://www.amazon.com/favicon.ico”, site does not permit cross-origin framing from “http://example.com/”.

Actual:

• The frame shows Amazon's favicon.

Comment 1

[Daniel Veditz [:dveditz]]

Need to check CSP frame-ancestors -- I believe the code for that and XFO have not been merged yet but they should behave the same. moz-extension:// pages should be able to force-frame XFO pages just as browser UI can, but privileged scripts (system or extension) should not be able to cause XFO pages to load in a normal-page frame.

Comment 2

[Rob Wu [:robwu]]

(In reply to Daniel Veditz [:dveditz] from comment #1)

moz-extension:// pages should be able to force-frame XFO pages just as browser UI can,

Note that just comparing the document/principal's URL against moz-extension: is not sufficient. Pages from extensions that use web_accessible_resources can also be embedded by web content. These embedded pages are not as privileged as extension pages, and it should not be possible to bypass XFO via them.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Boris, it seems currently we use loadInfo->TriggeringPrincipal()->GetIsAddonOrExpandedAddonPrincipal() [1] to determine whether a load was triggered by an extension and hence should bypass x-frame-options check. Unfortunately, using that principal check also allows content scripts to bypass XFO. Is there a good and easy alternative, so we could basically simply replace loadInfo->TriggeringPrincipal()->GetIsAddonOrExpandedAddonPrincipal() with a better check to get the desired behavior?

[1] https://searchfox.org/mozilla-central/rev/91f6c02fcf4c16f78fdc4417f61f192688294066/dom/security/FramingChecker.cpp#252

Comment 4

[Boris Zbarsky [:bzbarsky]]

The triggering principal is the thing that started the load, right? That doesn't seem like the right thing to be checking if we want to disallow extensions putting things in content pages.

What is the loadingPrincipal in these "extension navigates a subframe of a webpage" cases? Is it the extension or the webpage? What is it when the load is in the cases when we do want to bypass XFO?

If an extension loads a content page inside itself and then loads a subframe inside that page, should XFO apply to that subframe, by the way? Comment 0's "if the top-level is the extension's principal" makes it sound like "no", but it's not clear to me that this is what Rob actually meant to say...

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

I guess it's a question for robwu...

Comment 6

[Rob Wu [:robwu]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #4)

If an extension loads a content page inside itself and then loads a subframe inside that page, should XFO apply to that subframe, by the way?

I'd say yes, to have a secure default.

Comment 0's "if the top-level is the extension's principal" makes it sound like "no", but it's not clear to me that this is what Rob actually meant to say...

XFO should be respected as much as possible, except for the special case when a privileged extension page attempts to embed the frame (bug 1595652).
As mentioned in comment 2, just checking the principal (of the frame host / request initiator) is not sufficient, because moz-extension:-pages are unprivileged when embedded as a document in a different frame. This means that the top-level principal (and more generally: all ancestors) should have the same moz-extension:-principal.

Comment 7

[Tomislav Jovanovic :zombie]

The "all ancestors have the same moz-extension: principal" function we use to determine if something is a privileged extension page is [1], but the full check confirms the process as well [2].

https://searchfox.org/mozilla-central/rev/5a52cec97c/toolkit/components/extensions/ExtensionPolicyService.cpp#418
https://searchfox.org/mozilla-central/rev/5a52cec97c/toolkit/components/extensions/ExtensionPolicyService.cpp#476

I'm not sure we can easily get to the window from the aChannel in FramingChecker::CheckFrameOptions, though down the line with Fission, this whole check might boil down to just confirming the correct process.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1619338_xfo_moz_extension.patch

Rob, I finally found some time to look into this problem with some more care. I can reproduce using the provided xfo-bypass.zip. As discussed on slack one possible option would be to traverse all the documents up to the root and check that all NodePrincipals are of type 'moz-extension'. I incorporated that idea in the attached patch.

Now here is the catch:
The attached patch fixes the problem in the testcase, but mostly because of those three lines:

if (!aLoadInfo->TriggeringPrincipal()->SchemeIs("moz-extension")) {
return false;
}

In other words, it seems GetIsAddonOrExpandedAddonPrincipal() returns a different result than Principal()->SchemeIs("moz-extension"). Is that expected? Put differently, would it suffice to just update the GetIsAddonOrExpandedAddonPrincipal() to check for moz-extension like in the patch or do we still need to travese all the way up to the root document?

FWIW, after writing the patch I realized that XFO is enforced in the parent, so I don't think we can actually query the Document. If the fundamental idea of the patch is right however I think we could add some information about the scheme to the browsingContext.

Comment 9

[Rob Wu [:robwu]]

In other words, it seems GetIsAddonOrExpandedAddonPrincipal() returns a different result than Principal()->SchemeIs("moz-extension")

This is expected and desired. Content scripts have an Expanded principal, and we shouldn't allow them to bypass XFO.

Currently you're allowing nested moz-extension URLs from different extensions to bypass XFO. I would make the condition even stricter.
Content in a frame can only bypass XFO if the parent is a moz-extension:-principal (principal, to allow about:blank but rule out sandboxed frames), and all ancestors until the top have the same principal.

Such logic could also be useful for other situations, from https://bugzilla.mozilla.org/show_bug.cgi?id=1629436#c7

FYI: using triggeringPrincipal to make decisions may not always result in the desired behavior for add-on devs (too strict). That's not a concern for this bug, but you may be interested in https://bugzilla.mozilla.org/show_bug.cgi?id=1595652#c6 .

Comment 10

[Rob Wu [:robwu]]

As seen in https://bugzilla.mozilla.org/show_bug.cgi?id=1595652#c11 , the patch there was misguided.

I'm in favor of just reverting https://hg.mozilla.org/mozilla-central/rev/2ac103491660 to fix this issue. Do you agree?

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Rob Wu [:robwu] from comment #10)

I'm in favor of just reverting https://hg.mozilla.org/mozilla-central/rev/2ac103491660 to fix this issue. Do you agree?

I think that makes the most sense in the end. I just reverted that change and testet:
(a) https://bugzilla.mozilla.org/show_bug.cgi?id=1619338#c0 -> The frame gets correctly blocked by XFO.
(b) https://bugzilla.mozilla.org/show_bug.cgi?id=1595652#c0 -> The extension popup works correctly.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Bug 1619338: Enforcing XFO in parent allows us to remove extension carve-out. r=robwu

Comment 13

[Rob Wu [:robwu]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #11)

(In reply to Rob Wu [:robwu] from comment #10)

I'm in favor of just reverting https://hg.mozilla.org/mozilla-central/rev/2ac103491660 to fix this issue. Do you agree?

I think that makes the most sense in the end. I just reverted that change and testet:
(a) https://bugzilla.mozilla.org/show_bug.cgi?id=1619338#c0 -> The frame gets correctly blocked by XFO.

As expected from the change :)

(b) https://bugzilla.mozilla.org/show_bug.cgi?id=1595652#c0 -> The extension popup works correctly.

That's because the extension already uses the webRequest API to remove the X-Frame-Options header. It would have worked regardless of the original patch.

Comment 14

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

https://hg.mozilla.org/integration/autoland/rev/a9df87dcb3919493299e9c601ca1151f9bddd4a2
https://hg.mozilla.org/mozilla-central/rev/a9df87dcb391

Comment 15

[Alin Ilea, Desktop QA]

Hi,

I have managed to reproduce the initial issue using Beta build 76.0 (build id: 20200429185419) using Windows 10.
In latest Nightly 77.0a1 (build id: 20200503214759) the following message is displayed inside the frame:
"Blocked by X-Frame-Options Policy
An error occurred during a connection to www.amazon.com.
Nightly prevented this page from loading in this context because the page has an X-Frame-Options policy that disallows it."

From comment 11 and comment 13 I assume this is the expected behavior now, but before marking this as verified,@Christoph can you please confirm this?

Thanks.

Comment 16

[Rob Wu [:robwu]]

Hi Alin, which test are you running? The test case from this bug report does not reference translate.google.com at all.

Comment 17

[Alin Ilea, Desktop QA]

Hi Rob,

I'm running the test case from this bug, but I was wrong about the message from comment 15, I just update it.
"Blocked by X-Frame-Options Policy
An error occurred during a connection to www.amazon.com.
Nightly prevented this page from loading in this context because the page has an X-Frame-Options policy that disallows it."

This is the message displayed inside the frame.

Thanks.

Comment 18

[Rob Wu [:robwu]]

Looks good to me.

Comment 19

[Alin Ilea, Desktop QA]

Great, I will update the flags accordingly.

Thank you.