Bug 1329238 - Make public CryptoKey.h methods return UniqueX NSS types instead of raw pointers. r=ttaubert

The std::unique_ptr based UniqueX types provide better safety over managing raw
pointers.

MozReview-Commit-ID: EwwOfs6RHqy

--HG--
extra : rebase_source : 7fbfca837c09b641bfffcba854d46b3f79645c0d

dom/crypto/CryptoKey.cpp

@@ -79,7 +79,7 @@ DestroyPrivateKeyWithoutDestroyingPKCS11Object(SECKEYPrivateKey* key)
 // generates a random ID for each key. The given template must contain an
 // attribute slot for a key ID, but it must consist of a null pointer and have a
 // length of 0.
-SECKEYPrivateKey*
+UniqueSECKEYPrivateKey
 PrivateKeyFromPrivateKeyTemplate(CK_ATTRIBUTE* aTemplate,
                                  CK_ULONG aTemplateSize)
 {
@@ -147,7 +147,8 @@ PrivateKeyFromPrivateKeyTemplate(CK_ATTRIBUTE* aTemplate,
   }
 
   // Have NSS translate the object to a private key.
-  return PK11_FindKeyByKeyID(slot.get(), objID.get(), nullptr);
+  return UniqueSECKEYPrivateKey(
+    PK11_FindKeyByKeyID(slot.get(), objID.get(), nullptr));
 }
 
 CryptoKey::CryptoKey(nsIGlobalObject* aGlobal)
@@ -364,8 +365,8 @@ CryptoKey::AddPublicKeyData(SECKEYPublicKey* aPublicKey)
     { CKA_VALUE,            value.data,           value.len },
   };
 
-  mPrivateKey = UniqueSECKEYPrivateKey(
-    PrivateKeyFromPrivateKeyTemplate(keyTemplate, ArrayLength(keyTemplate)));
+  mPrivateKey = PrivateKeyFromPrivateKeyTemplate(keyTemplate,
+                                                 ArrayLength(keyTemplate));
   NS_ENSURE_TRUE(mPrivateKey, NS_ERROR_DOM_OPERATION_ERR);
 
   return NS_OK;
@@ -485,24 +486,24 @@ CryptoKey::GetSymKey() const
   return mSymKey;
 }
 
-SECKEYPrivateKey*
+UniqueSECKEYPrivateKey
 CryptoKey::GetPrivateKey() const
 {
   nsNSSShutDownPreventionLock locker;
   if (!mPrivateKey || isAlreadyShutDown()) {
     return nullptr;
   }
-  return SECKEY_CopyPrivateKey(mPrivateKey.get());
+  return UniqueSECKEYPrivateKey(SECKEY_CopyPrivateKey(mPrivateKey.get()));
 }
 
-SECKEYPublicKey*
+UniqueSECKEYPublicKey
 CryptoKey::GetPublicKey() const
 {
   nsNSSShutDownPreventionLock locker;
   if (!mPublicKey || isAlreadyShutDown()) {
     return nullptr;
   }
-  return SECKEY_CopyPublicKey(mPublicKey.get());
+  return UniqueSECKEYPublicKey(SECKEY_CopyPublicKey(mPublicKey.get()));
 }
 
 void CryptoKey::virtualDestroyNSSReference()
@@ -519,11 +520,10 @@ void CryptoKey::destructorSafeDestroyNSSReference()
 
 // Serialization and deserialization convenience methods
 
