Implementation of subresource integrity attribute for secure origins.

Implementation of subresource integrity attribute for secure origins.

This is an implementation of subresource integrity only for script tags
and secure origins. This uses the previously added integrity attribute
to calculate a digest for subresources and allow access to the resource
only if the digest matches the specified integrity value.

See http://www.w3.org/TR/SRI/ for the W3C standard proposal.

Intent to implement discussion:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/hTDUpMk_TV8

BUG=355467
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182523

[jww, 2014-09-13 01:16:09]

jww@chromium.org changed reviewers:
+ mkwst@chromium.org

[jww, 2014-09-13 01:17:31]

mkwst@, here's a start on subresource integrity. Lots left to do, but it does
work for the tests specified. Obviously, not ready for a full review, but I'd
love any thoughts or comments you have on it so far. For example, currently I'm
implementing it totally independent of CSP, but if you disagree, let me know (I
have a variety of thoughts on why this is better that I'm happy to share, if you
care).

[Mike West, 2014-09-13 03:44:31]

The approach looks totally reasonable, thanks for kicking this off! Comments
inline.

https://codereview.chromium.org/566083003/diff/20001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-blocked.html
(right):

https://codereview.chromium.org/566083003/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-blocked.html:11:
<script src='fail.js'
integrity="pgUiYTMl3j+Nyfyc6EJkFsn//acTBz/ZqOtWnzTuSC4="></script>
I'm sure you realize that this isn't the `ni:///` syntax from the spec. :)

I'm fine with landing this as a first pass, but we'll need a mechanism for
specifying algorithms going forward (and will need to support multiple URLs for
forward agility, etc).

https://codereview.chromium.org/566083003/diff/20001/Source/core/dom/ScriptLo...
File Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/566083003/diff/20001/Source/core/dom/ScriptLo...
Source/core/dom/ScriptLoader.cpp:327: if
(resourceSecurityOrigin->canAccessFeatureRequiringSecureOrigin(insecureOriginMsg)
&&
m_element->document().securityOrigin()->canAccessFeatureRequiringSecureOrigin(insecureOriginMsg)
&& m_element->fastHasAttribute(HTMLNames::integrityAttr) &&
!SubresourceIntegrity::CheckSubresourceIntegrity(sourceCode.source(),
m_element->fastGetAttribute(HTMLNames::integrityAttr)))
0. This needs to be gated on the runtime flag that I think I introduced way back
when I added the attribute definition.

1. Since we're going to have to do something vaguely like this for all the
elements that support integrity attributes, I think it would be good to hide
this complexity inside the CheckSubresourceIntegrity check. You could, for
example, pass in a reference to the element and do all the checks internally.

2. I don't think the second 'canAccessFeatureRequiringSecureOrigin' is correct
(also, we should rename that method to match the MIX 'authenticated origin'
terminology). You're totally right to verify that the origin in which the
element exists is authenticated, but I think you only need to check
'SecurityOrigin::isSecure' for the resource URL.

3. Please do the integrity attribute check first; there's no point in doing the
secure origin checks if there's no integrity attribute.

4. Consider splitting this up into multiple lines? :)

5. Metrics: we should measure the number of elements with integrity attributes,
and the number of passes/failures. And anything else that seems interesting.

https://codereview.chromium.org/566083003/diff/20001/Source/core/frame/Subres...
File Source/core/frame/SubresourceIntegrity.cpp (right):

https://codereview.chromium.org/566083003/diff/20001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.cpp:35: base64Decode(integrity,
hashVector);
Please add some TODOs about parsing the `ni:///` syntax to extract the algorithm
and expected hash, and about supporting multiple sets of integrity metadata, as
discussed in the agility section.

[jww, 2014-09-15 21:20:16]

mkwst@, I've addressed most of your issues, and I've also added in proper ni://
support as well as supported for the usually gang of hashing functions.

https://codereview.chromium.org/566083003/diff/20001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-blocked.html
(right):

https://codereview.chromium.org/566083003/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-blocked.html:11:
<script src='fail.js'
integrity="pgUiYTMl3j+Nyfyc6EJkFsn//acTBz/ZqOtWnzTuSC4="></script>
On 2014/09/13 03:44:31, Mike West wrote:
> I'm sure you realize that this isn't the `ni:///` syntax from the spec. :)
> 
> I'm fine with landing this as a first pass, but we'll need a mechanism for
> specifying algorithms going forward (and will need to support multiple URLs
for
> forward agility, etc).

