Bug 155247 – [Win] Implement CryptoDigest

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Daniel Bates]

We should implement CryptoDigest for Windows to have access to cryptographically secure hash algorithms. This will allow us to support Content Security Policy inline script and inline stylesheet hashes on Windows among other benefits.

[Radar WebKit Bug Importer]

<rdar://problem/25065843>

[Daniel Bates]

Created attachment 273453 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 273453 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273453&action=review

> Source/WebCore/ChangeLog:9
> +        Implement the CryptoDigest abstraction for Windows so that we can compute cryptographically

Is this covered by any regression tests that can be unskipped?

> Source/WebCore/platform/crypto/win/CryptoDigestWin.cpp:83
> +    CryptHashData(m_context->hHash, input, length, 0);

Please add a RELEASE_ASSERT for return value.

Also, what does the function do for null input and zero length? I don't remember if we ever pass such arguments, or what CommonCrypto does.

> Source/WebCore/platform/crypto/win/CryptoDigestWin.cpp:91
> +    CryptGetHashParam(m_context->hHash, HP_HASHSIZE, nullptr, &digestLength, 0);

This function also returns a result that we should check.

> Source/WebCore/platform/crypto/win/CryptoDigestWin.cpp:94
> +    CryptGetHashParam(m_context->hHash, HP_HASHVAL, result.data(), result.size());

I don't think that this is going to compile, as this call has fewer arguments. We should probably assert that the newly provided size matches digestLength.

[Daniel Bates]

(In reply to comment #3)
> > Source/WebCore/ChangeLog:9
> > +        Implement the CryptoDigest abstraction for Windows so that we can compute cryptographically
> 
> Is this covered by any regression tests that can be unskipped?
> 

Yes, this code is covered by the Content Security Policy tests for inline script and inline stylesheet hashes that I will unskip in the patch for bug #155007.

> > Source/WebCore/platform/crypto/win/CryptoDigestWin.cpp:83
> > +    CryptHashData(m_context->hHash, input, length, 0);
> 
> Please add a RELEASE_ASSERT for return value.
> 

Will do.

> Also, what does the function do for null input and zero length? [...]
> 

Will bail out early if either input is null or length is 0.

From reading <https://msdn.microsoft.com/en-us/library/windows/desktop/aa380202(v=vs.85).aspx> I suspect that this function is likely unhappy when passed a null pointer or zero length based on the presence of the possible error value ERROR_INVALID_PARAMETER.

> > Source/WebCore/platform/crypto/win/CryptoDigestWin.cpp:91
> > +    CryptGetHashParam(m_context->hHash, HP_HASHSIZE, nullptr, &digestLength, 0);
> 
> This function also returns a result that we should check.
> 

Will do.

> > Source/WebCore/platform/crypto/win/CryptoDigestWin.cpp:94
> > +    CryptGetHashParam(m_context->hHash, HP_HASHVAL, result.data(), result.size());
> 
> I don't think that this is going to compile, as this call has fewer
> arguments. We should probably assert that the newly provided size matches
> digestLength.

Oops! Will fix.

[Daniel Bates]

Created attachment 273479 [details]
Patch

[Daniel Bates]

Created attachment 273483 [details]
Patch

[Brent Fulgham]

Comment on attachment 273483 [details]
Patch

r=me. Please land if the tests are green.

[Build Bot]

Comment on attachment 273483 [details]
Patch

Attachment 273483 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/950532

New failing tests:
js/function-apply.html

[Build Bot]

Created attachment 273495 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Daniel Bates]

Committed r197905: <http://trac.webkit.org/changeset/197905>

[Daniel Bates]

Committed Windows build fix in <https://trac.webkit.org/changeset/197911>.