SRI fail open on ineligible resources.

SRI fail open on ineligible resources.

Previously, SRI failed closed if a resource was ineligible (i.e. if it's
a cross-origin request and was not a CORS request). However, for
forwards compatibility, the spec now states that ineligible resources
should fail open, with a developer console warning
(https://github.com/w3c/webappsec/pull/394).

This is okay from a security perspective because if the reverse case
happens (a CORS request is made, but the server responds without or with
unusable CORS headers), SRI still fails closed because Fetch() will not
let it reach the integrity check. This is important because an attacker
could modify or drop the CORS headers on the server if they have
control, which is the attack vector SRI is protecting against.

BUG=355467
R=mkwst@chromium.org

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

[Mike West, 2015-06-07 14:09:15]

LGTM, though I find the decision a bit strange.

https://codereview.chromium.org/1166003004/diff/1/LayoutTests/http/tests/secu...
File
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-invalid-integrity-expected.txt
(right):

https://codereview.chromium.org/1166003004/diff/1/LayoutTests/http/tests/secu...
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-invalid-integrity-expected.txt:1:
CONSOLE ERROR: Error parsing 'integrity' attribute
('sha256;B0_62fJSJFrdjEFR9ba04m_D-LHQ-zG6PGcaR0Trpxg='). The specified hash
algorithm must be one of 'sha256', 'sha384', or 'sha512'.
Nit: For `testharness.js` tests, drop the `-expected.txt` file.

[jww, 2015-06-07 17:20:49]

Hi Mike. Can you clarify why you find the decision strange? I'll try to clarify
our goal here, but I'd love to understand your concerns.

From my perspective, the only reason to ever block is if there's reason to think
an attack is possible. In our threat model, we're worried about modifications on
the server to the external resource being loaded. In this case, we're not
talking about that; we're talking about a modification to the request being
made. Thus, from an abstract perspective, this is consistent with how we treat
other client errors that, in our threat model, can't be an attack, such as
having a bad or no longer valid hash algorithm.

This does have practical effect as well, though. For example, if we block, we
can never go back on our choice. So if we decide in some future that we don't
need a CORS request (I, for one, don't consider this a likely outcome, but
that's besides the point), we'd be sad that we required a crossorigin attribute
to start with, because old user agents would block any resources that lacked
one. Thus, by failing open with a console error, we remain backwards compatible.

My only concern about this is that it might be harder for developers to detect.
If that turns out to be a real issue, we can always *start* blocking if the
request lacks CORS in the future. Or we can even add a JavaScript error event
later to handle this case. But, again, this is also consistent with how we've
treated other things, like invalid hash algorithms.

Additionally, I responded to your inline comment below. Let me know if you
disagree with my thought.

https://codereview.chromium.org/1166003004/diff/1/LayoutTests/http/tests/secu...
File
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-invalid-integrity-expected.txt
(right):

https://codereview.chromium.org/1166003004/diff/1/LayoutTests/http/tests/secu...
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-invalid-integrity-expected.txt:1:
CONSOLE ERROR: Error parsing 'integrity' attribute
('sha256;B0_62fJSJFrdjEFR9ba04m_D-LHQ-zG6PGcaR0Trpxg='). The specified hash
algorithm must be one of 'sha256', 'sha384', or 'sha512'.
On 2015/06/07 14:09:14, Mike West wrote:
> Nit: For `testharness.js` tests, drop the `-expected.txt` file.

Part of the point of this test is that the appropriate console messages are
being logged, so I believe it's pretty important to leave this one in. Let me
know if you disagree.

[Mike West, 2015-06-08 08:30:53]

On 2015/06/07 at 17:20:49, jww wrote:
> From my perspective, the only reason to ever block is if there's reason to
think an attack is possible. In our threat model, we're worried about
modifications on the server to the external resource being loaded. In this case,
we're not talking about that; we're talking about a modification to the request
being made. Thus, from an abstract perspective, this is consistent with how we
treat other client errors that, in our threat model, can't be an attack, such as
having a bad or no longer valid hash algorithm.

I'd suggest that the other reason to block would be to force developers to
recognize that they're not doing what they think they're doing. That is, I add
an `integrity` attribute to my resource, and reload the page. It loads! "Boy, I
must be protected from evil CDN operators, hooray!", I think.

Hard-failing in this scenario has the dual effect of 1) actually providing
integrity protection, and 2) educating developers about the CORS requirement.