-SECKEYPrivateKey*
+UniqueSECKEYPrivateKey
 CryptoKey::PrivateKeyFromPkcs8(CryptoBuffer& aKeyData,
                          const nsNSSShutDownPreventionLock& /*proofOfLock*/)
 {
-  SECKEYPrivateKey* privKey;
   UniquePK11SlotInfo slot(PK11_GetInternalSlot());
   if (!slot) {
     return nullptr;
@@ -542,6 +542,7 @@ CryptoKey::PrivateKeyFromPkcs8(CryptoBuffer& aKeyData,
   // Allow everything, we enforce usage ourselves
   unsigned int usage = KU_ALL;
 
+  SECKEYPrivateKey* privKey;
   SECStatus rv = PK11_ImportDERPrivateKeyInfoAndReturnKey(
                  slot.get(), &pkcs8Item, nullptr, nullptr, false, false,
                  usage, &privKey, nullptr);
@@ -549,10 +550,11 @@ CryptoKey::PrivateKeyFromPkcs8(CryptoBuffer& aKeyData,
   if (rv == SECFailure) {
     return nullptr;
   }
-  return privKey;
+
+  return UniqueSECKEYPrivateKey(privKey);
 }
 
-SECKEYPublicKey*
+UniqueSECKEYPublicKey
 CryptoKey::PublicKeyFromSpki(CryptoBuffer& aKeyData,
                        const nsNSSShutDownPreventionLock& /*proofOfLock*/)
 {
@@ -607,7 +609,7 @@ CryptoKey::PublicKeyFromSpki(CryptoBuffer& aKeyData,
     return nullptr;
   }
 
-  return SECKEY_CopyPublicKey(tmp.get());
+  return UniqueSECKEYPublicKey(SECKEY_CopyPublicKey(tmp.get()));
 }
 
 nsresult
@@ -748,7 +750,7 @@ CreateECPointForCoordinates(const CryptoBuffer& aX,
   return point;
 }
 
-SECKEYPrivateKey*
+UniqueSECKEYPrivateKey
 CryptoKey::PrivateKeyFromJwk(const JsonWebKey& aJwk,
                              const nsNSSShutDownPreventionLock& /*proofOfLock*/)
 {
@@ -1000,7 +1002,7 @@ CryptoKey::PrivateKeyToJwk(SECKEYPrivateKey* aPrivKey,
   }
 }
 
-SECKEYPublicKey*
+UniqueSECKEYPublicKey
 CreateECPublicKey(const SECItem* aKeyData, const nsString& aNamedCurve)
 {
   UniquePLArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE));
@@ -1037,10 +1039,10 @@ CreateECPublicKey(const SECItem* aKeyData, const nsString& aNamedCurve)
     return nullptr;
   }
 
-  return SECKEY_CopyPublicKey(key.get());
+  return UniqueSECKEYPublicKey(SECKEY_CopyPublicKey(key.get()));
 }
 
-SECKEYPublicKey*
+UniqueSECKEYPublicKey
 CryptoKey::PublicKeyFromJwk(const JsonWebKey& aJwk,
                             const nsNSSShutDownPreventionLock& /*proofOfLock*/)
 {
@@ -1074,7 +1076,7 @@ CryptoKey::PublicKeyFromJwk(const JsonWebKey& aJwk,
       return nullptr;
     }
 
-    return SECKEY_ImportDERPublicKey(pkDer.get(), CKK_RSA);
+    return UniqueSECKEYPublicKey(SECKEY_ImportDERPublicKey(pkDer.get(), CKK_RSA));
   }
 
   if (aJwk.mKty.EqualsLiteral(JWK_TYPE_EC)) {
@@ -1140,7 +1142,7 @@ CryptoKey::PublicKeyToJwk(SECKEYPublicKey* aPubKey,
   }
 }
 
-SECKEYPublicKey*
+UniqueSECKEYPublicKey
 CryptoKey::PublicDhKeyFromRaw(CryptoBuffer& aKeyData,
                               const CryptoBuffer& aPrime,
                               const CryptoBuffer& aGenerator,
@@ -1171,7 +1173,7 @@ CryptoKey::PublicDhKeyFromRaw(CryptoBuffer& aKeyData,
   key->u.dh.base.type = siUnsignedInteger;
   key->u.dh.publicValue.type = siUnsignedInteger;
 
-  return SECKEY_CopyPublicKey(key);
+  return UniqueSECKEYPublicKey(SECKEY_CopyPublicKey(key));
 }
 
 nsresult
@@ -1185,7 +1187,7 @@ CryptoKey::PublicDhKeyToRaw(SECKEYPublicKey* aPubKey,
   return NS_OK;
 }
 
-SECKEYPublicKey*
+UniqueSECKEYPublicKey
 CryptoKey::PublicECKeyFromRaw(CryptoBuffer& aKeyData,
                               const nsString& aNamedCurve,
                               const nsNSSShutDownPreventionLock& /*proofOfLock*/)
@@ -1322,12 +1324,10 @@ CryptoKey::ReadStructuredClone(JSStructuredCloneReader* aReader)
     return false;
   }
   if (priv.Length() > 0) {
-    mPrivateKey = UniqueSECKEYPrivateKey(
-      CryptoKey::PrivateKeyFromPkcs8(priv, locker));
+    mPrivateKey = CryptoKey::PrivateKeyFromPkcs8(priv, locker);
   }
   if (pub.Length() > 0)  {
-    mPublicKey = UniqueSECKEYPublicKey(
-      CryptoKey::PublicKeyFromSpki(pub, locker));
+    mPublicKey = CryptoKey::PublicKeyFromSpki(pub, locker);
   }
 
   // Ensure that what we've read is consistent

