fix ECDH JWK key imports do not check alg

Completed alternative to #1404 Fixes: #1403 Co-authored-by: Filip Skokan <[email protected]>
cloudflare · Dec 21, 2023 · aff8686 · aff8686
1 parent ef2c11f
commit aff8686
Show file tree

Hide file tree

Showing 3 changed files with 47 additions and 22 deletions.
diff --git a/src/workerd/api/crypto-impl-asymmetric-test.js b/src/workerd/api/crypto-impl-asymmetric-test.js
@@ -70,3 +70,20 @@ export const publicExponent_type_test = {
     assert.ok(key.publicKey.algorithm.publicExponent[Symbol.toStringTag] == "Uint8Array");
   }
 }
+
+export const ecdhJwkTest = {
+  async test() {
+    const publicJwk = {
+      kty: 'EC',
+      crv: 'P-256',
+      alg: 'THIS CAN BE ANYTHING',
+      x: 'Ze2loSV3wrroKUN_4zhwGhCqo3Xhu1td4QjeQ5wIVR0',
+      y: 'HlLtdXARY_f55A3fnzQbPcm6hgr34Mp8p-nuzQCE0Zw',
+    }
+
+    // The import should succeed with no errors.
+    // Refs: https://github.com/cloudflare/workerd/issues/1403
+    await crypto.subtle.importKey('jwk', publicJwk,
+                                  { name: 'ECDH', namedCurve: 'P-256' }, true, []);
+  }
+};
diff --git a/src/workerd/api/crypto-impl-asymmetric.c++ b/src/workerd/api/crypto-impl-asymmetric.c++
@@ -1729,7 +1729,8 @@ ImportAsymmetricResult importEllipticRaw(SubtleCrypto::ImportKeyData keyData, in
 
 }  // namespace
 
