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

attempt to write down `require-sri-for` directive as part of SRI #32

Merged
merged 7 commits into from Jun 6, 2016

Conversation

@shekyan
Copy link
Contributor

shekyan commented May 5, 2016

Background: w3c/webappsec-csp#85.

@@ -343,7 +354,47 @@ implementation detail. It is not an API that implementors
provide to web applications. It is used in this document
only to simplify the algorithm description.

## Response verification algorithms ## {#verification-algorithms}
## Request verification algorithms ## {#request-verification-algorithms}

This comment has been minimized.

@mozfreddyb

mozfreddyb May 9, 2016 Contributor

Don't we still want to verify the response?

This comment has been minimized.

@shekyan

shekyan May 9, 2016 Author Contributor

It is still there

This comment has been minimized.

@mozfreddyb

mozfreddyb May 10, 2016 Contributor

Right, sorry. I got confused by the diff viewer.


### Opting-in

Authors may opt a Document to requre SRI metadata be present for

This comment has been minimized.

@mozfreddyb

mozfreddyb May 9, 2016 Contributor

s/requre/require/

@mozfreddyb
Copy link
Contributor

mozfreddyb commented May 9, 2016

Please don't remove index.html in your branch.

@shekyan shekyan force-pushed the shekyan:require-sri-for branch from bc96528 to 39f23c0 May 9, 2016
@shekyan
Copy link
Contributor Author

shekyan commented May 9, 2016

Please don't remove index.html in your branch.

I thought approach here is the same as in CSP, where maintainer commits bikeshed output.
I amended commit to include index.bikeshed.html.
I haven't changed spec.markdown, which is the producer of index.html assuming development was switched to used Bikeshed. Should I port my changes to Markdown after we are done, to get updated index.html?

@mozfreddyb
Copy link
Contributor

mozfreddyb commented May 10, 2016

spec.markdown and index.html are SRI version 1, which we can't modify lightly.
require-sri-for should land in the next SRI version, so I'd prefer you only patch index.bikeshed.bs :)

@mozfreddyb mozfreddyb added the SRI-next label May 10, 2016
@mozfreddyb
Copy link
Contributor

mozfreddyb commented May 10, 2016

Looks good from my side. Thank you for writing this down, Sergey!
paging other spec editors @metromoxie, @devd @fmarier

@shekyan shekyan force-pushed the shekyan:require-sri-for branch 2 times, most recently from c004594 to e9c485f May 10, 2016

### Opting-in {#opt-in-require-sri-for}

Authors may opt a Document to require SRI metadata be present for

This comment has been minimized.

@metromoxie

metromoxie May 13, 2016 Contributor

Should "Document" be a link to a definition?


The directive recognizes a number of potential token values:

* `script` requires SRI for scripts

This comment has been minimized.

@metromoxie

metromoxie May 13, 2016 Contributor

These should be in the grammar above, I think. Something like:
token = "script" | "style"

This section should then link to those token entries.

This comment has been minimized.

@shekyan

shekyan May 14, 2016 Author Contributor

Uh. I proposed it in w3c/webappsec-csp#64 (comment), but following discussion convinced me that token values shouldn't be locked down by the grammar. Thoughts?

This comment has been minimized.

@metromoxie

metromoxie May 14, 2016 Contributor

Haha, well @mikewest knows best, so feel free to ignore my comment :-)


### Apply |algorithm| to |request| ### {#apply-algorithm-to-request}