Soft-failing basically means that we'll end up with pages on the internet that
think they're integrity-checked, but actually aren't. That doesn't seem like a
good outcome.

> This does have practical effect as well, though. For example, if we block, we
can never go back on our choice. So if we decide in some future that we don't
need a CORS request (I, for one, don't consider this a likely outcome, but
that's besides the point), we'd be sad that we required a crossorigin attribute
to start with, because old user agents would block any resources that lacked
one. Thus, by failing open with a console error, we remain backwards compatible.

This is why I suggested forcing `crossorigin=anonymous` behavior if
`crossorigin` wasn't present. :) 

> My only concern about this is that it might be harder for developers to
detect. If that turns out to be a real issue, we can always *start* blocking if
the request lacks CORS in the future. Or we can even add a JavaScript error
event later to handle this case. But, again, this is also consistent with how
we've treated other things

This is my concern. *shrug* As I said, I'm not going to second-guess the four of
you, but this decision surprised me.

> like invalid hash algorithms.

We accept these, I think, because we know we'll be adding new algorithms in the
future. That's an integral part of the design of the feature. I think that's a
bit different than this case.

In any event, if this is what y'all have agreed on, that's fine. It's your
feature, and I'll hope Dev is sufficiently representative of developers that
you're properly evaluating both their desires and needs. :)

> Additionally, I responded to your inline comment below. Let me know if you
disagree with my thought.
> 
>
https://codereview.chromium.org/1166003004/diff/1/LayoutTests/http/tests/secu...
> File
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-invalid-integrity-expected.txt
(right):
> 
>
https://codereview.chromium.org/1166003004/diff/1/LayoutTests/http/tests/secu...
>
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-invalid-integrity-expected.txt:1:
CONSOLE ERROR: Error parsing 'integrity' attribute
('sha256;B0_62fJSJFrdjEFR9ba04m_D-LHQ-zG6PGcaR0Trpxg='). The specified hash
algorithm must be one of 'sha256', 'sha384', or 'sha512'.
> On 2015/06/07 14:09:14, Mike West wrote:
> > Nit: For `testharness.js` tests, drop the `-expected.txt` file.
> 
> Part of the point of this test is that the appropriate console messages are
being logged, so I believe it's pretty important to leave this one in. Let me
know if you disagree.

I disagree insofar as a) it's not going to be possible to run the same test in
Firefox and Chrome and get the same textual output, and b) submit exactly the
tests we're running to the `web-platform-test` repository. If we care about
Chrome's console messages, we can have a specific test for that (it can even be
a testharness test, who cares?).

P.S. I would like to encourage you to take our layout tests, and put together a
pull request against https://github.com/w3c/web-platform-tests. Also, encourage
Mozilla to do the same. :)

[jww, 2015-06-08 19:09:19]

