fixup! crypto: add optional callback to crypto.sign and crypto.verify

nodejs · panva · Feb 24, 2021 · Feb 24, 2021 · Feb 24, 2021 · Feb 25, 2021
commit 0c9f8cf36b0e43f43942a87fa3aaf114fa5225e8
diff --git a/lib/internal/crypto/dsa.js b/lib/internal/crypto/dsa.js
@@ -10,6 +10,7 @@ const {
   KeyObjectHandle,
   SignJob,
   kCryptoJobAsync,
+  kSigEncDER,
   kKeyTypePrivate,
   kSignJobModeSign,
   kSignJobModeVerify,
@@ -254,8 +255,8 @@ function dsaSignVerify(key, data, algorithm, signature) {
     normalizeHashName(key.algorithm.hash.name),
     undefined,  // Salt-length is not used in DSA
     undefined,  // Padding is not used in DSA
-    kSigEncDER, // TODO(@jasnell): revise the inconsistency with WebCrypto's EC
-    signature));
+    signature,
+    kSigEncDER));
 }
 
 module.exports = {

diff --git a/lib/internal/crypto/ec.js b/lib/internal/crypto/ec.js
@@ -471,8 +471,8 @@ function ecdsaSignVerify(key, data, { name, hash }, signature) {
     hashname,
     undefined,  // Salt length, not used with ECDSA
     undefined,  // PSS Padding, not used with ECDSA
-    kSigEncP1363,
-    signature));
+    signature,
+    kSigEncP1363));
 }
 
 module.exports = {

diff --git a/lib/internal/crypto/rsa.js b/lib/internal/crypto/rsa.js
@@ -359,7 +359,6 @@ function rsaSignVerify(key, data, { saltLength }, signature) {
     normalizeHashName(key.algorithm.hash.name),
     saltLength,
     padding,
-    undefined, // EC(DSA) Signature Encoding is not used in RSA
     signature));
 }
 

diff --git a/lib/internal/crypto/sig.js b/lib/internal/crypto/sig.js
@@ -183,7 +183,6 @@ function signOneShot(algorithm, data, key, callback) {
     keyData = createPrivateKey(key)[kHandle];
   }
 
