Implement Subresource Integrity (SRI)

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[sideshowbarker]

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

[sideshowbarker]

Subresource Integrity is now a final W3C Recommendation https://www.w3.org/TR/SRI/

[Barrett Harber]

+1 for this

[Dan Fabulich]

I filed a Radar issue for this; it was marked as a duplicate of rdar://18945879

[Dan Fabulich]

Oops, I mean rdar://problem/18945879

[Sam Weinig]

Created attachment 303875 [details]
Patch

[Sam Weinig]

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 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]

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 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]

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 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]

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]

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]

(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]

(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]

(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]

> 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]

Created attachment 304219 [details]
Patch

[Sam Weinig]

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]

(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]

(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]

(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]

(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]

> 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]

Created attachment 308558 [details]
Patch

[Sam Weinig]

(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 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 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]

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 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]

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 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]

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 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]

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]

(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]

Created attachment 308676 [details]
Patch

[youenn fablet]

> 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]

(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 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 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]

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]

(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]

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]

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]

(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]

(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]

(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]

> > 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]

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]

(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]

(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]

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]

(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]

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]

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]

(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]

> >  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]

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]

Created attachment 309315 [details]
Patch (just <link> and <script>)

[Sam Weinig]

(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 on attachment 309315 [details]
Patch (just <link> and <script>)

r=me

[WebKit Commit Bot]

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]

All reviewed patches have been landed.  Closing bug.

[Sam Weinig]

Re-opening to complete fetch aspects.

[Sam Weinig]

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]

(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]

Created attachment 309508 [details]
Patch

[Sam Weinig]

(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]

(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]

(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 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]

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]

(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]

Committed r216553: <http://trac.webkit.org/changeset/216553>

[Maciej Stachowiak]

(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]

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