Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTML's "navigate" algorithm should handle 'X-Frame-Options' and CSP's 'frame-ancestors' #1230

Open
mikewest opened this issue May 11, 2016 · 17 comments · May be fixed by #5737
Open

HTML's "navigate" algorithm should handle 'X-Frame-Options' and CSP's 'frame-ancestors' #1230

mikewest opened this issue May 11, 2016 · 17 comments · May be fixed by #5737

Comments

@mikewest
Copy link
Member

@mikewest mikewest commented May 11, 2016

See whatwg/fetch#302 where I initially filed this request. @annevk's suggestion that we handle the header inside the navigation algorithm (probably as step 21, after the response checks?) SGTM.

This will require some changes to CSP as well to move frame-ancestors from a response check in Fetch to a new check in the navigate algorithm.

@annevk
Copy link
Member

@annevk annevk commented Jul 5, 2016

This should be pretty easy to add now.

@mikewest
Copy link
Member Author

@mikewest mikewest commented Aug 1, 2016

Poking at this today. For frame-ancestor, I expect to add the relevant hooks to CSP, and, then call them from https://html.spec.whatwg.org/#process-a-navigate-response. For X-F-O, I'll make up something that at least somewhat aligns with RFC7034.

mikewest added a commit to w3c/webappsec-csp that referenced this issue Aug 1, 2016
This introduces a new 'navigation check', which we'll need to wire up to
HTML. That, in turn, requires HTML to use Fetch. WHATWG's does, W3C's
does not (w3c/html#548).
mikewest added a commit to w3c/webappsec-csp that referenced this issue Aug 2, 2016
The initial pass at whatwg/html#1230 was too simple. Let's complexify
it up a little bit, shall we?
@mikewest
Copy link
Member Author

@mikewest mikewest commented Aug 2, 2016

"Pretty easy to add", he says.

Basically, we didn't think about this enough, and it's more subtle than I though, because we need the source browsing context's policy in some cases (form-action), and the target browsing context's parent's policy in others (frame-src), and the response's policy in still others (frame-ancestors). I think these two hooks give me the tools I'll need to poke at things in CSP, but I wonder if there's a better way...

WDYT?

@annevk
Copy link
Member

@annevk annevk commented Aug 4, 2016

One way to do form-action is to just hook it directly in form submission. And just navigate to a network error if it blocks. If it's specific to forms invoking navigate we might as well keep it there.

The others remain complicated.

@mikewest
Copy link
Member Author

@mikewest mikewest commented Aug 4, 2016

Hooking directly into form submission wouldn't catch redirects, would it? I think we need to live in navigate for that to work.

@annevk
Copy link
Member

@annevk annevk commented Aug 4, 2016

Good point.

@mikewest
Copy link
Member Author

@mikewest mikewest commented Aug 16, 2016

Are you waiting on me for this PR? (Sorry, I've been out for almost two weeks, so I'm trying to page this back in....)

@annevk
Copy link
Member

@annevk annevk commented Aug 16, 2016

Yeah, the review comments are not addressed yet I thought.

annevk added a commit that referenced this issue Aug 18, 2016
This will enable implementation of 'frame-ancestors' and 'form-action', and makes a bit of progress towards #1230.
mikewest added a commit to w3c/webappsec-csp that referenced this issue Aug 18, 2016
triple-underscore added a commit to triple-underscore/triple-underscore.github.io that referenced this issue Aug 18, 2016
Previous version.
w3c/webappsec-csp@3c625fd
fc46063fb902

 whatwg/html#1230 landed.
w3c/webappsec-csp@366e5de
c273f28bd3cb
@domenic
Copy link
Member

@domenic domenic commented May 30, 2018

Today @travisleithead discovered that Chrome does not support the ALLOW-FROM variant. Edge had strict parsing, which broke when it encountered a web site with ALLOW-FROM=https://example.com/.

If we do manage to get around to speccing this, so that nobody else runs into these sorts of interop issues while building their browsers, we'll need to figure out what to do with ALLOW-FROM. Probably also questions around whitespace trimming etc. So yeah, not just semantic issues, also syntactic ones :(

alice added a commit to alice/html that referenced this issue Jan 8, 2019
This will enable implementation of 'frame-ancestors' and 'form-action', and makes a bit of progress towards whatwg#1230.
@annevk
Copy link
Member

@annevk annevk commented Dec 3, 2019

As mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1599256 this should also define what kind of document does get loaded (about:blank?) and whether that creates a load event. (That's what Firefox will implement unless Chrome starts treating this as a network error soon.)

@mikewest
Copy link
Member Author

@mikewest mikewest commented Dec 3, 2019

Last I checked, Chrome (and WebKit) loads an error page with an opaque origin (data:... in WebKit, and chrome://interstitial (I think) in Chrome), and does fire onload when a page is blocked via XFO (note that Chrome has different behavior for CSP's frame-ancestors because that's still handled in our renderer, but it's moving out to the browser in ~80, and aims to have the same behavior as XFO).

@annevk
Copy link
Member

@annevk annevk commented Dec 3, 2019

Ah, I guess Chrome in general fires the load event for network errors? I.e., I get it both for https://expired.badssl.com/ and https://google.com. Maybe that's reasonable enough.

@mikewest
Copy link
Member Author

@mikewest mikewest commented Dec 3, 2019

Yes, that's my understanding of Chrome's (and WebKit's, I believe) behavior. We move network errors to an opaque-origin'd error page, and fire an onload.

domenic added a commit to WICG/portals that referenced this issue May 4, 2020
This brings the spec closer to how it would look upon landing in HTML, and explicitly calls out what sections it would expect things to land in. Notable changes:

* Moved window.portalHost from "Miscellaneous extensions" into a new "Portal hosts" section, which contains the definitions for both window.portalHost and the PortalHost interface.

* Made the onportalactivate event handler addition match HTML's existing structure.

* Moved subsections of "Security considerations" which were spec patches into a new dedicated section, "Updates to other specifications".

* Noted the connection to whatwg/html#1230 in addition to RFC 7034.

* Tweaks the wording in the "Fetch Metadata Request Headers" section's non-normative summary notes.
domenic added a commit to WICG/portals that referenced this issue May 4, 2020
This brings the spec closer to how it would look upon landing in HTML, and explicitly calls out what sections it would expect things to land in. Notable changes:

* Moved window.portalHost from "Miscellaneous extensions" into a new "Portal hosts" section, which contains the definitions for both window.portalHost and the PortalHost interface.

* Made the onportalactivate event handler addition match HTML's existing structure.

* Moved subsections of "Security considerations" which were spec patches into a new dedicated section, "Updates to other specifications".

* Noted the connection to whatwg/html#1230 in addition to RFC 7034.

* Tweaks the wording in the "Fetch Metadata Request Headers" section's non-normative summary notes.
domenic added a commit to WICG/portals that referenced this issue May 4, 2020
This brings the spec closer to how it would look upon landing in HTML, and explicitly calls out what sections it would expect things to land in. Notable changes:

* Moved window.portalHost from "Miscellaneous extensions" into a new "Portal hosts" section, which contains the definitions for both window.portalHost and the PortalHost interface.

* Made the onportalactivate event handler addition match HTML's existing structure.

* Moved subsections of "Security considerations" which were spec patches into a new dedicated section, "Updates to other specifications".

* Noted the connection to whatwg/html#1230 in addition to RFC 7034.

* Tweaks the wording in the "Fetch Metadata Request Headers" section's non-normative summary notes.
@annevk annevk mentioned this issue Jul 9, 2020
3 of 3 tasks complete
@domenic
Copy link
Member

@domenic domenic commented Jul 15, 2020

I'm interested in tackling this in the near future.

It looks like currently frame-ancestors is completely handled, right? That is, https://html.spec.whatwg.org/#process-a-navigate-response step 1 handles frame-ancestors (and other CSP checks, like form-action and frame-src). It creates a network error. Network error pages have an opaque origin (clearer after #5736) and fire a load event (per https://html.spec.whatwg.org/#read-ua-inline "stopped parsing" step).

So this bug reduces to only X-Frame-Options handling. There'll be a bit of trickiness around parsing, but otherwise it should be pretty straightforward once parsed. Assuming we don't support ALLOWFROM (tests needed), then we should have all the info we need. Something like:

  1. If an enforce-disposition frame-ancestors is present, then ignore the X-Frame-Options header.
  2. If deny, and browsingContext is a child browsing context, then network error.
  3. If sameorigin, and browsingContext is a child browsing context, then check if finalResponseOrigin is same-origin with browsingContext's container's relevant settings object's top-level origin. If not, network error.

The last step only checking top-level origin (instead of all intermediate origins) is based off of the remarks in https://w3c.github.io/webappsec-csp/#frame-ancestors-and-frame-options about "many user agents"

@domenic
Copy link
Member

@domenic domenic commented Jul 15, 2020

Hmm per the discussions in https://bugzilla.mozilla.org/show_bug.cgi?id=725490 it looks like Chrome and Firefox implement all-ancestors checking, which is nice. And everyone passes the associated WPTs. https://wpt.fyi/results/x-frame-options?label=master&label=experimental&aligned&q=x-frame-options

@domenic
Copy link
Member

@domenic domenic commented Jul 17, 2020

So I've got an initial spec for this up in #5737. The question remains on what to do in "conflict" cases like DENY,SAMEORIGIN. Per the description of Chrome's algorithm in web-platform-tests/wpt#21730 (comment), and the test results in web-platform-tests/wpt#21730 and web-platform-tests/wpt#24618, we could expand #5737 with something along the following lines:

  1. Remove all tokens from xFrameOptions that are not ASCII case-insensitive matches for one of "sameorigin", "deny", "allowall".
  2. Remove duplicate tokens from xFrameOptions
  3. If xFrameOptions contains >=2 tokens, then return false (block the response).
  4. If xFrameOptions contains 0 tokens, then return true (do not block the response).
  5. If xFrameOptions[0] is "deny", then return false.
  6. If xFrameOptions[0] is "allowall", then return true.
  7. If xFrameOptions[0] is "sameorigin", then perform the steps in the PR to walk the tree.

(this is meant to be equivalent to the algorithm @mikewest outlines, but with more clarity at the cost of more O(n) operations. We could also just use the Chrome algorithm as-is.)

Note that this conflict cases, according to Chromium metrics back in February, affects 0.002% of page loads.

I'm OK with either the current simple spec proposal in #5737 (which I believe matches Firefox behavior), or with the more complicated Chromium version. I like simplicity, but I am sympathetic to the argument that conflicts are a sign that something has gone wrong and we should err on the side of blocking. Also, blackbox testing indicates that the Chromium model and the Safari model are probably the same, so it might be easier to just align there.

What would Mozilla prefer? /cc @mozfreddyb

EDIT: further testing reveals that the above algorithm proposal doesn't seem quite correct. In particular invalid,sameorigin blocks same-origin framing in Chrome and Safari but the above algorithm would let it through.

domenic added a commit that referenced this issue Jul 17, 2020
@domenic
Copy link
Member

@domenic domenic commented Aug 5, 2020

Alright. I did another pass through the tests, and rewrote everything to be less copy-pastey, and made sure that every same-origin test has a corresponding cross-origin test. Results over at web-platform-tests/wpt#24618.

Here is the current status of browsers vs. the spec draft in #5737:

  • Firefox has a timeout failure on "DENY blocks cross-origin framing with CSP frame-ancestors 'self'". Not sure what could be going on there.

  • Safari and Chrome both treat the presence of any invalid tokens, along with valid ones, as causing the result to be treated as DENY. Firefox and the proposed spec ignore invalid tokens.

I don't have strong feelings about which model is better. The proposed spec is simpler, but it seems like the Chrome and Safari behavior might be a bit more strict, which could be good.

/cc @dveditz

/cc @ArthurSonzogni who I noticed working a bit on XFO in Chromium recently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.