1584998 - Make x-frame-options work with fission enabled

Status: RESOLVED FIXED

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

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Sorry, I picked the wrong one - Bug 1584993 is what I was looking for, because I think we should fix Bug 1584993 before this one.

Comment 2

[Neha Kochar [:neha]]

This work will be required to re-enable dom/base/test/test_x-frame-options.html for Mochitests-fission.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Nika, in case XFO denies framing of a document our current implementation cancels the load and instead loads about:blank into that iframe. Now that we are moving XFO into the parent (which will happen somewhere around here [1]) we would like to keep current semantics. Please note at this point no docshell has been created yet, we are really just observing http response headers. Anyway, if a load gets blocked I am currently doing something like

channel->Cancel(errorCode);
// fill loadstate with appropriate info
ctx->LoadURI(nullptr, loadState);

Since we are in the parent process I am hoping to enter that branch [2] which should then cause the child to load about:blank. Problem being, Canonical()->GetCurrentWindowGlobal() returns a nullptr and hence we crash. Shouldn't we always have a WindowGlobal at that point?

Or put differently, is this the right approach, or is there a better way to load about:blank into that iframe?

[1] https://hg.mozilla.org/mozilla-central/rev/e21ad27bfd0a#l3.115
[2] https://searchfox.org/mozilla-central/source/docshell/base/BrowsingContext.cpp#882

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Bug 1584998: Make x-frame-options work with fission enabled. r=farre,jkt,johannh

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: xfo_blocked.png

Hey Dan, whenever XFO blocked a load we simply displayed about:blank. Since we are already on it rewriting that feature I also added an error page for XFO (see attachment). By itself I like the new error page as it is very descriptive.

Problem being, wpt tests (see underneath) are failing because those rely on the 'onload' event [1] which fired when we displayed about:blank but does not fire when displaying the new error page.

What's your take? Should we keep the new error page and trade the nicer design for less automated test coverage or should we discard the new design (which no aligns with the CSP error page) and have more automated test coverage?

[1] https://searchfox.org/mozilla-central/source/testing/web-platform/tests/x-frame-options/deny.sub.html#13

%--snip-------------
/x-frame-options/deny.sub.html
TIMEOUT XFO: DENY blocks same-origin framing. - Test timed out
TIMEOUT XFO: DENY blocks cross-origin framing. - Test timed out
TIMEOUT /x-frame-options/deny.sub.html
/x-frame-options/multiple.sub.html
TIMEOUT XFO: SAMEORIGIN; XFO: DENY blocks same-origin framing. - Test timed out
TIMEOUT XFO: DENY; XFO: SAMEORIGIN blocks same-origin framing. - Test timed out
TIMEOUT XFO: SAMEORIGIN; XFO: SAMEORIGIN blocks cross-origin framing. - Test timed out
TIMEOUT /x-frame-options/multiple.sub.html
/x-frame-options/redirect.sub.html
TIMEOUT XFO on redirect responses is ignored. - Test timed out
TIMEOUT /x-frame-options/redirect.sub.html
/x-frame-options/sameorigin.sub.html
TIMEOUT XFO: SAMEORIGIN blocks cross-origin framing. - Test timed out
TIMEOUT XFO: SAMEORIGIN blocks cross-origin nested in same-origin framing. - Test timed out
TIMEOUT XFO: SAMEORIGIN blocks same-origin nested in cross-origin framing. - Test timed out
TIMEOUT XFO: SAMEORIGIN blocks cross-origin nested in cross-origin framing. - Test timed out
TIMEOUT /x-frame-options/sameorigin.sub.html

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Dan, it seems I forgot to set the ni? flag for you on the previous comment.

Comment 7

[Daniel Veditz [:dveditz]]

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

Hey Dan, whenever XFO blocked a load we simply displayed about:blank. Since we are already on it rewriting that feature I also added an error page for XFO (see attachment). By itself I like the new error page as it is very descriptive.

Huh, I thought we already had a page for it back when bsterne first implemented it. Obligatory bikeshedding: Does the text change if it's blocked by frame-ancestors instead? "in this way" might be better as "in this frame" or "in this context" -- "way" is pretty vague. I like having the error page -- nice!

Problem being, wpt tests (see underneath) are failing because those rely on the 'onload' event [1] which fired when we displayed about:blank but does not fire when displaying the new error page.

Interesting. iirc Chrome and IE/Edge shows error content in this context, so they fire onload? Do our error pages fire onerror instead? Or just nothing? I wonder if we're leaking cross-origin data no sending onload and should change the behavior of error pages? I wonder if there are WPT that test generic framing errors (e.g. server not found, page not found), and if so, what have we done there? In theory our behavior might allow a site to probe another site to figure out which non-public pages a user can reach, for an example attack.

What's your take? Should we keep the new error page and trade the nicer design for less automated test coverage or should we discard the new design (which no aligns with the CSP error page) and have more automated test coverage?

Or a 3rd unlikely option: change the WPT tests (e.g. check for onerror as well? do we issue onerror?)? I'd really like to honor the intent of the WPT to get cross-browser consistency in detectable behaviors if possible. It's also good to be consistent with our own error pages so...

• Do WPT tests cover other kinds of error pages?
• If so do we pass them? Maybe this particular error page needs to do something different
• if we don't pass do we ignore the tests as is an option in this case?
  ** In that case maybe that's an OK option for now.
  ** but we should have a bug to fix our error pages to comply with the tests

Note that sending onerror instead of nothing doesn't fix the cross-origin leak potential so I'm not 100% behind that.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

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

• The CSP frame-ancestors has a similar error page (https://searchfox.org/mozilla-central/source/browser/locales/en-US/chrome/overrides/appstrings.properties#37). I mostly followed the structure of the CSP error page to create a similar one for x-frame-options.

• Our error pages do not fire .onload() neither .onerror() and I don't know of any other structure that would allow us to capture the error in a wpt test. For our own mochitests I added an observer-structure which allows us to use register error observers in the test itself, which is Firefox specific and hence not a feasible approach in a wpt test.

• I guess our best option is to annotate those wpt tests and file a follow up bug - maybe we can consider firing onload() even in the case we load an error page.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac8f6632f7e0
Make x-frame-options work with fission enabled. r=jkt,farre,johannh

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/03efc87340f9
Backed out changeset ac8f6632f7e0 on request by dev. On a CLOSED TREE

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5f185a11889b
Make x-frame-options work with fission enabled. r=jkt,farre,johannh

Comment 13

[Tom S]

The load/error event not firing sounds like bug 1418975.

Comment 14

[Razvan Maries]

Backed out for perma fails on browser_bug593387.js

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=273763485&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=linux%2Cx64%2Cdebug%2Cmochitests%2Cwith%2Cfission%2Cenabled%2Ctest-linux64%2Fdebug-mochitest-browser-chrome-fis-e10s-1%2Cm-fis%28bc1%29&revision=5a900d3343e086c9a8d1f4d3ab88bee55764940a

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

Backout: https://hg.mozilla.org/integration/autoland/rev/eddb9fcaaa4bd4fdb4e32024f92f969abfe92f58

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Razvan Maries from comment #14)

Backed out for perma fails on browser_bug593387.js

SpecialPowers.spawn to the rescue ...

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d5bee3db1b4f
Make x-frame-options work with fission enabled. r=jkt,farre,johannh,flod

Comment 17

[Cosmin Sabou [:CosminS]]

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