Oh, yes, I'm definitely aware. I actually plan on landing it with the proper
syntax, but I just wanted to get a basic working version to have you make sure
the approach looks reasonable. I'll have a better version up shortly with some
of these additions.

https://codereview.chromium.org/566083003/diff/20001/Source/core/dom/ScriptLo...
File Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/566083003/diff/20001/Source/core/dom/ScriptLo...
Source/core/dom/ScriptLoader.cpp:327: if
(resourceSecurityOrigin->canAccessFeatureRequiringSecureOrigin(insecureOriginMsg)
&&
m_element->document().securityOrigin()->canAccessFeatureRequiringSecureOrigin(insecureOriginMsg)
&& m_element->fastHasAttribute(HTMLNames::integrityAttr) &&
!SubresourceIntegrity::CheckSubresourceIntegrity(sourceCode.source(),
m_element->fastGetAttribute(HTMLNames::integrityAttr)))
On 2014/09/13 03:44:31, Mike West wrote:
> 0. This needs to be gated on the runtime flag that I think I introduced way
back
> when I added the attribute definition.
Done.
> 
> 1. Since we're going to have to do something vaguely like this for all the
> elements that support integrity attributes, I think it would be good to hide
> this complexity inside the CheckSubresourceIntegrity check. You could, for
> example, pass in a reference to the element and do all the checks internally.
Good call. Done.
> 
> 2. I don't think the second 'canAccessFeatureRequiringSecureOrigin' is correct
> (also, we should rename that method to match the MIX 'authenticated origin'
> terminology). You're totally right to verify that the origin in which the
> element exists is authenticated, but I think you only need to check
> 'SecurityOrigin::isSecure' for the resource URL.
Hm, I think I see your point, but I still think we should stick with
canAccessFeatureRequiringSecureOrigin. It effectively acts as a superset of
isSecure because it also allows connections to local resources, either via the
file:// protocol or an http:// localhost URL. I spoke to palmer, and he's okay
with allowing integrity for those resources since it might be useful for
testing. Let me know if you disagree.

As for the terminology, I agree we should change it, but I think we should do it
in a separate CL, otherwise this one is going to touch a bunch of unrelated
code.
> 
> 3. Please do the integrity attribute check first; there's no point in doing
the
> secure origin checks if there's no integrity attribute.
Done.
> 
> 4. Consider splitting this up into multiple lines? :)
Done.
> 
> 5. Metrics: we should measure the number of elements with integrity
attributes,
> and the number of passes/failures. And anything else that seems interesting.
I'll start with the two basic metrics you mentioned, and we can add any more
that we come up with.

[Mike West, 2014-09-16 06:45:04]

Thanks, this is looking great! A few comments below, mostly related to testing.

https://codereview.chromium.org/566083003/diff/40001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/566083003/diff/40001/Source/core/core.gypi#ne...
Source/core/core.gypi:1214: 'frame/SubresourceIntegrity.cpp',
Please add the .h as well.

https://codereview.chromium.org/566083003/diff/40001/Source/core/dom/ScriptLo...
File Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/566083003/diff/40001/Source/core/dom/ScriptLo...
Source/core/dom/ScriptLoader.cpp:323: // TODO(jww): On failure, SRI should
probably provide an error message for the console.
nit: Blink style is '// FIXME:'. Rather than tagging your name to the comment,
consider adding a crbug link.

https://codereview.chromium.org/566083003/diff/40001/Source/core/dom/ScriptLo...
Source/core/dom/ScriptLoader.cpp:324: if
(!SubresourceIntegrity::CheckSubresourceIntegrity(*m_element,
sourceCode.source(), sourceCode.resource()->url()))
Perhaps this could be simplified by just passing in |sourceCode|?

https://codereview.chromium.org/566083003/diff/40001/Source/core/frame/Subres...
File Source/core/frame/SubresourceIntegrity.cpp (right):

https://codereview.chromium.org/566083003/diff/40001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.cpp:29: return isASCIIAlphanumeric(c) ||
c == '+' || c == '/' || c == '=';
Nit: It's probably worth sharing the base64 check with CSP (in a future CL).

https://codereview.chromium.org/566083003/diff/40001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.cpp:62: // allows them here.
I don't like that we've ended up with two levels of "secure", and I think we
should either consider renaming one or both methods, or merging them.

It's nothing you need to resolve in this CL, but I'd appreciate it if you filed
a bug about this so we can figure out whether it's a problem worth solving.

