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

Block all non sri resources #64

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
9 participants
@oreoshake

oreoshake commented Mar 17, 2016

Discussed here: https://lists.w3.org/Archives/Public/public-webappsec/2015Dec/0045.html

@mikewest I really have no idea what I'm doing here. In particular, the algorithms section was just sorta slapped together.

/cc @ptoomey3

oreoshake added some commits Mar 17, 2016

<pre>
directive-name = "block-non-sri-resources"
directive-value = "" / token *( RWS token )

This comment has been minimized.

@ptoomey3

ptoomey3 Mar 17, 2016

I'm not familiar with the syntax here, so does the * imply an "all" wildcard? If not, it should probably be added.

@ptoomey3

ptoomey3 Mar 17, 2016

I'm not familiar with the syntax here, so does the * imply an "all" wildcard? If not, it should probably be added.

This comment has been minimized.

@shekyan

shekyan Mar 17, 2016

Contributor

this should looks something like

directive-name = "block-non-sri-resources"
directive-value = sri-token

sri-token = "'script'" / "'style'" / "*"
@shekyan

shekyan Mar 17, 2016

Contributor

this should looks something like

directive-name = "block-non-sri-resources"
directive-value = sri-token

sri-token = "'script'" / "'style'" / "*"

This comment has been minimized.

@mikewest

mikewest Mar 18, 2016

Member

I don't think the grammar should be that strict. I'd prefer to see something generic in the grammar, and a note that unrecognized tokens should be ignored so that we can add new types in the future when SRI advances. That is, link token to https://tools.ietf.org/html/rfc7230#section-3.2.6, RWS to https://tools.ietf.org/html/rfc7230#appendix-B, and define directive-value as [ token *( RWS token ) ]. You'd then need to define how to match the various tokens in algorithms below.

Note: The link between the types in this list and the behavors that SRI defines is actually a good reason to move this directive to SRI rather than tying it to this document's status.

@mikewest

mikewest Mar 18, 2016

Member

I don't think the grammar should be that strict. I'd prefer to see something generic in the grammar, and a note that unrecognized tokens should be ignored so that we can add new types in the future when SRI advances. That is, link token to https://tools.ietf.org/html/rfc7230#section-3.2.6, RWS to https://tools.ietf.org/html/rfc7230#appendix-B, and define directive-value as [ token *( RWS token ) ]. You'd then need to define how to match the various tokens in algorithms below.

Note: The link between the types in this list and the behavors that SRI defines is actually a good reason to move this directive to SRI rather than tying it to this document's status.

This comment has been minimized.

@michaelficarra

michaelficarra Mar 20, 2016

Contributor

I disagree. Implementations already ignore a directive if they cannot recognise it. The grammar suggested by @shekyan is more clear.

@michaelficarra

michaelficarra Mar 20, 2016

Contributor

I disagree. Implementations already ignore a directive if they cannot recognise it. The grammar suggested by @shekyan is more clear.

This comment has been minimized.

@mikewest

mikewest Mar 20, 2016

Member

I agree that implementations ignore unknown directives, and I agree that it's a good thing. I'd suggest that that means that we ought to ignore unknown tokens in a directive's value as well, which is what I'm suggesting.

@mikewest

mikewest Mar 20, 2016

Member

I agree that implementations ignore unknown directives, and I agree that it's a good thing. I'd suggest that that means that we ought to ignore unknown tokens in a directive's value as well, which is what I'm suggesting.

This comment has been minimized.

@mikewest

mikewest Mar 20, 2016

Member

Among other things, that would have meant that we couldn't add nonces or hashes to script-src, as they weren't known tokens in CSP1. Given that we don't really know what we want from this directive, ignoring unknown values with an eye towards forward compatibility seems reasonable.

@mikewest

mikewest Mar 20, 2016

Member

Among other things, that would have meant that we couldn't add nonces or hashes to script-src, as they weren't known tokens in CSP1. Given that we don't really know what we want from this directive, ignoring unknown values with an eye towards forward compatibility seems reasonable.

This comment has been minimized.

@shekyan

shekyan Mar 21, 2016

Contributor

