1599131 - Remove carve outs for downloads within x-frame-options when fission enabled

Status: RESOLVED FIXED

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

Description

[Christoph Kerschbaumer [:ckerschb]]

Within Bug 1593832 we add some carve outs for downloads within x-frame-options when fission is enabled. Once Bug 1574372 is resolved, we should revisit those carve outs and potentially even enforce XFO entirely in the parent process.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

This one should also block the master Bug 1584991 so we don't forget about it.

Comment 2

[Chris Peterson [:cpeterson]]

Tracking for Fission dogfooding (M5)

Comment 3

[Andrew McCreight [:mccr8]]

Christoph, is this something you could look into now that bug 1574372 has landed? Thanks.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Andrew McCreight [:mccr8] from comment #3)

Christoph, is this something you could look into now that bug 1574372 has landed? Thanks.

Thanks for letting me know. I am working on XFO stuff within Bug 1601887 at the moment and fix this one right after - on it :-)

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Bug 1599131: Remove carve outs for downloads within x-frame-options when fission enabled. r=bz

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Matt, within Bug 1574372 you added supporting functionality which allows us to figure out whether a load will result in a download within the parent process.

In this bug I would need to query that information within FramingChecker::CheckFrameOptions (see attached patch). Can I somehow query that information? Or do I have to set some flag on the channel/loadinfo within the DocumentLoadListener? And if I have to add a flag, where within DocumentLoadListener would be the best place to do so?

Comment 7

[Matt Woodrow (:mattwoodrow)]

I'd take a look at ParentProcessDocumentOpenInfo, which is the helper that runs in the parent and makes decisions about whether we should handle the load as a Document, or if it should be handled by something else (download, xpi install etc).

If you specifically want downloads, then you could add a flag to the channel in TryExternalHelperApp?

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Hey Smaug, we are trying to handle x-frame-options entirely in the parent process. Initially we planned to do that by using observers a la NS_HTTP_ON_EXAMINE_RESPONSE_TOPIC. The problem is that at that time we still do not know whether the resulting load will result in a download or not. And XFO should not block downloads.

I guess my question is:
When is the first time we would know that a load results in a document (top-level/iframe) load?

I think we should move the XFO and also CSP frame-ancestors checks there and get away from NS_HTTP_ON_EXAMINE_RESPONSE_TOPIC. Alternatively (though hacky) we could add a bit to the loadinfo, something like XFOWouldBlockLoad and just check for that bit before we actually start loading the document. I guess moving the XFO checks entirely is the better strategy.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Matt, smaug said it's probably to better ask you - please see my previous comment - thanks!

Comment 10

[Matt Woodrow (:mattwoodrow)]

The ParentProcessDocumentOpenInfo runs before DocumentLoadListener, and handles targeting all cases that aren't going to be handled by the docshell (see diagram on page 2 of https://docs.google.com/document/d/177b4LgwoEAYZt9xwttC5w_TSepqGiMCrom1qrBLCPn8).

Once we get to DocumentLoadListener::OnStartRequest, we know that we're going to be displaying the load as a document (though this includes top-level image loads etc), and the code there handles which process to load it in.

I think it should work to add extra checks at that point.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Bug 1599131: Remove carve outs for downloads within x-frame-options when fission enabled. r=smaug,mattwoodrow

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Hey Smaug, Matt,

as suggested, I moved the CSP frame-ancestor checks as well as x-frame-options checks into DocumentLoadListener::OnStartRequest (see attached patch). That all works fine and downloads are, as expected, not subject to XFO anymore. The only problem is that x-frame-options should display an error page and still fire the onload event. We implemented that within Bug 1601887. Now that we have moved XFO checks into DocumentLoadListener::OnStartRequest, we do not go back to nsDocshell [1] where we would load the error page.

Any suggestions on how to fix that?

[1] https://searchfox.org/mozilla-central/rev/557a0e222dd104c5d805ba344c45d6abc27d3db0/docshell/base/nsDocShell.cpp#9863

Comment 13

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

I think matt should asnwer to this, given how new all the relevant code is.

Comment 14

