Closed Bug 1584998 Opened 2 months ago Closed 17 days ago

Make x-frame-options work with fission enabled

Categories

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

task

Tracking

()

RESOLVED FIXED
mozilla72
Fission Milestone M4
Tracking Status
firefox72 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [domsecurity-active])

Attachments

(2 files)

No description provided.
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [domsecurity-active]

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.

Assignee: ckerschb → nobody
Status: ASSIGNED → NEW
Priority: P1 → P3
Whiteboard: [domsecurity-active] → [domsecurity-backlog1]
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Whiteboard: [domsecurity-backlog1] → [domsecurity-active]

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

Fission Milestone: --- → M4
Depends on: 1590784

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

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

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

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

Flags: needinfo?(dveditz)

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

Blocks: 1590108
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/autoland/rev/ac8f6632f7e0
Make x-frame-options work with fission enabled. r=jkt,farre,johannh
Backout by dvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/03efc87340f9
Backed out changeset ac8f6632f7e0 on request by dev. On a CLOSED TREE
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/autoland/rev/5f185a11889b
Make x-frame-options work with fission enabled. r=jkt,farre,johannh
Duplicate of this bug: 1590108

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

(In reply to Razvan Maries from comment #14)

Backed out for perma fails on browser_bug593387.js

SpecialPowers.spawn to the rescue ...

Flags: needinfo?(ckerschb)
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/autoland/rev/d5bee3db1b4f
Make x-frame-options work with fission enabled. r=jkt,farre,johannh,flod
Status: ASSIGNED → RESOLVED
Closed: 17 days ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
See Also: → 1593321
Regressions: 1593321
Blocks: 1594063
You need to log in before you can comment on or make changes to this bug.