Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12326029:
Porting of HKDF changes from server. (Closed)
|
Created:
7 years, 10 months ago by ramant (doing other things) Modified:
7 years, 10 months ago CC:
chromium-reviews, wtc, Ryan Hamilton, jar (doing other things) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPorting of HKDF changes from server.
Merge internal CL: 40300624
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184133
Patch Set 1 #Patch Set 2 : Small fix to unit test. #Patch Set 3 : Fix for Mac build #Patch Set 4 : Mac build fix# 2 #
Total comments: 23
Patch Set 5 : Fixes to comments and deletion of scoped_array. #
Total comments: 4
Patch Set 6 : minor comments update #
Total comments: 6
Messages
Total messages: 12 (0 generated)
LGTM. Substantially a rubber stamp since this is just moving code around. https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.h File crypto/hkdf.h (right): https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.h#newcode5 crypto/hkdf.h:5: #ifndef GFE_QUIC_CRYPTO_BASE_HKDF_H_ I'm not sure whether we are changing the include guards in the Chromium code. rch would know.
https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.cc File crypto/hkdf.cc (right): https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.cc#newc... crypto/hkdf.cc:20: base::StringPiece actual_salt = salt; BUG? You're not actually making a copy here - you're mutating |salt| directly. https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.cc#newc... crypto/hkdf.cc:21: char zeros[kSHA256HashLength]; Just do char zeros = {0}; https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.cc#newc... crypto/hkdf.cc:28: HMAC prk_hmac(HMAC::SHA256); naming: prk (while used in RFC 5869 heavily), is not an obvious abbreviation (it's only expanded as "pseudorandom key" once in 5869) https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.cc#newc... crypto/hkdf.cc:47: scoped_array<char> buf(new char[kSHA256HashLength + info.size() + 1]); scoped_array is deprecated - use scoped_ptr with the appropriate deleter https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.h File crypto/hkdf.h (right): https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.h#newcode6 crypto/hkdf.h:6: #define GFE_QUIC_CRYPTO_BASE_HKDF_H_ Yes, this header should be fixed https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.h#newco... crypto/hkdf.h:19: // SHA-256) and outputs key material, as needed by QUIC. This seems fairly specific to QUIC, in the client/server write_key and IVs. Normally, a class in crypto:: would provide the basic interface that would expose |output_|, and then code in the higher layer would handle splitting out these keys. However, consider the comments below regarding the use of |info| below. https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.h#newco... crypto/hkdf.h:26: // iv_bytes_to_generate: the number of bytes of IV to generate. Can you please provide comments on these that explain the security parameters? eg: does |secret| need to be some minimum length? How should |salt| be generated - random data or from some protocol? Why do you distinguish between |key_bytes| and |iv_bytes|, rather than a unified form? Doesn't RFC 5869 suggest using different |info|s for different usages (IV and key)?
https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.cc File crypto/hkdf.cc (right): https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.cc#newc... crypto/hkdf.cc:20: base::StringPiece actual_salt = salt; On 2013/02/21 17:48:31, Ryan Sleevi wrote: > BUG? You're not actually making a copy here - you're mutating |salt| directly. I don't think that |actual_salt| / |salt| is ever mutated. |actual_salt| is just a fat pointer that either points to the argument, or to a zero block. https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.cc#newc... crypto/hkdf.cc:21: char zeros[kSHA256HashLength]; On 2013/02/21 17:48:31, Ryan Sleevi wrote: > Just do > > char zeros = {0}; I don't really care, but I try to avoid using these odd corners of the language where possible. https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.cc#newc... crypto/hkdf.cc:28: HMAC prk_hmac(HMAC::SHA256); On 2013/02/21 17:48:31, Ryan Sleevi wrote: > naming: prk (while used in RFC 5869 heavily), is not an obvious abbreviation > (it's only expanded as "pseudorandom key" once in 5869) Any better suggestions than the RFC's naming?
Longer term, we may wish/need to consider using NSS directly for this ( see http://src.chromium.org/viewvc/chrome/trunk/deps/third_party/nss/mozilla/secu... , added in 3.12.9) The reason for this is I've been getting pinged regarding FIPS 140-2 support for Chrome & ChromeOS. Right now, CAVP only requires KBKDF testing (eg: those using SP 800-108), and doesn't have any testing requirements on the SP 800-56B or C (although it does for A). Still, LGTM, mod the header guard and comment nits below. https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.cc File crypto/hkdf.cc (right): https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.cc#newc... crypto/hkdf.cc:20: base::StringPiece actual_salt = salt; On 2013/02/21 18:16:11, agl wrote: > On 2013/02/21 17:48:31, Ryan Sleevi wrote: > > BUG? You're not actually making a copy here - you're mutating |salt| directly. > > > I don't think that |actual_salt| / |salt| is ever mutated. |actual_salt| is just > a fat pointer that either points to the argument, or to a zero block. Ah right, parsing error on my part. https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.cc#newc... crypto/hkdf.cc:21: char zeros[kSHA256HashLength]; On 2013/02/21 18:16:11, agl wrote: > On 2013/02/21 17:48:31, Ryan Sleevi wrote: > > Just do > > > > char zeros = {0}; > > I don't really care, but I try to avoid using these odd corners of the language > where possible. > fair enough https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.cc#newc... crypto/hkdf.cc:28: HMAC prk_hmac(HMAC::SHA256); On 2013/02/21 18:16:11, agl wrote: > On 2013/02/21 17:48:31, Ryan Sleevi wrote: > > naming: prk (while used in RFC 5869 heavily), is not an obvious abbreviation > > (it's only expanded as "pseudorandom key" once in 5869) > > Any better suggestions than the RFC's naming? We've used spec-related variable names before, but we've made sure to spell out their meaning (and why they're named inconsistently) // Perform the Extract step to transform the input key and // salt into the pseudorandom key (PRK) used for Expand. HMAC prk_hmac(HMAC::SHA256); https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.cc#newc... crypto/hkdf.cc:37: // https://tools.ietf.org/html/rfc5869#section-2.3 // https://tools.ietf.org/html/rfc5869#section-2.3 // Perform the Expand phase to turn the pseudorandom key // and info into the output keying material.
Hi agl@ and Ryan Sleevi, Added all the comments Sleevi suggested. Added some more comments to the header file explaining args. It looked simpler (to me) to use char* and delete the array at the end of the function (compared to using scoped_ptr<char>). What do you both think? PTAL. https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.cc File crypto/hkdf.cc (right): https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.cc#newc... crypto/hkdf.cc:28: HMAC prk_hmac(HMAC::SHA256); On 2013/02/21 19:30:44, Ryan Sleevi wrote: > On 2013/02/21 18:16:11, agl wrote: > > On 2013/02/21 17:48:31, Ryan Sleevi wrote: > > > naming: prk (while used in RFC 5869 heavily), is not an obvious abbreviation > > > (it's only expanded as "pseudorandom key" once in 5869) > > > > Any better suggestions than the RFC's naming? > > We've used spec-related variable names before, but we've made sure to spell out > their meaning (and why they're named inconsistently) > > // Perform the Extract step to transform the input key and > // salt into the pseudorandom key (PRK) used for Expand. > HMAC prk_hmac(HMAC::SHA256); Done. https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.cc#newc... crypto/hkdf.cc:37: // https://tools.ietf.org/html/rfc5869#section-2.3 On 2013/02/21 19:30:44, Ryan Sleevi wrote: > // https://tools.ietf.org/html/rfc5869#section-2.3 > // Perform the Expand phase to turn the pseudorandom key > // and info into the output keying material. Done. https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.cc#newc... crypto/hkdf.cc:47: scoped_array<char> buf(new char[kSHA256HashLength + info.size() + 1]); On 2013/02/21 17:48:31, Ryan Sleevi wrote: > scoped_array is deprecated - use scoped_ptr with the appropriate deleter Done. https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.h File crypto/hkdf.h (right): https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.h#newcode5 crypto/hkdf.h:5: #ifndef GFE_QUIC_CRYPTO_BASE_HKDF_H_ On 2013/02/21 15:00:43, agl wrote: > I'm not sure whether we are changing the include guards in the Chromium code. > rch would know. QUIC has included guards. Thanks for the catch. Done. https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.h#newcode6 crypto/hkdf.h:6: #define GFE_QUIC_CRYPTO_BASE_HKDF_H_ On 2013/02/21 17:48:31, Ryan Sleevi wrote: > Yes, this header should be fixed Done. https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.h#newco... crypto/hkdf.h:19: // SHA-256) and outputs key material, as needed by QUIC. On 2013/02/21 17:48:31, Ryan Sleevi wrote: > This seems fairly specific to QUIC, in the client/server write_key and IVs. > > Normally, a class in crypto:: would provide the basic interface that would > expose |output_|, and then code in the higher layer would handle splitting out > these keys. > > However, consider the comments below regarding the use of |info| below. Will like to leave this to agl@ (because it is API change). https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.h#newco... crypto/hkdf.h:26: // iv_bytes_to_generate: the number of bytes of IV to generate. On 2013/02/21 17:48:31, Ryan Sleevi wrote: > Can you please provide comments on these that explain the security parameters? > > eg: does |secret| need to be some minimum length? How should |salt| be generated > - random data or from some protocol? Why do you distinguish between |key_bytes| > and |iv_bytes|, rather than a unified form? > > Doesn't RFC 5869 suggest using different |info|s for different usages (IV and > key)? agl@: added some comments from RFC 5869. What do you think?
https://chromiumcodereview.appspot.com/12326029/diff/14001/crypto/hkdf.cc File crypto/hkdf.cc (right): https://chromiumcodereview.appspot.com/12326029/diff/14001/crypto/hkdf.cc#new... crypto/hkdf.cc:64: buf[j++] = i + 1; Should you add a static_cast here? because buf is a char*, and i is a size_t buf[j++] = static_cast<char>((i + 1) & 0xFF); https://chromiumcodereview.appspot.com/12326029/diff/14001/crypto/hkdf.h File crypto/hkdf.h (right): https://chromiumcodereview.appspot.com/12326029/diff/14001/crypto/hkdf.h#newc... crypto/hkdf.h:25: // https://tools.ietf.org/html/rfc5869#section-2.1. // |secret|: The input shared secret (or, from RFC 5869, the IKM). // |salt|: an (optional) public salt / non-secret random value. While optional, callers are strongly recommended to provide a salt. There is no added security value in making // this larger than the SHA-256 block size of 64 bytes. // |info|: (leave as existing)
https://chromiumcodereview.appspot.com/12326029/diff/14001/crypto/hkdf.cc File crypto/hkdf.cc (right): https://chromiumcodereview.appspot.com/12326029/diff/14001/crypto/hkdf.cc#new... crypto/hkdf.cc:64: buf[j++] = i + 1; On 2013/02/21 23:30:08, Ryan Sleevi wrote: > Should you add a static_cast here? because buf is a char*, and i is a size_t > > buf[j++] = static_cast<char>((i + 1) & 0xFF); Done. https://chromiumcodereview.appspot.com/12326029/diff/14001/crypto/hkdf.h File crypto/hkdf.h (right): https://chromiumcodereview.appspot.com/12326029/diff/14001/crypto/hkdf.h#newc... crypto/hkdf.h:25: // https://tools.ietf.org/html/rfc5869#section-2.1. On 2013/02/21 23:30:08, Ryan Sleevi wrote: > // |secret|: The input shared secret (or, from RFC 5869, the IKM). > // |salt|: an (optional) public salt / non-secret random value. While optional, > callers are strongly recommended to provide a salt. There is no added security > value in making > // this larger than the SHA-256 block size of 64 bytes. > > // |info|: (leave as existing) Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/12326029/9004
Message was sent while issue was closed.
Change committed as 184133
Message was sent while issue was closed.
Patch set 6 LGTM. I suggest three small changes. https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.h File crypto/hkdf.h (right): https://chromiumcodereview.appspot.com/12326029/diff/6001/crypto/hkdf.h#newco... crypto/hkdf.h:26: // iv_bytes_to_generate: the number of bytes of IV to generate. On 2013/02/21 17:48:31, Ryan Sleevi wrote: > > Doesn't RFC 5869 suggest using different |info|s for different usages (IV and > key)? I copied the current usage from TLS, which considers the generation of client and server keys and IVs as a single usage (called "key expansion"). This seems to be allowed by the RFC. For example, the RFC says, on page 2: ... The second stage "expands" the key K into several additional pseudorandom keys (the output of the KDF). without saying the second stage should be invoked once for each output key. https://chromiumcodereview.appspot.com/12326029/diff/9004/crypto/hkdf.cc File crypto/hkdf.cc (right): https://chromiumcodereview.appspot.com/12326029/diff/9004/crypto/hkdf.cc#newc... crypto/hkdf.cc:52: char* buf = new char[kSHA256HashLength + info.size() + 1]; I think it is better to use scoped_ptr<char[]> here. This is common practice in the Chromium source tree. https://chromiumcodereview.appspot.com/12326029/diff/9004/crypto/hkdf.cc#newc... crypto/hkdf.cc:64: buf[j++] = static_cast<char>((i + 1) & 0xFF); Please remove & 0xFF. It implies i + 1 can be greater than 255 (0xFF), but the RFC requires that i + 1 <= 255. This requirement comes from the requirement on L (<= 255*HashLen) and is checked by the DCHECK on line 47. So i + 1 won't overflow an unsigned char. We can add a static_cast if compilers warn about it. https://chromiumcodereview.appspot.com/12326029/diff/9004/crypto/hkdf.h File crypto/hkdf.h (right): https://chromiumcodereview.appspot.com/12326029/diff/9004/crypto/hkdf.h#newco... crypto/hkdf.h:38: virtual ~HKDF(); I think it is wrong to mark the destructor virtual because this class has no virtual methods.
Message was sent while issue was closed.
Thanks very much wtc. Made the changes you have suggested in the following CL: https://chromiumcodereview.appspot.com/12330157/ https://chromiumcodereview.appspot.com/12326029/diff/9004/crypto/hkdf.cc File crypto/hkdf.cc (right): https://chromiumcodereview.appspot.com/12326029/diff/9004/crypto/hkdf.cc#newc... crypto/hkdf.cc:52: char* buf = new char[kSHA256HashLength + info.size() + 1]; On 2013/02/25 22:18:00, wtc wrote: > > I think it is better to use scoped_ptr<char[]> here. This is > common practice in the Chromium source tree. Done. https://chromiumcodereview.appspot.com/12326029/diff/9004/crypto/hkdf.cc#newc... crypto/hkdf.cc:64: buf[j++] = static_cast<char>((i + 1) & 0xFF); On 2013/02/25 22:18:00, wtc wrote: > > Please remove & 0xFF. > > It implies i + 1 can be greater than 255 (0xFF), > but the RFC requires that i + 1 <= 255. This requirement > comes from the requirement on L (<= 255*HashLen) and is > checked by the DCHECK on line 47. > > So i + 1 won't overflow an unsigned char. We can add a > static_cast if compilers warn about it. Done. https://chromiumcodereview.appspot.com/12326029/diff/9004/crypto/hkdf.h File crypto/hkdf.h (right): https://chromiumcodereview.appspot.com/12326029/diff/9004/crypto/hkdf.h#newco... crypto/hkdf.h:38: virtual ~HKDF(); On 2013/02/25 22:18:00, wtc wrote: > > I think it is wrong to mark the destructor virtual because > this class has no virtual methods. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
