Fix SRI bypass by loading same resource twice in same origin.

Fix SRI bypass by loading same resource twice in same origin.

This fixes a bug where the memory cache was bypassing subresource
integrity checks when a resource is loaded for a second time in the same
origin. The resource in the memory cache was correctly storing that an
integrity check had already been done so whene it was retrieved later,
it wouldn't need to be checked again, but it didn't store the fact that
this was a *failure*, so when the load happened a second time, it
assumed it was a good integrity.

This modifies the resources to store a disposition for the integrity
check, rather than just that the integrity check occurred. On a reload
of the resource, if the integrity had failed the first time, the
resource will fail to load.

BUG=584155
Committed: https://crrev.com/bf24693238d407f90bec71453b18aae8dd1c0f43
Cr-Commit-Position: refs/heads/master@{#374336}

[jww, 2016-02-08 12:20:09]

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

[jww, 2016-02-08 23:12:17]

Description was changed from

==========
Fix SRI bypass by loading same resource twice in same origin.

This fixes a bug where the memory cache was bypassing subresource
integrity checks when a resource is loaded for a second time in the same
origin. The resource in the memory cache was correctly storing that an
integrity check had already been done so whene it was retrieved later,
it wouldn't need to be checked again, but it didn't store the fact that
this was a *failure*, so when the load happened a second time, it
assumed it was a good integrity.

This modifies the resources to store a disposition for the integrity
check, rather than just that the integrity check occurred. On a reload
of the resource, if the integrity had failed the first time, the
resource will fail to load.

BUG=584155
==========

to

==========
Fix SRI bypass by loading same resource twice in same origin.

This fixes a bug where the memory cache was bypassing subresource
integrity checks when a resource is loaded for a second time in the same
origin. The resource in the memory cache was correctly storing that an
integrity check had already been done so whene it was retrieved later,
it wouldn't need to be checked again, but it didn't store the fact that
this was a *failure*, so when the load happened a second time, it
assumed it was a good integrity.

This modifies the resources to store a disposition for the integrity
check, rather than just that the integrity check occurred. On a reload
of the resource, if the integrity had failed the first time, the
resource will fail to load.

BUG=584155
==========

[jww, 2016-02-08 23:12:17]

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

[jww, 2016-02-08 23:15:28]

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

[jww, 2016-02-08 23:15:28]

dcheng@, would you mind reviewing this for me? It appears that mkwst@ is OOO,
and I'd like to get this in ASAP to start merging. Thanks!

[dcheng, 2016-02-09 01:12:46]

https://codereview.chromium.org/1675183003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/PendingScript.cpp (right):

https://codereview.chromium.org/1675183003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/PendingScript.cpp:161:
scriptResource->setIntegrityAlreadyChecked(!m_integrityFailure);
I don't think it's obvious from the callsite that true/false maps to failed /
passed. Maybe just pass the enum value through?

(And ASSERT(m_integrityFailure != ScriptIntegrityDisposition::NotChecked) for
sanity if you want)

https://codereview.chromium.org/1675183003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/ScriptResource.h (right):

https://codereview.chromium.org/1675183003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ScriptResource.h:37: enum
ScriptIntegrityDisposition {
Nit: use enum class? And just name the enum members NotChecked/Failed/Passed.

It'll make a few things more verbose, but having things in a real namespace is
nice so we don't have a situation like this: https://crbug.com/584543

[jww, 2016-02-09 04:36:44]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2016-02-09 04:37:24]

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

[commit-bot: I haz the power, 2016-02-09 04:37:29]

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

[commit-bot: I haz the power, 2016-02-09 04:37:29]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[jww, 2016-02-09 04:38:33]

The CQ bit was unchecked by jww@chromium.org

[jww, 2016-02-09 04:38:50]

Sorry, accidental commit queue :-) Nits addressed, I think.

https://codereview.chromium.org/1675183003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/PendingScript.cpp (right):

https://codereview.chromium.org/1675183003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/PendingScript.cpp:161:
scriptResource->setIntegrityAlreadyChecked(!m_integrityFailure);
On 2016/02/09 01:12:46, dcheng wrote:
> I don't think it's obvious from the callsite that true/false maps to failed /
> passed. Maybe just pass the enum value through?
> 
> (And ASSERT(m_integrityFailure != ScriptIntegrityDisposition::NotChecked) for
> sanity if you want)