Regardless if it's going to be implemented as rfc7230 token or a specification defined grammar, I realized that there is no place for U+002A ASTERISK character (*). Wildcard in other contexts of CSP represents most permissive value, while for this directive it would mean most restrictive, which can be a big source of confusion for developers.

@shekyan

shekyan Mar 21, 2016

Contributor

Regardless if it's going to be implemented as rfc7230 token or a specification defined grammar, I realized that there is no place for U+002A ASTERISK character (*). Wildcard in other contexts of CSP represents most permissive value, while for this directive it would mean most restrictive, which can be a big source of confusion for developers.

This comment has been minimized.

@fmarier

fmarier Mar 22, 2016

Member

sri-token = "'script'" / "'style'" / "*"

I'm not sure * makes sense here. I understand the desire to turn on all sri checks at once, but I can imagine the following scenario that would not be forward-compatible:

  1. Webdev adds require-sri-for * to CSP and annotates scripts and styles.
  2. Firefox only supports SRI for scripts and styles so it only checks that.
  3. (time passes, webdev forgets about CSP)
  4. Firefox adds support for SRI in img elements.
  5. Site breaks because Firefox refuses to load non-sri images.

If we force developers to be explicit about what they require then when browsers start adding sri support to new elements, developers will have to add the attributes and then update their CSP once they've done that.

@fmarier

fmarier Mar 22, 2016

Member

sri-token = "'script'" / "'style'" / "*"

I'm not sure * makes sense here. I understand the desire to turn on all sri checks at once, but I can imagine the following scenario that would not be forward-compatible:

  1. Webdev adds require-sri-for * to CSP and annotates scripts and styles.
  2. Firefox only supports SRI for scripts and styles so it only checks that.
  3. (time passes, webdev forgets about CSP)
  4. Firefox adds support for SRI in img elements.
  5. Site breaks because Firefox refuses to load non-sri images.

If we force developers to be explicit about what they require then when browsers start adding sri support to new elements, developers will have to add the attributes and then update their CSP once they've done that.

This comment has been minimized.

@ptoomey3

ptoomey3 Mar 22, 2016

Isn't the same true for something like default-src 'none' if/when other new directives are subject to default-src? We had been paranoid about this and specified an explicit policy for every directive we knew about and set a default-source * to avoid potential breakage. We only recently gave up on the paranoia and switched it to 'none', assuming we will catch any breakage from following the csp spec and reports from preview release users on chrome/Firefox.

@ptoomey3

ptoomey3 Mar 22, 2016

Isn't the same true for something like default-src 'none' if/when other new directives are subject to default-src? We had been paranoid about this and specified an explicit policy for every directive we knew about and set a default-source * to avoid potential breakage. We only recently gave up on the paranoia and switched it to 'none', assuming we will catch any breakage from following the csp spec and reports from preview release users on chrome/Firefox.

This comment has been minimized.

@oreoshake

oreoshake Mar 22, 2016

We want to explicitly avoid * value for forward compatibility. The * I originally added was to denote "any number of tokens".

EDIT: I chatted with @ptoomey3 and I've come around to his thinking.

@oreoshake

oreoshake Mar 22, 2016

We want to explicitly avoid * value for forward compatibility. The * I originally added was to denote "any number of tokens".

EDIT: I chatted with @ptoomey3 and I've come around to his thinking.

@mikewest

This comment has been minimized.

Show comment
Hide comment
@mikewest

mikewest Mar 18, 2016

Member

Thanks, this is a good start! But it needs a lot more work. :) Will add comments inline.

Member

mikewest commented Mar 18, 2016

Thanks, this is a good start! But it needs a lot more work. :) Will add comments inline.

4. If |policy|'s <a for="policy">disposition</a> is "`enforce`",
return "`Blocked`".
2. Return "`Allowed`".
<h4 id="directive-block-non-sri-resources">`block-non-sri-resources`</h4>

This comment has been minimized.

@mikewest

mikewest Mar 18, 2016

Member

Bikeshed: I'd prefer something non-double-negative. require-sri-for might work, for instance. I'm open to whatever, but "block things that aren't X unless they're not in the following list" gets hard to parse.

@mikewest

mikewest Mar 18, 2016

Member

