WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
148363
Implement Subresource Integrity (SRI)
https://bugs.webkit.org/show_bug.cgi?id=148363
Summary
Implement Subresource Integrity (SRI)
sideshowbarker
Reported
2015-08-22 18:21:49 PDT
The SRI specification "defines a mechanism by which user agents may verify that a fetched resource has been delivered without unexpected manipulation" using a validation scheme and "extending several HTML elements with an integrity attribute that contains a cryptographic hash of the representation of the resource the author expects to load."
http://w3c.github.io/webappsec/specs/subresourceintegrity/
Example: If a document loads some JavaScript library code from a shared server at
https://example.com/example-framework.js
rather than from the same own origin as the document, the document can specify the expected SHA-256 hash of
https://example.com/example-framework.js
(e.g., C6CB9UYIS9UJeqinPHWTHVqh/E1uhG5Twh+Y5qFQmYg=) and the UA, before executing the JavaScript, can verify that the data matches that expected hash. <script src="
https://example.com/example-framework.js
" integrity="sha256-C6CB9UYIS9UJeqinPHWTHVqh/E1uhG5Twh+Y5qFQmYg=" crossorigin="anonymous"></script> The mechanism can also be used for resources loaded through <link> elements. As far as support in other UAs, Chrome has supported Subresource Integrity since v45, and Firefox has since v43.
https://developer.mozilla.org/en/docs/Web/HTML/Element/script#Browser_compatibility
Attachments
Patch
(132.71 KB, patch)
2017-03-08 17:37 PST
,
Sam Weinig
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
(1.10 MB, application/zip)
2017-03-08 19:01 PST
,
Build Bot
no flags
Details
Archive of layout-test-results from ews101 for mac-elcapitan
(1002.25 KB, application/zip)
2017-03-08 19:38 PST
,
Build Bot
no flags
Details
Archive of layout-test-results from ews123 for ios-simulator-wk2
(827.92 KB, application/zip)
2017-03-08 19:55 PST
,
Build Bot
no flags
Details
Patch
(143.49 KB, patch)
2017-03-12 20:43 PDT
,
Sam Weinig
no flags
Details
Formatted Diff
Diff
Patch
(106.14 KB, patch)
2017-04-28 11:06 PDT
,
Sam Weinig
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews102 for mac-elcapitan
(983.88 KB, application/zip)
2017-04-28 12:32 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
(1.30 MB, application/zip)
2017-04-28 12:34 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews117 for mac-elcapitan
(1.76 MB, application/zip)
2017-04-28 12:57 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews121 for ios-simulator-wk2
(22.89 MB, application/zip)
2017-04-28 13:08 PDT
,
Build Bot
no flags
Details
Patch
(98.04 KB, patch)
2017-04-29 15:39 PDT
,
Sam Weinig
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews112 for mac-elcapitan
(1.98 MB, application/zip)
2017-04-30 14:53 PDT
,
Build Bot
no flags
Details
Patch (just <link> and <script>)
(84.79 KB, patch)
2017-05-06 19:54 PDT
,
Sam Weinig
no flags
Details
Formatted Diff
Diff
Patch
(30.77 KB, patch)
2017-05-09 09:44 PDT
,
Sam Weinig
youennf
: review+
Details
Formatted Diff
Diff
Show Obsolete
(13)
View All
Add attachment
proposed patch, testcase, etc.
sideshowbarker
Comment 1
2016-06-27 02:03:44 PDT
Subresource Integrity is now a final W3C Recommendation
https://www.w3.org/TR/SRI/
Barrett Harber
Comment 2
2016-12-19 13:12:47 PST
+1 for this
Dan Fabulich
Comment 3
2017-03-08 10:17:45 PST
I filed a Radar issue for this; it was marked as a duplicate of
rdar://18945879
Dan Fabulich
Comment 4
2017-03-08 12:46:10 PST
Oops, I mean
rdar://problem/18945879
Sam Weinig
Comment 5
2017-03-08 17:37:40 PST
Created
attachment 303875
[details]
Patch
Sam Weinig
Comment 6
2017-03-08 17:40:00 PST
WIP patch. I would like some folks who have played in the Fetch / Loader stack more recently than I to take a look. Some known things I need to improve and test: - Fetch with a stream. - Fix FIXME in HTMLLinkElement about using the correct integrity metadata. - Probably more.
Build Bot
Comment 7
2017-03-08 19:01:53 PST
Comment on
attachment 303875
[details]
Patch
Attachment 303875
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.webkit.org/results/3273336
New failing tests: imported/w3c/web-platform-tests/html/dom/reflection-metadata.html imported/w3c/web-platform-tests/html/dom/reflection-misc.html
Build Bot
Comment 8
2017-03-08 19:01:57 PST
Created
attachment 303887
[details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Build Bot
Comment 9
2017-03-08 19:38:20 PST
Comment on
attachment 303875
[details]
Patch
Attachment 303875
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.webkit.org/results/3273635
New failing tests: imported/w3c/web-platform-tests/html/dom/reflection-metadata.html imported/w3c/web-platform-tests/html/dom/reflection-misc.html
Build Bot
Comment 10
2017-03-08 19:38:24 PST
Created
attachment 303890
[details]
Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Build Bot
Comment 11
2017-03-08 19:55:44 PST
Comment on
attachment 303875
[details]
Patch
Attachment 303875
[details]
did not pass ios-sim-ews (ios-simulator-wk2): Output:
http://webkit-queues.webkit.org/results/3273716
New failing tests: imported/w3c/web-platform-tests/html/dom/reflection-metadata.html imported/w3c/web-platform-tests/html/dom/reflection-misc.html
Build Bot
Comment 12
2017-03-08 19:55:49 PST
Created
attachment 303891
[details]
Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
youenn fablet
Comment 13
2017-03-08 21:12:37 PST
Looking quickly at the patch, I am wondering why some integrity checks are done at client loader level. Why not doing that at SubresourceLoader/CachedResource/CachedResourceLoader level? The one thing needed for clients would be to ensure not leaking any data before didFinishLoading is called. I guess this might only apply to fetch API.
Sam Weinig
Comment 14
2017-03-09 08:42:15 PST
(In reply to
comment #13
)
> Looking quickly at the patch, I am wondering why some integrity checks are > done at client loader level. > Why not doing that at SubresourceLoader/CachedResource/CachedResourceLoader > level?
I ended up doing them at the client level because I couldn't find a good way to handle the case where where there are two requests for the same cached resource (which has yet to finish loading). In that case, when the load finishes, the CachedResource only knows about clients of the resource, not the requests that initiated them, so can't retrieve the integrity metadata needed. We could consider changing CachedResource so that it keeps tracks of the CachedResourceRequests as well, but that didn't immediately seem like an improvement worth making without re-evaluating loading more holistically.
> The one thing needed for clients would be to ensure not leaking any data > before didFinishLoading is called. > I guess this might only apply to fetch API.
Yeah, the fetch one is tricky, since the CachedResource doesn't seem to even be buffering the data (due to explicitly asking the ThreadableLoader not to). I am kind of curious if we are missing out on some memory cache goodness for fetch requests.
youenn fablet
Comment 15
2017-03-09 09:26:37 PST
(In reply to
comment #14
)
> (In reply to
comment #13
) > > Looking quickly at the patch, I am wondering why some integrity checks are > > done at client loader level. > > Why not doing that at SubresourceLoader/CachedResource/CachedResourceLoader > > level? > > I ended up doing them at the client level because I couldn't find a good way > to handle the case where where there are two requests for the same cached > resource (which has yet to finish loading). In that case, when the load > finishes, the CachedResource only knows about clients of the resource, not > the requests that initiated them, so can't retrieve the integrity metadata > needed. We could consider changing CachedResource so that it keeps tracks of > the CachedResourceRequests as well, but that didn't immediately seem like an > improvement worth making without re-evaluating loading more holistically.
I see. DocumentThreadableLoader would be a better place for fetch and the likes. At least, it would be closer to ResourceLoader et al. CORS checks are now removed at the client level and this seems like the right thing to do. The added complexity in CachedResource/CachedResourceLoader is not very good though, but origin is more fundamental than SRI. Maybe we need a better communication channel with CachedResourceClient to accommodate SRI. For instance, we only have notifyFinished. Why not having one for success and one for error.
> > The one thing needed for clients would be to ensure not leaking any data > > before didFinishLoading is called. > > I guess this might only apply to fetch API. > > Yeah, the fetch one is tricky, since the CachedResource doesn't seem to even > be buffering the data (due to explicitly asking the ThreadableLoader not > to). I am kind of curious if we are missing out on some memory cache > goodness for fetch requests.
IIRC, RawResourceRequest memory caching is indeed something we should work on. Especially with prefetching arriving. As of the hash computation, it should probably be done on the fly. We should not require to store the whole data just for this purpose.
Sam Weinig
Comment 16
2017-03-09 10:04:59 PST
(In reply to
comment #15
)
> (In reply to
comment #14
) > > (In reply to
comment #13
) > > > Looking quickly at the patch, I am wondering why some integrity checks are > > > done at client loader level. > > > Why not doing that at SubresourceLoader/CachedResource/CachedResourceLoader > > > level? > > > > I ended up doing them at the client level because I couldn't find a good way > > to handle the case where where there are two requests for the same cached > > resource (which has yet to finish loading). In that case, when the load > > finishes, the CachedResource only knows about clients of the resource, not > > the requests that initiated them, so can't retrieve the integrity metadata > > needed. We could consider changing CachedResource so that it keeps tracks of > > the CachedResourceRequests as well, but that didn't immediately seem like an > > improvement worth making without re-evaluating loading more holistically. > > I see. > DocumentThreadableLoader would be a better place for fetch and the likes. > At least, it would be closer to ResourceLoader et al. > > CORS checks are now removed at the client level and this seems like the > right thing to do. The added complexity in > CachedResource/CachedResourceLoader is not very good though, but origin is > more fundamental than SRI. > > Maybe we need a better communication channel with CachedResourceClient to > accommodate SRI. For instance, we only have notifyFinished. Why not having > one for success and one for error. > > > > The one thing needed for clients would be to ensure not leaking any data > > > before didFinishLoading is called. > > > I guess this might only apply to fetch API. > > > > Yeah, the fetch one is tricky, since the CachedResource doesn't seem to even > > be buffering the data (due to explicitly asking the ThreadableLoader not > > to). I am kind of curious if we are missing out on some memory cache > > goodness for fetch requests. > > IIRC, RawResourceRequest memory caching is indeed something we should work > on. > Especially with prefetching arriving. > As of the hash computation, it should probably be done on the fly. > We should not require to store the whole data just for this purpose.
Alas, I think we have to store the whole data for this purpose in the fetch API case, as we have to wait until we have checked the integrity to pass any data to the client. We could totally calculate the digest as the data comes in, I'm just not sure it adds anything.
youenn fablet
Comment 17
2017-03-09 14:53:46 PST
> Alas, I think we have to store the whole data for this purpose in the fetch > API case, as we have to wait until we have checked the integrity to pass any > data to the client. We could totally calculate the digest as the data comes > in, I'm just not sure it adds anything.
If the checks are moved to DocumentThreadableLoader and/or below, it might be better to do so. DTL is just passing the data to the client for instance atm.
Sam Weinig
Comment 18
2017-03-12 20:43:16 PDT
Created
attachment 304219
[details]
Patch
Sam Weinig
Comment 19
2017-03-12 20:56:39 PDT
Things I would like to still do for this patch: - Add copy (or variant) of imported tests to http directory that use localhost / 127.0.0.1 instead of localhost vs. www.localhost for cross origin checks. - Add tests of fetch that use a stream. - Reconsider if the checks in CachedResourceLoader are necessary. Since we also check in the CachedResourceClient, I'm not sure they are. - Decide if moving integrity check (and buffering) to DocumentThreadableLoader from Fetch code makes sense for this first pass.
Sam Weinig
Comment 20
2017-03-12 21:05:16 PDT
(In reply to
comment #19
)
> Things I would like to still do for this patch: > - Add copy (or variant) of imported tests to http directory that use > localhost / 127.0.0.1 instead of localhost vs. www.localhost for cross > origin checks. > - Add tests of fetch that use a stream. > - Reconsider if the checks in CachedResourceLoader are necessary. Since we > also check in the CachedResourceClient, I'm not sure they are. > - Decide if moving integrity check (and buffering) to > DocumentThreadableLoader from Fetch code makes sense for this first pass.
Additional thoughts: - Add tests that use both base64 styles (normal and URL) and ensure this matches other browsers. - Add tests that have options (
https://w3c.github.io/webappsec-subresource-integrity/#grammardef-option-expression
), and make sure they are ignored correctly (noting the spec says "In order for user agents to remain fully forwards compatible with future options, the user agent MUST ignore all unrecognized option-expressions.")
youenn fablet
Comment 21
2017-03-12 22:06:39 PDT
(In reply to
comment #19
)
> Things I would like to still do for this patch: > - Add copy (or variant) of imported tests to http directory that use > localhost / 127.0.0.1 instead of localhost vs. www.localhost for cross > origin checks.
It would be better to update w3c wpt to use
https://github.com/w3c/web-platform-tests/blob/master/common/get-host-info.sub.js
Just include this file in the html file, then use get_host_info(). HTTP_REMOTE_ORIGIN instead of creating the cross-origin directly in the test. We have done so for other tests and upstreaming the changes to wpt should not cause any issue.
> - Add tests of fetch that use a stream. > - Reconsider if the checks in CachedResourceLoader are necessary. Since we > also check in the CachedResourceClient, I'm not sure they are.
Agreed, it is better to catch that at CachedResourceClient since there may be other paths leading to resource loading anyway. Also, we could try to have a more declarative way to say that a CachedResource is SRI-enabled and if so, should check SRI for each of its client before calling loading finished callback. In such a case, the client would only be responsible to log the error if any. That would be more inline with the fetch algorithm concept.
> - Decide if moving integrity check (and buffering) to > DocumentThreadableLoader from Fetch code makes sense for this first pass.
That makes a lot of sense to me. SRI is a fetch concept, not a fetch API one. Fetch API should just be a pass-through. We could add a boolean flag asking whether to send the data once validated or whether progressive is fine to the threadable loader options in addition to the integrity parameter. That would remove the need to modify fetch API implementation.
youenn fablet
Comment 22
2017-03-18 17:26:44 PDT
(In reply to
comment #14
)
> (In reply to
comment #13
) > > Looking quickly at the patch, I am wondering why some integrity checks are > > done at client loader level. > > Why not doing that at SubresourceLoader/CachedResource/CachedResourceLoader > > level? > > I ended up doing them at the client level because I couldn't find a good way > to handle the case where where there are two requests for the same cached > resource (which has yet to finish loading).
I forgot to mention: is this a practical use case? Is it worth optimising it? Can we just have separate loads until we agree on a better approach?
Sam Weinig
Comment 23
2017-04-18 19:15:46 PDT
(In reply to youenn fablet from
comment #22
)
> (In reply to
comment #14
) > > (In reply to
comment #13
) > > > Looking quickly at the patch, I am wondering why some integrity checks are > > > done at client loader level. > > > Why not doing that at SubresourceLoader/CachedResource/CachedResourceLoader > > > level? > > > > I ended up doing them at the client level because I couldn't find a good way > > to handle the case where where there are two requests for the same cached > > resource (which has yet to finish loading). > > I forgot to mention: is this a practical use case? Is it worth optimising > it? Can we just have separate loads until we agree on a better approach?
I think it is common enough (think, multiple image sprites on a page) that it is worth avoiding an additional load.
youenn fablet
Comment 24
2017-04-18 20:16:18 PDT
> I think it is common enough (think, multiple image sprites on a page) that > it is worth avoiding an additional load.
There are well known cases for the same resource loaded at about the same time from the same page with different CORS. Is there such a case for SRI? If not, I think it is fine implementing a first version without optimizing this particular case. Serving already cached resources with different SRI than the one they were fetched with seems a likely scenario.
Sam Weinig
Comment 25
2017-04-28 11:06:35 PDT
Created
attachment 308558
[details]
Patch
Sam Weinig
Comment 26
2017-04-28 11:08:33 PDT
(In reply to Sam Weinig from
comment #25
)
> Created
attachment 308558
[details]
> Patch
This simplifies things a bit, and moves logic out of Fetch and into DocumentThreadableLoader. Still not completely satisfied with this, but I'm not sure I will be without re-working loading quite a bit more. Might be worth getting in in some form.
youenn fablet
Comment 27
2017-04-28 11:56:20 PDT
Comment on
attachment 308558
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=308558&action=review
> Source/WebCore/Modules/fetch/FetchLoader.cpp:82 > + options.dataBufferingPolicy = options.integrity.isEmpty() ? DoNotBufferData : BufferData;
Do we need that change? DTL should not send us the response before getting the full data anyway.
> Source/WebCore/bindings/js/CachedScriptFetcher.h:47 > + , m_integrityMetadata(integrityMetadata)
Should we try to move towards String&&? If so, probably need to do that for all parameters. Maybe future refactoring? Also the case for other parts of this patch.
> Source/WebCore/bindings/js/CachedScriptFetcher.h:61 > + String integrityMetadata() { return m_integrityMetadata; }
const probably, rename it to integrity() or the other side? But do we need it?
> Source/WebCore/bindings/js/JSDOMBindingCaller.h:99 > +}
No need for that change. Please remove it.
> Source/WebCore/dom/LoadableClassicScript.cpp:105 > + }
I still think this is not the right place for these checks. I don't know whether a FIXME about moving that to CachedResource/loader Level would be useful there
> Source/WebCore/html/HTMLLinkElement.cpp:285 > + options.integrity = m_integrityMetadataForPendingSheetRequest;
It seems strange to do both setting this option here and keeping m_integrityMetadataForPendingSheetRequest.
> Source/WebCore/loader/DocumentThreadableLoader.cpp:380 > + m_client->didReceiveData(m_resource->resourceBuffer()->data(), m_resource->resourceBuffer()->size());
We should probably call didReceiveResponse and not m_client->didReceiveResponse. cq- for that reason. Can you add some cors filtering+integrity tests?
Build Bot
Comment 28
2017-04-28 12:32:21 PDT
Comment on
attachment 308558
[details]
Patch
Attachment 308558
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.webkit.org/results/3627953
New failing tests: imported/w3c/web-platform-tests/subresource-integrity/subresource-integrity.sub.html
Build Bot
Comment 29
2017-04-28 12:32:23 PDT
Created
attachment 308571
[details]
Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Build Bot
Comment 30
2017-04-28 12:34:27 PDT
Comment on
attachment 308558
[details]
Patch
Attachment 308558
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.webkit.org/results/3627950
New failing tests: imported/w3c/web-platform-tests/subresource-integrity/subresource-integrity.sub.html
Build Bot
Comment 31
2017-04-28 12:34:29 PDT
Created
attachment 308573
[details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Build Bot
Comment 32
2017-04-28 12:57:19 PDT
Comment on
attachment 308558
[details]
Patch
Attachment 308558
[details]
did not pass mac-debug-ews (mac): Output:
http://webkit-queues.webkit.org/results/3627978
New failing tests: imported/w3c/web-platform-tests/subresource-integrity/subresource-integrity.sub.html
Build Bot
Comment 33
2017-04-28 12:57:21 PDT
Created
attachment 308580
[details]
Archive of layout-test-results from ews117 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Build Bot
Comment 34
2017-04-28 13:08:54 PDT
Comment on
attachment 308558
[details]
Patch
Attachment 308558
[details]
did not pass ios-sim-ews (ios-simulator-wk2): Output:
http://webkit-queues.webkit.org/results/3628046
New failing tests: imported/w3c/web-platform-tests/subresource-integrity/subresource-integrity.sub.html
Build Bot
Comment 35
2017-04-28 13:08:57 PDT
Created
attachment 308586
[details]
Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Sam Weinig
Comment 36
2017-04-29 15:36:21 PDT
(In reply to youenn fablet from
comment #27
)
> Comment on
attachment 308558
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=308558&action=review
> > > Source/WebCore/loader/DocumentThreadableLoader.cpp:380 > > + m_client->didReceiveData(m_resource->resourceBuffer()->data(), m_resource->resourceBuffer()->size()); > > We should probably call didReceiveResponse and not > m_client->didReceiveResponse. cq- for that reason.
Calling didReceiveResponse seems weird. In certain circumstances it calls m_client->didFinishLoading() in addition to m_client->didReceiveResponse(), and I am not familiar enough with reasoning to figure out if that is something I need to deal with (and avoid calling _client->didFinishLoading() as second time) or something that can't happen. (This code makes me sad).
> Can you add some cors filtering+integrity tests?
I'm not exactly sure what you are getting at here. Do you mean a fetch with "no-cors" set? The behavior there is if you have "no-cors" and a non-empty "integrity", we throw. This is existing behavior that I'm pretty sure we have tests for.
Sam Weinig
Comment 37
2017-04-29 15:39:18 PDT
Created
attachment 308676
[details]
Patch
youenn fablet
Comment 38
2017-04-29 15:51:21 PDT
> Calling didReceiveResponse seems weird. In certain circumstances it calls > m_client->didFinishLoading() in addition to m_client->didReceiveResponse(), > and I am not familiar enough with reasoning to figure out if that is > something I need to deal with (and avoid calling _client->didFinishLoading() > as second time) or something that can't happen. (This code makes me sad).
Sure and the integrity boolean would need to be updated as well which would be misleading. What I wanted to say is that headers/Response filtering happens in didReceiveResponse. This should also be the case when doing integrity checks. Hence the usefulness of a test doing cors-mode+integrity+response-header-filtering. Given the need to do that filtering in integrity code path, we could refactor a bit the code and make clearer its intent. The filtering in DTL is a nice thing as it simplifies XHR/fetch exposing algorithms. That said, we will probably need in the future to keep all information in fetch so as to pass it to other users like image loaders and so on. Let's not worry about this case for now.
Sam Weinig
Comment 39
2017-04-30 12:04:49 PDT
(In reply to youenn fablet from
comment #38
)
> > Calling didReceiveResponse seems weird. In certain circumstances it calls > > m_client->didFinishLoading() in addition to m_client->didReceiveResponse(), > > and I am not familiar enough with reasoning to figure out if that is > > something I need to deal with (and avoid calling _client->didFinishLoading() > > as second time) or something that can't happen. (This code makes me sad). > > Sure and the integrity boolean would need to be updated as well which would > be misleading. > What I wanted to say is that headers/Response filtering happens in > didReceiveResponse. > This should also be the case when doing integrity checks. > Hence the usefulness of a test doing > cors-mode+integrity+response-header-filtering. > > Given the need to do that filtering in integrity code path, we could > refactor a bit the code and make clearer its intent. > > The filtering in DTL is a nice thing as it simplifies XHR/fetch exposing > algorithms. > That said, we will probably need in the future to keep all information in > fetch so as to pass it to other users like image loaders and so on. Let's > not worry about this case for now.
Ok. Do you think this patch is good to go then?
Daniel Bates
Comment 40
2017-04-30 14:21:17 PDT
Comment on
attachment 308676
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=308676&action=review
> Source/WebCore/ChangeLog:4 > +
https://bugs.webkit.org/show_bug.cgi?id=148363
Please add the radar bug URL under this line.
> Source/WebCore/ChangeLog:22 > + ThreadableLoaderOptions::isolatedCopy to work correctly (it was missing isolated > + copy derivedCachedDataTypesToRetrieve).
Can we add a test for this change?
> Source/WebCore/ChangeLog:50 > + When requesting a stylesheet, cache the integrity metadata so it can > + be used when the load completes (accessing the attribute then would be > + incorrect, as it a script might have changed its value since the request).
This sentence does not read well. In particular, the "as it a script" part does not read well.
> Source/WebCore/html/HTMLLinkElement.cpp:384 > + notifyLoadedSheetAndAllCriticalSubresources(true);
This is OK as-is. We should have notifyLoadedSheetAndAllCriticalSubresources() take a enum instead of a boolean to indicate that an error occurred to make the code read well at the call site.
> Source/WebCore/loader/SubresourceIntegrity.cpp:55 > + auto crypographicDigest = parseCryptographicDigest(position, end);
crypographicDigest => cryptographicDigest (Notice the presence of the letter t in "cryptographic")
> Source/WebCore/loader/SubresourceIntegrity.cpp:107 > + // FIXME: The spec says this should check XXX.
XXX?
> Source/WebCore/loader/SubresourceIntegrity.cpp:115 > + return (a > b) ? a : b;
The parentheses are not necessary.
> Source/WebCore/loader/SubresourceIntegrity.cpp:122 > + ResourceCryptographicDigest::Algorithm strongest;
The Win-EWS bots is not happy that this is uninitialized when it compiles the assignment on line 134. VC++ cannot verify that this local would have been initialized by line 128, which is guaranteed because the vector is initially empty.
> LayoutTests/imported/w3c/web-platform-tests/subresource-integrity/subresource-integrity.sub.html:12 > + // WEBKIT CHANGES
We should upstream these changes, find a better place for this test, come up with a naming convention for modified WPTs that have not been upstreamed, or add a remark to LayoutTests/imported/w3c/web-platform-tests/subresource-integrity/tools/w3c-import.log so as to prevent a person from inadvertently overwriting this test with the upstream test of the same name when resyncing WPT.
> LayoutTests/imported/w3c/web-platform-tests/subresource-integrity/subresource-integrity.sub.html:17 > + // - Changed script tests to match the style tests and operate on a queue > + // to address issue where console messages where coming in at inconsistent > + // times, making the results flaky.
This problem, out-of-order console messages, is not specific to just this test. It affects our ability to run other web platform test. We do not need to solve this issue now, but we should figure out how to solve it or come up with a plan on how we are going to deal wit this issue moving forward. Individually modifying WPTs and maintaining these variants does not seem practical in the long term. We could fix all upstream asynchronous tests to use a queue to ensure ordering. Another option is to leave the tests as-is and add the tests to TestExpectations with the modifier DumpJSConsoleLogInStdErr to dump console messages to standard error instead of emitting to standard output as part of the test result. Obviously, the disadvantage of the latter approach is that run-webkit-tests does not compare standard error output (at least at the time of writing) and people are unlikely to compare such output manually so we may miss output that could indicate a regression.
> LayoutTests/imported/w3c/web-platform-tests/subresource-integrity/tools/w3c-import.log:2 > +Do NOT modify these tests directly in WebKit.
We did this in this patch :)
Build Bot
Comment 41
2017-04-30 14:53:13 PDT
Comment on
attachment 308676
[details]
Patch
Attachment 308676
[details]
did not pass mac-debug-ews (mac): Output:
http://webkit-queues.webkit.org/results/3646373
New failing tests: fast/dom/Window/remove-timeout-crash.html
Build Bot
Comment 42
2017-04-30 14:53:15 PDT
Created
attachment 308702
[details]
Archive of layout-test-results from ews112 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
youenn fablet
Comment 43
2017-04-30 15:05:57 PDT
(In reply to youenn fablet from
comment #38
)
> > Calling didReceiveResponse seems weird. In certain circumstances it calls > > m_client->didFinishLoading() in addition to m_client->didReceiveResponse(), > > and I am not familiar enough with reasoning to figure out if that is > > something I need to deal with (and avoid calling _client->didFinishLoading() > > as second time) or something that can't happen. (This code makes me sad). > > Sure and the integrity boolean would need to be updated as well which would > be misleading. > What I wanted to say is that headers/Response filtering happens in > didReceiveResponse. > This should also be the case when doing integrity checks. > Hence the usefulness of a test doing > cors-mode+integrity+response-header-filtering. > > Given the need to do that filtering in integrity code path, we could > refactor a bit the code and make clearer its intent. > > The filtering in DTL is a nice thing as it simplifies XHR/fetch exposing > algorithms. > That said, we will probably need in the future to keep all information in > fetch so as to pass it to other users like image loaders and so on. Let's > not worry about this case for
It is an important hole to stop filtering cross origin responses. I am fine replicating the code that filters the response in didReceiveResponse using filterResponse routine. And add a fixme for refactoring.
youenn fablet
Comment 44
2017-04-30 15:08:20 PDT
As of console log issues, dumjsconsoleloginstderr seems like the best short term approach. We should then move from async_test to promise_test
Maciej Stachowiak
Comment 45
2017-04-30 15:34:04 PDT
It looks like this landed without a runtime or compile-time flag (if I'm reading this correctly). Could one be added please? (I can file a separate bug for that if it would be helpful).
Sam Weinig
Comment 46
2017-04-30 18:18:48 PDT
(In reply to Maciej Stachowiak from
comment #45
)
> It looks like this landed without a runtime or compile-time flag (if I'm > reading this correctly). Could one be added please? (I can file a separate > bug for that if it would be helpful).
Fair point, though it hasn't landed yet. I can add a runtime flag. I didn't because the spec is quite mature and Chrome and Firefox (not sure about Edge) have had implementations for a while. But it is straightforward to do if desirable.
Sam Weinig
Comment 47
2017-04-30 18:26:28 PDT
(In reply to Daniel Bates from
comment #40
)
> Comment on
attachment 308676
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=308676&action=review
> > > Source/WebCore/ChangeLog:4 > > +
https://bugs.webkit.org/show_bug.cgi?id=148363
> > Please add the radar bug URL under this line.
Ok.
> > > Source/WebCore/ChangeLog:22 > > + ThreadableLoaderOptions::isolatedCopy to work correctly (it was missing isolated > > + copy derivedCachedDataTypesToRetrieve). > > Can we add a test for this change?
I'm not sure of a good way to test it, as all it is switching is a normal ref bump copy, to and isolated copy. I guess an API test showing all the impls are different would do it. Ok.
> > > Source/WebCore/ChangeLog:50 > > + When requesting a stylesheet, cache the integrity metadata so it can > > + be used when the load completes (accessing the attribute then would be > > + incorrect, as it a script might have changed its value since the request). > > This sentence does not read well. In particular, the "as it a script" part > does not read well.
Ok.
> > > Source/WebCore/loader/SubresourceIntegrity.cpp:55 > > + auto crypographicDigest = parseCryptographicDigest(position, end); > > crypographicDigest => cryptographicDigest > > (Notice the presence of the letter t in "cryptographic")
Yup.
> > > Source/WebCore/loader/SubresourceIntegrity.cpp:107 > > + // FIXME: The spec says this should check XXX. > > XXX?
Eek.
> > > Source/WebCore/loader/SubresourceIntegrity.cpp:115 > > + return (a > b) ? a : b; > > The parentheses are not necessary.
I think it reads cleaner, ambiguity rules being somewhat opaque.
> > > Source/WebCore/loader/SubresourceIntegrity.cpp:122 > > + ResourceCryptographicDigest::Algorithm strongest; > > The Win-EWS bots is not happy that this is uninitialized when it compiles > the assignment on line 134. VC++ cannot verify that this local would have > been initialized by line 128, which is guaranteed because the vector is > initially empty.
Fun.
> > LayoutTests/imported/w3c/web-platform-tests/subresource-integrity/subresource-integrity.sub.html:12 > > + // WEBKIT CHANGES > > We should upstream these changes, find a better place for this test, come up > with a naming convention for modified WPTs that have not been upstreamed, or > add a remark to > LayoutTests/imported/w3c/web-platform-tests/subresource-integrity/tools/w3c- > import.log so as to prevent a person from inadvertently overwriting this > test with the upstream test of the same name when resyncing WPT. >
That's quite a bit more than I think is called for for this patch. We have made quite a few changes to imported tests already. I agree a we need a better process, but I'm not sure it needs to hold this up.
> > LayoutTests/imported/w3c/web-platform-tests/subresource-integrity/subresource-integrity.sub.html:17 > > + // - Changed script tests to match the style tests and operate on a queue > > + // to address issue where console messages where coming in at inconsistent > > + // times, making the results flaky. > > This problem, out-of-order console messages, is not specific to just this > test. It affects our ability to run other web platform test. We do not need > to solve this issue now, but we should figure out how to solve it or come up > with a plan on how we are going to deal wit this issue moving forward. > Individually modifying WPTs and maintaining these variants does not seem > practical in the long term. We could fix all upstream asynchronous tests to > use a queue to ensure ordering. Another option is to leave the tests as-is > and add the tests to TestExpectations with the modifier > DumpJSConsoleLogInStdErr to dump console messages to standard error instead > of emitting to standard output as part of the test result. Obviously, the > disadvantage of the latter approach is that run-webkit-tests does not > compare standard error output (at least at the time of writing) and people > are unlikely to compare such output manually so we may miss output that > could indicate a regression.
I'd like to keep the modifications.
Sam Weinig
Comment 48
2017-04-30 18:26:40 PDT
(In reply to youenn fablet from
comment #43
)
> (In reply to youenn fablet from
comment #38
) > > > Calling didReceiveResponse seems weird. In certain circumstances it calls > > > m_client->didFinishLoading() in addition to m_client->didReceiveResponse(), > > > and I am not familiar enough with reasoning to figure out if that is > > > something I need to deal with (and avoid calling _client->didFinishLoading() > > > as second time) or something that can't happen. (This code makes me sad). > > > > Sure and the integrity boolean would need to be updated as well which would > > be misleading. > > What I wanted to say is that headers/Response filtering happens in > > didReceiveResponse. > > This should also be the case when doing integrity checks. > > Hence the usefulness of a test doing > > cors-mode+integrity+response-header-filtering. > > > > Given the need to do that filtering in integrity code path, we could > > refactor a bit the code and make clearer its intent. > > > > The filtering in DTL is a nice thing as it simplifies XHR/fetch exposing > > algorithms. > > That said, we will probably need in the future to keep all information in > > fetch so as to pass it to other users like image loaders and so on. Let's > > not worry about this case for > > It is an important hole to stop filtering cross origin responses. I am fine > replicating the code that filters the response in didReceiveResponse using > filterResponse routine. And add a fixme for refactoring.
Ok. Will do.
youenn fablet
Comment 49
2017-04-30 20:08:56 PDT
> > We should upstream these changes, find a better place for this test, come up > > with a naming convention for modified WPTs that have not been upstreamed, or > > add a remark to > > LayoutTests/imported/w3c/web-platform-tests/subresource-integrity/tools/w3c- > > import.log so as to prevent a person from inadvertently overwriting this > > test with the upstream test of the same name when resyncing WPT. > > > > That's quite a bit more than I think is called for for this patch. We have > made quite a few changes to imported tests already. I agree a we need a > better process, but I'm not sure it needs to hold this up.
We did change some tests and either upstreamed to W3C repo or reverted these changes later on, typically at resync time. The only current exception is some newly added files in WebKit and not in W3C repo. All other files present in W3C repo should be the same. Changing w3c-import.log without updating test-importer.py will not help much here. As discussed with Sam, we could add a WebKit folder in imported/w3c/web-platform-tests or update our infra to let wpt server serve an additional directory (
bug 171479
).
> > > > LayoutTests/imported/w3c/web-platform-tests/subresource-integrity/subresource-integrity.sub.html:17 > > > + // - Changed script tests to match the style tests and operate on a queue > > > + // to address issue where console messages where coming in at inconsistent > > > + // times, making the results flaky. > > > > This problem, out-of-order console messages, is not specific to just this > > test. It affects our ability to run other web platform test. We do not need > > to solve this issue now, but we should figure out how to solve it or come up > > with a plan on how we are going to deal wit this issue moving forward. > > Individually modifying WPTs and maintaining these variants does not seem > > practical in the long term. We could fix all upstream asynchronous tests to > > use a queue to ensure ordering. Another option is to leave the tests as-is > > and add the tests to TestExpectations with the modifier > > DumpJSConsoleLogInStdErr to dump console messages to standard error instead > > of emitting to standard output as part of the test result. Obviously, the > > disadvantage of the latter approach is that run-webkit-tests does not > > compare standard error output (at least at the time of writing) and people > > are unlikely to compare such output manually so we may miss output that > > could indicate a regression. > > I'd like to keep the modifications.
Next time the wpt tests will be resynced, chances are high that the changes will be removed if not upstreamed. Please make sure that you upstream your changes to W3C repo. Patch at
bug 169462
might help you create the PR.
youenn fablet
Comment 50
2017-04-30 20:11:58 PDT
I am not sure this patch requires a flag. I agree that a runtime/experimental-feature flag for SRI in the context of mixed-content checks makes sense.
Maciej Stachowiak
Comment 51
2017-05-01 19:15:20 PDT
(In reply to Sam Weinig from
comment #46
) >
> Fair point, though it hasn't landed yet. I can add a runtime flag. I didn't > because the spec is quite mature and Chrome and Firefox (not sure about > Edge) have had implementations for a while. But it is straightforward to do > if desirable.
Apologies for the misreading. Feature flags are useful not just to hedge against spec changes but also to let downstream vendors control when they ship new features (as they may want to ensure sufficient bake time, testing, etc). This particular feature has security implications as well, so it may need extra scrutiny. That's basically my interest in having a flag here. ---- As far as the Official Rules(tm): My read of WebKit's Feature Policy is that we should typically add a runtime flag even for a feature with a stable spec, and only remove it once all ports are shipping it:
https://webkit.org/feature-policy/
I am not sure we follow this very consistently; I only commented in this bug because I was Cc'd from long ago and bug mail piqued my interest. But it's probably not good to make ad-hoc exceptions. I do think per the policy it would be appropriate to have the flag be on by default in trunk.
Sam Weinig
Comment 52
2017-05-01 19:42:15 PDT
(In reply to Maciej Stachowiak from
comment #51
)
> (In reply to Sam Weinig from
comment #46
) > > > > Fair point, though it hasn't landed yet. I can add a runtime flag. I didn't > > because the spec is quite mature and Chrome and Firefox (not sure about > > Edge) have had implementations for a while. But it is straightforward to do > > if desirable. > > Apologies for the misreading. > > Feature flags are useful not just to hedge against spec changes but also to > let downstream vendors control when they ship new features (as they may want > to ensure sufficient bake time, testing, etc). This particular feature has > security implications as well, so it may need extra scrutiny. > > That's basically my interest in having a flag here. > > ---- > > As far as the Official Rules(tm): > > My read of WebKit's Feature Policy is that we should typically add a runtime > flag even for a feature with a stable spec, and only remove it once all > ports are shipping it:
https://webkit.org/feature-policy/
> > I am not sure we follow this very consistently; I only commented in this bug > because I was Cc'd from long ago and bug mail piqued my interest. But it's > probably not good to make ad-hoc exceptions. > > I do think per the policy it would be appropriate to have the flag be on by > default in trunk.
I am afraid we are quite inconsistent in the application of this, but it is also made more challenging in that we never really defined what a "feature" constitutes. What level of change to the HTML living standard constitutes a "feature"? But I think we can all agree that SRI, being in its whole own spec is probably in the realm of what we were thinking a feature was. There is one slight complication that I am not sure how to settle. SRI defines the infrastructure for resource integrity, and amends the HTML <link> and <script> elements to have support a new integrity attribute. In addition, however, the Fetch spec, which we support, and I believe has been shipped by all parties, though I may be wrong, is incomplete in its implementation as it depends on the SRI spec for its integrity property (see
https://fetch.spec.whatwg.org/#dom-request-integrity
). This patch adds that support. I'm honestly not sure what to do in a case like this. Should that part be behind both the Fetch runtime flag and the SRI flag? Anyway, if you have opinions on what would be the right set of flags, I'd love the input.
youenn fablet
Comment 53
2017-05-01 20:27:50 PDT
It seems to me that we could ship fetch API/fetch algorithm SRI bits without too much concerns, contrary to the other SRI bits (HTML integration, security checks). That is where I would draw the lines for a flag.
Maciej Stachowiak
Comment 54
2017-05-01 21:31:58 PDT
(In reply to Sam Weinig from
comment #52
)
> (In reply to Maciej Stachowiak from
comment #51
) > > (In reply to Sam Weinig from
comment #46
) > > > > > > Fair point, though it hasn't landed yet. I can add a runtime flag. I didn't > > > because the spec is quite mature and Chrome and Firefox (not sure about > > > Edge) have had implementations for a while. But it is straightforward to do > > > if desirable. > > > > Apologies for the misreading. > > > > Feature flags are useful not just to hedge against spec changes but also to > > let downstream vendors control when they ship new features (as they may want > > to ensure sufficient bake time, testing, etc). This particular feature has > > security implications as well, so it may need extra scrutiny. > > > > That's basically my interest in having a flag here. > > > > ---- > > > > As far as the Official Rules(tm): > > > > My read of WebKit's Feature Policy is that we should typically add a runtime > > flag even for a feature with a stable spec, and only remove it once all > > ports are shipping it:
https://webkit.org/feature-policy/
> > > > I am not sure we follow this very consistently; I only commented in this bug > > because I was Cc'd from long ago and bug mail piqued my interest. But it's > > probably not good to make ad-hoc exceptions. > > > > I do think per the policy it would be appropriate to have the flag be on by > > default in trunk. > > I am afraid we are quite inconsistent in the application of this, but it is > also made more challenging in that we never really defined what a "feature" > constitutes. What level of change to the HTML living standard constitutes a > "feature"? But I think we can all agree that SRI, being in its whole own > spec is probably in the realm of what we were thinking a feature was.
I agree there is ambiguity at the boundaries, and it might be worthwhile to spell it out more. I also agree that anything that is its whole own spec is well on the feature side of the boundary.
> There is one slight complication that I am not sure how to settle. SRI > defines the infrastructure for resource integrity, and amends the HTML > <link> and <script> elements to have support a new integrity attribute. > > In addition, however, the Fetch spec, which we support, and I believe has > been shipped by all parties, though I may be wrong, is incomplete in its > implementation as it depends on the SRI spec for its integrity property (see >
https://fetch.spec.whatwg.org/#dom-request-integrity
). This patch adds that > support. > > I'm honestly not sure what to do in a case like this. Should that part be > behind both the Fetch runtime flag and the SRI flag? > > Anyway, if you have opinions on what would be the right set of flags, I'd > love the input.
Putting Fetch+SRI under the Fetch flag only would be reasonable, as would having it controlled by both Fetch and SRI flags (so it's only there when both flags are on). I agree the intersection here makes it tricky. There may be practical considerations that lean one way or the other. Stuff like: - Is Fetch-using content likely to assume Request.integrity and break if we don't have it? - Are there sites that would find it useful to have SRI support in Fetch but not elsewhere? - Would inconsistent SRI presence mess up anyone's feature testing approach? - Do we need the change to the mixed content rule for Fetch-only SRI? I don't know which of these is true and probably none is a big deal. If I tried to imagine a website that might care, I'd think of a site that uses HTTPS for non-SRI browsers but HTTP+SRI for SRI browsers. It wants to avoid triggering the mixed content warning, but given that, it wants to go for the more efficient transfer. If there was such a site, it may be important for SRI to be fully present or fully absent so it doesn't accidentally make a wrong choice. (Setting all this aside SRI is a cool feature and it will be sweet to have it in WebKit.)
youenn fablet
Comment 55
2017-05-02 10:46:16 PDT
I don't think there is a way to sniff fetch API + integrity except than actually using it and checking the fetch result. As long as we do not support service workers, fetch API + integrity will probably have low usage. Just supporting this will probably not break web sites. Supporting SRI for HTML without mixed content updates might be more disturbing.
Sam Weinig
Comment 56
2017-05-02 18:27:35 PDT
Youenn, While I finish up some blocking things, is this kindof what you were thinking for the filtering: void DocumentThreadableLoader::didFinishLoading(unsigned long identifier) { ASSERT(m_client); + + if (m_delayCallbacksForIntegrityCheck) { + if (!matchIntegrityMetadata(*m_resource, m_options.integrity)) { + reportIntegrityMetadataError(m_resource->url()); + return; + } + + auto response = m_resource->response(); + + if (options().filteringPolicy == ResponseFilteringPolicy::Disable) { + m_client->didReceiveResponse(identifier, response); + m_client->didReceiveData(m_resource->resourceBuffer()->data(), m_resource->resourceBuffer()->size()); + } else { + if (response.type() == ResourceResponse::Type::Default) { + auto tainting = m_resource->responseTainting(); + m_client->didReceiveResponse(identifier, ResourceResponse::filterResponse(response, tainting)); + if (tainting == ResourceResponse::Tainting::Opaque) { + clearResource(); + if (m_client) + m_client->didFinishLoading(identifier); + return; + } + + m_client->didReceiveData(m_resource->resourceBuffer()->data(), m_resource->resourceBuffer()->size()); + } else { + ASSERT(response.type() == ResourceResponse::Type::Opaqueredirect); + m_client->didReceiveResponse(identifier, response); + } + } + } + m_client->didFinishLoading(identifier); }
Sam Weinig
Comment 57
2017-05-02 18:28:52 PDT
(In reply to Sam Weinig from
comment #56
)
> Youenn, > > While I finish up some blocking things, is this kindof what you were > thinking for the filtering: > > void DocumentThreadableLoader::didFinishLoading(unsigned long identifier) > { > ASSERT(m_client); > + > + if (m_delayCallbacksForIntegrityCheck) { > + if (!matchIntegrityMetadata(*m_resource, m_options.integrity)) { > + reportIntegrityMetadataError(m_resource->url()); > + return; > + } > + > + auto response = m_resource->response(); > + > + if (options().filteringPolicy == ResponseFilteringPolicy::Disable) { > + m_client->didReceiveResponse(identifier, response); > + m_client->didReceiveData(m_resource->resourceBuffer()->data(), > m_resource->resourceBuffer()->size()); > + } else { > + if (response.type() == ResourceResponse::Type::Default) { > + auto tainting = m_resource->responseTainting(); > + m_client->didReceiveResponse(identifier, > ResourceResponse::filterResponse(response, tainting)); > + if (tainting == ResourceResponse::Tainting::Opaque) { > + clearResource(); > + if (m_client) > + m_client->didFinishLoading(identifier); > + return; > + } > + > + > m_client->didReceiveData(m_resource->resourceBuffer()->data(), > m_resource->resourceBuffer()->size()); > + } else { > + ASSERT(response.type() == > ResourceResponse::Type::Opaqueredirect); > + m_client->didReceiveResponse(identifier, response); > + } > + } > + } > + > m_client->didFinishLoading(identifier); > }
One thing I'm not sure of is if the ResourceResponse::Type::Opaqueredirect case should call m_client->didReceiveData(...). I don't think it should, but not 100% sure what all these things are.
youenn fablet
Comment 58
2017-05-02 20:09:45 PDT
> > void DocumentThreadableLoader::didFinishLoading(unsigned long identifier) > > { > > ASSERT(m_client); > > + > > + if (m_delayCallbacksForIntegrityCheck) { > > + if (!matchIntegrityMetadata(*m_resource, m_options.integrity)) { > > + reportIntegrityMetadataError(m_resource->url()); > > + return; > > + } > > + > > + auto response = m_resource->response(); > > + > > + if (options().filteringPolicy == ResponseFilteringPolicy::Disable) { > > + m_client->didReceiveResponse(identifier, response); > > + m_client->didReceiveData(m_resource->resourceBuffer()->data(), > > m_resource->resourceBuffer()->size()); > > + } else { > > + if (response.type() == ResourceResponse::Type::Default) { > > + auto tainting = m_resource->responseTainting(); > > + m_client->didReceiveResponse(identifier, > > ResourceResponse::filterResponse(response, tainting)); > > + if (tainting == ResourceResponse::Tainting::Opaque) { > > + clearResource(); > > + if (m_client) > > + m_client->didFinishLoading(identifier); > > + return; > > + } > > + > > + > > m_client->didReceiveData(m_resource->resourceBuffer()->data(), > > m_resource->resourceBuffer()->size()); > > + } else { > > + ASSERT(response.type() == > > ResourceResponse::Type::Opaqueredirect); > > + m_client->didReceiveResponse(identifier, response); > > + } > > + } > > + } > > + > > m_client->didFinishLoading(identifier); > > } > > One thing I'm not sure of is if the ResourceResponse::Type::Opaqueredirect > case should call m_client->didReceiveData(...). I don't think it should, but > not 100% sure what all these things are.
This looks almost good to me. If type is opaque redirect, integrity check should fail as per the spec. This could be done directly when getting the response, no need to wait for didFinishLoading. I would think that ResponseFilteringPolicy::Disable is exclusive from integrity as well. So maybe we could ASSERT(options().filteringPolicy != ResponseFilteringPolicy::Disable) here and not special case. We should also check CORS first, then integrity. Either add a FIXME+do a follow-up patch or do it now in DTL::didFinishLoading and I'll clean the code as a follow-up. In terms of tests, it would be cool to add the following tests: - integrity + cors + filtering of headers - integrity + cors + response not passing cors checks and not passing integrity checks. - integrity + redirect mode="manual" + redirection happening: integrity check should fail.
youenn fablet
Comment 59
2017-05-02 20:23:20 PDT
I was wrong about the opaque case, this cannot happen so we probably only need to assert it in DTL::didFinishLoading. And cors checks will be done before integrity anyway so no need to worry about the order. Anyway, I can do the clean-up in a follow-up patch once this lands.
Sam Weinig
Comment 60
2017-05-06 19:54:05 PDT
Created
attachment 309315
[details]
Patch (just <link> and <script>)
Sam Weinig
Comment 61
2017-05-06 19:56:36 PDT
(In reply to Sam Weinig from
comment #60
)
> Created
attachment 309315
[details]
> Patch (just <link> and <script>)
I decided to split things up a bit, and just get support for the basic algorithm, plus <link> and <script>. And, since I fear the tests getting reset on the next merge of the imported web-platform-tests, I copied and modified the tests and placed them in the http directory. Next, I will add Fetch support, but this can land on its own.
Daniel Bates
Comment 62
2017-05-07 02:54:34 PDT
Comment on
attachment 309315
[details]
Patch (just <link> and <script>) r=me
WebKit Commit Bot
Comment 63
2017-05-07 03:24:52 PDT
Comment on
attachment 309315
[details]
Patch (just <link> and <script>) Clearing flags on attachment: 309315 Committed
r216347
: <
http://trac.webkit.org/changeset/216347
>
WebKit Commit Bot
Comment 64
2017-05-07 03:24:54 PDT
All reviewed patches have been landed. Closing bug.
Sam Weinig
Comment 65
2017-05-07 12:36:45 PDT
Re-opening to complete fetch aspects.
Sam Weinig
Comment 66
2017-05-07 12:39:59 PDT
For those who have mentioned mixed-content in relation to SRI, I can't find any concrete spec support for changing mixed-content blocking or reporting based on integrity metadata (
https://w3c.github.io/webappsec-mixed-content/
). Also, a basic test in Firefox seems to confirm that adding integrity metadata to a <script> element referencing an http script, from an https page, still causes the script to be blocked. Am I missing something?
youenn fablet
Comment 67
2017-05-07 13:24:47 PDT
(In reply to Sam Weinig from
comment #66
)
> For those who have mentioned mixed-content in relation to SRI, I can't find > any concrete spec support for changing mixed-content blocking or reporting > based on integrity metadata > (
https://w3c.github.io/webappsec-mixed-content/
). Also, a basic test in > Firefox seems to confirm that adding integrity metadata to a <script> > element referencing an http script, from an https page, still causes the > script to be blocked. > > Am I missing something?
Aren't there CSP checks specific to SRI which are only meaningful in case of integrity metadata retrieved through HTTPS ?
Sam Weinig
Comment 68
2017-05-09 09:44:17 PDT
Created
attachment 309508
[details]
Patch
Sam Weinig
Comment 69
2017-05-09 09:45:47 PDT
(In reply to Sam Weinig from
comment #68
)
> Created
attachment 309508
[details]
> Patch
This implements the fetch part. It adds additional tests for more cors combinations, I am not sure I hit all the cases desired, but we can add more going forward. I also did not add a test for the options isolated copying, as that would have bloated the patch a bit; I will do it in a follow up.
Sam Weinig
Comment 70
2017-05-09 09:48:04 PDT
(In reply to youenn fablet from
comment #67
)
> (In reply to Sam Weinig from
comment #66
) > > For those who have mentioned mixed-content in relation to SRI, I can't find > > any concrete spec support for changing mixed-content blocking or reporting > > based on integrity metadata > > (
https://w3c.github.io/webappsec-mixed-content/
). Also, a basic test in > > Firefox seems to confirm that adding integrity metadata to a <script> > > element referencing an http script, from an https page, still causes the > > script to be blocked. > > > > Am I missing something? > > Aren't there CSP checks specific to SRI which are only meaningful in case of > integrity metadata retrieved through HTTPS > ?
I don't see anything in
https://w3c.github.io/webappsec-subresource-integrity/#request-verification-algorithms
that indicates it is https only, though I may not be pulling the thread far enough.
Sam Weinig
Comment 71
2017-05-09 09:48:33 PDT
(In reply to Sam Weinig from
comment #70
)
> (In reply to youenn fablet from
comment #67
) > > (In reply to Sam Weinig from
comment #66
) > > > For those who have mentioned mixed-content in relation to SRI, I can't find > > > any concrete spec support for changing mixed-content blocking or reporting > > > based on integrity metadata > > > (
https://w3c.github.io/webappsec-mixed-content/
). Also, a basic test in > > > Firefox seems to confirm that adding integrity metadata to a <script> > > > element referencing an http script, from an https page, still causes the > > > script to be blocked. > > > > > > Am I missing something? > > > > Aren't there CSP checks specific to SRI which are only meaningful in case of > > integrity metadata retrieved through HTTPS > > ? > > I don't see anything in >
https://w3c.github.io/webappsec-subresource-integrity/#request-verification
- > algorithms that indicates it is https only, though I may not be pulling the > thread far enough.
Also, that is in the never version of the spec, which we should look at independently of this.
youenn fablet
Comment 72
2017-05-09 09:56:30 PDT
Comment on
attachment 309508
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=309508&action=review
> Source/WebCore/loader/DocumentThreadableLoader.cpp:381 > + if (options().filteringPolicy == ResponseFilteringPolicy::Disable) {
I am not sure we need this case now but it seems fine to have it. We should really make the default filteringPolicy Enable and not Disable.
> Source/WebCore/loader/DocumentThreadableLoader.cpp:388 > + if (tainting == ResourceResponse::Tainting::Opaque) {
Can this case actually happen? If we have integrity, we should be using cors or same-origin and never end up with Tainting::Opaque. Can we place an ASSERT here at least?
> Source/WebCore/loader/DocumentThreadableLoader.cpp:398 > + m_client->didReceiveResponse(identifier, response);
Probably not needed here as well either. ASSERT(response.type() == ResourceResponse::Type::Default) should be good enough.
> LayoutTests/ChangeLog:10 > + Platform Tests. Additional tests for more CORS combinations have been added.
Can you file a bug about removing these tests and updating WPT ones? Maybe adding a FIXME in the tests as well?
youenn fablet
Comment 73
2017-05-09 09:59:03 PDT
Spec might not be mandating using HTTPS to convey integrity metadata, but sending this metadata through HTTP would probably defeat its purpose anyway.
Sam Weinig
Comment 74
2017-05-09 12:59:50 PDT
(In reply to youenn fablet from
comment #73
)
> Spec might not be mandating using HTTPS to convey integrity metadata, but > sending this metadata through HTTP would probably defeat its purpose anyway.
Indeed. The debate I thought you and Maciej were referring to was as to whether an https page that contained <script> or <link> tags with http sources but integrity metadata might not get blocked. I honestly don't remember where that debate was held, but it seems the outcome was that integrity does not influence mixed content blocking, which is fine.
Sam Weinig
Comment 75
2017-05-09 15:53:15 PDT
Committed
r216553
: <
http://trac.webkit.org/changeset/216553
>
Maciej Stachowiak
Comment 76
2017-05-10 17:39:01 PDT
(In reply to Sam Weinig from
comment #74
)
> (In reply to youenn fablet from
comment #73
) > > Spec might not be mandating using HTTPS to convey integrity metadata, but > > sending this metadata through HTTP would probably defeat its purpose anyway. > > Indeed. The debate I thought you and Maciej were referring to was as to > whether an https page that contained <script> or <link> tags with http > sources but integrity metadata might not get blocked. I honestly don't > remember where that debate was held, but it seems the outcome was that > integrity does not influence mixed content blocking, which is fine.
That does seem fine. (Though it makes me wonder what SRI is for - I guess checking integrity of scripts served from a third-party HTTPS server?)
John Wilander
Comment 77
2017-05-10 17:46:28 PDT
(In reply to Maciej Stachowiak from
comment #76
)
> (In reply to Sam Weinig from
comment #74
) > > (In reply to youenn fablet from
comment #73
) > > > Spec might not be mandating using HTTPS to convey integrity metadata, but > > > sending this metadata through HTTP would probably defeat its purpose anyway. > > > > Indeed. The debate I thought you and Maciej were referring to was as to > > whether an https page that contained <script> or <link> tags with http > > sources but integrity metadata might not get blocked. I honestly don't > > remember where that debate was held, but it seems the outcome was that > > integrity does not influence mixed content blocking, which is fine. > > That does seem fine. (Though it makes me wonder what SRI is for - I guess > checking integrity of scripts served from a third-party HTTPS server?)
Yes, so that sites can be sure their CDNs have not been compromised and that the script they load is the script they have tested with.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug