1595762 - Honor X-Frame-Options / frame-ancestors for all embed/object loads

Status: RESOLVED FIXED

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

Description

[Anne (:annevk)]

This is important for the security guarantees around Fetch metadata headers and for being able to simplify our loading setup around the embed and object elements.

Comment 1

[Jonathan Kingston [:jkt] he/him]

I thought this is what was just re-fixed with the fission bug? Perhaps I a missing more of this task.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Please note that we are already doing the correct checks for XFO [0] but apparently we missed embed/object for frame-ancestors (the attached patch will fix that) when we made frame-ancestors fission compatible (Bug 1584993). Further, TYPE_INTERNAL_EMBED [1] as well as TYPE_INTERNAL_OBJECT will both be mapped to TYPE_OBJECT.

Was there any specific testcase you came across this? Besides the missing bits for frame-ancestors (which the patch will fix) I suppose that should just work with fetch, or is there anything we are missing in addition?

[0] https://searchfox.org/mozilla-central/source/dom/security/FramingChecker.cpp#221
[1] https://searchfox.org/mozilla-central/rev/6566d92dd46417a2f57e75c515135ebe84c9cef5/dom/base/nsIContentPolicy.idl#213

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Bug 1595762: Make type object loads subject to CSP frame ancestors. r=annevk

Comment 4

[Anne (:annevk)]

The idea would be to also block if the response is for an image or plugin, e.g., data:text/html,<embed src="https://avatars3.githubusercontent.com/u/665379?s=88&v=4"> or data:text/html,<embed src="https://avatars3.githubusercontent.com/u/665379?s=88&v=4" type="image/jpeg">, but there are some compatibility risks with doing that so we'd need to roll that out carefully.

(This is now tracked by bug 1596402.)

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/da9476881d62
Make type object loads subject to CSP frame ancestors. r=jkt,annevk

Comment 6

[Daniel Varga [:dvarga]]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

I'm a little worried by the resulting code here. The code in PermitsAncestry happens to be checking the window attached to the loadinfo, not just its ancestors, which is arguably wrong for the subdocument case but critically important for the object case, right? At the very least, that code should have comments explaining why that window, not just its ancestors, needs to be checked...

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

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

I'm a little worried by the resulting code here. The code in PermitsAncestry happens to be checking the window attached to the loadinfo, not just its ancestors, which is arguably wrong for the subdocument case but critically important for the object case, right? At the very least, that code should have comments explaining why that window, not just its ancestors, needs to be checked...

Sorry, I don't fully understand what you mean and if there is a problem I would like to understand it entirely. The code in permitsAncestry [1] queries the browsing context from the loadinfo, which is the context this frame will be loaded into, and then walks all the parent browsing contexts (the ancestor chain), correct? As far as I understand this is what should be happening, but as I said, maybe I am missing something. If so, please let me know!

[1] https://searchfox.org/mozilla-central/rev/cac340f10816707e91c46db6d285f80259420f07/dom/security/nsCSPContext.cpp#1531

Comment 9

[Boris Zbarsky [:bzbarsky]]

For iframe loads, is the browsing context from the loadinfo the browsing context inside the iframe, or the browsing context of the iframe's parent document?

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

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

For iframe loads, is the browsing context from the loadinfo the browsing context inside the iframe, or the browsing context of the iframe's parent document?

It's the iframe's parent document, for quering the iframe browsing context itself I thought one would have to query frameBrowsingContext, see:
https://searchfox.org/mozilla-central/rev/cac340f10816707e91c46db6d285f80259420f07/netwerk/base/nsILoadInfo.idl#679

Comment 11

[Boris Zbarsky [:bzbarsky]]

Ah, I see. I wish this were clearly documented in the loadinfo API... Right now the documentation for all the windowid and browsing context id bits is somewhere between "confusing" and "misleading". :(

Reading through the code carefully, it looks like .browsingContext on the loadinfo returns the following things (for a selected subset of possible loadinfos; I didn't look hard into all the other cases):

• Iframe loads: the browsing context of the document the <iframe> element is in.
• Toplevel loads: the browsing context the load is happening in.
• object/embed loads: the browsing context of the document the <object>/<embed> element is in.

So the PermitsAncestry bits are OK as long as we never have toplevel loads come through there.

As for .frameBrowsingContext, its behavior is even more bizarre:

• iframe loads: the browsing context inside the iframe
• object/embed loads: Hard to say. Might be the browsing context of the document the <object>/<embed>` is in, but might be the browsing context inside the object/embed if there happens to be one from the previous load, depending on what exactly got loaded previously. I'm not actually sure what happens here; might depend on whether we drop the old frameloader when a new load starts...

It really would be good to get this documented. Do you know who might be in a good position to do that?

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

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

It really would be good to get this documented. Do you know who might be in a good position to do that?

I agree, it's too confusing. It seems Nika added browsingContext as well as frameBrowsingContext to nsILoadInfo within Bug 1467223 and Bug 1522637 but it seems Nika is on leave currently.

Neha, who might be able to update documentation to reflect the scenarios Boris outlined in the previous comment? Could you please forward the ni? to someone in the team?

Comment 13

[Neha Kochar [:neha]]

Andreas, can you update the documentation to add clarity? Thanks!

Comment 14

[Neha Kochar [:neha]]

Filed bug 1600375 for this request.