Bug 171070

Created attachment 307627 [details] Patch

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

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?

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.

(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.

Created attachment 307727 [details] Patch

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.

Created attachment 307930 [details] Patch

(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.

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

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

(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.

Created attachment 308392 [details] Patch

(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.

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

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

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

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

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

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

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

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

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.

Created attachment 308574 [details] Patch

Comment on attachment 308574 [details] Patch Clearing flags on attachment: 308574 Committed r216060: <http://trac.webkit.org/changeset/216060>

All reviewed patches have been landed. Closing bug.