crypto: Implement ECSignatureCreatorImpl for OpenSSL

crypto/ec_signature_creator_openssl.cc

Index: crypto/ec_signature_creator_openssl.cc
diff --git a/crypto/ec_signature_creator_openssl.cc b/crypto/ec_signature_creator_openssl.cc
index 8854f5ed464de9c8fa1f658e1ca31b646cbbdea7..af3c1cc66eab62499a4121b2e448c0a23771ce7d 100644
--- a/crypto/ec_signature_creator_openssl.cc
+++ b/crypto/ec_signature_creator_openssl.cc
@@ -4,13 +4,21 @@
 
 #include "crypto/ec_signature_creator_impl.h"
 
+#include <openssl/bn.h>
+#include <openssl/ec.h>
+#include <openssl/ecdsa.h>
+#include <openssl/evp.h>
+#include <openssl/sha.h>
+
 #include "base/logging.h"
+#include "crypto/ec_private_key.h"
+#include "crypto/openssl_util.h"
 
 namespace crypto {
 
 ECSignatureCreatorImpl::ECSignatureCreatorImpl(ECPrivateKey* key)
-    : key_(key) {
-  NOTIMPLEMENTED();
+    : key_(key), signature_len_(0) {
+  EnsureOpenSSLInit();
 }
 
 ECSignatureCreatorImpl::~ECSignatureCreatorImpl() {}
@@ -18,14 +26,71 @@ ECSignatureCreatorImpl::~ECSignatureCreatorImpl() {}
 bool ECSignatureCreatorImpl::Sign(const uint8* data,
                                   int data_len,
                                   std::vector<uint8>* signature) {
-  NOTIMPLEMENTED();
-  return false;
+  OpenSSLErrStackTracer err_tracer(FROM_HERE);
+  // OpenSSL provides APIs to perform hashing + signing directly, but the
+  // result is a byte string that cannot be easily converted into the required
+  // output format. To work-around this, perform the operations separately.

[agl, 2013/10/15 15:00:08]

The output format of this function is a DER encoded signature. Don't the
EVP_DigestSign functions do that too?

[digit1, 2013/10/15 15:41:52]

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 :)

+
+  // First, compute the SHA-256 hash of the input data.
+  // Don't use SHA256(), while convenient, it uses a static global buffer
+  // and thus isn't thread-safe :-(
+  SHA256_CTX hash_ctx;
+  unsigned char digest[SHA256_DIGEST_LENGTH];
+  if (!SHA256_Init(&hash_ctx) || !SHA256_Update(&hash_ctx, data, data_len) ||

[agl, 2013/10/15 15:00:08]

The three terms of the disjunction should each have their own line.

[digit1, 2013/10/15 15:41:52]

That was the output of "git cl format" :-/ Not relevant anymore though.

+      !SHA256_Final(digest, &hash_ctx))
+    return false;
+
+  // Sign it with our key, then encode it directly.
+  ScopedOpenSSL<EC_KEY, EC_KEY_free> ec_key(EVP_PKEY_get1_EC_KEY(key_->key()));
+  if (!ec_key.get())
+    return false;
+
+  signature->resize(ECDSA_size(ec_key.get()));
+
+  unsigned int siglen = 0;
+  if (!ECDSA_sign_ex(0 /* type - ignored */,
+                     digest,
+                     static_cast<int>(sizeof(digest)),
+                     &(*signature)[0],
+                     &siglen,
+                     NULL /* kinv - optional */,
+                     NULL /* rp - optional */,
+                     ec_key.get()))
+    return false;
+
+  return true;
 }
 
 bool ECSignatureCreatorImpl::DecodeSignature(const std::vector<uint8>& der_sig,
                                              std::vector<uint8>* out_raw_sig) {
-  NOTIMPLEMENTED();
-  return false;
+  OpenSSLErrStackTracer err_tracer(FROM_HERE);
+  // Create ECDSA_SIG object from DER-encoded data.
+  const unsigned char* der_data =
+      reinterpret_cast<const unsigned char*>(der_sig.front());
+  ScopedOpenSSL<ECDSA_SIG, ECDSA_SIG_free> ecdsa_sig(
+      d2i_ECDSA_SIG(NULL, &der_data, static_cast<long>(der_sig.size())));
+  if (!ecdsa_sig.get())
+    return false;
+
+  // The result is made of two 256-bit vectors.
+  const size_t kMaxBitsPerBN = 256;
+  const size_t kMaxBytesPerBN = (kMaxBitsPerBN + 7) / 8;
+  std::vector<uint8> result;
+  result.resize(2 * kMaxBytesPerBN);
+  memset(&result[0], 0, result.size());
+
+  // NOTE: Can't really check for equality here since sometimes the value
+  // returned by BN_num_bytes() will be slightly smaller than kMaxBytesPerBN.
+  if (BN_num_bytes(ecdsa_sig.get()->r) > kMaxBytesPerBN ||
+      BN_num_bytes(ecdsa_sig.get()->s) > kMaxBytesPerBN) {
+    DLOG(ERROR) << "Invalid key sizes.";
+    return false;
+  }
+
+  BN_bn2bin(ecdsa_sig.get()->r, &result[0]);

[agl, 2013/10/15 15:00:08]

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);

[digit1, 2013/10/15 15:41:52]

I admit I had no idea. I've implemented your recommendation though.

+  BN_bn2bin(ecdsa_sig.get()->s, &result[kMaxBytesPerBN]);
+  out_raw_sig->swap(result);
+  return true;
 }
 
 }  // namespace crypto