On 2015/06/08 08:30:53, Mike West wrote:
> On 2015/06/07 at 17:20:49, jww wrote:
> > From my perspective, the only reason to ever block is if there's reason to
> think an attack is possible. In our threat model, we're worried about
> modifications on the server to the external resource being loaded. In this
case,
> we're not talking about that; we're talking about a modification to the
request
> being made. Thus, from an abstract perspective, this is consistent with how we
> treat other client errors that, in our threat model, can't be an attack, such
as
> having a bad or no longer valid hash algorithm.
> 
> I'd suggest that the other reason to block would be to force developers to
> recognize that they're not doing what they think they're doing. That is, I add
> an `integrity` attribute to my resource, and reload the page. It loads! "Boy,
I
> must be protected from evil CDN operators, hooray!", I think.
> 
> Hard-failing in this scenario has the dual effect of 1) actually providing
> integrity protection, and 2) educating developers about the CORS requirement.
> 
> Soft-failing basically means that we'll end up with pages on the internet that
> think they're integrity-checked, but actually aren't. That doesn't seem like a
> good outcome.
> 
> > This does have practical effect as well, though. For example, if we block,
we
> can never go back on our choice. So if we decide in some future that we don't
> need a CORS request (I, for one, don't consider this a likely outcome, but
> that's besides the point), we'd be sad that we required a crossorigin
attribute
> to start with, because old user agents would block any resources that lacked
> one. Thus, by failing open with a console error, we remain backwards
compatible.
> 
> This is why I suggested forcing `crossorigin=anonymous` behavior if
> `crossorigin` wasn't present. :) 
> 
> > My only concern about this is that it might be harder for developers to
> detect. If that turns out to be a real issue, we can always *start* blocking
if
> the request lacks CORS in the future. Or we can even add a JavaScript error
> event later to handle this case. But, again, this is also consistent with how
> we've treated other things
> 
> This is my concern. *shrug* As I said, I'm not going to second-guess the four
of
> you, but this decision surprised me.
> 
> > like invalid hash algorithms.
> 
> We accept these, I think, because we know we'll be adding new algorithms in
the
> future. That's an integral part of the design of the feature. I think that's a
> bit different than this case.
> 
> In any event, if this is what y'all have agreed on, that's fine. It's your
> feature, and I'll hope Dev is sufficiently representative of developers that
> you're properly evaluating both their desires and needs. :)
> 
> > Additionally, I responded to your inline comment below. Let me know if you
> disagree with my thought.
> > 
> >
>
https://codereview.chromium.org/1166003004/diff/1/LayoutTests/http/tests/secu...
> > File
>
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-invalid-integrity-expected.txt
> (right):
> > 
> >
>
https://codereview.chromium.org/1166003004/diff/1/LayoutTests/http/tests/secu...
> >
>
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-invalid-integrity-expected.txt:1:
> CONSOLE ERROR: Error parsing 'integrity' attribute
> ('sha256;B0_62fJSJFrdjEFR9ba04m_D-LHQ-zG6PGcaR0Trpxg='). The specified hash
> algorithm must be one of 'sha256', 'sha384', or 'sha512'.
> > On 2015/06/07 14:09:14, Mike West wrote:
> > > Nit: For `testharness.js` tests, drop the `-expected.txt` file.
> > 
> > Part of the point of this test is that the appropriate console messages are
> being logged, so I believe it's pretty important to leave this one in. Let me
> know if you disagree.
> 
> I disagree insofar as a) it's not going to be possible to run the same test in
> Firefox and Chrome and get the same textual output, and b) submit exactly the
> tests we're running to the `web-platform-test` repository. If we care about
> Chrome's console messages, we can have a specific test for that (it can even
be
> a testharness test, who cares?).
Sure, I'll go ahead and make a secondary test; it just seems silly to me since
it
will be nearly identical to the testharness test, and it seems like we could
make
dual use of that test.
> 
> P.S. I would like to encourage you to take our layout tests, and put together
a
> pull request against https://github.com/w3c/web-platform-tests. Also,
encourage
> Mozilla to do the same. :)

[jww, 2015-06-08 22:05:26]

The CQ bit was checked by jww@chromium.org

[jww, 2015-06-08 22:05:26]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1166003004/#ps20001
(title: "Remove a bunch of -expected.txt files")

[commit-bot: I haz the power, 2015-06-08 22:05:56]

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

[commit-bot: I haz the power, 2015-06-08 23:05:03]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196703