Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
26911006:
crypto: Implement ECSignatureCreatorImpl for OpenSSL (Closed)
|
Created:
7 years, 2 months ago by digit1 Modified:
7 years, 1 month ago CC:
chromium-reviews Base URL:
https://codereview.chromium.org/27195002/ Visibility:
Public. |
Descriptioncrypto: Implement ECSignatureCreatorImpl for OpenSSL
BUG=306176
TEST=crypto_unittests --gtest_filter=ECSignatureCreatorTest.*
R=rsleevi@chromium.org,agl@chromium.org,wtc@chromium.org
Patch Set 1 #
Total comments: 6
Patch Set 2 : man BN_num_bytes #Patch Set 3 : Formatting #
Total comments: 4
Patch Set 4 : Rebase + nits. #
Total comments: 10
Patch Set 5 : wtc fixes. #
Total comments: 1
Patch Set 6 : Fix Android build #Patch Set 7 : Fix mac build. #Patch Set 8 : Trivial rebase #
Messages
Total messages: 15 (0 generated)
https://codereview.chromium.org/26911006/diff/1/crypto/ec_signature_creator_o... File crypto/ec_signature_creator_openssl.cc (right): https://codereview.chromium.org/26911006/diff/1/crypto/ec_signature_creator_o... crypto/ec_signature_creator_openssl.cc:32: // output format. To work-around this, perform the operations separately. The output format of this function is a DER encoded signature. Don't the EVP_DigestSign functions do that too? https://codereview.chromium.org/26911006/diff/1/crypto/ec_signature_creator_o... crypto/ec_signature_creator_openssl.cc:39: if (!SHA256_Init(&hash_ctx) || !SHA256_Update(&hash_ctx, data, data_len) || The three terms of the disjunction should each have their own line. https://codereview.chromium.org/26911006/diff/1/crypto/ec_signature_creator_o... crypto/ec_signature_creator_openssl.cc:90: BN_bn2bin(ecdsa_sig.get()->r, &result[0]); Isn't this wrong 1/128 of the time? If r or s are such that they are only 31 bytes long then they'll be written to the start of the 32 byte space. But, in that case, the padding zero should be at the beginning, because it's big endian. BN_bn2bin(ecdsa_sig.get()->r, &result[kMaxBytesPerBN - BN_num_bytes(ecdsa_sig.get()->r); BN_bn2bin(ecdsa_sig.get()->s, &result[2*kMaxBytesPerBN - BN_num_bytes(ecdsa_sig.get()->s);
https://codereview.chromium.org/26911006/diff/1/crypto/ec_signature_creator_o... File crypto/ec_signature_creator_openssl.cc (right): https://codereview.chromium.org/26911006/diff/1/crypto/ec_signature_creator_o... crypto/ec_signature_creator_openssl.cc:32: // output format. To work-around this, perform the operations separately. I must admit that the documentation for these functions doesn't mention any specific output format (nor EVP_PKEY_sign() which they use internally), I thus preferred not assuming anything about them. It seems to work though, so I've changed the code to do just that, thanks :) https://codereview.chromium.org/26911006/diff/1/crypto/ec_signature_creator_o... crypto/ec_signature_creator_openssl.cc:39: if (!SHA256_Init(&hash_ctx) || !SHA256_Update(&hash_ctx, data, data_len) || That was the output of "git cl format" :-/ Not relevant anymore though. https://codereview.chromium.org/26911006/diff/1/crypto/ec_signature_creator_o... crypto/ec_signature_creator_openssl.cc:90: BN_bn2bin(ecdsa_sig.get()->r, &result[0]); I admit I had no idea. I've implemented your recommendation though.
lgtm https://codereview.chromium.org/26911006/diff/8001/crypto/ec_signature_creato... File crypto/ec_signature_creator_openssl.cc (right): https://codereview.chromium.org/26911006/diff/8001/crypto/ec_signature_creato... crypto/ec_signature_creator_openssl.cc:39: if (!EVP_DigestSignFinal(ctx.get(), &(*signature)[0], &sig_len)) &signature->front() might be easier on the eyes? Up to you. https://codereview.chromium.org/26911006/diff/8001/crypto/ec_signature_creato... crypto/ec_signature_creator_openssl.cc:63: // NOTE: Can't really check for equality here since sometimes the value Move this comment down four lines?
https://codereview.chromium.org/26911006/diff/8001/crypto/ec_signature_creato... File crypto/ec_signature_creator_openssl.cc (right): https://codereview.chromium.org/26911006/diff/8001/crypto/ec_signature_creato... crypto/ec_signature_creator_openssl.cc:39: if (!EVP_DigestSignFinal(ctx.get(), &(*signature)[0], &sig_len)) On 2013/10/15 16:24:08, agl wrote: > &signature->front() might be easier on the eyes? Up to you. Done. https://codereview.chromium.org/26911006/diff/8001/crypto/ec_signature_creato... crypto/ec_signature_creator_openssl.cc:63: // NOTE: Can't really check for equality here since sometimes the value On 2013/10/15 16:24:08, agl wrote: > Move this comment down four lines? Done.
lgtm
lgtm https://codereview.chromium.org/26911006/diff/13001/crypto/ec_signature_creat... File crypto/ec_signature_creator_openssl.cc (right): https://codereview.chromium.org/26911006/diff/13001/crypto/ec_signature_creat... crypto/ec_signature_creator_openssl.cc:20: : key_(key), signature_len_(0) { STYLE: per the style guide, I believe this should be one-per-line because the ctor body itself is multi-line. defer to clang-format here.
https://codereview.chromium.org/26911006/diff/13001/crypto/ec_signature_creat... File crypto/ec_signature_creator_openssl.cc (right): https://codereview.chromium.org/26911006/diff/13001/crypto/ec_signature_creat... crypto/ec_signature_creator_openssl.cc:20: : key_(key), signature_len_(0) { Thanks, but that's already the output of "git cl format" which invokes clang-format. Even if I put the second member on a second line, the tool will put it back there.
Patch set 4 LGTM. https://codereview.chromium.org/26911006/diff/13001/crypto/ec_signature_creat... File crypto/ec_signature_creator_openssl.cc (right): https://codereview.chromium.org/26911006/diff/13001/crypto/ec_signature_creat... crypto/ec_signature_creator_openssl.cc:36: return false; Nit: we usually add curly braces if the conditional expression spans multiple lines. If this is what "git cl format" outputs, then it's fine. https://codereview.chromium.org/26911006/diff/13001/crypto/ec_signature_creat... crypto/ec_signature_creator_openssl.cc:40: return false; Nit: it may be a good idea to call signature->resize(sig_len) again just in case the first EVP_DigestSignFinal call returns a generous estimate of the signature size. https://codereview.chromium.org/26911006/diff/13001/crypto/ec_signature_creat... crypto/ec_signature_creator_openssl.cc:50: reinterpret_cast<const unsigned char*>(der_sig.front()); This reinterpret_cast is not necessary because uint8 is the same as unsigned char. https://codereview.chromium.org/26911006/diff/13001/crypto/ec_signature_creat... crypto/ec_signature_creator_openssl.cc:58: const size_t kMaxBytesPerBN = (kMaxBitsPerBN + 7) / 8; Nit: It is OK to just define kMaxBytesPerBN as 32 and omit kMaxBitsPerBN. 256 bits = 32 bytes is common in crypto code. If you make this change, also change "two 256-bit vectors" to "two 32-byte vectors" or "two 32-byte big integers".
https://codereview.chromium.org/26911006/diff/13001/crypto/ec_signature_creat... File crypto/ec_signature_creator_openssl.cc (right): https://codereview.chromium.org/26911006/diff/13001/crypto/ec_signature_creat... crypto/ec_signature_creator_openssl.cc:36: return false; I've added them. Apparently the formatter doesn't seem to care here. https://codereview.chromium.org/26911006/diff/13001/crypto/ec_signature_creat... crypto/ec_signature_creator_openssl.cc:40: return false; I didn't notice this in the function's documentation, this indeed seems important. Fixed. https://codereview.chromium.org/26911006/diff/13001/crypto/ec_signature_creat... crypto/ec_signature_creator_openssl.cc:50: reinterpret_cast<const unsigned char*>(der_sig.front()); On 2013/10/15 23:25:29, wtc wrote: > > This reinterpret_cast is not necessary because uint8 is the same as unsigned > char. Done. https://codereview.chromium.org/26911006/diff/13001/crypto/ec_signature_creat... crypto/ec_signature_creator_openssl.cc:58: const size_t kMaxBytesPerBN = (kMaxBitsPerBN + 7) / 8; On 2013/10/15 23:25:29, wtc wrote: > > Nit: It is OK to just define kMaxBytesPerBN as 32 and omit kMaxBitsPerBN. 256 > bits = 32 bytes is common in crypto code. > > If you make this change, also change "two 256-bit vectors" to "two 32-byte > vectors" or "two 32-byte big integers". Done.
Patch set 5 LGTM. https://codereview.chromium.org/26911006/diff/21001/crypto/ec_signature_creat... File crypto/ec_signature_creator_openssl.cc (right): https://codereview.chromium.org/26911006/diff/21001/crypto/ec_signature_creat... crypto/ec_signature_creator_openssl.cc:61: const size_t kMaxBytesPerBN = 32; Starting with kMaxBitsPerBN will be useful when we use an elliptic curve whose size is not a multiple of 8 bits, for example, P-521. For now, let's go for simplicity.
I can't seem to trigger a CQ build when toggling the "Commit" checkbox on this page. Ok to manually commit then?
On 2013/10/23 17:22:26, digit1 wrote: > I can't seem to trigger a CQ build when toggling the "Commit" checkbox on this > page. Ok to manually commit then? I'm not sure why the CQ bot isn't picking it up but, yes, manual commits are generally ok when the CQ should have committed and the trybots aren't showing any obvious issues.
Thanks, I'll first upload a trivial rebase, to check just in case, then do just that.
Commited as https://chromiumcodereview.appspot.com/43663005/, closing this issue now. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