dom/crypto/CryptoKey.h

@@ -132,12 +132,9 @@ public:
   nsresult SetPublicKey(SECKEYPublicKey* aPublicKey);
 
   // Accessors for the keys themselves
-  // Note: GetPrivateKey and GetPublicKey return copies of the internal
-  // key handles, which the caller must free with SECKEY_DestroyPrivateKey
-  // or SECKEY_DestroyPublicKey.
   const CryptoBuffer& GetSymKey() const;
-  SECKEYPrivateKey* GetPrivateKey() const;
-  SECKEYPublicKey* GetPublicKey() const;
+  UniqueSECKEYPrivateKey GetPrivateKey() const;
+  UniqueSECKEYPublicKey GetPublicKey() const;
 
   // For nsNSSShutDownObject
   virtual void virtualDestroyNSSReference() override;
@@ -148,41 +145,47 @@ public:
   // 1. The inputs aKeyData are non-const only because the NSS import
   //    functions lack the const modifier.  They should not be modified.
   // 2. All of the NSS key objects returned need to be freed by the caller.
-  static SECKEYPrivateKey* PrivateKeyFromPkcs8(CryptoBuffer& aKeyData,
-                                               const nsNSSShutDownPreventionLock& /*proofOfLock*/);
+  static UniqueSECKEYPrivateKey PrivateKeyFromPkcs8(
+    CryptoBuffer& aKeyData,
+    const nsNSSShutDownPreventionLock& /*proofOfLock*/);
   static nsresult PrivateKeyToPkcs8(SECKEYPrivateKey* aPrivKey,
                                     CryptoBuffer& aRetVal,
                                     const nsNSSShutDownPreventionLock& /*proofOfLock*/);
 
-  static SECKEYPublicKey* PublicKeyFromSpki(CryptoBuffer& aKeyData,
-                                            const nsNSSShutDownPreventionLock& /*proofOfLock*/);
+  static UniqueSECKEYPublicKey PublicKeyFromSpki(
+    CryptoBuffer& aKeyData,
+    const nsNSSShutDownPreventionLock& /*proofOfLock*/);
   static nsresult PublicKeyToSpki(SECKEYPublicKey* aPubKey,
                                   CryptoBuffer& aRetVal,
                                   const nsNSSShutDownPreventionLock& /*proofOfLock*/);
 
-  static SECKEYPrivateKey* PrivateKeyFromJwk(const JsonWebKey& aJwk,
-                                             const nsNSSShutDownPreventionLock& /*proofOfLock*/);
+  static UniqueSECKEYPrivateKey PrivateKeyFromJwk(
+    const JsonWebKey& aJwk,
+    const nsNSSShutDownPreventionLock& /*proofOfLock*/);
   static nsresult PrivateKeyToJwk(SECKEYPrivateKey* aPrivKey,
                                   JsonWebKey& aRetVal,
                                   const nsNSSShutDownPreventionLock& /*proofOfLock*/);
 
