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

shekyan · 2016-05-05T22:43:37Z

mozfreddyb · 2016-05-09T13:48:42Z

index.bikeshed.bs

@@ -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}


Don't we still want to verify the response?

It is still there

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

mozfreddyb · 2016-05-09T13:50:50Z

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

shekyan · 2016-05-09T15:41:15Z

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 · 2016-05-10T12:00:55Z

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 · 2016-05-10T12:20:09Z

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

joelweinberger · 2016-05-13T23:49:55Z

index.bikeshed.bs

+
+### Opting-in {#opt-in-require-sri-for}
+
+Authors may opt a Document to require SRI metadata be present for 


Should "Document" be a link to a definition?

joelweinberger · 2016-05-13T23:57:31Z

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

mikewest · 2016-05-15T16:56:03Z

index.bikeshed.bs

+### 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.


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

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?

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

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?

mikewest · 2016-05-15T16:58:24Z

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

shekyan · 2016-05-16T22:00:19Z

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

mikewest · 2016-05-17T17:09:09Z

index.bikeshed.bs

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


Nit: No #.

mikewest · 2016-05-17T19:01:17Z

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 · 2016-05-17T22:29:55Z

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 · 2016-06-02T21:55:58Z

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.

joelweinberger · 2016-06-02T23:41:20Z

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 · 2016-06-03T01:25:16Z

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 · 2016-06-03T04:58:28Z

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 · 2016-06-03T05:20:15Z

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 · 2016-06-03T05:24:12Z

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 · 2016-06-03T05:48:57Z

Aah ok. If require will from the start enforce sri requirement for fetches Sri support isn't implemented/spec'ed for, then lgtm.

shekyan · 2016-06-03T06:02:18Z

@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 · 2016-06-03T07:56:30Z

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 · 2016-06-03T07:58:52Z

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 · 2016-06-03T08:08:38Z

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 · 2016-06-03T08:13:16Z

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 · 2016-06-03T08:16:08Z

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 · 2016-06-04T17:14:00Z

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 · 2016-06-04T17:34:54Z

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 · 2016-06-04T22:19:20Z

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 · 2016-06-06T02:52:28Z

@devd, added a note.

devd · 2016-06-06T02:57:47Z

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

mikewest · 2016-06-21T13:42:16Z

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 · 2016-06-21T18:23:59Z

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 · 2016-06-25T02:36:03Z

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

mozfreddyb · 2016-09-12T06:16:20Z

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

mozfreddyb reviewed May 9, 2016
View reviewed changes

shekyan force-pushed the require-sri-for branch from bc96528 to 39f23c0 Compare May 9, 2016 15:23

mozfreddyb added the SRI-next label May 10, 2016

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

shekyan mentioned this pull request May 13, 2016

Block all non sri resources 2 w3c/webappsec-csp#85

Closed

joelweinberger reviewed May 13, 2016
View reviewed changes

mikewest reviewed May 15, 2016
View reviewed changes

shekyan force-pushed the require-sri-for branch from af0a778 to f07a660 Compare May 16, 2016 21:59

mikewest reviewed May 17, 2016
View reviewed changes

shekyan force-pushed the require-sri-for branch from f07a660 to 64463a7 Compare May 18, 2016 00:53

shekyan force-pushed the require-sri-for branch from 61af64c to a3fe14f Compare May 23, 2016 07:42

shekyan added 2 commits May 23, 2016 00:42

example of require-sri-for

a3fe14f

Removed violation reporting per w3c@884f3f0#r64164831

0afd7c0

Added a note on require-sri-for matching

29db421

devd merged commit d776be7 into w3c:master Jun 6, 2016

oreoshake mentioned this pull request Jul 9, 2016

add support for 'require-sri-for' CSP directive github/secure_headers#271

Closed

2 tasks

This was referenced Aug 16, 2016

Implement Subresource integrity (SRI) PrivateBin/PrivateBin#6

Closed

Use require-sri-for in CSP PrivateBin/PrivateBin#82

Closed


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

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

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

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

Conversation

shekyan commented May 5, 2016 • edited Loading

mozfreddyb May 9, 2016

Choose a reason for hiding this comment

shekyan May 9, 2016 • edited Loading

Choose a reason for hiding this comment

mozfreddyb May 10, 2016

Choose a reason for hiding this comment

mozfreddyb commented May 9, 2016

shekyan commented May 9, 2016

mozfreddyb commented May 10, 2016

mozfreddyb commented May 10, 2016

joelweinberger May 13, 2016

Choose a reason for hiding this comment

joelweinberger commented May 13, 2016

mikewest May 15, 2016

Choose a reason for hiding this comment

mikewest commented May 15, 2016

shekyan commented May 16, 2016

mikewest May 17, 2016

Choose a reason for hiding this comment

mikewest commented May 17, 2016

mozfreddyb commented May 17, 2016

shekyan commented Jun 2, 2016 • edited Loading

joelweinberger commented Jun 2, 2016

devd commented Jun 3, 2016

mikewest commented Jun 3, 2016

devd commented Jun 3, 2016

mikewest commented Jun 3, 2016

devd commented Jun 3, 2016 via email

shekyan commented Jun 3, 2016 • edited Loading

mozfreddyb commented Jun 3, 2016

mozfreddyb commented Jun 3, 2016

shekyan commented Jun 3, 2016 • edited Loading

mikewest commented Jun 3, 2016

mozfreddyb commented Jun 3, 2016

shekyan commented Jun 4, 2016 • edited Loading

shekyan commented Jun 4, 2016

devd commented Jun 4, 2016

shekyan commented Jun 6, 2016

devd commented Jun 6, 2016

mikewest commented Jun 21, 2016

devd commented Jun 21, 2016

ptoomey3 commented Jun 25, 2016

mozfreddyb commented Sep 12, 2016

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

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

shekyan commented May 5, 2016 •

edited

Loading

shekyan May 9, 2016 •

edited

Loading

shekyan commented Jun 2, 2016 •

edited

Loading

shekyan commented Jun 3, 2016 •

edited

Loading

shekyan commented Jun 3, 2016 •

edited

Loading

shekyan commented Jun 4, 2016 •

edited

Loading