-kj::Own<EVP_PKEY> ellipticJwkReader(int curveId, SubtleCrypto::JsonWebKey&& keyDataJwk) {
+kj::Own<EVP_PKEY> ellipticJwkReader(int curveId, SubtleCrypto::JsonWebKey&& keyDataJwk,
+                                    kj::StringPtr normalizedName) {
   if (curveId == NID_ED25519 || curveId == NID_X25519) {
     auto evpId = curveId == NID_X25519 ? EVP_PKEY_X25519 : EVP_PKEY_ED25519;
     auto curveName = curveId == NID_X25519 ? "X25519" : "Ed25519";
@@ -1782,22 +1783,25 @@ kj::Own<EVP_PKEY> ellipticJwkReader(int curveId, SubtleCrypto::JsonWebKey&& keyD
       "Elliptic curve \"jwk\" key import requires a JSON Web Key with Key Type parameter "
       "\"kty\" (\"", keyDataJwk.kty, "\") equal to \"EC\".");
 
-  KJ_IF_SOME(alg, keyDataJwk.alg) {
-    // If this JWK specifies an algorithm, make sure it jives with the hash we were passed via
-    // importKey().
-    static std::map<kj::StringPtr, int> ecdsaAlgorithms {
-      {"ES256", NID_X9_62_prime256v1},
-      {"ES384", NID_secp384r1},
-      {"ES512", NID_secp521r1},
-    };
+  if (normalizedName == "ECDSA") {
+    KJ_IF_SOME(alg, keyDataJwk.alg) {
+      // If this JWK specifies an algorithm, make sure it jives with the hash we were passed via
+      // importKey().
+      static std::map<kj::StringPtr, int> ecdsaAlgorithms {
+        {"ES256", NID_X9_62_prime256v1},
+        {"ES384", NID_secp384r1},
+        {"ES512", NID_secp521r1},
+      };
 
-    auto iter = ecdsaAlgorithms.find(alg);
-    JSG_REQUIRE(iter != ecdsaAlgorithms.end(), DOMNotSupportedError,
-        "Unrecognized or unimplemented algorithm \"", alg,
-        "\" listed in JSON Web Key Algorithm parameter.");
+      auto iter = ecdsaAlgorithms.find(alg);
+      JSG_REQUIRE(iter != ecdsaAlgorithms.end(), DOMNotSupportedError,
+          "Unrecognized or unimplemented algorithm \"", alg,
+          "\" listed in JSON Web Key Algorithm parameter.");
 
-    JSG_REQUIRE(iter->second == curveId, DOMDataError,
-        "JSON Web Key Algorithm parameter \"alg\" (\"", alg, "\") does not match requested curve.");
+      JSG_REQUIRE(iter->second == curveId, DOMDataError,
+          "JSON Web Key Algorithm parameter \"alg\" (\"", alg,
+          "\") does not match requested curve.");
+    }
   }
 
   auto ecKey = OSSLCALL_OWN(EC_KEY, EC_KEY_new_by_curve_name(curveId), DOMOperationError,
@@ -1863,8 +1867,9 @@ kj::Own<CryptoKey::Impl> CryptoKey::Impl::importEcdsa(
       return importAsymmetric(
           js, format, kj::mv(keyData), normalizedName, extractable, keyUsages,
           // Verbose lambda capture needed because: https://bugs.llvm.org/show_bug.cgi?id=35984
-          [curveId = curveId](SubtleCrypto::JsonWebKey keyDataJwk) -> kj::Own<EVP_PKEY> {
-        return ellipticJwkReader(curveId, kj::mv(keyDataJwk));
+          [curveId = curveId, normalizedName = kj::str(normalizedName)]
+          (SubtleCrypto::JsonWebKey keyDataJwk) -> kj::Own<EVP_PKEY> {
+        return ellipticJwkReader(curveId, kj::mv(keyDataJwk), normalizedName);
       }, CryptoKeyUsageSet::sign() | CryptoKeyUsageSet::verify());
     } else {
       return importEllipticRaw(kj::mv(keyData), curveId, normalizedName, keyUsages,
@@ -1919,8 +1924,9 @@ kj::Own<CryptoKey::Impl> CryptoKey::Impl::importEcdh(
       return importAsymmetric(
           js, format, kj::mv(keyData), normalizedName, extractable, keyUsages,
           // Verbose lambda capture needed because: https://bugs.llvm.org/show_bug.cgi?id=35984
-          [curveId = curveId](SubtleCrypto::JsonWebKey keyDataJwk) -> kj::Own<EVP_PKEY> {
-        return ellipticJwkReader(curveId, kj::mv(keyDataJwk));
+          [curveId = curveId, normalizedName = kj::str(normalizedName)]
+          (SubtleCrypto::JsonWebKey keyDataJwk) -> kj::Own<EVP_PKEY> {
+        return ellipticJwkReader(curveId, kj::mv(keyDataJwk), normalizedName);
       }, CryptoKeyUsageSet::derivationKeyMask());
     } else {
       // The usage set is required to be empty for public ECDH keys, including raw keys.
@@ -2274,8 +2280,9 @@ kj::Own<CryptoKey::Impl> CryptoKey::Impl::importEddsa(
     if (format != "raw") {
       return importAsymmetric(
           js, format, kj::mv(keyData), normalizedName, extractable, keyUsages,
-          [nid](SubtleCrypto::JsonWebKey keyDataJwk) -> kj::Own<EVP_PKEY> {
-        return ellipticJwkReader(nid, kj::mv(keyDataJwk));
+          [nid, normalizedName = kj::str(normalizedName)]
+          (SubtleCrypto::JsonWebKey keyDataJwk) -> kj::Own<EVP_PKEY> {
+        return ellipticJwkReader(nid, kj::mv(keyDataJwk), normalizedName);
       }, normalizedName == "X25519" ? CryptoKeyUsageSet::derivationKeyMask() :
          CryptoKeyUsageSet::sign() | CryptoKeyUsageSet::verify());
     } else {

diff --git a/src/workerd/api/crypto-impl.h b/src/workerd/api/crypto-impl.h
@@ -318,7 +318,8 @@ static inline T integerCeilDivision(T a, T b) {
   return a == 0 ? 0 : 1 + (a - 1) / b;
 }
 
-kj::Own<EVP_PKEY> ellipticJwkReader(int curveId, SubtleCrypto::JsonWebKey&& keyDataJwk);
+kj::Own<EVP_PKEY> ellipticJwkReader(int curveId, SubtleCrypto::JsonWebKey&& keyDataJwk,
+                                    kj::StringPtr normalizedName);
 kj::Own<EVP_PKEY> rsaJwkReader(SubtleCrypto::JsonWebKey&& keyDataJwk);
 
 // A wrapper for kj::Array<kj::byte> that will ensure the memory is overwritten