w3c / webappsec-csp

WebAppSec Content Security Policy
https://w3c.github.io/webappsec-csp/
Other
210 stars 78 forks source link

frame-src spec does not match implementations in terms of which CSP is used #400

Open bzbarsky opened 5 years ago

bzbarsky commented 5 years ago

Consider the following testcase:

1) A parent page has a frame-src CSP directive restricting to the same site. 2) The parent page has a subframe that does not have CSP defined at all and contains a link to a different site. 3) The user clicks the link. What should happen?

You can see a live version of this testcase at https://web.mit.edu/bzbarsky/www/testcases/security/csp-frame-src-parent.html -- click the "Google" link in the second subframe to trigger this exact scenario.

What I observe in browsers (Firefox, Safari, Chrome) is that the load is blocked.

What the spec says right now is that the load should NOT be blocked.

Specifically, both https://w3c.github.io/webappsec-csp/#should-block-request and https://w3c.github.io/webappsec-csp/#should-block-response (which are the two places that could trigger frame-src checks) get their CSP from "request’s client’s global object’s CSP list". So what is "request's client's global object" here? The user clicked a link that has no download attribute, so we start at https://html.spec.whatwg.org/multipage/links.html#following-hyperlinks-2 and proceed to https://html.spec.whatwg.org/multipage/browsing-the-web.html#navigate where step 13 lands us in https://html.spec.whatwg.org/multipage/browsing-the-web.html#process-a-navigate-fetch

Note that at this point sourceBrowsingContext and browsingContext are both the subframe, because there is no targeted linking here.

https://html.spec.whatwg.org/multipage/browsing-the-web.html#process-a-navigate-fetch sets the request's client to "sourceBrowsingContext's active document's relevant settings object", so "request’s client’s global object" is the Window inside the subframe, and its CSP list is empty. So per spec, links with arbitrary destinations should be allowed here.

That's definitely not what browsers do, though. Browsers seem to be using the CSP of the frameElement's node document for frame navigations, not the CSP of the request's client's global. At least for frame-src. For some other directives (e.g. upgrade-insecure-requests), I know Gecko uses the request's client's global; I haven't tested other browsers.

@mikewest @annevk @dveditz @ckerschb

bzbarsky commented 5 years ago

Put another way, browsers seem to treat "frame-src" as "prevent other things from being loaded in subframes", but the spec has "frame-src" as "prevent this page from loading other things in subframes, but allow other pages to load other things in its subframes, subject to their own CSPs". Which are the intended semantics?

dveditz commented 5 years ago

CSP2 was clear on intent (https://w3c.github.io/webappsec-csp/2/#directive-frame-src)

Whenever the user agent fetches a URL in the course of one of the following activities [...]

  • Requesting data for display in a nested browsing context [...]
  • Navigated such a nested browsing context.

CSP3 would not have intentionally created a compatibility problem with CSP2 which already had multiple implementations.

Even in CSP3 it says "The frame-src directive restricts the URLs which may be loaded into nested browsing contexts" without making distinctions about who is doing the loading. https://w3c.github.io/webappsec-csp/#directive-frame-src

So the generic "algorithm called from fetch and html" doesn't work right for this directive. Does the HTML spec need to consider a separate "navigate a frame" algorithm to have the right hooks for CSP?

bzbarsky commented 5 years ago

Yeah, what CSP2 says here is quite different from what CSP3 says, ok.

In terms of how best to make this work, there's the question of what should happen with redirects. If a load allowed by frame-src does a redirect, presumably that redirect needs to also have frame-src checks applied to it, right?

Furthermore, it's not just frame-src; child-src and default-src need to work the same way, when applied to document navigations.

Given that, it seems like the simplest fix would in fact be for https://w3c.github.io/webappsec-csp/#should-block-request and https://w3c.github.io/webappsec-csp/#should-block-response to define an algorithm for getting a CSP list that works sort of like this:

1) If the request's destination is "document" and the request's reserved client's target browsing context is a https://html.spec.whatwg.org/multipage/browsers.html#nested-browsing-context get the CSP list of the document that the nested browsing context is nested through

2) Otherwise get the request's client's global's CSP.

Or so. @annevk does that kinda make sense? Will all requests with "document" destination always have a "reserved client's target browsing context"? Are there simpler ways to get the thing we want?

The other option, and one I would prefer even more, is to stop doing this dance altogether and store the relevant CSP directly on the request at the point when the fetch starts. Redirects would copy it across the redirect. Then navigation could do its own special setup if it wanted, and fetch would set it from the client if not set otherwise. If we do that, I would prefer that we also store a snapshot of the CSP on the request. That would avoid the problems with the current setup, which is (a) racy when CSP is mutated while a request is in-flight and (b) not really multiprocess-friendly, because it requires synchronously poking a CSP list that's live and in another process.

dveditz commented 5 years ago

Then there's the complication of a navigating the frame when the framed document has a CSP with a "navigate-to" directive. To honor the intent we'd want to use the framed document's CSP to check against "navigate-to" and then use the parent's CSP to check frame-src.

All CSP directives also apply to redirects of the covered request (with any path-matching part ignored for cross-origin requests).

Furthermore, it's not just frame-src; child-src and default-src need to work the same way, when applied to document navigations.

"default-src" only needs to work that way when used as a replacement for a missing frame-src. This could be implemented so there is always a frame-src by copying default-src or setting a pointer from frame-src to a read-only default-src policy. The distinction only matters when we report violations (actual vs effective policy).

bzbarsky commented 5 years ago

"navigate-to" is already handled in the HTML spec by doing the navigate-to/form-action check in the HTML navigation algorithm at https://html.spec.whatwg.org/multipage/browsing-the-web.html#process-a-navigate-fetch step 5.6. This does the navigate-to check on redirects as well; that's what the "loop until done" bit is about. Right now this uses the client, not the target for the source of the CSP, per https://w3c.github.io/webappsec-csp/#should-block-navigation-request step 2. If the intent is to use the target, then there needs to be a change here in the CSP3 spec. Firefox does not implement "navigate-to", but does implement "form-action", where we use the client's, not the target's CSP (that is, the CSP of the document the form is in, not the CSP of the document in the browsing context the form targets). If the form submission is redirected, we don't do a second form-action check for that redirect, in case that matters. I think the HTML spec says there should in fact be separate "form-action" checks on redirect?

All CSP directives also apply to redirects of the covered request

OK.

"default-src" only needs to work that way when used as a replacement for a missing frame-src.

Yes.

dveditz commented 5 years ago

By "framed document" I meant client -- CSP3 is correct about navigate-to. I just meant that the same request could need to be checked against CSPs in two different contexts: the client's "navigate-to" and the target's "frame-src". For each URL in a redirect chain.

bzbarsky commented 5 years ago

That seems reasonable, sure.

bzbarsky commented 5 years ago

We need test coverage for all this, btw, and UA bugs as needed...