1. Let |protected resource types| be the result of [[#parse-require-sri-for]].

This comment has been minimized.

@metromoxie

metromoxie May 13, 2016 Contributor

Probably should read "...be the result of applying [[#parse-require-sri-for]] to the value of the <a>require-sri-for</a> directive."

@metromoxie
Copy link
Contributor

metromoxie commented May 13, 2016

lgtm % nits/minor comments. Thanks a lot, @shekyan!

### Apply |algorithm| to |request| ### {#apply-algorithm-to-request}

1. Let |protected resource types| be the result of applying [[#parse-require-sri-for]]
to the value of the <a>require-sri-for</a> directive.

This comment has been minimized.

@mikewest

mikewest May 15, 2016 Member

I think this needs to be clarified. A few questions come to mind:

  1. It's not at all clear what "the require-sri-for directive" refers to. I think you probably want to talk about |request|'s client's (responsible document/global object's) CSP list?
  2. Once you have a CSP list, it's not clear what you expect the behavior to be in the case of conflicting instructions (e.g. one policy with require-sri-for script and another with require-sri-for style).
  3. Lots of things have a type of script that you might not intend. For example, do you intend for this to apply to {Web,Shared,Service}Workers as well (e.g. new Worker in the document, and importScript inside the worker)? If so, SRI probably needs to define that mechanism. Likewise, we treat XSLT as script-src: should we treat it as "script" here?

3. Return the set of |protected resource types|.

### Apply |algorithm| to |request| ### {#apply-algorithm-to-request}

This comment has been minimized.

@mikewest

mikewest May 15, 2016 Member

When does this get called? It sounds like it ought to be defined as the directive's "pre-request check", which would take care of hooking it into Fetch...

@mikewest
Copy link
Member

mikewest commented May 15, 2016

This looks like a good start, but I have some questions that I've left inline. Thanks!

@shekyan shekyan force-pushed the shekyan:require-sri-for branch from af0a778 to f07a660 May 16, 2016
@shekyan
Copy link
Contributor Author

shekyan commented May 16, 2016

@mikewest , please review if this looks sane when you have a chance.

type: dfn
text: Content Security Policy; urlPrefix: #
text: policy; url: policy
text: pre-request check; url: #directive-pre-request-check

This comment has been minimized.

@mikewest

mikewest May 17, 2016 Member

Nit: No #.

1. Let the set of |protected resource types| that require SRI be the empty set.

2. For each token returned by <a>splitting tokens on spaces</a>,
if token matches the grammar for <a>require-sri-for</a>,

This comment has been minimized.

@mikewest

mikewest May 17, 2016 Member

Tiny style nit: Line this up with the first sentence.

directive-value = <a grammar>token</a> *( <a>RWS</a> <a>token</a> )
</pre>

The directive recognizes a number of potential token values:

This comment has been minimized.

@mikewest

mikewest May 17, 2016 Member

Can you add a definition here that we can link to below? Perhaps, "The following list contains the set of <dfn noexport>known tokens</dfn>:"?


### Parsing `require-sri-for` ### {#parse-require-sri-for}

To parse the |token| list, the user agent MUST use an algorithm equivalent to the following:

This comment has been minimized.

@mikewest

mikewest May 17, 2016 Member

Nit: I'd rephrase this to something like: "Given a string (|token list|), this algorithm returns a list of resource types which will require integrity checks:"


1. Let the set of |protected resource types| that require SRI be the empty set.

2. For each token returned by <a>splitting tokens on spaces</a>,

This comment has been minimized.

@mikewest

mikewest May 17, 2016 Member

Nit: "For each |token| in the result of <a lt="split a string on spaces">splitting |token list| on spaces</a>"


2. For each token returned by <a>splitting tokens on spaces</a>,
if token matches the grammar for <a>require-sri-for</a>,
add the token to the set of |protected resource types|. Otherwise, ignore the token.

This comment has been minimized.

@mikewest

mikewest May 17, 2016 Member

Nit: Rather than "matches the grammar", perhaps something like "add |token| to |protected resource types| if |token| is a <a>known token</a>."


1. Let |protected resource types| be an empty set.

2. If a directive whose name is "require-sri-for" is present in policy’s directive set,

This comment has been minimized.

@mikewest

mikewest May 17, 2016 Member

This will only be called when such a directive is present. You can rephrase this a bit by replacing 1 and 2 with something like:

1.  Let |protected resource types| be the result of executing [[#parse-require-sri-for]] on this directive's <a for="directive">value</a>.
@mikewest
Copy link
Member

mikewest commented May 17, 2016

Seems pretty reasonable to me % the comments I left. I'll leave the question of which files to touch to the SRI editors who know what's up there.

@mozfreddyb
Copy link
Contributor

mozfreddyb commented May 17, 2016

Again, thank you Sergey for bearing with our endless feedback :-)
(Also thanks to @mikewest for reviewing!)

I think this PR needs wording that suggests a missing integrity attribute will trigger a CSP violation report.

@shekyan shekyan force-pushed the shekyan:require-sri-for branch from f07a660 to 64463a7 May 18, 2016
@shekyan
Copy link
Contributor Author

shekyan commented Jun 2, 2016

Folks, there is currently a problem in @mozfreddyb's implementation around resources loaded from CORS-disabled servers. It needs to be reflected in the spec, so I'll start the discussion here:

require-sri-for matching algorithm should allow requests that are missing both integrity metadata and CORS setting attribute, Otherwise, require-sri-for usage is limited to pages that load resources only from CORS-enabled servers.

Example: Content-Security-Policy: require-sri-for script implies that all script elements should have integrity metadata present, but it is not possible to have integrity metadata on resources that are served by CORS-disabled servers, like https://www.google-analytics.com/analytics.js.

What do you think?

P.S. Is there a way to load a resource anonymously from CORS-unaware servers without violating SOP?
Do not see why crossorigin='anonymous' requires server to be CORS-aware.

@metromoxie
Copy link
Contributor

metromoxie commented Jun 2, 2016

FWIW, lgtm with this PR now that @mikewest has approved the change. @devd do you want to review the change? Or should we go ahead and merge?

@devd
Copy link
Contributor

devd commented Jun 3, 2016

My understanding is that this only affects scripts/styles inserted through HTML elements. (because that's what SRI is defined for right now). If so, should the tokens be named script-element rather than just script? e.g., what if a service worker is served with a CSP policy of require-sri-for? How will that work?

@mikewest
Copy link
Member

mikewest commented Jun 3, 2016

I think it's substantially cleaner to reuse the terms from Fetch (https://fetch.spec.whatwg.org/#concept-request-destination) rather than inventing new subsets. Doing so ensures that you'll actually have the detail necessary to block requests. Note, for instance, that Fetch doesn't have any information that would allow it to distinguish <script src='whatever'> and importScripts(whatever).

The argument that SRI doesn't yet support importScripts seems like a poor reason for introducing more complexity to script loading. It instead seems like a good reason to extend SRI to support more resource types, which I think y'all are planning to do anyway. :)

@devd
Copy link
Contributor

devd commented Jun 3, 2016

But we ship this and then turn on support for importScripts or @import,
then sites might break?
On Jun 2, 2016 9:58 PM, "Mike West" notifications@github.com wrote:

I think it's substantially cleaner to reuse the terms from Fetch (
https://fetch.spec.whatwg.org/#concept-request-destination) rather than
inventing new subsets. Doing so ensures that you'll actually have the
detail necessary to block requests. Note, for instance, that Fetch doesn't
have any information that would allow it to distinguish <script src='whatever'> and importScripts(whatever).

The argument that SRI doesn't yet support importScripts seems like a poor
reason for introducing more complexity to script loading. It instead seems
like a good reason to extend SRI to support more resource types, which I
think y'all are planning to do anyway. :)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#32 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAIXGc94g4j3_sNmNP-lu0h-hpYDrvaNks5qH7R2gaJpZM4IYdBU
.

@mikewest
Copy link
Member

mikewest commented Jun 3, 2016

They'd break right away in the presence of this directive, since we'd be blocking the requests. They'd start working once we support those mechanisms. Sounds like a good reason to start widening our support. :)

@devd
Copy link
Contributor

devd commented Jun 3, 2016

@shekyan
Copy link
Contributor Author

shekyan commented Jun 3, 2016

@mikewest, I agree with @devd. In Fetch, destination "script" is for both <script> and importScripts(). Or, destination "media" is for HTML's <audio>, <track>, <video>. So if we reuse terms from Fetch 1-to-1, it might lead to less granular control in the future.

@mozfreddyb
Copy link
Contributor

mozfreddyb commented Jun 3, 2016

require-sri-for matching algorithm should allow requests that are missing both integrity metadata and CORS setting attribute, Otherwise, require-sri-for usage is limited to pages that load resources only from CORS-enabled servers.

I thought that's the intention behind all this? That nobody can use scripts without using integrity (and thus requesting from a CORS-enabled server or a same-origin server).

@mozfreddyb
Copy link
Contributor

mozfreddyb commented Jun 3, 2016

To answer the other question: require-sri-for should work in <meta> CSP policies. Firefox Nightly does not this and that's a bug 🐞 .

@shekyan
Copy link
Contributor Author

shekyan commented Jun 3, 2016

I thought that's the intention behind all this? That nobody can use scripts without using integrity (and thus requesting from a CORS-enabled server or a same-origin server).

Yes, but if SRI does not provide a way to verify integrity of a resource from CORS-disabled server, then that resource should not be subject to require-sri-for, no?

@mikewest
Copy link
Member

mikewest commented Jun 3, 2016

Again, that sounds to me like an argument that SRI should be extended to support resource types it doesn't yet support, not that the current model is a bad one.

@mozfreddyb
Copy link
Contributor

mozfreddyb commented Jun 3, 2016

Yes, but if SRI does not provide a way to verify integrity of a resource from CORS-disabled server, then that resource should not be subject to require-sri-for, no?

I strongly disagree. If there's no way to verify integrity, then there's no way that resource is safe to use. The keyword quite literally says require integrity ;)

@shekyan
Copy link
Contributor Author

shekyan commented Jun 4, 2016

I strongly disagree. If there's no way to verify integrity, then there's no way that resource is safe to use. The keyword quite literally says require integrity ;)

Ultimately, I agree with you, e.g. SRI should be able to verify the integrity of all <script> elements, except if they should not be verified for security reasons.
Practically, require-sri-for would have very limited usage today, because, for example, Google Analytics serves it's code without Access-Control-Allow-Origin, so no way to load the resource anonymously, thus enable SRI on it.
I don't know if anything can be done on SRI land to fix this.
I'll try to tackle CORS spec authors to understand why it is not possible to send anonymous requests to CORS-disabled servers.

@shekyan
Copy link
Contributor Author

shekyan commented Jun 4, 2016

Oh man. I totally missed @mikewest comment. I am ok with that and this PR seems to be ready to go then if there is no other objections!

@devd
Copy link
Contributor

devd commented Jun 4, 2016

If we are going with "enforce this even for cases where you can't specify
integrity" then I would really prefer a note about that before landing.
On Jun 4, 2016 10:34 AM, "Sergey Shekyan" notifications@github.com wrote:

Oh man. I totally missed @mikewest https://github.com/mikewest comment.
I am ok with that and this PR seems to be ready to go then if there is no
other objections!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#32 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAIXGek0onFU_zdZEudRIh1SKv-qkP0Tks5qIbdAgaJpZM4IYdBU
.

@shekyan
Copy link
Contributor Author

shekyan commented Jun 6, 2016

@devd, added a note.

@devd devd merged commit d776be7 into w3c:master Jun 6, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@devd
Copy link
Contributor

devd commented Jun 6, 2016

thanks! this looks good. merging since everyone else ok'ed it.

@mikewest
Copy link
Member

mikewest commented Jun 21, 2016

Would y'all mind publishing this document somewhere? It's tough to skim through sections when reviewing patches, as I end up needing to come back to this PR rather than reading a nicely formatted doc. :)

@devd
Copy link
Contributor

devd commented Jun 21, 2016

done at https://w3c.github.io/webappsec-subresource-integrity/index.bikeshed.html

Since v1 is in the final stages of becoming a rec, I din't want to touch index.html. Once it is done, I will update index.html too.

@ptoomey3
Copy link

ptoomey3 commented Jun 25, 2016

Just now saw this was merged. Thanks so much for the effort ❤️.

@mozfreddyb
Copy link
Contributor

mozfreddyb commented Sep 12, 2016

FYI, I started a discussion on the mailing list. It is about APIs that do not know about integrity metadata (e.g. CSS @import and importScript() and the new Worker constructor in JS) and whether they should be blocked by require-sri-for.

Please read the thread and send your feedback!
https://lists.w3.org/Archives/Public/public-webappsec/2016Sep/0027.html

cc @ptoomey3 @shekyan @devd

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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