Closed Bug 1599131 Opened 2 years ago Closed 1 year ago

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

Categories

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

task

Tracking

()

RESOLVED FIXED
mozilla77
Fission Milestone M6
Tracking Status
firefox77 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file, 1 obsolete file)

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.

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

Tracking for Fission dogfooding (M5)

Fission Milestone: --- → M5

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

Flags: needinfo?(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 :-)

Flags: needinfo?(ckerschb)
Blocks: 1614408

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?

Flags: needinfo?(matt.woodrow)

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?

Flags: needinfo?(matt.woodrow)

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.

Flags: needinfo?(bugs)

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

Flags: needinfo?(bugs) → needinfo?(matt.woodrow)

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.

Flags: needinfo?(matt.woodrow)
Attachment #9130127 - Attachment is obsolete: true

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

Flags: needinfo?(matt.woodrow)
Flags: needinfo?(bugs)
Blocks: 631862

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

Flags: needinfo?(bugs)

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.

Flags: needinfo?(matt.woodrow)
See Also: → 1620709
Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/93abc56ef420
Remove carve outs for downloads within x-frame-options when fission enabled. r=smaug,mattwoodrow

(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!

Flags: needinfo?(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

Flags: needinfo?(snorp)
Flags: needinfo?(matt.woodrow)

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

Flags: needinfo?(matt.woodrow)

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!

Depends on: 1589982

Moving P2 M5 bugs to M5b milestone

Fission Milestone: M5 → M5b

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

Deferring to Fission Nightly (M6)

Fission Milestone: M5b → M6

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

Flags: needinfo?(snorp)

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (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.

Flags: needinfo?(snorp)

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

Flags: needinfo?(snorp)

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (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).

Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/autoland/rev/ba5511bfb76b
Remove carve outs for downloads within x-frame-options when fission enabled. r=smaug,mattwoodrow
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77
Duplicate of this bug: 1620709
You need to log in before you can comment on or make changes to this bug.