Bug 171070 – [GCrypt] ECDH bit derivation support

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

[GCrypt] ECDH support

[Zan Dobersek]

Created attachment 307627 [details]
Patch

[Michael Catanzaro]

I don't understand it, but rs=me if Jiewen says it looks good.

[Jiewen Tan]

Comment on attachment 307627 [details]
Patch

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

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmECDHGCrypt.cpp:64
> +    PAL::GCrypt::Handle<gcry_sexp_t> cipherSexp;

A little bit confused about this step. Why do we need this encryption? Is it equivalent to adding the public key d times?

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmECDHGCrypt.cpp:109
> +        error = gcry_mpi_print(GCRYMPI_FMT_USG, output.data(), output.size(), nullptr, xMPI);

Why doing gcry_mpi_print twice here? What's the usage of the first gcry_mpi_print?

[Zan Dobersek]

Comment on attachment 307627 [details]
Patch

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

>> Source/WebCore/crypto/gcrypt/CryptoAlgorithmECDHGCrypt.cpp:64
>> +    PAL::GCrypt::Handle<gcry_sexp_t> cipherSexp;
> 
> A little bit confused about this step. Why do we need this encryption? Is it equivalent to adding the public key d times?

It performs the complete ECDH operation, but yes, the public key is added d times which yields the EC point from which the x-coordinate is extracted and returned.

The extraction steps are done below. What's missing is using the specified length to validate and clip the coordinate data. I'll upload a new patch.

>> Source/WebCore/crypto/gcrypt/CryptoAlgorithmECDHGCrypt.cpp:109
>> +        error = gcry_mpi_print(GCRYMPI_FMT_USG, output.data(), output.size(), nullptr, xMPI);
> 
> Why doing gcry_mpi_print twice here? What's the usage of the first gcry_mpi_print?

The first one only retrieves the required buffer size. That's then used to allocate a large-enough Vector into which the data is printed out.

[Zan Dobersek]

(In reply to Zan Dobersek from comment #4)
> The extraction steps are done below. What's missing is using the specified
> length to validate and clip the coordinate data. I'll upload a new patch.
> 

This is actually already done in CryptoAlgorithmECDH::deriveBits(), so we don't have to do it ourselves. So this patch is still valid.

The EWS failure was due to missing 1.7.0 version of libgcrypt on the bot. I'll add the SUBTLE_CRYPTO dependency on 1.7.0 in a separate patch.

[Zan Dobersek]

Created attachment 307727 [details]
Patch

[Jiewen Tan]

Comment on attachment 307727 [details]
Patch

Looks good. However, I suggest you should at least add some false tests for gcrypt specific implementations as they are more complicated than my CommonCrypto ones. I believe I didn't add that many false tests to cover all the cases there.

[Zan Dobersek]

Created attachment 307930 [details]
Patch

[Zan Dobersek]

(In reply to Jiewen Tan from comment #7)
> Comment on attachment 307727 [details]
> Patch
> 
> Looks good. However, I suggest you should at least add some false tests for
> gcrypt specific implementations as they are more complicated than my
> CommonCrypto ones. I believe I didn't add that many false tests to cover all
> the cases there.

Similar question as the one in bug #171074 -- should the additional tests focus on anything else apart from the requested length?

I'm assuming all these tests should be layout tests. But on top of that we could also move the libgcrypt-specific code down to PAL and write C++ unit tests for that.

[Build Bot]

Comment on attachment 307930 [details]
Patch

Attachment 307930 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3589570

New failing tests:
fast/loader/form-submission-after-beforeunload-cancel.html

[Build Bot]

Created attachment 307936 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Jiewen Tan]

(In reply to Zan Dobersek from comment #9)
> (In reply to Jiewen Tan from comment #7)
> > Comment on attachment 307727 [details]
> > Patch
> > 
> > Looks good. However, I suggest you should at least add some false tests for
> > gcrypt specific implementations as they are more complicated than my
> > CommonCrypto ones. I believe I didn't add that many false tests to cover all
> > the cases there.
> 
> Similar question as the one in bug #171074 -- should the additional tests
> focus on anything else apart from the requested length?
> 
> I'm assuming all these tests should be layout tests. But on top of that we
> could also move the libgcrypt-specific code down to PAL and write C++ unit
> tests for that.

Well, I think layout tests should be enough. You might be able to manipulate parameters to force some of the gcrypt apis to fail. That would help to ensure that the exception callback is implemented correctly.

[Zan Dobersek]

Created attachment 308392 [details]
Patch

[Zan Dobersek]

(In reply to Zan Dobersek from comment #13)
> Created attachment 308392 [details]
> Patch

This adds a layout test that tests specific length when deriving from EC keys, making sure a properly-sized result is returned for valid lengths, and that the operation fails if the requested length is larger than the key size.

(In reply to Jiewen Tan from comment #12)
> (In reply to Zan Dobersek from comment #9)
> > (In reply to Jiewen Tan from comment #7)
> > > Comment on attachment 307727 [details]
> > > Patch
> > > 
> > > Looks good. However, I suggest you should at least add some false tests for
> > > gcrypt specific implementations as they are more complicated than my
> > > CommonCrypto ones. I believe I didn't add that many false tests to cover all
> > > the cases there.
> > 
> > Similar question as the one in bug #171074 -- should the additional tests
> > focus on anything else apart from the requested length?
> > 
> > I'm assuming all these tests should be layout tests. But on top of that we
> > could also move the libgcrypt-specific code down to PAL and write C++ unit
> > tests for that.
> 
> Well, I think layout tests should be enough. You might be able to manipulate
> parameters to force some of the gcrypt apis to fail. That would help to
> ensure that the exception callback is implemented correctly.

This one isn't that simple, and it's not immediately related to the deriveBits operation. The GCrypt-specific implementation would fail if the provided public and private keys are somehow invalid when the deriveBits operation is invoked. The import and generation steps ensure to some degree that it doesn't come to that, but I won't guarantee that the current implementations are flawless.

So I would argue that the GCrypt-specific bits of the deriveBits() implementations should expect to work on valid keys. If an error occurs, I would presume it's because of some external factor that we don't have effect on. Again, this is presuming we have bulletproof key import and generation implementations and that the keys are properly validated at that point.

[Build Bot]

Comment on attachment 308392 [details]
Patch

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

New failing tests:
crypto/subtle/ecdh-derive-bits-length-limits.html

[Build Bot]

Created attachment 308400 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308392 [details]
Patch

Attachment 308392 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3620205

New failing tests:
crypto/subtle/ecdh-derive-bits-length-limits.html

[Build Bot]

Created attachment 308401 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308392 [details]
Patch

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

New failing tests:
crypto/subtle/ecdh-derive-bits-length-limits.html

[Build Bot]

Created attachment 308402 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

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

[Build Bot]

Comment on attachment 308392 [details]
Patch

Attachment 308392 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3620223

New failing tests:
crypto/subtle/ecdh-derive-bits-length-limits.html

[Build Bot]

Created attachment 308403 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Jiewen Tan]

Comment on attachment 308392 [details]
Patch

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

> LayoutTests/crypto/subtle/ecdh-derive-bits-length-limits.html:1
> +<!DOCTYPE html>

Can you fix the test? I can't run it on Safari, Chrome and Firefox.

[Zan Dobersek]

Created attachment 308574 [details]
Patch

[Zan Dobersek]

Comment on attachment 308574 [details]
Patch

Clearing flags on attachment: 308574

Committed r216060: <http://trac.webkit.org/changeset/216060>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.