https://codereview.chromium.org/566083003/diff/40001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.cpp:66:
UseCounter::count(element.document(),
UseCounter::SRIElementWithIntegrityAttributeAndInsecureResource);
This will also trigger if the document itself is insecure. Perhaps it's worth
separating those two cases so we know what we're measuring?

https://codereview.chromium.org/566083003/diff/40001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.cpp:98: bool
SubresourceIntegrity::parseIntegrityAttribute(const String& attribute, String&
integrity, HashAlgorithm& algorithm)
This parsing algorithm looks solid, but I'd really like to see some unit tests
with weird input to have a bit more confidence that it covers all the bases.

https://codereview.chromium.org/566083003/diff/40001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.cpp:116: if
(!equalIgnoringCase(integrityPrefix.characters8(), begin,
integrityPrefix.length()))
I don't recall if 'fastGetAttribute' strips whitespace or not. I don't think it
does. If that's the case, you'll want to do something to support '  
ni://lefkjw'.

https://codereview.chromium.org/566083003/diff/40001/Source/core/frame/Subres...
File Source/core/frame/SubresourceIntegrity.h (right):

https://codereview.chromium.org/566083003/diff/40001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.h:24: static bool
parseIntegrityAttribute(const WTF::String& attribute, WTF::String& integrity,
HashAlgorithm&);
These are both very unit-testable methods. I'd suggesting adding a
'SubresourceIntegrityTest.cpp' file with some unit tests in addition to the
(nice!) layout tests you've already added. See
'Source/platform/weborigin/SecurityOriginTest.cpp' for an example of how those
tests might look.

[jww, 2014-09-16 22:34:49]

https://codereview.chromium.org/566083003/diff/40001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/566083003/diff/40001/Source/core/core.gypi#ne...
Source/core/core.gypi:1214: 'frame/SubresourceIntegrity.cpp',
On 2014/09/16 06:45:03, Mike West wrote:
> Please add the .h as well.

Done.

https://codereview.chromium.org/566083003/diff/40001/Source/core/dom/ScriptLo...
File Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/566083003/diff/40001/Source/core/dom/ScriptLo...
Source/core/dom/ScriptLoader.cpp:323: // TODO(jww): On failure, SRI should
probably provide an error message for the console.
On 2014/09/16 06:45:03, Mike West wrote:
> nit: Blink style is '// FIXME:'. Rather than tagging your name to the comment,
> consider adding a crbug link.

Done.

https://codereview.chromium.org/566083003/diff/40001/Source/core/dom/ScriptLo...
Source/core/dom/ScriptLoader.cpp:324: if
(!SubresourceIntegrity::CheckSubresourceIntegrity(*m_element,
sourceCode.source(), sourceCode.resource()->url()))
On 2014/09/16 06:45:03, Mike West wrote:
> Perhaps this could be simplified by just passing in |sourceCode|?

My thought is that we're going to do this for Style soon, and that has just a
straight String for the source, so I'd like to keep this general for that as
well.

https://codereview.chromium.org/566083003/diff/40001/Source/core/frame/Subres...
File Source/core/frame/SubresourceIntegrity.cpp (right):

https://codereview.chromium.org/566083003/diff/40001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.cpp:29: return isASCIIAlphanumeric(c) ||
c == '+' || c == '/' || c == '=';
On 2014/09/16 06:45:04, Mike West wrote:
> Nit: It's probably worth sharing the base64 check with CSP (in a future CL).

Acknowledged.

https://codereview.chromium.org/566083003/diff/40001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.cpp:62: // allows them here.
On 2014/09/16 06:45:04, Mike West wrote:
> I don't like that we've ended up with two levels of "secure", and I think we
> should either consider renaming one or both methods, or merging them.
> 
> It's nothing you need to resolve in this CL, but I'd appreciate it if you
filed
> a bug about this so we can figure out whether it's a problem worth solving.

Okay, I will do that, although there *is* a subtle difference. localhost and
file:// are not inherently authenticated. That is, there's nothing about the
host or protocol, respectively, that are authenticated, it's just that in
practice we're cool with them. So it does sort of make sense to me, but I think
it's worth a debate on a bug.

In any case, I filed https://crbug.com/414928.

https://codereview.chromium.org/566083003/diff/40001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.cpp:66:
UseCounter::count(element.document(),
UseCounter::SRIElementWithIntegrityAttributeAndInsecureResource);
On 2014/09/16 06:45:04, Mike West wrote:
> This will also trigger if the document itself is insecure. Perhaps it's worth
> separating those two cases so we know what we're measuring?