[Matt Woodrow (:mattwoodrow)]

I think the simplest thing to do is to just not early return from DocumentLoadListener::OnStartRequest.

If we failed the checks, then we should have cancelled mChannel, so what we forward down to the content process will just be the empty OnStart/StopRequest with the cancel status.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/93abc56ef420
Remove carve outs for downloads within x-frame-options when fission enabled. r=smaug,mattwoodrow

Comment 16

[Alexandru Michis [:malexandru]]

Backed out changeset 93abc56ef420 for causing failures in frame-ancestors-from-serviceworker.https.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/67c81abe14b1c05974a505a9491d33be4bc3c623

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=android%2C7.0%2Cx86-64%2Copt%2Cweb%2Cplatform%2Ctests%2Ctest-android-em-7.0-x86_64%2Fopt-geckoview-web-platform-tests-e10s-16%2Cw%28wpt16%29&fromchange=3bdc96cfe1e638e2bd28a21030dde343b07d8a44&tochange=157a80e6e571a2826d0afebb8b3d3bb666b4bbc8&selectedJob=293051661

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=293051661&repo=autoland&lineNumber=1927

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Alexandru Michis [:malexandru] from comment #16)

Backed out changeset 93abc56ef420 for causing failures in frame-ancestors-from-serviceworker.https.html

huh, failures on android? I'll take a look - thanks!

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Matt and/or Snorp, it seems DocumentChannel [0] is not enabled on Android yet (see also Bug 1589982) which causes xfo and frame-ancestors tests to fail on Android [1]. Personally I don't really want to add a carveout for Android (though technically possible) for x-frame-options and frame-ancestors.

What's your take?
Should we wait with this bug to land till Android also relies on DocumentChannel?

[0] https://searchfox.org/mozilla-central/rev/fb3b0075d1a9c4dafbdf339b835d462b5ae55a0e/modules/libpref/init/StaticPrefList.yaml#973
[1] https://treeherder.mozilla.org/pushhealth.html?repo=try&revision=822b39a64fcc8dc9cc5f2603884cb065349176d2

Comment 19

[Matt Woodrow (:mattwoodrow)]

It's being worked on at the moment, so hopefully you won't have to wait too long.

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

In that case I guess we'll just wait with this bug. I think it's not worth adding another carve-out which we have to follow-up on. Thanks for the update!

Comment 21

[Chris Peterson [:cpeterson]]

Moving P2 M5 bugs to M5b milestone

Comment 22

[Chris Peterson [:cpeterson]]

ckerschb says this bug is blocked waiting for Android to support DocumentChannel (bug 1617189).

Deferring to Fission Nightly (M6)

Comment 23

[James Willcox (:snorp) ([e-mail]) (he/him)]

Yeah we're actively working on migrating to DocumentChannel, so I think just disabling the tests for now is fine.

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

(In reply to James Willcox (:snorp) ([e-mail]) (he/him) from comment #23)

Yeah we're actively working on migrating to DocumentChannel, so I think just disabling the tests for now is fine.

James, unfortunately we can't just simply disable tests. If we land as is and Android does not support DocumentChannel, then CSP frame-ancestors and x-frame-options would not be supported on current Android versions.

Just curious, do you have any time estimate on when Android will support DocumentChannel? If you think it takes longer then I might re-consider my decision for this bug and potentially re-write the patch so we only have a carve out for Android but regular Desktop and Desktop fission would rely on the new architecture.

Comment 25

[James Willcox (:snorp) ([e-mail]) (he/him)]

We're shooting for this week, but you know how these things go....

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

(In reply to James Willcox (:snorp) ([e-mail]) (he/him) from comment #25)

We're shooting for this week, but you know how these things go....

Thanks for the update. In that case I'll wait. And yeah, I know how those things are. (FWIW, if your answer would have been weeks from now, then probably I would have added a carveout - in that case I can wait a week or two).

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba5511bfb76b
Remove carve outs for downloads within x-frame-options when fission enabled. r=smaug,mattwoodrow

Comment 28

[Andrei Ciure[:aciure]]

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