Bug 1770885 - Avoid key copy + re-verification in CreateECPublicKey. r=keeler

This avoids the key copy, and should prevent further verification when
using the returned key.

Differential Revision: https://phabricator.services.mozilla.com/D147650

dom/crypto/CryptoKey.cpp

@@ -862,8 +862,7 @@ nsresult CryptoKey::PrivateKeyToJwk(SECKEYPrivateKey* aPrivKey,
 }
 
 UniqueSECKEYPublicKey CreateECPublicKey(const SECItem* aKeyData,
-                                        const nsAString& aNamedCurve,
-                                        bool aVerifyValid) {
+                                        const nsAString& aNamedCurve) {
   UniquePLArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE));
   if (!arena) {
     return nullptr;
@@ -878,28 +877,32 @@ UniqueSECKEYPublicKey CreateECPublicKey(const SECItem* aKeyData,
     return nullptr;
   }
 
-  key->arena = nullptr;  // key doesn't own the arena; it won't get double-freed
+  // Transfer arena ownership to the key.
+  key->arena = arena.release();
   key->keyType = ecKey;
   key->pkcs11Slot = nullptr;
   key->pkcs11ID = CK_INVALID_HANDLE;
 
   // Create curve parameters.
-  SECItem* params = CreateECParamsForCurve(aNamedCurve, arena.get());
+  SECItem* params = CreateECParamsForCurve(aNamedCurve, key->arena);
   if (!params) {
     return nullptr;
   }
   key->u.ec.DEREncodedParams = *params;
 
   // Set public point.
-  key->u.ec.publicValue = *aKeyData;
-
-  // Ensure the given point is on the curve.
-  if (aVerifyValid && !CryptoKey::PublicKeyValid(key.get())) {
+  SECStatus ret =
+      SECITEM_CopyItem(key->arena, &key->u.ec.publicValue, aKeyData);
+  if (NS_WARN_IF(ret != SECSuccess)) {
     return nullptr;
   }
-  MOZ_ASSERT(aVerifyValid || CryptoKey::PublicKeyValid(key.get()));
 
-  return UniqueSECKEYPublicKey(SECKEY_CopyPublicKey(key.get()));
+  // Ensure the given point is on the curve.
+  if (!CryptoKey::PublicKeyValid(key.get())) {
+    return nullptr;
+  }
+
+  return key;
 }
 
 UniqueSECKEYPublicKey CryptoKey::PublicKeyFromJwk(const JsonWebKey& aJwk) {
@@ -1058,12 +1061,7 @@ bool CryptoKey::PublicKeyValid(SECKEYPublicKey* aPubKey) {
   // it is imported into a PKCS#11 module, and returns CK_INVALID_HANDLE
   // if it is invalid.
   CK_OBJECT_HANDLE id = PK11_ImportPublicKey(slot.get(), aPubKey, PR_FALSE);
-  if (id == CK_INVALID_HANDLE) {
-    return false;
-  }
-
-  SECStatus rv = PK11_DestroyObject(slot.get(), id);
-  return (rv == SECSuccess);
+  return id != CK_INVALID_HANDLE;
 }
 
 bool CryptoKey::WriteStructuredClone(JSContext* aCX,

dom/crypto/WebCryptoCommon.h

@@ -315,8 +315,7 @@ inline SECItem* CreateECParamsForCurve(const nsAString& aNamedCurve,
 
 // Implemented in CryptoKey.cpp
 UniqueSECKEYPublicKey CreateECPublicKey(const SECItem* aKeyData,
-                                        const nsAString& aNamedCurve,
-                                        bool aVerifyValid = true);
+                                        const nsAString& aNamedCurve);
 
 }  // namespace mozilla::dom
 

dom/origin-trials/OriginTrials.cpp

@@ -70,11 +70,7 @@ SECKEYPublicKey* StaticCachedPublicKey::Get(const RawKeyRef aRawKey) {
     const SECItem item{siBuffer, const_cast<unsigned char*>(aRawKey.data()),
                        unsigned(aRawKey.Length())};
     MOZ_RELEASE_ASSERT(item.data[0] == EC_POINT_FORM_UNCOMPRESSED);
-
-    // Key verification takes a lot of time when verifying tokens, and it is
-    // unnecessary work since the keys are trusted.
-    const bool kVerifyValid = false;
-    mKey = dom::CreateECPublicKey(&item, kEcAlgorithm, kVerifyValid).release();
+    mKey = dom::CreateECPublicKey(&item, kEcAlgorithm);
     if (mKey) {
       // It's fine to capture [this] by pointer because we are always static.
       if (NS_IsMainThread()) {