Bikeshed: I'd prefer something non-double-negative. require-sri-for might work, for instance. I'm open to whatever, but "block things that aren't X unless they're not in the following list" gets hard to parse.

1. For each resource in document:
1. If the resource's type aligns with the token values and that resource
type is contained in the directive, return "`Blocked`".

This comment has been minimized.

@mikewest

mikewest Mar 18, 2016

Member

type aligns with the token values

This needs to be specified a little more tightly, probably along the same lines as https://w3c.github.io/webappsec-csp/#effective-directive-for-a-request (maybe making use of that algorithm directly to map a Fetch Directive name to one of the tokens you define above?).

@mikewest

mikewest Mar 18, 2016

Member

type aligns with the token values

This needs to be specified a little more tightly, probably along the same lines as https://w3c.github.io/webappsec-csp/#effective-directive-for-a-request (maybe making use of that algorithm directly to map a Fetch Directive name to one of the tokens you define above?).

Given a <a>request</a> (|request|), a <a>response</a> (|response|), and a
<a>policy</a> (|policy|):
1. For each resource in document:

This comment has been minimized.

@mikewest

mikewest Mar 18, 2016

Member

This check is executed at the bottom of a resource fetch; there's no "document" that you can directly grab, nor do I think you need to grab a document, as you should have all the data you need in the |response|.

@mikewest

mikewest Mar 18, 2016

Member

This check is executed at the bottom of a resource fetch; there's no "document" that you can directly grab, nor do I think you need to grab a document, as you should have all the data you need in the |response|.

<h5 id="sandbox-algorithms">Algorithms</h5>
This directive's <a for="directive">response check</a> is as follows:

This comment has been minimized.

@mikewest

mikewest Mar 18, 2016

Member

I think you can do a little better than this by defining a "pre-request" check for the directive that examines the Request and compares it to the token list. Something like:


Given a request (|request|) and a policy (|policy|):

  1. If |request|'s type [insert some sort of matching algorithm here, as discussed below], and |request|'s integrity metadata is the empty string, return "Blocked":
  2. Return "Allowed".

You'll probably need to work with @metromoxie, @mozfreddyb, @fmarier, @devd, and @annevk to wire up HTML's fetching mechanisms for <script> and <style> (and whatever comes next) to Fetch (basically, start by moving everything from section 3.6 and section 3.8 to HTML. But once that infrastructure is done, the check here should be pretty straightforward.

@mikewest

mikewest Mar 18, 2016

Member

I think you can do a little better than this by defining a "pre-request" check for the directive that examines the Request and compares it to the token list. Something like:


Given a request (|request|) and a policy (|policy|):

  1. If |request|'s type [insert some sort of matching algorithm here, as discussed below], and |request|'s integrity metadata is the empty string, return "Blocked":
  2. Return "Allowed".

You'll probably need to work with @metromoxie, @mozfreddyb, @fmarier, @devd, and @annevk to wire up HTML's fetching mechanisms for <script> and <style> (and whatever comes next) to Fetch (basically, start by moving everything from section 3.6 and section 3.8 to HTML. But once that infrastructure is done, the check here should be pretty straightforward.

This comment has been minimized.

@metromoxie

metromoxie Mar 22, 2016

Contributor

It seems to me like this could be greatly simplified just by modifying Fetch with something like:


In 5.1 "Main Fetch", add the following step after step 5:
If "should block fetch due to lack of integrity metadata" returns blocked, set response to a network error.


Then, you can define in the CSP spec the "should block fetch due to lack of integrity metadata" similar to what @mikewest described above.

@metromoxie

metromoxie Mar 22, 2016

Contributor

It seems to me like this could be greatly simplified just by modifying Fetch with something like:


In 5.1 "Main Fetch", add the following step after step 5:
If "should block fetch due to lack of integrity metadata" returns blocked, set response to a network error.


Then, you can define in the CSP spec the "should block fetch due to lack of integrity metadata" similar to what @mikewest described above.

This comment has been minimized.

@mikewest

mikewest Mar 22, 2016

Member

I don't think we need a new algorithm for that; hooking into the "pre-request" check would wire this directive up to the existing CSP hook in step 5 of Main Fetch. I'd prefer to keep that simple integration rather than adding a new step to Fetch for every feature that influences fetching.

@mikewest

mikewest Mar 22, 2016

Member

I don't think we need a new algorithm for that; hooking into the "pre-request" check would wire this directive up to the existing CSP hook in step 5 of Main Fetch. I'd prefer to keep that simple integration rather than adding a new step to Fetch for every feature that influences fetching.

This comment has been minimized.

@metromoxie

metromoxie Mar 22, 2016

Contributor

Ah, you're absolutely right. I was misreading Step 5. My apologies. In summary: I completely concur with @mikewest's suggested structure ;-)

@metromoxie

metromoxie Mar 22, 2016

Contributor

Ah, you're absolutely right. I was misreading Step 5. My apologies. In summary: I completely concur with @mikewest's suggested structure ;-)