Done.

https://codereview.chromium.org/566083003/diff/40001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.cpp:98: bool
SubresourceIntegrity::parseIntegrityAttribute(const String& attribute, String&
integrity, HashAlgorithm& algorithm)
On 2014/09/16 06:45:03, Mike West wrote:
> This parsing algorithm looks solid, but I'd really like to see some unit tests
> with weird input to have a bit more confidence that it covers all the bases.

Done.

https://codereview.chromium.org/566083003/diff/40001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.cpp:116: if
(!equalIgnoringCase(integrityPrefix.characters8(), begin,
integrityPrefix.length()))
On 2014/09/16 06:45:03, Mike West wrote:
> I don't recall if 'fastGetAttribute' strips whitespace or not. I don't think
it
> does. If that's the case, you'll want to do something to support '  
> ni://lefkjw'.

Done.

https://codereview.chromium.org/566083003/diff/40001/Source/core/frame/Subres...
File Source/core/frame/SubresourceIntegrity.h (right):

https://codereview.chromium.org/566083003/diff/40001/Source/core/frame/Subres...
Source/core/frame/SubresourceIntegrity.h:24: static bool
parseIntegrityAttribute(const WTF::String& attribute, WTF::String& integrity,
HashAlgorithm&);
On 2014/09/16 06:45:04, Mike West wrote:
> These are both very unit-testable methods. I'd suggesting adding a
> 'SubresourceIntegrityTest.cpp' file with some unit tests in addition to the
> (nice!) layout tests you've already added. See
> 'Source/platform/weborigin/SecurityOriginTest.cpp' for an example of how those
> tests might look.

Done.

[Mike West, 2014-09-17 07:19:06]

LGTM, thanks for adding a test and taking another pass.

https://codereview.chromium.org/566083003/diff/60001/Source/core/frame/UseCou...
File Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/566083003/diff/60001/Source/core/frame/UseCou...
Source/core/frame/UseCounter.h:502:
SRIElementWithIntegrityAttributeAndInsecureResource = 530,
You'll need to rebase this. :)

[jww, 2014-09-23 18:52:34]

The CQ bit was checked by jww@chromium.org

[jww, 2014-09-23 20:02:22]

The CQ bit was unchecked by jww@chromium.org

[jww, 2014-09-23 20:02:28]

The CQ bit was checked by jww@chromium.org

[jww, 2014-09-23 20:02:51]

The CQ bit was unchecked by jww@chromium.org

[jww, 2014-09-23 20:03:12]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-09-23 21:02:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/566083003/80001

[commit-bot: I haz the power, 2014-09-23 21:02:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-23 21:02:52]

Failed to apply patch for Source/core/frame/UseCounter.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/frame/UseCounter.h
  Hunk #1 FAILED at 504.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/frame/UseCounter.h.rej

Patch:       Source/core/frame/UseCounter.h
Index: Source/core/frame/UseCounter.h
diff --git a/Source/core/frame/UseCounter.h b/Source/core/frame/UseCounter.h
index
60098fa56a75ecdff8691ff23609f45a333129c6..1b9cd31a7e02297961c6d33acfc80dbeff8ed040
100644
--- a/Source/core/frame/UseCounter.h
+++ b/Source/core/frame/UseCounter.h
@@ -504,7 +504,11 @@ public:
         NotificationPermissionRequested = 535,
         MediaStreamLabel = 536,
         MediaStreamStop = 537,
-
+        SRIElementWithMatchingIntegrityAttribute = 538,
+        SRIElementWithNonMatchingIntegrityAttribute = 539,
+        SRIElementWithUnparsableIntegrityAttribute = 540,
+        SRIElementWithIntegrityAttributeAndInsecureOrigin = 541,
+        SRIElementWithIntegrityAttributeAndInsecureResource = 542,
         // Add new features immediately above this line. Don't change assigned
         // numbers of any item, and don't reuse removed slots.
         // Also, run update_use_counter_feature_enum.py in
chromium/src/tools/metrics/histograms/

[jww, 2014-09-23 21:29:55]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-09-23 21:30:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/566083003/100001

[commit-bot: I haz the power, 2014-09-23 21:41:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-23 21:41:08]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_chromium_gn_comp...)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[jww, 2014-09-23 21:50:09]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-09-23 21:51:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/566083003/120001

[commit-bot: I haz the power, 2014-09-23 23:11:09]

Committed patchset #7 (id:120001) as 182523