-  // TODO: add dsaSigEnc to SignJob
   // TODO: recognize pssSaltLength RSA_PSS_SALTLEN_* constants in SignJob
   const job = new SignJob(
     kCryptoJobAsync,
@@ -193,6 +192,7 @@ function signOneShot(algorithm, data, key, callback) {
     algorithm,
     pssSaltLength,
     rsaPadding,
+    undefined,
     dsaSigEnc);
 
   job.ondone = (error, signature) => {
@@ -295,7 +295,6 @@ function verifyOneShot(algorithm, data, key, signature, callback) {
     keyData = createPublicKey(key)[kHandle];
   }
 
-  // TODO: add dsaSigEnc to SignJob
   // TODO: recognize pssSaltLength RSA_PSS_SALTLEN_* constants in SignJob
   const job = new SignJob(
     kCryptoJobAsync,
@@ -305,8 +304,8 @@ function verifyOneShot(algorithm, data, key, signature, callback) {
     algorithm,
     pssSaltLength,
     rsaPadding,
-    dsaSigEnc,
-    signature);
+    signature,
+    dsaSigEnc);
 
   job.ondone = (error, result) => {
     if (error) return FunctionPrototypeCall(callback, job, error);

diff --git a/src/crypto/crypto_ec.cc b/src/crypto/crypto_ec.cc
@@ -926,62 +926,114 @@ size_t GroupOrderSize(ManagedEVPPKey key) {
   return BN_num_bytes(order.get());
 }
 
+// TODO(@jasnell): Reconcile with ConvertSignatureToP1363 in crypto_sig.cc
+// TODO(@jasnell): Move out of crypto_ec since this is also doing DSA now also
 ByteSource ConvertToWebCryptoSignature(
-    ManagedEVPPKey key,
+    const ManagedEVPPKey& key,
     const ByteSource& signature) {
   const unsigned char* data =
       reinterpret_cast<const unsigned char*>(signature.get());
-  EcdsaSigPointer ecsig(d2i_ECDSA_SIG(nullptr, &data, signature.size()));
-
-  if (!ecsig)
-    return ByteSource();
-
-  size_t order_size_bytes = GroupOrderSize(key);
-  char* outdata = MallocOpenSSL<char>(order_size_bytes * 2);
-  ByteSource out = ByteSource::Allocated(outdata, order_size_bytes * 2);
-  unsigned char* ptr = reinterpret_cast<unsigned char*>(outdata);
 
+  ECDSASigPointer ecsig;
+  DsaSigPointer dsasig;
   const BIGNUM* pr;
   const BIGNUM* ps;
-  ECDSA_SIG_get0(ecsig.get(), &pr, &ps);
+  size_t len = 0;
 
-  if (!BN_bn2binpad(pr, ptr, order_size_bytes) ||
-      !BN_bn2binpad(ps, ptr + order_size_bytes, order_size_bytes)) {
+  switch (EVP_PKEY_id(key.get())) {
+    case EVP_PKEY_EC: {
+      ecsig = ECDSASigPointer(d2i_ECDSA_SIG(nullptr, &data, signature.size()));
+
+      if (!ecsig)
+        return ByteSource();
+
+      len = GroupOrderSize(key);
+
+      ECDSA_SIG_get0(ecsig.get(), &pr, &ps);
+      break;
+    }
+    case EVP_PKEY_DSA: {
+      dsasig = DsaSigPointer(d2i_DSA_SIG(nullptr, &data, signature.size()));
+
+      if (!dsasig)
+        return ByteSource();
+
+      DSA_SIG_get0(dsasig.get(), &pr, &ps);
+      len = BN_num_bytes(pr);
+    }
+  }
+
+  CHECK_GT(len, 0);
+
+  char* outdata = MallocOpenSSL<char>(len * 2);
+  ByteSource out = ByteSource::Allocated(outdata, len * 2);
+  unsigned char* ptr = reinterpret_cast<unsigned char*>(outdata);
+
+  if (!BN_bn2binpad(pr, ptr, len) || !BN_bn2binpad(ps, ptr + len, len)) {
     return ByteSource();
   }
   return out;
 }
 
+// TODO(@jasnell): Reconcile with ConvertSignatureToDER in crypto_sig.cc
+// TODO(@jasnell): Move out of crypto_ec since this is also doing DSA now also
 ByteSource ConvertFromWebCryptoSignature(
-    ManagedEVPPKey key,
+    const ManagedEVPPKey& key,
     const ByteSource& signature) {
-  size_t order_size_bytes = GroupOrderSize(key);
+  BignumPointer r(BN_new());
+  BignumPointer s(BN_new());
+  const unsigned char* sig = signature.data<unsigned char>();
 
-  // If the size of the signature is incorrect, verification
-  // will fail.
-  if (signature.size() != 2 * order_size_bytes)
-    return ByteSource();  // Empty!
+  switch (EVP_PKEY_id(key.get())) {
+    case EVP_PKEY_EC: {
+      size_t order_size_bytes = GroupOrderSize(key);
 
-  EcdsaSigPointer ecsig(ECDSA_SIG_new());
-  if (!ecsig)
-    return ByteSource();
+      // If the size of the signature is incorrect, verification
+      // will fail.
+      if (signature.size() != 2 * order_size_bytes)
+        return ByteSource();  // Empty!
 
-  BignumPointer r(BN_new());
-  BignumPointer s(BN_new());
+      ECDSASigPointer ecsig(ECDSA_SIG_new());
+      if (!ecsig)
+        return ByteSource();
 
-  const unsigned char* sig = signature.data<unsigned char>();
+      if (!BN_bin2bn(sig, order_size_bytes, r.get()) ||
+          !BN_bin2bn(sig + order_size_bytes, order_size_bytes, s.get()) ||
+          !ECDSA_SIG_set0(ecsig.get(), r.release(), s.release())) {
+        return ByteSource();
+      }
 
-  if (!BN_bin2bn(sig, order_size_bytes, r.get()) ||
-      !BN_bin2bn(sig + order_size_bytes, order_size_bytes, s.get()) ||
-      !ECDSA_SIG_set0(ecsig.get(), r.release(), s.release())) {
-    return ByteSource();
-  }
+      int size = i2d_ECDSA_SIG(ecsig.get(), nullptr);
+      char* data = MallocOpenSSL<char>(size);
+      unsigned char* ptr = reinterpret_cast<unsigned char*>(data);
+      CHECK_EQ(i2d_ECDSA_SIG(ecsig.get(), &ptr), size);
+      return ByteSource::Allocated(data, size);
+    }
+    case EVP_PKEY_DSA: {
+      size_t len = signature.size() / 2;
+
+      if (signature.size() != 2 * len)
+        return ByteSource();
+
+      DsaSigPointer dsasig(DSA_SIG_new());
+      if (!dsasig)
+        return ByteSource();
+
+      if (!BN_bin2bn(sig, len, r.get()) ||
+          !BN_bin2bn(sig + len, len, s.get()) ||
+          !DSA_SIG_set0(dsasig.get(), r.release(), s.release())) {
+        return ByteSource();
+      }
 
-  int size = i2d_ECDSA_SIG(ecsig.get(), nullptr);
-  char* data = MallocOpenSSL<char>(size);
-  unsigned char* ptr = reinterpret_cast<unsigned char*>(data);
-  CHECK_EQ(i2d_ECDSA_SIG(ecsig.get(), &ptr), size);
-  return ByteSource::Allocated(data, size);
+      int size = i2d_DSA_SIG(dsasig.get(), nullptr);
+      char* data = MallocOpenSSL<char>(size);
+      unsigned char* ptr = reinterpret_cast<unsigned char*>(data);
+      CHECK_EQ(i2d_DSA_SIG(dsasig.get(), &ptr), size);
+      return ByteSource::Allocated(data, size);
+    }
+    default:
+      UNREACHABLE();
+  }
 }
 
 }  // namespace crypto

diff --git a/src/crypto/crypto_ec.h b/src/crypto/crypto_ec.h
@@ -164,11 +164,11 @@ v8::Maybe<bool> GetEcKeyDetail(
     v8::Local<v8::Object> target);
 
 ByteSource ConvertToWebCryptoSignature(
-  ManagedEVPPKey key,
+  const ManagedEVPPKey& key,
   const ByteSource& signature);
 
 ByteSource ConvertFromWebCryptoSignature(
-    ManagedEVPPKey key,
+    const ManagedEVPPKey& key,
     const ByteSource& signature);
 
 }  // namespace crypto

diff --git a/src/crypto/crypto_sig.cc b/src/crypto/crypto_sig.cc
@@ -231,6 +231,14 @@ bool IsOneShot(const ManagedEVPPKey& key) {
       return false;
   }
 }
+
+bool UseP1363Encoding(const SignConfiguration& params) {
+  switch (EVP_PKEY_id(params.key->GetAsymmetricKey().get())) {
+    case EVP_PKEY_EC:
+    case EVP_PKEY_DSA: return params.dsa_encoding == kSigEncP1363;
+  }
+  return false;
+}
 }  // namespace
 
 SignBase::Error SignBase::Init(const char* sign_type) {
@@ -295,6 +303,8 @@ void Sign::Initialize(Environment* env, Local<Object> target) {
 
   NODE_DEFINE_CONSTANT(target, kSignJobModeSign);
   NODE_DEFINE_CONSTANT(target, kSignJobModeVerify);
+  NODE_DEFINE_CONSTANT(target, kSigEncDER);
+  NODE_DEFINE_CONSTANT(target, kSigEncP1363);
   NODE_DEFINE_CONSTANT(target, RSA_PKCS1_PSS_PADDING);
 }
 
@@ -679,7 +689,8 @@ SignConfiguration::SignConfiguration(SignConfiguration&& other) noexcept
       digest(other.digest),
       flags(other.flags),
       padding(other.padding),
-      salt_length(other.salt_length) {}
+      salt_length(other.salt_length),
+      dsa_encoding(other.dsa_encoding) {}
 
 SignConfiguration& SignConfiguration::operator=(
     SignConfiguration&& other) noexcept {
@@ -742,6 +753,16 @@ Maybe<bool> SignTraits::AdditionalConfig(
     params->padding = args[offset + 5].As<Uint32>()->Value();
   }
 
+  if (args[offset + 7]->IsUint32()) {  // DSA Encoding
+    params->dsa_encoding =
+        static_cast<DSASigEnc>(args[offset + 7].As<Uint32>()->Value());
+    if (params->dsa_encoding != kSigEncDER &&
+        params->dsa_encoding != kSigEncP1363) {
+      THROW_ERR_OUT_OF_RANGE(env, "invalid signature encoding");
+      return Nothing<bool>();
+    }
+  }
+
   if (params->mode == SignConfiguration::kVerify) {
     ArrayBufferOrViewContents<char> signature(args[offset + 6]);
     if (UNLIKELY(!signature.CheckSizeInt32())) {
@@ -752,7 +773,7 @@ Maybe<bool> SignTraits::AdditionalConfig(
     // the signature from WebCrypto format into DER format...
     ManagedEVPPKey m_pkey = params->key->GetAsymmetricKey();
     Mutex::ScopedLock lock(*m_pkey.mutex());
-    if (EVP_PKEY_id(m_pkey.get()) == EVP_PKEY_EC) {
+    if (UseP1363Encoding(*params)) {
       params->signature =
           ConvertFromWebCryptoSignature(m_pkey, signature.ToByteSource());
     } else {
@@ -849,9 +870,7 @@ bool SignTraits::DeriveBits(
         if (!EVP_DigestSignFinal(context.get(), data, &len))
           return false;
 
-        // If this is an EC key (assuming ECDSA) we have to
-        // convert the signature in to the proper format.
-        if (EVP_PKEY_id(params.key->GetAsymmetricKey().get()) == EVP_PKEY_EC) {
+        if (UseP1363Encoding(params)) {
           *out = ConvertToWebCryptoSignature(
               params.key->GetAsymmetricKey(), buf);
         } else {

diff --git a/src/crypto/crypto_sig.h b/src/crypto/crypto_sig.h
@@ -15,7 +15,8 @@ namespace crypto {
 static const unsigned int kNoDsaSignature = static_cast<unsigned int>(-1);
 
 enum DSASigEnc {
-  kSigEncDER, kSigEncP1363
+  kSigEncDER,
+  kSigEncP1363
 };
 
 class SignBase : public BaseObject {
@@ -117,6 +118,7 @@ struct SignConfiguration final : public MemoryRetainer {
   int flags = SignConfiguration::kHasNone;
   int padding = 0;
   int salt_length = 0;
+  DSASigEnc dsa_encoding = kSigEncDER;
 
   SignConfiguration() = default;
 

diff --git a/src/crypto/crypto_util.h b/src/crypto/crypto_util.h
@@ -75,7 +75,7 @@ using HMACCtxPointer = DeleteFnPtr<HMAC_CTX, HMAC_CTX_free>;
 using CipherCtxPointer = DeleteFnPtr<EVP_CIPHER_CTX, EVP_CIPHER_CTX_free>;
 using RsaPointer = DeleteFnPtr<RSA, RSA_free>;
 using DsaPointer = DeleteFnPtr<DSA, DSA_free>;
-using EcdsaSigPointer = DeleteFnPtr<ECDSA_SIG, ECDSA_SIG_free>;
+using DsaSigPointer = DeleteFnPtr<DSA_SIG, DSA_SIG_free>;
 
 // Our custom implementation of the certificate verify callback
 // used when establishing a TLS handshake. Because we cannot perform