Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11175015:
crypto: add GHASH implementation. (Closed)
|
Created:
8 years, 2 months ago by agl Modified:
8 years, 1 month ago CC:
chromium-reviews, Ryan Sleevi Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptioncrypto: add GHASH implementation.
Can be used to implement GCM until GCM support in NSS is widespread.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=166952
Patch Set 1 #Patch Set 2 : ... #
Total comments: 36
Patch Set 3 : Addressing wtc's comments #
Total comments: 8
Patch Set 4 : ... #Patch Set 5 : Add CRYPTO_EXPORT_PRIVATE for unittests #Patch Set 6 : ... and remember to include the header #
Messages
Total messages: 12 (0 generated)
high level: I'm not enthused about this approach for a few reasons: 1) It's yet another low-level crypto implementation that has to be carefully reviewed for both implementation and security 2) It's going to be difficult to share any meaningful CPU optimization with NSS 3) While no BUG is referenced here, I can only infer that you plan to implement a crypto/ side implementation of AES-GCM by combining ECB with this GHASH function This only affects Linux, since we control the NSS version for Windows, Mac, iOS, and ChromeOS, and OpenSSL on Android has GCM if I'm not mistaken. An alternative approach to this, one which would be less intrusive in Chrome land and, IMO, more maintainable overall, would be to build a PKCS#11 module that exposed AES-GCM that way, which is how it will presumably eventually be used. FWIW, NSS already has ckfw that can be used to quickly bootstrap a PKCS#11 module (it's what the builtin cert data uses), and you could plugin the existing freebl implementation (with it's recently contributed by Intel optimized assembly path). I realize this is much work up front, especially with the implementation already here, but I really would prefer to avoid including crypto primitives in Chromium itself as much as possible. Just my $.02
Patch set 2 LGTM. Thanks! You are really good at implementing finite fields :-) https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.cc File crypto/ghash.cc (right): https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.cc#newcode37 crypto/ghash.cc:37: memcpy(bytes, &x, 8); Nit: 8 => sizeof(x) ? (You use sizeof(t) on line 30.) https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.cc#newcode43 crypto/ghash.cc:43: i = ((i << 1)&0xa) | ((i >> 1)&0x5); Nit: add spaces around the & operators. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.cc#newcode56 crypto/ghash.cc:56: // index 0010b = 2. Nit: your use of the 'b' suffix confused me for a bit because 'b' is also a hexadecimal digit. I wonder if we should be more verbose: 0010 (base 2) = 2. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.cc#newcode64 crypto/ghash.cc:64: productTable[Reverse(i+1)] = Add(productTable[Reverse(i)], x); In Reverse(i+1), you are relying on the fact that Reverse(16) is the same as Reverse(0), right? https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.cc#newcode114 crypto/ghash.cc:114: // block. The lengths are the number of bits. This shows you implemented the original GHASH. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.cc#newcode120 crypto/ghash.cc:120: Put64(result+8, y_.hi); I suggest adding spaces around the operators in this function. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.cc#newcode134 crypto/ghash.cc:134: const bool msbSet = x.hi & 1; msbSet => msb_set https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.cc#newcode149 crypto/ghash.cc:149: xx.low ^= 0xe100000000000000; You may need to add a ULL suffix to the constant to avoid a compiler warning. Nit: omit the curly braces. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.h File crypto/ghash.h (right): https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.h#newcode9 crypto/ghash.h:9: // GaliosHash implements the polynomial authenticator part of GCM as specified Typo: Galios => Galois https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.h#newcode11 crypto/ghash.h:11: // Specifically it implements the GHASH function, defined in section 6.4 of Since your GaliosHash function hashes the lengths of the additional data and ciphertext, it is actually the original GHASH specified by McGrew and Viega, not the final GHASH specified in NIST SP 800-38D. This difference was mentioned in the footnote on page 9 of 800-38D. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.h#newcode37 crypto/ghash.h:37: void Digest(uint8 result[16]); This method should probably be named Finish(), with a prototype similar to the Finish() method in crypto/secure_hash.h. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.h#newcode63 crypto/ghash.h:63: void Update(const uint8* bytes, size_t length); Nit: document Update(). https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.h#newcode71 crypto/ghash.h:71: FieldElement productTable[16]; productTable => product_table_ Note the trailing _. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash_unittest.cc File crypto/ghash_unittest.cc (right): https://codereview.chromium.org/11175015/diff/2001/crypto/ghash_unittest.cc#n... crypto/ghash_unittest.cc:7: Delete this line. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash_unittest.cc#n... crypto/ghash_unittest.cc:10: using namespace crypto; We usually just put the tests inside the |crypto| namespace. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash_unittest.cc#n... crypto/ghash_unittest.cc:15: // http://csrc.nist.gov/groups/ST/toolkit/BCM/documents/proposedmodes/gcm/gcm-sp... There is a revised version of this file. Search for gcm-revised-spec.pdf. You can also get the revised spec from David McGrew's home page: http://www.mindspring.com/~dmcgrew/dam.htm#GCM https://codereview.chromium.org/11175015/diff/2001/crypto/ghash_unittest.cc#n... crypto/ghash_unittest.cc:105: TEST(GaliosHash, TestCases) { Galios => Galois https://codereview.chromium.org/11175015/diff/2001/crypto/ghash_unittest.cc#n... crypto/ghash_unittest.cc:135: } Nit: omit the curly braces for one-line if or for loop bodies in this file. This is the more common convention in crypto and net. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash_unittest.cc#n... crypto/ghash_unittest.cc:141: } // anonymous namespace Nit: in the code example in the Style Guide, this comment just says // namespace http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces
On Fri, Oct 19, 2012 at 5:28 PM, <rsleevi@chromium.org> wrote: > 1) It's yet another low-level crypto implementation that has to be carefully > reviewed for both implementation and security > 2) It's going to be difficult to share any meaningful CPU optimization with > NSS > 3) While no BUG is referenced here, I can only infer that you plan to > implement > a crypto/ side implementation of AES-GCM by combining ECB with this GHASH > function I wrote this because a) it was a Friday and b) wtc had suggested that Linux's use of the system NSS library would hinder our use of AES-GCM because it was a recent NSS addition. That seemed very sad given that GCM just isn't very complex. > An alternative approach to this, one which would be less intrusive in Chrome > land and, IMO, more maintainable overall, would be to build a PKCS#11 module > that exposed AES-GCM that way, which is how it will presumably eventually be > used. I'm afraid that I fear for your sanity if you think that a PKCS#11 module is the maintainable way to go! I imaging that, on most platforms we'll simply be able to call the NSS function to do AES-GCM because we control the NSS version. On Linux, we might need an #ifdef to call fill in code in crypto/, of which this code would be part. The Linux fill-in code wouldn't have AES-NI support etc, but all the other platforms would. Over time, as distro's get the newer NSS, they would use the NSS code. I'll hold off on landing it until someone tells me that it would be useful. I might have misunderstood. Cheers AGL
https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.cc File crypto/ghash.cc (right): https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.cc#newcode37 crypto/ghash.cc:37: memcpy(bytes, &x, 8); On 2012/10/19 21:35:22, wtc wrote: > > Nit: 8 => sizeof(x) ? > > (You use sizeof(t) on line 30.) Done. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.cc#newcode43 crypto/ghash.cc:43: i = ((i << 1)&0xa) | ((i >> 1)&0x5); On 2012/10/19 21:35:22, wtc wrote: > > Nit: add spaces around the & operators. Done. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.cc#newcode56 crypto/ghash.cc:56: // index 0010b = 2. On 2012/10/19 21:35:22, wtc wrote: > > Nit: your use of the 'b' suffix confused me for a bit > because 'b' is also a hexadecimal digit. I wonder if we > should be more verbose: 0010 (base 2) = 2. Done. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.cc#newcode64 crypto/ghash.cc:64: productTable[Reverse(i+1)] = Add(productTable[Reverse(i)], x); On 2012/10/19 21:35:22, wtc wrote: > > In Reverse(i+1), you are relying on the fact that > Reverse(16) is the same as Reverse(0), right? Reverse(16) should never be called, I believe. The for loop is advancing by two each time, so the maximum value for i is 14. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.cc#newcode120 crypto/ghash.cc:120: Put64(result+8, y_.hi); On 2012/10/19 21:35:22, wtc wrote: > > I suggest adding spaces around the operators in this function. Done. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.cc#newcode134 crypto/ghash.cc:134: const bool msbSet = x.hi & 1; On 2012/10/19 21:35:22, wtc wrote: > > msbSet => msb_set Done. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.cc#newcode149 crypto/ghash.cc:149: xx.low ^= 0xe100000000000000; On 2012/10/19 21:35:22, wtc wrote: > > You may need to add a ULL suffix to the constant to > avoid a compiler warning. > > Nit: omit the curly braces. Done. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.h File crypto/ghash.h (right): https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.h#newcode9 crypto/ghash.h:9: // GaliosHash implements the polynomial authenticator part of GCM as specified On 2012/10/19 21:35:22, wtc wrote: > > Typo: Galios => Galois Done. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.h#newcode11 crypto/ghash.h:11: // Specifically it implements the GHASH function, defined in section 6.4 of On 2012/10/19 21:35:22, wtc wrote: > > Since your GaliosHash function hashes the lengths of the > additional data and ciphertext, it is actually the original > GHASH specified by McGrew and Viega, not the final GHASH > specified in NIST SP 800-38D. This difference was mentioned > in the footnote on page 9 of 800-38D. Both the original paper and SP-800-38D produce the same results. The footnote remarks that the boundary between GHASH and GCM has moved. In SP-800-38D, GHASH takes a single data argument, which happens to include the lengths of the authenticated and encrypted data at the end. In the original paper [1], GHASH takes a pair of data arguments and the behaviour of appending the lengths is contained within GHASH. I've made a note of this in the comment, but I believe that the implementation works exactly the same, although this code could not cope with SP-800-38D's GHASH being called with a non-standard argument form. [1] http://eprint.iacr.org/2004/193.pdf, end of section 2. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.h#newcode37 crypto/ghash.h:37: void Digest(uint8 result[16]); On 2012/10/19 21:35:22, wtc wrote: > > This method should probably be named Finish(), with a prototype > similar to the Finish() method in crypto/secure_hash.h. Done. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.h#newcode63 crypto/ghash.h:63: void Update(const uint8* bytes, size_t length); On 2012/10/19 21:35:22, wtc wrote: > > Nit: document Update(). Done. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash.h#newcode71 crypto/ghash.h:71: FieldElement productTable[16]; On 2012/10/19 21:35:22, wtc wrote: > > productTable => product_table_ > > Note the trailing _. Done. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash_unittest.cc File crypto/ghash_unittest.cc (right): https://codereview.chromium.org/11175015/diff/2001/crypto/ghash_unittest.cc#n... crypto/ghash_unittest.cc:7: On 2012/10/19 21:35:22, wtc wrote: > > Delete this line. Done. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash_unittest.cc#n... crypto/ghash_unittest.cc:10: using namespace crypto; On 2012/10/19 21:35:22, wtc wrote: > > We usually just put the tests inside the |crypto| > namespace. Done. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash_unittest.cc#n... crypto/ghash_unittest.cc:105: TEST(GaliosHash, TestCases) { On 2012/10/19 21:35:22, wtc wrote: > > Galios => Galois Done. (Opps!) https://codereview.chromium.org/11175015/diff/2001/crypto/ghash_unittest.cc#n... crypto/ghash_unittest.cc:135: } On 2012/10/19 21:35:22, wtc wrote: > > Nit: omit the curly braces for one-line if or for loop bodies > in this file. This is the more common convention in crypto > and net. Done. https://codereview.chromium.org/11175015/diff/2001/crypto/ghash_unittest.cc#n... crypto/ghash_unittest.cc:141: } // anonymous namespace On 2012/10/19 21:35:22, wtc wrote: > > Nit: in the code example in the Style Guide, this comment > just says > // namespace > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces Done.
Patch set 3 LGTM. I think this is a reasonable way to provide AES-GCM on Linux distros that have NSS 3.13.x or older. https://codereview.chromium.org/11175015/diff/7002/crypto/ghash.cc File crypto/ghash.cc (right): https://codereview.chromium.org/11175015/diff/7002/crypto/ghash.cc#newcode131 crypto/ghash.cc:131: } Nit: omit the curly braces. https://codereview.chromium.org/11175015/diff/7002/crypto/ghash.h File crypto/ghash.h (right): https://codereview.chromium.org/11175015/diff/7002/crypto/ghash.h#newcode16 crypto/ghash.h:16: // This assumption is built into the GaloisHash interface. As we discussed in the conference call this afternoon, this comment is still inaccurate. https://codereview.chromium.org/11175015/diff/7002/crypto/ghash.h#newcode40 crypto/ghash.h:40: // Finish completes the hash computation and at most |len| bytes of the Add "writes" before "at most". https://codereview.chromium.org/11175015/diff/7002/crypto/ghash_unittest.cc File crypto/ghash_unittest.cc (right): https://codereview.chromium.org/11175015/diff/7002/crypto/ghash_unittest.cc#n... crypto/ghash_unittest.cc:14: // http://csrc.nist.gov/groups/ST/toolkit/BCM/documents/proposedmodes/gcm/gcm-sp... I suggest citing http://csrc.nist.gov/groups/ST/toolkit/BCM/documents/proposedmodes/gcm/gcm-re... instead.
https://codereview.chromium.org/11175015/diff/7002/crypto/ghash.cc File crypto/ghash.cc (right): https://codereview.chromium.org/11175015/diff/7002/crypto/ghash.cc#newcode131 crypto/ghash.cc:131: } On 2012/10/25 00:54:51, wtc wrote: > > Nit: omit the curly braces. Done. https://codereview.chromium.org/11175015/diff/7002/crypto/ghash.h File crypto/ghash.h (right): https://codereview.chromium.org/11175015/diff/7002/crypto/ghash.h#newcode16 crypto/ghash.h:16: // This assumption is built into the GaloisHash interface. On 2012/10/25 00:54:51, wtc wrote: > > As we discussed in the conference call this afternoon, this > comment is still inaccurate. I have updated it again. https://codereview.chromium.org/11175015/diff/7002/crypto/ghash.h#newcode40 crypto/ghash.h:40: // Finish completes the hash computation and at most |len| bytes of the On 2012/10/25 00:54:51, wtc wrote: > > Add "writes" before "at most". Done. https://codereview.chromium.org/11175015/diff/7002/crypto/ghash_unittest.cc File crypto/ghash_unittest.cc (right): https://codereview.chromium.org/11175015/diff/7002/crypto/ghash_unittest.cc#n... crypto/ghash_unittest.cc:14: // http://csrc.nist.gov/groups/ST/toolkit/BCM/documents/proposedmodes/gcm/gcm-sp... On 2012/10/25 00:54:51, wtc wrote: > > I suggest citing > http://csrc.nist.gov/groups/ST/toolkit/BCM/documents/proposedmodes/gcm/gcm-re... > instead. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/agl@chromium.org/11175015/12001
Sorry for I got bad news for ya. Compile failed with a clobber build on mac. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
Patch set 4 LGTM. For the rest of AES-GCM: Another option would be to copy the NSS GCM implementation (four files in lib/freebl: ctr.h, ctr.c, gcm.h, gcm.c) to the Chromium source tree. (I did this for SHA-256 before.) This would allow Chromium to run the same NSS code as much as possible. It would require integrating this GHASH implementation with lib/freebl/gcm.c because the GHASH implementation in lib/freebl/gcm.c depends on lib/freebl/mpi, which we cannot copy (would be too big).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/agl@chromium.org/11175015/21001
Retried try job too often for step(s) build |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
