Implement hash function prioritization for SRI.

Implement hash function prioritization for SRI.

The Subresource Integrity spec states that if multiple hash values are
present in the integrity, only those with the strongest hash functions
should be considered for the integrity check. This implements that
prioritization.

BUG=355467
R=mkwst@chromium.org

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

[Mike West, 2015-06-03 09:13:34]

Cross-time-zone LGTM, but I'd like to see some changes before landing. See
comments.

https://codereview.chromium.org/1156413005/diff/1/Source/core/frame/Subresour...
File Source/core/frame/SubresourceIntegrity.cpp (right):

https://codereview.chromium.org/1156413005/diff/1/Source/core/frame/Subresour...
Source/core/frame/SubresourceIntegrity.cpp:72: const HashAlgorithm
weakerThanSha256[] = { HashAlgorithmSha1 };
Why do we support SHA-1 at all? I don't remember it being in the spec. Can we
ASSERT that we never get to this check for that known-bad algorithm?

https://codereview.chromium.org/1156413005/diff/1/Source/core/frame/Subresour...
Source/core/frame/SubresourceIntegrity.cpp:134: for (IntegrityMetadata& metadata
: metadataList) {
Nit: `const IntegrityMetadata&`

https://codereview.chromium.org/1156413005/diff/1/Source/core/frame/Subresour...
Source/core/frame/SubresourceIntegrity.cpp:136: strongestAlgorithm =
metadata.algorithm;
If you just return the strongest algorithm, you can drop the `if` entirely.

https://codereview.chromium.org/1156413005/diff/1/Source/core/frame/Subresour...
Source/core/frame/SubresourceIntegrity.cpp:142: if (result != SameStrength)
Nit: It seems like this could be simplified to `if (result !=
strongestAlgorithm)`

https://codereview.chromium.org/1156413005/diff/1/Source/core/frame/Subresour...
File Source/core/frame/SubresourceIntegrity.h (right):

https://codereview.chromium.org/1156413005/diff/1/Source/core/frame/Subresour...
Source/core/frame/SubresourceIntegrity.h:56: static PrioritizationResult
getPrioritizedHashFunction(HashAlgorithm, HashAlgorithm);
Why not return the stronger `HashAlgorithm`? The "first" and "second" thing is
strange.

[jww, 2015-06-03 19:26:56]

https://codereview.chromium.org/1156413005/diff/1/Source/core/frame/Subresour...
File Source/core/frame/SubresourceIntegrity.cpp (right):

https://codereview.chromium.org/1156413005/diff/1/Source/core/frame/Subresour...
Source/core/frame/SubresourceIntegrity.cpp:72: const HashAlgorithm
weakerThanSha256[] = { HashAlgorithmSha1 };
On 2015/06/03 09:13:33, Mike West wrote:
> Why do we support SHA-1 at all? I don't remember it being in the spec. Can we
> ASSERT that we never get to this check for that known-bad algorithm?

SHA1 is not supported, so, yes, we can ASSERT that. It's not that we support
SHA-1 (in fact, there's a list of supported hash functions later in
SubresourceIntegrity.cpp that doesn't include SHA-1), but it does exist as an
algorithm listed in HashAlgorithm.

I'll take it out of the lists and add asserts that neither algorithm is ever
SHA1.

https://codereview.chromium.org/1156413005/diff/1/Source/core/frame/Subresour...
Source/core/frame/SubresourceIntegrity.cpp:134: for (IntegrityMetadata& metadata
: metadataList) {
On 2015/06/03 09:13:33, Mike West wrote:
> Nit: `const IntegrityMetadata&`

Done.

https://codereview.chromium.org/1156413005/diff/1/Source/core/frame/Subresour...
Source/core/frame/SubresourceIntegrity.cpp:136: strongestAlgorithm =
metadata.algorithm;
On 2015/06/03 09:13:33, Mike West wrote:
> If you just return the strongest algorithm, you can drop the `if` entirely.

Done.

https://codereview.chromium.org/1156413005/diff/1/Source/core/frame/Subresour...
Source/core/frame/SubresourceIntegrity.cpp:142: if (result != SameStrength)
On 2015/06/03 09:13:33, Mike West wrote:
> Nit: It seems like this could be simplified to `if (result !=
> strongestAlgorithm)`

Long term, that's not strictly true because in theory two different hash
functions can be equally strong (and the spec's definition of
getPrioritizedHashFunction is supposed to account for that). However, given
that's not currently the case, I'll modify getPrioritizedHashFunction to just
return the strongest of the two.

https://codereview.chromium.org/1156413005/diff/1/Source/core/frame/Subresour...
File Source/core/frame/SubresourceIntegrity.h (right):

https://codereview.chromium.org/1156413005/diff/1/Source/core/frame/Subresour...
Source/core/frame/SubresourceIntegrity.h:56: static PrioritizationResult
getPrioritizedHashFunction(HashAlgorithm, HashAlgorithm);
On 2015/06/03 09:13:33, Mike West wrote:
> Why not return the stronger `HashAlgorithm`? The "first" and "second" thing is
> strange.

See other comments for details, but I've changed it to do this. The reason I
wrote it this way was to be more general in case a day comes when we have
different algorithms but are the same strength. Since that's not today's world,
though, we won't do that.

[jww, 2015-06-03 19:27:01]

The CQ bit was checked by jww@chromium.org

[jww, 2015-06-03 19:27:01]

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/1156413005/#ps20001
(title: "Nits")

[commit-bot: I haz the power, 2015-06-03 19:27:25]

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

[commit-bot: I haz the power, 2015-06-03 19:48:51]

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

[commit-bot: I haz the power, 2015-06-03 19:48:53]

Try jobs failed on following builders:
  win_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[jww, 2015-06-03 20:38:05]

The CQ bit was checked by jww@chromium.org

[jww, 2015-06-03 20:38:05]

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/1156413005/#ps40001
(title: "Compile fixes")

[commit-bot: I haz the power, 2015-06-03 20:38:31]

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

[commit-bot: I haz the power, 2015-06-03 21:44:02]

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

[commit-bot: I haz the power, 2015-06-03 21:44:03]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

[jww, 2015-06-04 00:59:46]

The CQ bit was checked by jww@chromium.org

[jww, 2015-06-04 00:59:47]

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/1156413005/#ps60001
(title: "Rebase on ToT")

[commit-bot: I haz the power, 2015-06-04 01:00:00]

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

[commit-bot: I haz the power, 2015-06-04 01:25:18]

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

[commit-bot: I haz the power, 2015-06-04 01:25:19]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

[jww, 2015-06-04 04:51:49]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2015-06-04 04:51:59]

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

[commit-bot: I haz the power, 2015-06-04 05:50:55]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196461