-  static SECKEYPublicKey* PublicKeyFromJwk(const JsonWebKey& aKeyData,
-                                           const nsNSSShutDownPreventionLock& /*proofOfLock*/);
+  static UniqueSECKEYPublicKey PublicKeyFromJwk(
+    const JsonWebKey& aKeyData,
+    const nsNSSShutDownPreventionLock& /*proofOfLock*/);
   static nsresult PublicKeyToJwk(SECKEYPublicKey* aPubKey,
                                  JsonWebKey& aRetVal,
                                  const nsNSSShutDownPreventionLock& /*proofOfLock*/);
 
-  static SECKEYPublicKey* PublicDhKeyFromRaw(CryptoBuffer& aKeyData,
-                                             const CryptoBuffer& aPrime,
-                                             const CryptoBuffer& aGenerator,
-                                             const nsNSSShutDownPreventionLock& /*proofOfLock*/);
+  static UniqueSECKEYPublicKey PublicDhKeyFromRaw(
+    CryptoBuffer& aKeyData,
+    const CryptoBuffer& aPrime,
+    const CryptoBuffer& aGenerator,
+    const nsNSSShutDownPreventionLock& /*proofOfLock*/);
   static nsresult PublicDhKeyToRaw(SECKEYPublicKey* aPubKey,
                                    CryptoBuffer& aRetVal,
                                    const nsNSSShutDownPreventionLock& /*proofOfLock*/);
 
-  static SECKEYPublicKey* PublicECKeyFromRaw(CryptoBuffer& aKeyData,
-                                             const nsString& aNamedCurve,
-                                             const nsNSSShutDownPreventionLock& /*proofOfLock*/);
+  static UniqueSECKEYPublicKey PublicECKeyFromRaw(
+    CryptoBuffer& aKeyData,
+    const nsString& aNamedCurve,
+    const nsNSSShutDownPreventionLock& /*proofOfLock*/);
   static nsresult PublicECKeyToRaw(SECKEYPublicKey* aPubKey,
                                    CryptoBuffer& aRetVal,
                                    const nsNSSShutDownPreventionLock& /*proofOfLock*/);

dom/crypto/WebCryptoTask.cpp

