Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
749183004:
WebCrypto: Implement crypto.subtle.deriveKey (chromium-side). (Closed)
|
Created:
6 years ago by eroman Modified:
6 years ago Reviewers:
Ryan Sleevi CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@ecdh Project:
chromium Visibility:
Public. |
DescriptionWebCrypto: Implement crypto.subtle.deriveKey (chromium-side).
BUG=437577
Committed: https://crrev.com/20bf4c3c8f6c408cd2214d76e57265d424dae13e
Cr-Commit-Position: refs/heads/master@{#308110}
Patch Set 1 : #Patch Set 2 : #
Total comments: 14
Patch Set 3 : Address comments #Patch Set 4 : rebase #
Messages
Total messages: 26 (11 generated)
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
eroman@chromium.org changed reviewers: + rsleevi@chromium.org
https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... File content/child/webcrypto/algorithm_dispatch.h (right): https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... content/child/webcrypto/algorithm_dispatch.h:99: const blink::WebCryptoAlgorithm& key_length_algorithm, Wait, why is |key_length_algorithm| different than |import_algorithm| here? Your naming here doesn't match the spec naming, and so I'm confused as to what your parameters are here. https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... File content/child/webcrypto/openssl/ecdh_openssl.cc (right): https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... content/child/webcrypto/openssl/ecdh_openssl.cc:103: // |&derived_bytes->front()| later. Is this really still applicable, given line 100? This only handles if the caller sets Length Bits == 0, which is... not a key? https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... File content/child/webcrypto/test/ecdh_unittest.cc (right): https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... content/child/webcrypto/test/ecdh_unittest.cc:124: // Assume that the 7th key in the test data is for P-521. I'm not terribly keen on assumptions like this baked into the files. https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... content/child/webcrypto/test/ecdh_unittest.cc:263: // output of ECDH is not that random...). Wait, what's up with this comment?
https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... File content/child/webcrypto/algorithm_dispatch.h (right): https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... content/child/webcrypto/algorithm_dispatch.h:99: const blink::WebCryptoAlgorithm& key_length_algorithm, On 2014/12/10 03:21:26, Ryan Sleevi wrote: > Wait, why is |key_length_algorithm| different than |import_algorithm| here? > > Your naming here doesn't match the spec naming, and so I'm confused as to what > your parameters are here. I'll add a comment. In the spec there is a single "derivedKeyType". This dictionary is interpreted both as an import algorithm, and also as an input to get key length. The Blink side parses and normalizes the two algorithm flavors into separate objects -- import_algorithm and key_length_algorithm. It is true that "key_length_algorithm" ends up being a superset of the information contained by import_algorithm in the case of Aes and Hmac (whose "get key length" params are AesDerivedKeyParams and HmacImportParams respectively). However because of the rigid types it is convenient to pass an import parameters which can be used directly by ImportKey(), rather than having to make code aware that ImportKey() may also receive an AesDerivedKeyParams. https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... File content/child/webcrypto/openssl/ecdh_openssl.cc (right): https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... content/child/webcrypto/openssl/ecdh_openssl.cc:103: // |&derived_bytes->front()| later. On 2014/12/10 03:21:26, Ryan Sleevi wrote: > Is this really still applicable, given line 100? This only handles if the caller > sets Length Bits == 0, which is... not a key? Calling deriveBits() with a length of zero is perfectly valid according to the spec. The spec says to generate the full output of ECDH, and then "Return the first length bits of secret." So in the case of specifying "length: 0" the result should be an empty buffer. This comment is simply saying that it handles the zero-length case before calling ECDH_compute_key() below, to avoid worrying about that line calling &derived_bytes->front() on an empty buffer (which would be incorrect) Note that "has_optional_length_bits" does not imply that that length was non-zero. Merely that it was provided https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... File content/child/webcrypto/test/ecdh_unittest.cc (right): https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... content/child/webcrypto/test/ecdh_unittest.cc:263: // output of ECDH is not that random...). On 2014/12/10 03:21:26, Ryan Sleevi wrote: > Wait, what's up with this comment? The first 7 bits of ECDH P-521's output is zero (field size is 521 bits, however output is 528 bits) Therefore using ECDH P-521 to generate an AES-128-bit key (as an example) is just asking for trouble. Your derived key has 7 of the 128 bits directly known (first bits will be zero). And traits of the key can be determined further reducing entropy. I am not a crypto expert, but it seems that truncating the ECDH output (especially from the high order bytes) to directly make a symmetric key is a bad idea. Surely you should instead be sending the un-truncated output of ECDH through a KDF to get a more randomly distributed 128-bit key? That said I am happy to remove the comment.
LGTM % nit https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... File content/child/webcrypto/algorithm_dispatch.h (right): https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... content/child/webcrypto/algorithm_dispatch.h:99: const blink::WebCryptoAlgorithm& key_length_algorithm, On 2014/12/10 17:58:55, eroman wrote: > On 2014/12/10 03:21:26, Ryan Sleevi wrote: > > Wait, why is |key_length_algorithm| different than |import_algorithm| here? > > > > Your naming here doesn't match the spec naming, and so I'm confused as to what > > your parameters are here. > > I'll add a comment. > > In the spec there is a single "derivedKeyType". > This dictionary is interpreted both as an import algorithm, and also as an input > to get key length. > > The Blink side parses and normalizes the two algorithm flavors into separate > objects -- import_algorithm and key_length_algorithm. > > It is true that "key_length_algorithm" ends up being a superset of the > information contained by import_algorithm in the case of Aes and Hmac (whose > "get key length" params are AesDerivedKeyParams and HmacImportParams > respectively). However because of the rigid types it is convenient to pass an > import parameters which can be used directly by ImportKey(), rather than having > to make code aware that ImportKey() may also receive an AesDerivedKeyParams. Thanks. A comment for this sounds perfect. https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... File content/child/webcrypto/openssl/ecdh_openssl.cc (right): https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... content/child/webcrypto/openssl/ecdh_openssl.cc:103: // |&derived_bytes->front()| later. On 2014/12/10 17:58:55, eroman wrote: > On 2014/12/10 03:21:26, Ryan Sleevi wrote: > > Is this really still applicable, given line 100? This only handles if the > caller > > sets Length Bits == 0, which is... not a key? > > Calling deriveBits() with a length of zero is perfectly valid according to the > spec. The spec says to generate the full output of ECDH, and then "Return the > first length bits of secret." > > So in the case of specifying "length: 0" the result should be an empty buffer. > > This comment is simply saying that it handles the zero-length case before > calling ECDH_compute_key() below, to avoid worrying about that line calling > &derived_bytes->front() on an empty buffer (which would be incorrect) > > Note that "has_optional_length_bits" does not imply that that length was > non-zero. Merely that it was provided Ah, right, for deriveBits, zero bits are allowed. Although I wonder if perhaps it should be prohibited? Makes it easier to specify short-circuiting here. My main goal in spec'ing it would be making sure that short circuits are permitted, and that you don't have to compute-and-discard but can instead short-out. Would you agree? https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... File content/child/webcrypto/test/ecdh_unittest.cc (right): https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... content/child/webcrypto/test/ecdh_unittest.cc:263: // output of ECDH is not that random...). On 2014/12/10 17:58:55, eroman wrote: > On 2014/12/10 03:21:26, Ryan Sleevi wrote: > > Wait, what's up with this comment? > > The first 7 bits of ECDH P-521's output is zero (field size is 521 bits, however > output is 528 bits) Do we do leading padding? I thought the result was trailing padding? > > Therefore using ECDH P-521 to generate an AES-128-bit key (as an example) is > just asking for trouble. Your derived key has 7 of the 128 bits directly known > (first bits will be zero). And traits of the key can be determined further > reducing entropy. I'm not sure how to interpret this last sentence. > > I am not a crypto expert, but it seems that truncating the ECDH output > (especially from the high order bytes) to directly make a symmetric key is a bad > idea. Oh, yes, it's absolutely, positively terribad. > > Surely you should instead be sending the un-truncated output of ECDH through a > KDF to get a more randomly distributed 128-bit key? Well, I'm not sure I'd go "more randomly distributed", since you're not going to make entropy appear out of nowhere. But it is going to let you do key expansion, as well as generate distinct keys, and those are both good things. > > That said I am happy to remove the comment. I think maybe the wording leaves a lot of nuance out "(In practice, authors won't be directly generating keys from key agreement schemes, as that is frequently insecure, and instead be using KDFs to expand and generate keys. For simplicity of testing, however, test using an HMAC key)" Or something.
Thanks for the review! https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... File content/child/webcrypto/algorithm_dispatch.h (right): https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... content/child/webcrypto/algorithm_dispatch.h:99: const blink::WebCryptoAlgorithm& key_length_algorithm, On 2014/12/11 23:23:48, Ryan Sleevi wrote: > On 2014/12/10 17:58:55, eroman wrote: > > On 2014/12/10 03:21:26, Ryan Sleevi wrote: > > > Wait, why is |key_length_algorithm| different than |import_algorithm| here? > > > > > > Your naming here doesn't match the spec naming, and so I'm confused as to > what > > > your parameters are here. > > > > I'll add a comment. > > > > In the spec there is a single "derivedKeyType". > > This dictionary is interpreted both as an import algorithm, and also as an > input > > to get key length. > > > > The Blink side parses and normalizes the two algorithm flavors into separate > > objects -- import_algorithm and key_length_algorithm. > > > > It is true that "key_length_algorithm" ends up being a superset of the > > information contained by import_algorithm in the case of Aes and Hmac (whose > > "get key length" params are AesDerivedKeyParams and HmacImportParams > > respectively). However because of the rigid types it is convenient to pass an > > import parameters which can be used directly by ImportKey(), rather than > having > > to make code aware that ImportKey() may also receive an AesDerivedKeyParams. > > Thanks. A comment for this sounds perfect. Done. I added this comment: // Derives a key by calling the underlying deriveBits/getKeyLength/importKey // operations. // // Note that whereas the WebCrypto spec uses a single "derivedKeyType" // AlgorithmIdentifier in its specification of deriveKey(), here two separate // AlgorithmIdentifiers are used: // // * |import_algorithm| -- The parameters required by the derived key's // "importKey" operation. // // * |key_length_algorithm| -- The parameters required by the derived key's // "get key length" operation. // // WebCryptoAlgorithm is not a flexible type like AlgorithmIdentifier (it cannot // be easily re-interpreted as a different parameter type). // // Therefore being provided with separate parameter types for the import // parameters and the key length parameters simplifies passing the right // parameters onto ImportKey() and GetKeyLength() respectively. https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... File content/child/webcrypto/openssl/ecdh_openssl.cc (right): https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... content/child/webcrypto/openssl/ecdh_openssl.cc:103: // |&derived_bytes->front()| later. On 2014/12/11 23:23:48, Ryan Sleevi wrote: > On 2014/12/10 17:58:55, eroman wrote: > > On 2014/12/10 03:21:26, Ryan Sleevi wrote: > > > Is this really still applicable, given line 100? This only handles if the > > caller > > > sets Length Bits == 0, which is... not a key? > > > > Calling deriveBits() with a length of zero is perfectly valid according to the > > spec. The spec says to generate the full output of ECDH, and then "Return the > > first length bits of secret." > > > > So in the case of specifying "length: 0" the result should be an empty buffer. > > > > This comment is simply saying that it handles the zero-length case before > > calling ECDH_compute_key() below, to avoid worrying about that line calling > > &derived_bytes->front() on an empty buffer (which would be incorrect) > > > > Note that "has_optional_length_bits" does not imply that that length was > > non-zero. Merely that it was provided > > Ah, right, for deriveBits, zero bits are allowed. > > Although I wonder if perhaps it should be prohibited? Makes it easier to specify > short-circuiting here. I am fine with prohibiting. > > My main goal in spec'ing it would be making sure that short circuits are > permitted, and that you don't have to compute-and-discard but can instead > short-out. Would you agree? I agree that not having to compute ECDH output in this case is the right implementation strategy (and what this code currently does). I am not convinced the spec needs more words to allow such a short-circuit. The only observable difference I can see is that by _not_ short-circuiting you have the possibility of hitting an operation error if ECDH_compute_key() fails. Computing ECDH output itself is unlikely to have observable side-effects. If short-circuiting is explicitly baked into the spec, I think all the preceding error tests (do the curves match, are they supported curves, etc.) should still run. Just not the actual ECDH algorithm. https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... File content/child/webcrypto/test/ecdh_unittest.cc (right): https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... content/child/webcrypto/test/ecdh_unittest.cc:263: // output of ECDH is not that random...). On 2014/12/11 23:23:48, Ryan Sleevi wrote: > On 2014/12/10 17:58:55, eroman wrote: > > On 2014/12/10 03:21:26, Ryan Sleevi wrote: > > > Wait, what's up with this comment? > > > > The first 7 bits of ECDH P-521's output is zero (field size is 521 bits, > however > > output is 528 bits) > > Do we do leading padding? I thought the result was trailing padding? The leading padding comes from using a big integer that is in big-ending order. This is most pronounced in P-521 where the rounded up degree adds 7 unused bits. From a conversation I had with Wan-Teh this is correct, and specified by SEC 1 section 2.3.7. > > > > > Therefore using ECDH P-521 to generate an AES-128-bit key (as an example) is > > just asking for trouble. Your derived key has 7 of the 128 bits directly > known > > (first bits will be zero). And traits of the key can be determined further > > reducing entropy. > > I'm not sure how to interpret this last sentence. Not sure I know what I was writing here either... I was trying to regurgitate what I learned from smarter people than myself on the matter, who summarized it to me as: "the result of an ECDH key agreement is not uniformly distributed in its range, which is one reason it must be fed into a KDF" The specific reasons why being, "math". > > > > > I am not a crypto expert, but it seems that truncating the ECDH output > > (especially from the high order bytes) to directly make a symmetric key is a > bad > > idea. > > Oh, yes, it's absolutely, positively terribad. > > > > > Surely you should instead be sending the un-truncated output of ECDH through a > > KDF to get a more randomly distributed 128-bit key? > > Well, I'm not sure I'd go "more randomly distributed", since you're not going to > make entropy appear out of nowhere. But it is going to let you do key expansion, > as well as generate distinct keys, and those are both good things. > > > > > That said I am happy to remove the comment. > > I think maybe the wording leaves a lot of nuance out > > "(In practice, authors won't be directly generating keys from key agreement > schemes, as that is frequently insecure, and instead be using KDFs to expand and > generate keys. For simplicity of testing, however, test using an HMAC key)" > > Or something. Sounds good, done
https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... File content/child/webcrypto/test/ecdh_unittest.cc (right): https://codereview.chromium.org/749183004/diff/80001/content/child/webcrypto/... content/child/webcrypto/test/ecdh_unittest.cc:124: // Assume that the 7th key in the test data is for P-521. On 2014/12/10 03:21:26, Ryan Sleevi wrote: > I'm not terribly keen on assumptions like this baked into the files. I have not addressed this comment yet. I don't much care for this either, but it seems simpler than duplicating the key data elsewhere. Some ways to improve this would be: (1) Give explicit names to the test keys, rather than relying on its position in the array. (2) Use a separate test file containing a P-521 key (3) Generate a key directly in the test (although will slow down the test) (4) Inline the key data into the C++ test file. I will address this as a follow-up changelist based on your response to these options.
The CQ bit was checked by eroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/749183004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by eroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/749183004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by eroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/749183004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by eroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/749183004/120001
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/20bf4c3c8f6c408cd2214d76e57265d424dae13e Cr-Commit-Position: refs/heads/master@{#308110} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