Done.

https://codereview.chromium.org/1675183003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/ScriptResource.h (right):

https://codereview.chromium.org/1675183003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ScriptResource.h:37: enum
ScriptIntegrityDisposition {
On 2016/02/09 01:12:46, dcheng wrote:
> Nit: use enum class? And just name the enum members NotChecked/Failed/Passed.
> 
> It'll make a few things more verbose, but having things in a real namespace is
> nice so we don't have a situation like this: https://crbug.com/584543

Done.

[dcheng, 2016-02-09 05:04:55]

lgtm

[jww, 2016-02-09 05:33:14]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2016-02-09 05:36:01]

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

[commit-bot: I haz the power, 2016-02-09 05:39:34]

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

[commit-bot: I haz the power, 2016-02-09 05:39:35]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jww, 2016-02-09 06:37:18]

The CQ bit was checked by jww@chromium.org

[jww, 2016-02-09 06:37:18]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1675183003/#ps40001
(title: "Rebase on ToT")

[commit-bot: I haz the power, 2016-02-09 06:37:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1675183003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1675183003/40001

[commit-bot: I haz the power, 2016-02-09 08:19:14]

Description was changed from

==========
Fix SRI bypass by loading same resource twice in same origin.

This fixes a bug where the memory cache was bypassing subresource
integrity checks when a resource is loaded for a second time in the same
origin. The resource in the memory cache was correctly storing that an
integrity check had already been done so whene it was retrieved later,
it wouldn't need to be checked again, but it didn't store the fact that
this was a *failure*, so when the load happened a second time, it
assumed it was a good integrity.

This modifies the resources to store a disposition for the integrity
check, rather than just that the integrity check occurred. On a reload
of the resource, if the integrity had failed the first time, the
resource will fail to load.

BUG=584155
==========

to

==========
Fix SRI bypass by loading same resource twice in same origin.

This fixes a bug where the memory cache was bypassing subresource
integrity checks when a resource is loaded for a second time in the same
origin. The resource in the memory cache was correctly storing that an
integrity check had already been done so whene it was retrieved later,
it wouldn't need to be checked again, but it didn't store the fact that
this was a *failure*, so when the load happened a second time, it
assumed it was a good integrity.

This modifies the resources to store a disposition for the integrity
check, rather than just that the integrity check occurred. On a reload
of the resource, if the integrity had failed the first time, the
resource will fail to load.

BUG=584155
==========

[commit-bot: I haz the power, 2016-02-09 08:19:15]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-09 08:19:59]

Description was changed from

==========
Fix SRI bypass by loading same resource twice in same origin.

This fixes a bug where the memory cache was bypassing subresource
integrity checks when a resource is loaded for a second time in the same
origin. The resource in the memory cache was correctly storing that an
integrity check had already been done so whene it was retrieved later,
it wouldn't need to be checked again, but it didn't store the fact that
this was a *failure*, so when the load happened a second time, it
assumed it was a good integrity.

This modifies the resources to store a disposition for the integrity
check, rather than just that the integrity check occurred. On a reload
of the resource, if the integrity had failed the first time, the
resource will fail to load.

BUG=584155
==========

to

==========
Fix SRI bypass by loading same resource twice in same origin.

This fixes a bug where the memory cache was bypassing subresource
integrity checks when a resource is loaded for a second time in the same
origin. The resource in the memory cache was correctly storing that an
integrity check had already been done so whene it was retrieved later,
it wouldn't need to be checked again, but it didn't store the fact that
this was a *failure*, so when the load happened a second time, it
assumed it was a good integrity.

This modifies the resources to store a disposition for the integrity
check, rather than just that the integrity check occurred. On a reload
of the resource, if the integrity had failed the first time, the
resource will fail to load.

BUG=584155

Committed: https://crrev.com/bf24693238d407f90bec71453b18aae8dd1c0f43
Cr-Commit-Position: refs/heads/master@{#374336}
==========

[commit-bot: I haz the power, 2016-02-09 08:20:00]

Patchset 3 (id:??) landed as
https://crrev.com/bf24693238d407f90bec71453b18aae8dd1c0f43
Cr-Commit-Position: refs/heads/master@{#374336}