@@ -1780,11 +1780,9 @@ private:
          !mJwk.mD.WasPassed())) {
       // Public key import
       if (mFormat.EqualsLiteral(WEBCRYPTO_KEY_FORMAT_SPKI)) {
-        pubKey = UniqueSECKEYPublicKey(
-          CryptoKey::PublicKeyFromSpki(mKeyData, locker));
+        pubKey = CryptoKey::PublicKeyFromSpki(mKeyData, locker);
       } else {
-        pubKey = UniqueSECKEYPublicKey(
-          CryptoKey::PublicKeyFromJwk(mJwk, locker));
+        pubKey = CryptoKey::PublicKeyFromJwk(mJwk, locker);
       }
 
       if (!pubKey) {
@@ -1801,11 +1799,9 @@ private:
          mJwk.mD.WasPassed())) {
       // Private key import
       if (mFormat.EqualsLiteral(WEBCRYPTO_KEY_FORMAT_PKCS8)) {
-        privKey = UniqueSECKEYPrivateKey(
-          CryptoKey::PrivateKeyFromPkcs8(mKeyData, locker));
+        privKey = CryptoKey::PrivateKeyFromPkcs8(mKeyData, locker);
       } else {
-        privKey = UniqueSECKEYPrivateKey(
-          CryptoKey::PrivateKeyFromJwk(mJwk, locker));
+        privKey = CryptoKey::PrivateKeyFromJwk(mJwk, locker);
       }
 
       if (!privKey) {
@@ -1929,8 +1925,7 @@ private:
     nsNSSShutDownPreventionLock locker;
     if (mFormat.EqualsLiteral(WEBCRYPTO_KEY_FORMAT_JWK) && mJwk.mD.WasPassed()) {
       // Private key import
-      privKey = UniqueSECKEYPrivateKey(
-        CryptoKey::PrivateKeyFromJwk(mJwk, locker));
+      privKey = CryptoKey::PrivateKeyFromJwk(mJwk, locker);
       if (!privKey) {
         return NS_ERROR_DOM_DATA_ERR;
       }
@@ -1946,14 +1941,11 @@ private:
                 !mJwk.mD.WasPassed())) {
       // Public key import
       if (mFormat.EqualsLiteral(WEBCRYPTO_KEY_FORMAT_RAW)) {
-        pubKey = UniqueSECKEYPublicKey(
-          CryptoKey::PublicECKeyFromRaw(mKeyData, mNamedCurve, locker));
+        pubKey = CryptoKey::PublicECKeyFromRaw(mKeyData, mNamedCurve, locker);
       } else if (mFormat.EqualsLiteral(WEBCRYPTO_KEY_FORMAT_SPKI)) {
-        pubKey = UniqueSECKEYPublicKey(
-          CryptoKey::PublicKeyFromSpki(mKeyData, locker));
+        pubKey = CryptoKey::PublicKeyFromSpki(mKeyData, locker);
       } else if (mFormat.EqualsLiteral(WEBCRYPTO_KEY_FORMAT_JWK)) {
-        pubKey = UniqueSECKEYPublicKey(
-          CryptoKey::PublicKeyFromJwk(mJwk, locker));
+        pubKey = CryptoKey::PublicKeyFromJwk(mJwk, locker);
       } else {
         MOZ_ASSERT(false);
       }
@@ -2087,11 +2079,10 @@ private:
         mFormat.EqualsLiteral(WEBCRYPTO_KEY_FORMAT_SPKI)) {
       // Public key import
       if (mFormat.EqualsLiteral(WEBCRYPTO_KEY_FORMAT_RAW)) {
-        pubKey = UniqueSECKEYPublicKey(
-          CryptoKey::PublicDhKeyFromRaw(mKeyData, mPrime, mGenerator, locker));
+        pubKey = CryptoKey::PublicDhKeyFromRaw(mKeyData, mPrime, mGenerator,
+                                               locker);
       } else if (mFormat.EqualsLiteral(WEBCRYPTO_KEY_FORMAT_SPKI)) {
-        pubKey = UniqueSECKEYPublicKey(
-          CryptoKey::PublicKeyFromSpki(mKeyData, locker));
+        pubKey = CryptoKey::PublicKeyFromSpki(mKeyData, locker);
       } else {
         MOZ_ASSERT(false);
       }
@@ -3025,7 +3016,7 @@ public:
     }
 
     CryptoKey* publicKey = params.mPublic;
-    mPubKey = UniqueSECKEYPublicKey(publicKey->GetPublicKey());
+    mPubKey = publicKey->GetPublicKey();
     if (!mPubKey) {
       mEarlyRv = NS_ERROR_DOM_INVALID_ACCESS_ERR;
       return;
@@ -3125,7 +3116,7 @@ public:
     }
 
     CryptoKey* publicKey = params.mPublic;
-    mPubKey = UniqueSECKEYPublicKey(publicKey->GetPublicKey());
+    mPubKey = publicKey->GetPublicKey();
     if (!mPubKey) {
       mEarlyRv = NS_ERROR_DOM_INVALID_ACCESS_ERR;
       return;

dom/media/webrtc/RTCCertificate.cpp

@@ -220,11 +220,11 @@ private:
   {
     // Make copies of the private key and certificate, otherwise, when this
     // object is deleted, the structures they reference will be deleted too.
-    SECKEYPrivateKey* key = mKeyPair->mPrivateKey.get()->GetPrivateKey();
+    UniqueSECKEYPrivateKey key = mKeyPair->mPrivateKey.get()->GetPrivateKey();
     CERTCertificate* cert = CERT_DupCertificate(mCertificate.get());
     RefPtr<RTCCertificate> result =
         new RTCCertificate(mResultPromise->GetParentObject(),
-                           key, cert, mAuthType, mExpires);
+                           key.release(), cert, mAuthType, mExpires);
     mResultPromise->MaybeResolve(result);
   }
 };
@@ -416,7 +416,7 @@ RTCCertificate::ReadPrivateKey(JSStructuredCloneReader* aReader,
   if (!jwk.Init(json)) {
     return false;
   }
-  mPrivateKey.reset(CryptoKey::PrivateKeyFromJwk(jwk, aLockProof));
+  mPrivateKey = CryptoKey::PrivateKeyFromJwk(jwk, aLockProof);
   return !!mPrivateKey;
 }