The directive recognizes a number of potential token values:
* `script` requires SRI for scripts
* `style` requires SRI for style sheets

This comment has been minimized.

@mikewest

mikewest Mar 18, 2016

Member

The parsing here needs a little more work to make it clear what you mean. See the matching algorithm discussion below.

For instance, define whether the tokens are case-sensitive.

@mikewest

mikewest Mar 18, 2016

Member

The parsing here needs a little more work to make it clear what you mean. See the matching algorithm discussion below.

For instance, define whether the tokens are case-sensitive.

@devd

This comment has been minimized.

Show comment
Hide comment
@devd

devd Mar 22, 2016

I am probably missing a discussion somewhere but I am curious if we considered a token inside each *-src directive instead of creating a new directive. So, script-src ... 'block-non-sri' and similarly each directive could state whether it cares about sri. (sorry, if this has been discussed already; please point me to it)

devd commented Mar 22, 2016

I am probably missing a discussion somewhere but I am curious if we considered a token inside each *-src directive instead of creating a new directive. So, script-src ... 'block-non-sri' and similarly each directive could state whether it cares about sri. (sorry, if this has been discussed already; please point me to it)

@mozfreddyb

This comment has been minimized.

Show comment
Hide comment
@mikewest

This comment has been minimized.

Show comment
Hide comment
@mikewest

mikewest Apr 18, 2016

Member

Ping? Is this going anywhere? Are you blocked on me or on something else? :)

Member

mikewest commented Apr 18, 2016

Ping? Is this going anywhere? Are you blocked on me or on something else? :)

@oreoshake

This comment has been minimized.

Show comment
Hide comment
@oreoshake

oreoshake Apr 18, 2016

Not blocked, just life is getting in the way. This has been bumped from my todo list two weeks in a row 😿

oreoshake commented Apr 18, 2016

Not blocked, just life is getting in the way. This has been bumped from my todo list two weeks in a row 😿

@mozfreddyb

This comment has been minimized.

Show comment
Hide comment
@mozfreddyb

mozfreddyb Apr 27, 2016

Contributor

@oreoshake Would you be OK with someone else taking this pull request?

Contributor

mozfreddyb commented Apr 27, 2016

@oreoshake Would you be OK with someone else taking this pull request?

@oreoshake

This comment has been minimized.

Show comment
Hide comment
@oreoshake

oreoshake Apr 27, 2016

@oreoshake Would you be OK with someone else taking this pull request?

@mozfreddyb I would be ecstatic if someone else handled this.

oreoshake commented Apr 27, 2016

@oreoshake Would you be OK with someone else taking this pull request?

@mozfreddyb I would be ecstatic if someone else handled this.

@shekyan

This comment has been minimized.

Show comment
Hide comment
@shekyan

shekyan Apr 27, 2016

Contributor

I'd be happy to help. Have time till May 9.

Contributor

shekyan commented Apr 27, 2016

I'd be happy to help. Have time till May 9.

@mozfreddyb

This comment has been minimized.

Show comment
Hide comment
@mozfreddyb

mozfreddyb Apr 28, 2016

Contributor

Great! Just go ahead, Sergey!

Contributor

mozfreddyb commented Apr 28, 2016

Great! Just go ahead, Sergey!

@oreoshake

This comment has been minimized.

Show comment
Hide comment

oreoshake commented May 18, 2016

@oreoshake oreoshake closed this May 18, 2016

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