Fixed sign/verify behavior when using ECDSA keys and/or crypto.subtle keys

Author: AndrewKahr

src/workerd/api/node/crypto-keys.c++

@@ -6,6 +6,7 @@
 
 #include <workerd/api/crypto/impl.h>
 #include <workerd/api/crypto/jwk.h>
+#include <workerd/api/crypto/keys.h>
 
 #include <ncrypto.h>
 #include <openssl/crypto.h>
@@ -862,10 +863,28 @@ jsg::JsUint8Array CryptoImpl::statelessDH(
   JSG_FAIL_REQUIRE(Error, "Unsupported keys for stateless diffie-hellman");
 }
 
-kj::Maybe<const ncrypto::EVPKeyPointer&> CryptoImpl::tryGetKey(jsg::Ref<CryptoKey>& key) {
-  KJ_IF_SOME(key, kj::dynamicDowncastIfAvailable<AsymmetricKey>(*key->impl)) {
-    const ncrypto::EVPKeyPointer& evp = key;
-    return evp;
+kj::Maybe<ncrypto::EVPKeyPointer> CryptoImpl::tryGetKey(jsg::Ref<CryptoKey>& key) {
+  // AsymmetricKeyCryptoKeyImpl doesn't provide a reference like AsymmetricKey,
+  // so this function must return a value type since we create the EVPKeyPointer
+  // in here
+  KJ_IF_SOME(asymKey, kj::dynamicDowncastIfAvailable<AsymmetricKey>(*key->impl)) {
+    const ncrypto::EVPKeyPointer& evp = asymKey;
+    // Internally just incrementing the ref count and returning a new pointer, no
+    // copied key data so impact should be minimal.
+    return evp.clone();
+  }
+  // Also handle keys created via the Web Crypto API (crypto.subtle), which use a
+  // different CryptoKey::Impl subclass.
+  KJ_IF_SOME(webCryptoKey, kj::dynamicDowncastIfAvailable<AsymmetricKeyCryptoKeyImpl>(*key->impl)) {
+    EVP_PKEY* raw = webCryptoKey.getEvpPkey();
+    // Mimicking the internal implementation of EVPKeyPointer::clone() since getEvpPkey()
+    // returns a raw pointer
+    if (raw != nullptr) {
+      if (!EVP_PKEY_up_ref(raw)) {
+        return kj::none;
+      }
+      return ncrypto::EVPKeyPointer(raw);
+    }
   }
   return kj::none;
 }

src/workerd/api/node/crypto.c++

@@ -441,7 +441,7 @@ jsg::JsUint8Array CryptoImpl::SignHandle::sign(jsg::Lock& js,
   ncrypto::ClearErrorOnReturn clear_error_on_return;
   JSG_REQUIRE(ctx, Error, "Signing context has already been finalized");
 
-  const auto& pkey = JSG_REQUIRE_NONNULL(tryGetKey(key), Error, "Invalid key for sign operation");
+  auto pkey = JSG_REQUIRE_NONNULL(tryGetKey(key), Error, "Invalid key for sign operation");
   JSG_REQUIRE(pkey.validateDsaParameters(), Error, "Invalid DSA parameters");
 
   // There's a bug in ncrypto that doesn't clear the EVPMDCtxPointer when
@@ -497,7 +497,7 @@ bool CryptoImpl::VerifyHandle::verify(jsg::Lock& js,
 
   JSG_REQUIRE(ctx, Error, "Verification context has already been finalized");
 
-  const auto& pkey = JSG_REQUIRE_NONNULL(tryGetKey(key), Error, "Invalid key for verify operation");
+  auto pkey = JSG_REQUIRE_NONNULL(tryGetKey(key), Error, "Invalid key for verify operation");
 
   JSG_REQUIRE(!pkey.isOneShotVariant(), Error, "Unsupported operation for this key");
 
@@ -529,7 +529,7 @@ jsg::JsUint8Array CryptoImpl::signOneShot(jsg::Lock& js,
   auto mdctx = ncrypto::EVPMDCtxPointer::New();
   JSG_REQUIRE(mdctx, Error, "Failed to create signing context");
 
-  const auto& pkey = JSG_REQUIRE_NONNULL(tryGetKey(key), Error, "Invalid key for sign operation");
+  auto pkey = JSG_REQUIRE_NONNULL(tryGetKey(key), Error, "Invalid key for sign operation");
 
   // The version of BoringSSL we use does not support DSA keys with EVP
   // When signing/verification. This may change in the future.
@@ -546,7 +546,13 @@ jsg::JsUint8Array CryptoImpl::signOneShot(jsg::Lock& js,
     .len = data.size(),
   };
 
-  ncrypto::DataPointer sig = mdctx.signOneShot(buf);
+  // For one-shot-only key types (e.g. Ed25519, Ed448), we must use
+  // EVP_DigestSign via signOneShot(). For other key types (e.g. ECDSA),
+  // we use sign() which calls EVP_DigestSignUpdate + EVP_DigestSignFinal
+  // and correctly resizes the output buffer to the actual signature length.
+  // This matters for ECDSA where DER-encoded signatures can be shorter
+  // than the maximum estimated size.
+  ncrypto::DataPointer sig = pkey.isOneShotVariant() ? mdctx.signOneShot(buf) : mdctx.sign(buf);
   auto sigBuf =
       jsg::JsUint8Array::create(js, kj::ArrayPtr<const kj::byte>(sig.get<kj::byte>(), sig.size()));
 
@@ -574,8 +580,7 @@ bool CryptoImpl::verifyOneShot(jsg::Lock& js,
   auto mdctx = ncrypto::EVPMDCtxPointer::New();
   JSG_REQUIRE(mdctx, Error, "Failed to create verification context");
 
-  const auto& pkey =
-      JSG_REQUIRE_NONNULL(tryGetKey(key), Error, "Invalid key for verification operation");
+  auto pkey = JSG_REQUIRE_NONNULL(tryGetKey(key), Error, "Invalid key for verification operation");
 
   // The version of BoringSSL we use does not support DSA keys with EVP
   // When signing/verification. This may change in the future.
@@ -1317,7 +1322,7 @@ jsg::JsUint8Array CryptoImpl::publicEncrypt(jsg::Lock& js,
     jsg::Ref<CryptoKey> key,
     jsg::JsBufferSource buffer,
     CryptoImpl::PublicPrivateCipherOptions options) {
-  auto& pkey = JSG_REQUIRE_NONNULL(tryGetKey(key), Error, "No key provided");
+  auto pkey = JSG_REQUIRE_NONNULL(tryGetKey(key), Error, "No key provided");
   JSG_REQUIRE(pkey.isRsaVariant(), Error, "publicEncrypt() currently only supports RSA keys");
   auto ctx = pkey.newCtx();
   JSG_REQUIRE(ctx.initForEncrypt(), Error, "Failed to init for encryption");
@@ -1328,7 +1333,7 @@ jsg::JsUint8Array CryptoImpl::privateDecrypt(jsg::Lock& js,
     jsg::Ref<CryptoKey> key,
     jsg::JsBufferSource buffer,
     CryptoImpl::PublicPrivateCipherOptions options) {
-  auto& pkey = JSG_REQUIRE_NONNULL(tryGetKey(key), Error, "No key provided");
+  auto pkey = JSG_REQUIRE_NONNULL(tryGetKey(key), Error, "No key provided");
   JSG_REQUIRE(pkey.isRsaVariant(), Error, "publicEncrypt() currently only supports RSA keys");
   auto ctx = pkey.newCtx();
   JSG_REQUIRE(ctx.initForDecrypt(), Error, "Failed to init for decryption");
@@ -1339,7 +1344,7 @@ jsg::JsUint8Array CryptoImpl::publicDecrypt(jsg::Lock& js,
     jsg::Ref<CryptoKey> key,
     jsg::JsBufferSource buffer,
     CryptoImpl::PublicPrivateCipherOptions options) {
-  auto& pkey = JSG_REQUIRE_NONNULL(tryGetKey(key), Error, "No key provided");
+  auto pkey = JSG_REQUIRE_NONNULL(tryGetKey(key), Error, "No key provided");
   JSG_REQUIRE(pkey.isRsaVariant(), Error, "publicEncrypt() currently only supports RSA keys");
   auto ctx = pkey.newCtx();
   JSG_REQUIRE(EVP_PKEY_verify_recover_init(ctx.get()) == 1, Error, "Failed to init for decryption");
@@ -1353,7 +1358,7 @@ jsg::JsUint8Array CryptoImpl::privateEncrypt(jsg::Lock& js,
     jsg::Ref<CryptoKey> key,
     jsg::JsBufferSource buffer,
     CryptoImpl::PublicPrivateCipherOptions options) {
-  auto& pkey = JSG_REQUIRE_NONNULL(tryGetKey(key), Error, "No key provided");
+  auto pkey = JSG_REQUIRE_NONNULL(tryGetKey(key), Error, "No key provided");
   JSG_REQUIRE(pkey.isRsaVariant(), Error, "publicEncrypt() currently only supports RSA keys");
   auto ctx = pkey.newCtx();
   JSG_REQUIRE(EVP_PKEY_sign_init(ctx.get()) == 1, Error, "Failed to init for encryption");

src/workerd/api/node/crypto.h

@@ -213,7 +213,7 @@ class CryptoImpl final: public jsg::Object {
   jsg::Ref<CryptoKey> createSecretKey(jsg::Lock& js, jsg::JsBufferSource keyData);
   jsg::Ref<CryptoKey> createPrivateKey(jsg::Lock& js, CreateAsymmetricKeyOptions options);
   jsg::Ref<CryptoKey> createPublicKey(jsg::Lock& js, CreateAsymmetricKeyOptions options);
-  static kj::Maybe<const ncrypto::EVPKeyPointer&> tryGetKey(jsg::Ref<CryptoKey>& key);
+  static kj::Maybe<ncrypto::EVPKeyPointer> tryGetKey(jsg::Ref<CryptoKey>& key);
   static kj::Maybe<kj::ArrayPtr<const kj::byte>> tryGetSecretKeyData(jsg::Ref<CryptoKey>& key);
 
   struct RsaKeyPairOptions {

src/workerd/api/node/tests/crypto_sign-test.js

@@ -182,3 +182,35 @@ export const testSignLength = {
     }
   },
 };
+
+// Test that Web Crypto keys (from crypto.subtle) can be used with
+// Node.js crypto sign/verify one-shot functions.
+export const webCryptoKeySignVerify = {
+  async test() {
+    const keyPair = await crypto.subtle.generateKey(
+      { name: 'ECDSA', namedCurve: 'P-256' },
+      true,
+      ['sign', 'verify']
+    );
+    const data = Buffer.from('hello world');
+    const sig = sign('SHA256', data, keyPair.privateKey);
+    ok(sig instanceof Buffer);
+    ok(sig.length > 0);
+    ok(verify('SHA256', data, keyPair.publicKey, sig));
+  },
+};
+
+export const webCryptoKeySignVerifyP384 = {
+  async test() {
+    const keyPair = await crypto.subtle.generateKey(
+      { name: 'ECDSA', namedCurve: 'P-384' },
+      true,
+      ['sign', 'verify']
+    );
+    const data = Buffer.from('test data');
+    const sig = sign('SHA384', data, keyPair.privateKey);
+    ok(sig instanceof Buffer);
+    ok(sig.length > 0);
+    ok(verify('SHA384', data, keyPair.publicKey, sig));
+  },
+};