Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12457004:
Curve25519-donna changes. (Closed)
|
Created:
7 years, 9 months ago by ramant (doing other things) Modified:
7 years, 9 months ago CC:
chromium-reviews Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdded Curve25519-donna changes.
Added a wrapper class that implements the following API calls which for
Curve25519.
+ ScalarMult to compute the shared key.
+ ScalarBaseMult to get public key.
+ ConvertToPrivateKey returns a private key from random bytes.
Per agl/wtc, grabbed the rev 234205ff from the git repo
(https://github.com/agl/curve25519-donna/tree/234205ff1ecaf6b3c1dc76798a462c4293f31fdb)
and checked it in to crypto/ because that version has pure Google copyright.
R=wtc@chromium.org,agl@chromium.org,rsleevi@chromium.org
TEST=crypto unit tests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187074
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 14
Patch Set 5 : #
Total comments: 16
Patch Set 6 : #Patch Set 7 : #
Total comments: 37
Patch Set 8 : #Patch Set 9 : #
Total comments: 4
Patch Set 10 : #
Total comments: 4
Patch Set 11 : #Patch Set 12 : #
Total comments: 2
Patch Set 13 : #
Messages
Total messages: 22 (0 generated)
Review comments on patch set 4: Note: I removed agl as a reviewer temporarily. Let's add him and rsleevi as reviewers when the CL is in a polished shape. 1. I suggest imitating the design of crypto/p224.h, which declares another curve (NIST curve P-224). 2. I suggest avoiding patching curve25519-donna. This means manually declaring the curve25519_donna function in crypto/curve25519.cc. https://chromiumcodereview.appspot.com/12457004/diff/20001/crypto/crypto.gyp File crypto/crypto.gyp (right): https://chromiumcodereview.appspot.com/12457004/diff/20001/crypto/crypto.gyp#... crypto/crypto.gyp:225: 'p224.h', I highly recommend you model curve25519.{h,cc} after p224.{h,cc}. Use the same style, conventions, etc. https://chromiumcodereview.appspot.com/12457004/diff/20001/crypto/curve25519.cc File crypto/curve25519.cc (right): https://chromiumcodereview.appspot.com/12457004/diff/20001/crypto/curve25519.... crypto/curve25519.cc:23: static const unsigned char basepoint[32] = {9}; Nit: basepoint => kBasepoint https://chromiumcodereview.appspot.com/12457004/diff/20001/crypto/curve25519.h File crypto/curve25519.h (right): https://chromiumcodereview.appspot.com/12457004/diff/20001/crypto/curve25519.... crypto/curve25519.h:14: #define crypto_scalarmult_curve25519_SCALARBYTES 32 In a C++ header, these should be defined as enum constants or static constants of the Curve25519 class. https://chromiumcodereview.appspot.com/12457004/diff/20001/crypto/curve25519.... crypto/curve25519.h:25: const uint8* peer_public_key); This class should follow the recommendation of our Style Guide on function parameter ordering: When defining a function, parameter order is: inputs, then outputs. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Param... You should document the meaning of the 'int' return values. If it is used to report success/failure, use 'bool' instead. https://chromiumcodereview.appspot.com/12457004/diff/20001/crypto/curve25519.... crypto/curve25519.h:39: std::string* private_key); This function should operate on |mysecret| in place: static bool ConvertToPrivateKey(uint8* mysecret, size_t mysecret_size); I suggest shortening "mysecret" to "secret". https://chromiumcodereview.appspot.com/12457004/diff/20001/crypto/third_party... File crypto/third_party/curve25519-donna/README.chromium (right): https://chromiumcodereview.appspot.com/12457004/diff/20001/crypto/third_party... crypto/third_party/curve25519-donna/README.chromium:3: License: MPL 1.1/GPL 2.0/LGPL 2.1 The license is the BSD license. https://chromiumcodereview.appspot.com/12457004/diff/20001/crypto/third_party... crypto/third_party/curve25519-donna/README.chromium:8: "basictypes.h" instead of "stdint.h" I highly recommend we keep curve25519-donna.c as a C file and not patch it. Today, only compilers that don't have <stdint.h> are older versions of Visual C++. Chromium requires Visual C++ 2010 or later, and Visual C++ 2010 has <stdint.h>. So we should be able to just use <stdint.h>. Our patches to third-party code need to be maintained. So it is best if we don't need to patch. This includes not adding the curve25519-donna.h header file.
Hi Wan-Teh, Made the changes you have suggested. Had a one small #ifdef to curve-donna code. I am hoping we can up-source that change. Made kCryptoScalarMultCurve25519Bytes (instead of crypto_scalar_....). If we use crypto_scalar_... merging becomes easier from internal source code (but on the other hand we may not make many changes to Curve25519KeyExchange code as much, so it may be ok to use chrome's naming convention for these constants). What do you think of the changes? thanks raman https://chromiumcodereview.appspot.com/12457004/diff/20001/crypto/crypto.gyp File crypto/crypto.gyp (right): https://chromiumcodereview.appspot.com/12457004/diff/20001/crypto/crypto.gyp#... crypto/crypto.gyp:225: 'p224.h', On 2013/03/06 00:55:58, wtc wrote: > > I highly recommend you model curve25519.{h,cc} after > p224.{h,cc}. Use the same style, conventions, etc. Done. https://chromiumcodereview.appspot.com/12457004/diff/20001/crypto/curve25519.cc File crypto/curve25519.cc (right): https://chromiumcodereview.appspot.com/12457004/diff/20001/crypto/curve25519.... crypto/curve25519.cc:23: static const unsigned char basepoint[32] = {9}; On 2013/03/06 00:55:58, wtc wrote: > > Nit: basepoint => kBasepoint Made the kBasePoint name similar to p224.cc Done. https://chromiumcodereview.appspot.com/12457004/diff/20001/crypto/curve25519.h File crypto/curve25519.h (right): https://chromiumcodereview.appspot.com/12457004/diff/20001/crypto/curve25519.... crypto/curve25519.h:14: #define crypto_scalarmult_curve25519_SCALARBYTES 32 On 2013/03/06 00:55:58, wtc wrote: > > In a C++ header, these should be defined as enum constants > or static constants of the Curve25519 class. Done. https://chromiumcodereview.appspot.com/12457004/diff/20001/crypto/curve25519.... crypto/curve25519.h:25: const uint8* peer_public_key); On 2013/03/06 00:55:58, wtc wrote: > > This class should follow the recommendation of our Style > Guide on function parameter ordering: > When defining a function, parameter order is: inputs, then outputs. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Param... > > You should document the meaning of the 'int' return values. > If it is used to report success/failure, use 'bool' instead. Made ScalarMult* methods look similar to p224's methods. It doesn't need to return any value. Done. https://chromiumcodereview.appspot.com/12457004/diff/20001/crypto/curve25519.... crypto/curve25519.h:39: std::string* private_key); On 2013/03/06 00:55:58, wtc wrote: > > This function should operate on |mysecret| in place: > > static bool ConvertToPrivateKey(uint8* mysecret, > size_t mysecret_size); > > I suggest shortening "mysecret" to "secret". Done. https://chromiumcodereview.appspot.com/12457004/diff/20001/crypto/third_party... File crypto/third_party/curve25519-donna/README.chromium (right): https://chromiumcodereview.appspot.com/12457004/diff/20001/crypto/third_party... crypto/third_party/curve25519-donna/README.chromium:3: License: MPL 1.1/GPL 2.0/LGPL 2.1 On 2013/03/06 00:55:58, wtc wrote: > > The license is the BSD license. Done. https://chromiumcodereview.appspot.com/12457004/diff/20001/crypto/third_party... crypto/third_party/curve25519-donna/README.chromium:8: "basictypes.h" instead of "stdint.h" On 2013/03/06 00:55:58, wtc wrote: > > I highly recommend we keep curve25519-donna.c as a C file > and not patch it. > > Today, only compilers that don't have <stdint.h> are older > versions of Visual C++. Chromium requires Visual C++ 2010 > or later, and Visual C++ 2010 has <stdint.h>. So we should > be able to just use <stdint.h>. > > Our patches to third-party code need to be maintained. > So it is best if we don't need to patch. This includes > not adding the curve25519-donna.h header file. Done.
Review comments on patch set 5. https://chromiumcodereview.appspot.com/12457004/diff/13003/crypto/curve25519.cc File crypto/curve25519.cc (right): https://chromiumcodereview.appspot.com/12457004/diff/13003/crypto/curve25519.... crypto/curve25519.cc:9: using std::string; Remove the <stdint.h> header and remove the using std::string. https://chromiumcodereview.appspot.com/12457004/diff/13003/crypto/curve25519.... crypto/curve25519.cc:15: #ifdef __cplusplus Remove the #ifdef __cplusplus and #endif. This is a C++ file, so __cplusplus is known to be defined. No need to test it. https://chromiumcodereview.appspot.com/12457004/diff/13003/crypto/curve25519.h File crypto/curve25519.h (right): https://chromiumcodereview.appspot.com/12457004/diff/13003/crypto/curve25519.... crypto/curve25519.h:20: static const size_t kCryptoScalarMultCurve25519ScalarBytes = 32; Shorten these constants to just kBytes and kScalarBytes. The namespace "curve25519" makes the "Curve25519" part unnecessary, and I think the "CryptoScalarMult" part is not useful (it comes from the name of the constants in nacl). https://chromiumcodereview.appspot.com/12457004/diff/13003/crypto/curve25519.... crypto/curve25519.h:22: // ScalarMultiply computes the |shared_key| from |private_key| and Typo: ScalarMultiply => ScalarMult https://chromiumcodereview.appspot.com/12457004/diff/13003/crypto/curve25519.... crypto/curve25519.h:29: // ScalarMultiply computes the |public_key| from |private_key| and a basepoint Typo: ScalarMultiply => ScalarBaseMult https://chromiumcodereview.appspot.com/12457004/diff/13003/crypto/curve25519.... crypto/curve25519.h:39: bool CRYPTO_EXPORT ConvertToPrivateKey(uint8* secret, size_t secret_size); Although it is a good idea to require the caller to specify the buffer size, it is inconsistent that we only do that in this function. https://chromiumcodereview.appspot.com/12457004/diff/13003/crypto/curve25519_... File crypto/curve25519_unittest.cc (right): https://chromiumcodereview.appspot.com/12457004/diff/13003/crypto/curve25519_... crypto/curve25519_unittest.cc:12: using std::string; I would avoid this using statement. Is this file shared with the internal tree? https://chromiumcodereview.appspot.com/12457004/diff/13003/crypto/third_party... File crypto/third_party/curve25519-donna/curve25519-donna.c (right): https://chromiumcodereview.appspot.com/12457004/diff/13003/crypto/third_party... crypto/third_party/curve25519-donna/curve25519-donna.c:54: #endif I guess this is the #ifdef you added. You need to add this as a patch file in crypto/third_party/curve25519-donna/patches and describe the patch in crypto/third_party/curve25519-donna/README.chromium. Please see net/third_party/nss/README.chromium and net/third_party/nss/patches as an example.
Hi Wan-Teh, Made all the changes you have suggested. Minor change: Deleted some unnecessary code from the unittests. PTAL. https://codereview.chromium.org/12457004/diff/13003/crypto/curve25519.cc File crypto/curve25519.cc (right): https://codereview.chromium.org/12457004/diff/13003/crypto/curve25519.cc#newc... crypto/curve25519.cc:9: using std::string; On 2013/03/06 19:21:07, wtc wrote: > > Remove the <stdint.h> header and remove the using std::string. Done. https://codereview.chromium.org/12457004/diff/13003/crypto/curve25519.cc#newc... crypto/curve25519.cc:15: #ifdef __cplusplus On 2013/03/06 19:21:07, wtc wrote: > > Remove the #ifdef __cplusplus and #endif. This is a C++ file, > so __cplusplus is known to be defined. No need to test it. Done. https://codereview.chromium.org/12457004/diff/13003/crypto/curve25519.h File crypto/curve25519.h (right): https://codereview.chromium.org/12457004/diff/13003/crypto/curve25519.h#newco... crypto/curve25519.h:20: static const size_t kCryptoScalarMultCurve25519ScalarBytes = 32; On 2013/03/06 19:21:07, wtc wrote: > > Shorten these constants to just kBytes and kScalarBytes. > The namespace "curve25519" makes the "Curve25519" part > unnecessary, and I think the "CryptoScalarMult" part is > not useful (it comes from the name of the constants in > nacl). Done. https://codereview.chromium.org/12457004/diff/13003/crypto/curve25519.h#newco... crypto/curve25519.h:22: // ScalarMultiply computes the |shared_key| from |private_key| and On 2013/03/06 19:21:07, wtc wrote: > > Typo: ScalarMultiply => ScalarMult Done. https://codereview.chromium.org/12457004/diff/13003/crypto/curve25519.h#newco... crypto/curve25519.h:29: // ScalarMultiply computes the |public_key| from |private_key| and a basepoint On 2013/03/06 19:21:07, wtc wrote: > > Typo: ScalarMultiply => ScalarBaseMult Done. https://codereview.chromium.org/12457004/diff/13003/crypto/curve25519.h#newco... crypto/curve25519.h:39: bool CRYPTO_EXPORT ConvertToPrivateKey(uint8* secret, size_t secret_size); On 2013/03/06 19:21:07, wtc wrote: > > Although it is a good idea to require the caller to specify > the buffer size, it is inconsistent that we only do that in > this function. Made it consistent with rest of the code. Removed the secret_size parameter. Done. https://codereview.chromium.org/12457004/diff/13003/crypto/curve25519_unittes... File crypto/curve25519_unittest.cc (right): https://codereview.chromium.org/12457004/diff/13003/crypto/curve25519_unittes... crypto/curve25519_unittest.cc:12: using std::string; On 2013/03/06 19:21:07, wtc wrote: > > I would avoid this using statement. Is this file shared with > the internal tree? Done. https://codereview.chromium.org/12457004/diff/13003/crypto/third_party/curve2... File crypto/third_party/curve25519-donna/curve25519-donna.c (right): https://codereview.chromium.org/12457004/diff/13003/crypto/third_party/curve2... crypto/third_party/curve25519-donna/curve25519-donna.c:54: #endif On 2013/03/06 19:21:07, wtc wrote: > > I guess this is the #ifdef you added. You need to add this > as a patch file in crypto/third_party/curve25519-donna/patches > and describe the patch in crypto/third_party/curve25519-donna/README.chromium. > > Please see net/third_party/nss/README.chromium and > net/third_party/nss/patches as an example. agl added the above change to https://github.com/agl/curve25519-donna/blob/master/curve25519-donna.c This file is same as what is in the github repository.
+rsleevi
https://codereview.chromium.org/12457004/diff/46007/crypto/crypto.gyp File crypto/crypto.gyp (right): https://codereview.chromium.org/12457004/diff/46007/crypto/crypto.gyp#newcode189 crypto/crypto.gyp:189: 'curve25519.cc', Should this be named curve_25519, per Chromium file naming? I'm not sure whether Curve-25519 or Curve25519 is the preferred naming, just pointing out that it "looks" like two words. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.cc File crypto/curve25519.cc (right): https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.cc#newc... crypto/curve25519.cc:22: static const unsigned char kBasePoint[32] = {9}; Can you provide more comments explaining the source/derivation of this parameter (which is 9 followed by 31 zeros). https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.cc#newc... crypto/curve25519.cc:33: secret[31] |= 64; Does our version not support curve25519_clamp? At least reference the section where this is described, or ideally where/why in the paper it's described. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.h File crypto/curve25519.h (right): https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.h#newco... crypto/curve25519.h:15: // Curve25519 implements an elliptic curve group, commonly known as Curve25519 The comma here is weird, when you read just the part before the comma "Curve25519 implements an elliptic curve group" ... Perhaps // Curve25519 implements the elliptic curve group described at [url], commonly known as Curve25519. Still, it seems like we want something more substantive than just the URL link. // Curve25519 implements the elliptic curve group known as // Curve25519, as described in "Curve 25519: new Diffie-Hellman Speed Records", by D.J. Bernstein. Additional information is available at [URL] I want to make sure that even if the URL isn't available, it's still available - hence the reference to the (published) paper's title. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.h#newco... crypto/curve25519.h:20: static const size_t kScalarBytes = 32; nit: Comments explaining what these constants are and what they're used for. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.h#newco... crypto/curve25519.h:24: // http://cr.yp.to/ecdh.html. I don't know if this is really a good comment description - that section just describes "How to call this API" - not what it does. The same for ScalarBaseMult. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.h#newco... crypto/curve25519.h:37: void CRYPTO_EXPORT ConvertToPrivateKey(uint8* secret); For all of these functions, you could provide clear descriptions about the expected variable lengths. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519_unittes... File crypto/curve25519_unittest.cc (right): https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519_unittes... crypto/curve25519_unittest.cc:19: crypto::RandBytes(alice_private_key, sizeof(alice_private_key)); This creates a non-deterministic unit test (as does line 26). Additionally, it is not clear how or why the magic value 5 was chosen as the loop - why not simply --gtest_repeat=5? Better comments explaining that motivation is good - and perhaps a few fixed/KATs that ensure identities hold (eg: for forwards/backwards compatibility, if it ever becomes an issue) The loop seems unnecessary, and if you are going to use it, you should be using SCOPED_TRACE as well - but I would recommend not using it. https://codereview.chromium.org/12457004/diff/46007/crypto/third_party/curve2... File crypto/third_party/curve25519-donna/README (right): https://codereview.chromium.org/12457004/diff/46007/crypto/third_party/curve2... crypto/third_party/curve25519-donna/README:1: See http://code.google.com/p/curve25519-donna/ for details. I do not believe you should be adding this to crypto/third_party You should be adding this via DEPS. You also need to have security-team review, as per discussion on Chromium-dev and described at http://www.chromium.org/developers/adding-3rd-party-libraries (Namely, all third_party additions go through Chrome Eng Review AND open-source-third-party-reviews@google.com ) https://codereview.chromium.org/12457004/diff/46007/crypto/third_party/curve2... File crypto/third_party/curve25519-donna/README.chromium (right): https://codereview.chromium.org/12457004/diff/46007/crypto/third_party/curve2... crypto/third_party/curve25519-donna/README.chromium:2: URL: https://github.com/agl/curve25519-donna Why are we pulling Adam's version, rather than upstream? https://codereview.chromium.org/12457004/diff/46007/crypto/third_party/curve2... crypto/third_party/curve25519-donna/README.chromium:3: License: BSD license Please follow the more substantive version of this template Name URL: Version: [or Revision:, if not versioned] License: License File: [optional] Security Critical: Yes Description:
Patch set 7 LGTM. Please wait for rsleevi's approval. agl: I have a question for you below. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.cc File crypto/curve25519.cc (right): https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.cc#newc... crypto/curve25519.cc:18: curve25519_donna(shared_key, private_key, peer_public_key); agl: |shared_key| is the x coordinate of the result. As the output of ECDH key agreement, we need to specify how to serialize the x coordinate as a byte sequence. Do you know whether the byte sequence is in little-endian or big-endian order? The usual byte order for ECDH shared secret over a prime field is big-endian. See NIST SP 800-56A, Appendix C.1 and C.2. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.h File crypto/curve25519.h (right): https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.h#newcode8 crypto/curve25519.h:8: #include <string> Remove this header. It is no longer necessary. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.h#newco... crypto/curve25519.h:20: static const size_t kScalarBytes = 32; Please document what these two constants are. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.h#newco... crypto/curve25519.h:34: // ConvertToPrivateKey converts |secret| into a private key, suitable Please note that the conversion is done in place, i.e., |secret| is an in/out argument.
https://codereview.chromium.org/12457004/diff/46007/crypto/crypto.gyp File crypto/crypto.gyp (right): https://codereview.chromium.org/12457004/diff/46007/crypto/crypto.gyp#newcode189 crypto/crypto.gyp:189: 'curve25519.cc', On 2013/03/06 21:18:32, Ryan Sleevi wrote: > Should this be named curve_25519, per Chromium file naming? I'm not sure whether > Curve-25519 or Curve25519 is the preferred naming, just pointing out that it > "looks" like two words. "curve25519" fits every other use. I think it's preferable. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.cc File crypto/curve25519.cc (right): https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.cc#newc... crypto/curve25519.cc:18: curve25519_donna(shared_key, private_key, peer_public_key); On 2013/03/06 21:26:44, wtc wrote: > As the output of ECDH key agreement, we need to specify how > to serialize the x coordinate as a byte sequence. Do you > know whether the byte sequence is in little-endian or > big-endian order? Curve25519 is specified in terms of byte strings, not numbers, so all implementations take and return the same sequence of bits. So the byte order is implicitly specified as in, say, SHA1. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.cc#newc... crypto/curve25519.cc:22: static const unsigned char kBasePoint[32] = {9}; On 2013/03/06 21:18:32, Ryan Sleevi wrote: > Can you provide more comments explaining the source/derivation of this parameter > (which is 9 followed by 31 zeros). It's defined as a magic value by the API. (It happens to be the little-endian version of '9'). I guess we can just cite the paper. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.cc#newc... crypto/curve25519.cc:33: secret[31] |= 64; On 2013/03/06 21:18:32, Ryan Sleevi wrote: > Does our version not support curve25519_clamp? _clamp has been removed from the API as I recall. Although this triplet of lines is harmless, it's also superfluous and can be removed. The clamping is performed inside the scalar-mult function. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.h File crypto/curve25519.h (right): https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.h#newco... crypto/curve25519.h:20: static const size_t kScalarBytes = 32; On 2013/03/06 21:18:32, Ryan Sleevi wrote: > nit: Comments explaining what these constants are and what they're used for. They can be moved into the .cc as well, now that I look at it. (They aren't part of the API.) For comments, since they are taken from the NaCl API: // kBytes is the number of bytes in the result of the Diffie-Hellman operation, which is an element of GF(2^255-19). // kScalarBytes is the number of bytes in an element of the scalar field: GF(2^252 + 27742317777372353535851937790883648493). https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519_unittes... File crypto/curve25519_unittest.cc (right): https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519_unittes... crypto/curve25519_unittest.cc:19: crypto::RandBytes(alice_private_key, sizeof(alice_private_key)); On 2013/03/06 21:18:32, Ryan Sleevi wrote: > This creates a non-deterministic unit test (as does line 26). Additionally, it > is not clear how or why the magic value 5 was chosen as the loop - why not > simply --gtest_repeat=5? > > Better comments explaining that motivation is good - and perhaps a few > fixed/KATs that ensure identities hold (eg: for forwards/backwards > compatibility, if it ever becomes an issue) > > The loop seems unnecessary, and if you are going to use it, you should be using > SCOPED_TRACE as well - but I would recommend not using it. Removing the loop is fine. This is coming from the server side of the code where I ran it for a few hundred thousand iterations and then turned it down, but not to zero. The standard test for curve25519 runs for too long to be reasonable in a unittest, but an easy test is to start with {1}, run scalar_base_mult and then feed the answer back into scalar_base_mult as another scalar for n iterations and then just check the final result. https://codereview.chromium.org/12457004/diff/46007/crypto/third_party/curve2... File crypto/third_party/curve25519-donna/README (right): https://codereview.chromium.org/12457004/diff/46007/crypto/third_party/curve2... crypto/third_party/curve25519-donna/README:1: See http://code.google.com/p/curve25519-donna/ for details. On 2013/03/06 21:18:32, Ryan Sleevi wrote: > I do not believe you should be adding this to crypto/third_party > > You should be adding this via DEPS. You also need to have security-team review, > as per discussion on Chromium-dev and described at > http://www.chromium.org/developers/adding-3rd-party-libraries > > (Namely, all third_party additions go through Chrome Eng Review AND > mailto:open-source-third-party-reviews@google.com ) Alternatively, you can grab rev 234205ff from the git repo and check it in to crypto/ directly. That version has pure Google copyright and works fine. If you do, please let me know and I'll do another pass of the code as a follow up. https://codereview.chromium.org/12457004/diff/46007/crypto/third_party/curve2... File crypto/third_party/curve25519-donna/README.chromium (right): https://codereview.chromium.org/12457004/diff/46007/crypto/third_party/curve2... crypto/third_party/curve25519-donna/README.chromium:2: URL: https://github.com/agl/curve25519-donna On 2013/03/06 21:18:32, Ryan Sleevi wrote: > Why are we pulling Adam's version, rather than upstream? Mine is upstream. NaCl includes donna-c64 but has an assembly version for x86. The assembly version is much faster on x86, but getting it building on Windows is a task for later.
https://codereview.chromium.org/12457004/diff/46007/crypto/third_party/curve2... File crypto/third_party/curve25519-donna/README (right): https://codereview.chromium.org/12457004/diff/46007/crypto/third_party/curve2... crypto/third_party/curve25519-donna/README:1: See http://code.google.com/p/curve25519-donna/ for details. On 2013/03/06 21:47:50, agl wrote: > > Alternatively, you can grab rev 234205ff from the git repo and check it in to > crypto/ directly. That version has pure Google copyright and works fine. I agree this is a good interim solution because Google owns the copyright and the license (http://opensource.org/licenses/BSD-3-Clause) is the same license that Chromium itself uses (src/LICENSE). We should still start the third-party review process for the current version of the curve25519-donna code. https://codereview.chromium.org/12457004/diff/46007/crypto/third_party/curve2... crypto/third_party/curve25519-donna/README:1: See http://code.google.com/p/curve25519-donna/ for details. On 2013/03/06 21:18:32, Ryan Sleevi wrote: > I do not believe you should be adding this to crypto/third_party > > You should be adding this via DEPS. I suggested crypto/third_party because I wanted to make crypto code easy to discover in the source tree. This was more important before because originally the consumer of this code, which lives in net/quic, called the curve25519-donna function directly. Now that we have the curve25519.{h,cc} wrapper in src/crypto, it is less important exactly where the curve25519-donna code is. If you think we should not make an exception for this code, we'll move it to deps/third_party and pull it via src/DEPS.
Tried to address all your comments. Implemented the interim solution suggested by agl/wtc. Removed third_party/curve25519 (will start the third-party review process for the current version of the curve25519-donna code). PTAL. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.cc File crypto/curve25519.cc (right): https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.cc#newc... crypto/curve25519.cc:18: curve25519_donna(shared_key, private_key, peer_public_key); On 2013/03/06 21:47:50, agl wrote: > On 2013/03/06 21:26:44, wtc wrote: > > As the output of ECDH key agreement, we need to specify how > > to serialize the x coordinate as a byte sequence. Do you > > know whether the byte sequence is in little-endian or > > big-endian order? > > Curve25519 is specified in terms of byte strings, not numbers, so all > implementations take and return the same sequence of bits. So the byte order is > implicitly specified as in, say, SHA1. Done. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.cc#newc... crypto/curve25519.cc:18: curve25519_donna(shared_key, private_key, peer_public_key); On 2013/03/06 21:47:50, agl wrote: > On 2013/03/06 21:26:44, wtc wrote: > > As the output of ECDH key agreement, we need to specify how > > to serialize the x coordinate as a byte sequence. Do you > > know whether the byte sequence is in little-endian or > > big-endian order? > > Curve25519 is specified in terms of byte strings, not numbers, so all > implementations take and return the same sequence of bits. So the byte order is > implicitly specified as in, say, SHA1. Added agl's comments to the file. Hope that is ok. Done. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.cc#newc... crypto/curve25519.cc:22: static const unsigned char kBasePoint[32] = {9}; On 2013/03/06 21:47:50, agl wrote: > On 2013/03/06 21:18:32, Ryan Sleevi wrote: > > Can you provide more comments explaining the source/derivation of this > parameter > > (which is 9 followed by 31 zeros). > > It's defined as a magic value by the API. (It happens to be the little-endian > version of '9'). I guess we can just cite the paper. Done. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.cc#newc... crypto/curve25519.cc:33: secret[31] |= 64; On 2013/03/06 21:18:32, Ryan Sleevi wrote: > Does our version not support curve25519_clamp? > > At least reference the section where this is described, or ideally where/why in > the paper it's described. Added a reference to the section in the paper. Done. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.h File crypto/curve25519.h (right): https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.h#newcode8 crypto/curve25519.h:8: #include <string> On 2013/03/06 21:26:44, wtc wrote: > > Remove this header. It is no longer necessary. Done. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.h#newco... crypto/curve25519.h:15: // Curve25519 implements an elliptic curve group, commonly known as Curve25519 On 2013/03/06 21:18:32, Ryan Sleevi wrote: > The comma here is weird, when you read just the part before the comma > > "Curve25519 implements an elliptic curve group" > > ... > > Perhaps > > // Curve25519 implements the elliptic curve group described at [url], commonly > known as Curve25519. > > Still, it seems like we want something more substantive than just the URL link. > > // Curve25519 implements the elliptic curve group known as > // Curve25519, as described in "Curve 25519: new Diffie-Hellman Speed Records", > by D.J. Bernstein. Additional information is available at [URL] > > I want to make sure that even if the URL isn't available, it's still available - > hence the reference to the (published) paper's title. Done. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.h#newco... crypto/curve25519.h:20: static const size_t kScalarBytes = 32; On 2013/03/06 21:47:50, agl wrote: > On 2013/03/06 21:18:32, Ryan Sleevi wrote: > > nit: Comments explaining what these constants are and what they're used for. > > They can be moved into the .cc as well, now that I look at it. (They aren't part > of the API.) > > For comments, since they are taken from the NaCl API: > > // kBytes is the number of bytes in the result of the Diffie-Hellman operation, > which is an element of GF(2^255-19). > > // kScalarBytes is the number of bytes in an element of the scalar field: > GF(2^252 + 27742317777372353535851937790883648493). Thanks very much agl. Added the above comments. Done. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.h#newco... crypto/curve25519.h:24: // http://cr.yp.to/ecdh.html. On 2013/03/06 21:18:32, Ryan Sleevi wrote: > I don't know if this is really a good comment description - that section just > describes "How to call this API" - not what it does. > > The same for ScalarBaseMult. Done. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.h#newco... crypto/curve25519.h:34: // ConvertToPrivateKey converts |secret| into a private key, suitable On 2013/03/06 21:26:44, wtc wrote: > > Please note that the conversion is done in place, i.e., > |secret| is an in/out argument. Done. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519.h#newco... crypto/curve25519.h:37: void CRYPTO_EXPORT ConvertToPrivateKey(uint8* secret); On 2013/03/06 21:18:32, Ryan Sleevi wrote: > For all of these functions, you could provide clear descriptions about the > expected variable lengths. Done. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519_unittes... File crypto/curve25519_unittest.cc (right): https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519_unittes... crypto/curve25519_unittest.cc:19: crypto::RandBytes(alice_private_key, sizeof(alice_private_key)); On 2013/03/06 21:18:32, Ryan Sleevi wrote: > This creates a non-deterministic unit test (as does line 26). Additionally, it > is not clear how or why the magic value 5 was chosen as the loop - why not > simply --gtest_repeat=5? > > Better comments explaining that motivation is good - and perhaps a few > fixed/KATs that ensure identities hold (eg: for forwards/backwards > compatibility, if it ever becomes an issue) > > The loop seems unnecessary, and if you are going to use it, you should be using > SCOPED_TRACE as well - but I would recommend not using it. Removed the loop. Used a fixed private key. https://codereview.chromium.org/12457004/diff/46007/crypto/curve25519_unittes... crypto/curve25519_unittest.cc:19: crypto::RandBytes(alice_private_key, sizeof(alice_private_key)); On 2013/03/06 21:47:50, agl wrote: > On 2013/03/06 21:18:32, Ryan Sleevi wrote: > > This creates a non-deterministic unit test (as does line 26). Additionally, it > > is not clear how or why the magic value 5 was chosen as the loop - why not > > simply --gtest_repeat=5? > > > > Better comments explaining that motivation is good - and perhaps a few > > fixed/KATs that ensure identities hold (eg: for forwards/backwards > > compatibility, if it ever becomes an issue) > > > > The loop seems unnecessary, and if you are going to use it, you should be > using > > SCOPED_TRACE as well - but I would recommend not using it. > > Removing the loop is fine. This is coming from the server side of the code where > I ran it for a few hundred thousand iterations and then turned it down, but not > to zero. > > The standard test for curve25519 runs for too long to be reasonable in a > unittest, but an easy test is to start with {1}, run scalar_base_mult and then > feed the answer back into scalar_base_mult as another scalar for n iterations > and then just check the final result. Changed the test to feed back answer from scalar_base_mult to modify the private key and ran the test again. Did this for 5 times (derived this test from agl's test-curve25519.c). Done.
Patch set 9 LGTM. Please wait for agl and rsleevi's approval. https://codereview.chromium.org/12457004/diff/60001/crypto/curve25519.h File crypto/curve25519.h (right): https://codereview.chromium.org/12457004/diff/60001/crypto/curve25519.h#newco... crypto/curve25519.h:24: // GF(2^252 + 27742317777372353535851937790883648493). Nit: the scalar of a point multiplication is a regular integer. I believe 2^252 + 27742317777372353535851937790883648493 is the group's order.
lgtm https://codereview.chromium.org/12457004/diff/60001/crypto/curve25519.h File crypto/curve25519.h (right): https://codereview.chromium.org/12457004/diff/60001/crypto/curve25519.h#newco... crypto/curve25519.h:24: // GF(2^252 + 27742317777372353535851937790883648493). On 2013/03/08 01:30:15, wtc wrote: > > Nit: the scalar of a point multiplication is a regular > integer. I believe > 2^252 + 27742317777372353535851937790883648493 is the > group's order. Right, but the scalar is a field too. The scalars n and n+2^252+ 27742317777372353535851937790883648493 are equivalent, and we can do arithmetic on scalars. (We don't here, because this is just DH, but we would for signatures.) https://codereview.chromium.org/12457004/diff/60001/crypto/curve25519.h#newco... crypto/curve25519.h:49: void CRYPTO_EXPORT ConvertToPrivateKey(uint8* secret); Again, this is harmless, but also superfluous because the clamping is done in curve25519_donna.
Deleted ConvertToPrivateKey, because curve25519_donna(0 has the same code (it does the clamping). PTAL. https://codereview.chromium.org/12457004/diff/60001/crypto/curve25519.h File crypto/curve25519.h (right): https://codereview.chromium.org/12457004/diff/60001/crypto/curve25519.h#newco... crypto/curve25519.h:49: void CRYPTO_EXPORT ConvertToPrivateKey(uint8* secret); On 2013/03/08 15:17:33, agl wrote: > Again, this is harmless, but also superfluous because the clamping is done in > curve25519_donna. I agree with you. Deleted this method because curve25519_donna() is doing the same thing as this function. Done.
Mostly LGTM, but still a question about the test and two nits. https://codereview.chromium.org/12457004/diff/9003/crypto/curve25519.h File crypto/curve25519.h (right): https://codereview.chromium.org/12457004/diff/9003/crypto/curve25519.h#newcode42 crypto/curve25519.h:42: void CRYPTO_EXPORT ScalarBaseMult(const uint8* private_key, uint8* public_key); Place CRYPTO_EXPORT before the return type CRYPTO_EXPORT void ScalarBaseMult(...) https://codereview.chromium.org/12457004/diff/9003/crypto/curve25519_unittest.cc File crypto/curve25519_unittest.cc (right): https://codereview.chromium.org/12457004/diff/9003/crypto/curve25519_unittest... crypto/curve25519_unittest.cc:24: // long time, thus turned it down to 5 times). I can understand the thousands of iterations test, because you originally started off with a random value, and so the thousands of iterations was trying to permute different inputs. But you now run this test with a fixed set of inputs (line 26-27), so every other part of the test is also deterministic. So it's not clear why you're repeating this test. It also seems like you're trying to duplicate the test from the C code, which shouldn't be necessary, since things should only break if the C code changes. Have I missed anything?
PTAL. thanks. https://codereview.chromium.org/12457004/diff/9003/crypto/curve25519.h File crypto/curve25519.h (right): https://codereview.chromium.org/12457004/diff/9003/crypto/curve25519.h#newcode42 crypto/curve25519.h:42: void CRYPTO_EXPORT ScalarBaseMult(const uint8* private_key, uint8* public_key); On 2013/03/08 18:27:33, Ryan Sleevi wrote: > Place CRYPTO_EXPORT before the return type > > CRYPTO_EXPORT void ScalarBaseMult(...) Done. https://codereview.chromium.org/12457004/diff/9003/crypto/curve25519_unittest.cc File crypto/curve25519_unittest.cc (right): https://codereview.chromium.org/12457004/diff/9003/crypto/curve25519_unittest... crypto/curve25519_unittest.cc:24: // long time, thus turned it down to 5 times). On 2013/03/08 18:27:33, Ryan Sleevi wrote: > I can understand the thousands of iterations test, because you originally > started off with a random value, and so the thousands of iterations was trying > to permute different inputs. > > But you now run this test with a fixed set of inputs (line 26-27), so every > other part of the test is also deterministic. So it's not clear why you're > repeating this test. > > It also seems like you're trying to duplicate the test from the C code, which > shouldn't be necessary, since things should only break if the C code changes. > > Have I missed anything? Removed the 5 times iteration. Done.
LGTM. Tiny nit, take it or leave it. https://codereview.chromium.org/12457004/diff/81002/crypto/curve25519_unittes... File crypto/curve25519_unittest.cc (right): https://codereview.chromium.org/12457004/diff/81002/crypto/curve25519_unittes... crypto/curve25519_unittest.cc:47: ASSERT_EQ(alice_shared, bob_shared); Since |bob_shared| and |alice_shared| may contain arbitrary bytes, you may not wish to use ASSERT_EQ (which will log the byte strings and their differences). ASSERT_EQ(0, memcmp(alice_shared, bob_shared, alice_shared.size())) This is minor though.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/12457004/81002
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
rsleevi: made the changes you have suggested. PTAL. :-). https://codereview.chromium.org/12457004/diff/81002/crypto/curve25519_unittes... File crypto/curve25519_unittest.cc (right): https://codereview.chromium.org/12457004/diff/81002/crypto/curve25519_unittes... crypto/curve25519_unittest.cc:47: ASSERT_EQ(alice_shared, bob_shared); On 2013/03/08 18:37:27, Ryan Sleevi wrote: > Since |bob_shared| and |alice_shared| may contain arbitrary bytes, you may not > wish to use ASSERT_EQ (which will log the byte strings and their differences). > > ASSERT_EQ(0, memcmp(alice_shared, bob_shared, alice_shared.size())) > > This is minor though. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/12457004/93001
Message was sent while issue was closed.
Change committed as 187074 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
