Bug 1029173 - Clean up nsDataSignatureVerifier. r=keeler draft
authorCykesiopka <cykesiopka.bmo@gmail.com>
Tue, 12 Apr 2016 18:09:06 -0700
changeset 350234 dc2714ae28a2
parent 350169 6fc0294f96c5
child 518275 7166262c4816
push id15276
push usercykesiopka.bmo@gmail.com
push dateWed, 13 Apr 2016 01:18:19 +0000
reviewerskeeler
bugs1029173
milestone48.0a1
Bug 1029173 - Clean up nsDataSignatureVerifier. r=keeler This patch does the following: - Implements nsNSSShutDownObject. - Replaces more raw pointers with smart pointers. - Fixes other misc issues. MozReview-Commit-ID: HulWdonEbP8
security/apps/AppSignatureVerification.cpp
security/manager/ssl/ScopedNSSTypes.h
security/manager/ssl/nsDataSignatureVerifier.cpp
security/manager/ssl/nsDataSignatureVerifier.h
--- a/security/apps/AppSignatureVerification.cpp
+++ b/security/apps/AppSignatureVerification.cpp
@@ -658,21 +658,28 @@ VerifyCertificate(CERTCertificate* signe
   return NS_OK;
 }
 
 nsresult
 VerifySignature(AppTrustedRoot trustedRoot, const SECItem& buffer,
                 const SECItem& detachedDigest,
                 /*out*/ ScopedCERTCertList& builtChain)
 {
+  // Currently, this function is only called within the CalculateResult() method
+  // of CryptoTasks. As such, NSS should not be shut down at this point and the
+  // CryptoTask implementation should already hold a nsNSSShutDownPreventionLock.
+  // We acquire a nsNSSShutDownPreventionLock here solely to prove we did to
+  // VerifyCMSDetachedSignatureIncludingCertificate().
+  nsNSSShutDownPreventionLock locker;
   VerifyCertificateContext context = { trustedRoot, builtChain };
   // XXX: missing pinArg
   return VerifyCMSDetachedSignatureIncludingCertificate(buffer, detachedDigest,
                                                         VerifyCertificate,
-                                                        &context, nullptr);
+                                                        &context, nullptr,
+                                                        locker);
 }
 
 NS_IMETHODIMP
 OpenSignedAppFile(AppTrustedRoot aTrustedRoot, nsIFile* aJarFile,
                   /*out, optional */ nsIZipReader** aZipReader,
                   /*out, optional */ nsIX509Cert** aSignerCert)
 {
   NS_ENSURE_ARG_POINTER(aJarFile);
--- a/security/manager/ssl/ScopedNSSTypes.h
+++ b/security/manager/ssl/ScopedNSSTypes.h
@@ -339,16 +339,19 @@ MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(Un
                                       CERTCertList,
                                       CERT_DestroyCertList)
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniqueCERTCertNicknames,
                                       CERTCertNicknames,
                                       CERT_FreeNicknames)
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniqueCERTOidSequence,
                                       CERTOidSequence,
                                       CERT_DestroyOidSequence)
+MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniqueCERTSubjectPublicKeyInfo,
+                                      CERTSubjectPublicKeyInfo,
+                                      SECKEY_DestroySubjectPublicKeyInfo)
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniqueCERTUserNotice,
                                       CERTUserNotice,
                                       CERT_DestroyUserNotice)
 
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniqueNSSCMSMessage,
                                       NSSCMSMessage,
                                       NSS_CMSMessage_Destroy)
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniqueNSSCMSSignedData,
--- a/security/manager/ssl/nsDataSignatureVerifier.cpp
+++ b/security/manager/ssl/nsDataSignatureVerifier.cpp
@@ -2,16 +2,17 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "nsDataSignatureVerifier.h"
 
 #include "cms.h"
 #include "cryptohi.h"
 #include "keyhi.h"
+#include "mozilla/unused.h"
 #include "nsCOMPtr.h"
 #include "nsNSSComponent.h"
 #include "nssb64.h"
 #include "pkix/pkixtypes.h"
 #include "ScopedNSSTypes.h"
 #include "secerr.h"
 #include "SharedCertVerifier.h"
 
@@ -30,95 +31,106 @@ const SEC_ASN1Template CERT_SignatureDat
     { SEC_ASN1_INLINE | SEC_ASN1_XTRN,
         offsetof(CERTSignedData,signatureAlgorithm),
         SEC_ASN1_SUB(SECOID_AlgorithmIDTemplate), },
     { SEC_ASN1_BIT_STRING,
         offsetof(CERTSignedData,signature), },
     { 0, }
 };
 
-NS_IMETHODIMP
-nsDataSignatureVerifier::VerifyData(const nsACString & aData,
-                                    const nsACString & aSignature,
-                                    const nsACString & aPublicKey,
-                                    bool *_retval)
+nsDataSignatureVerifier::~nsDataSignatureVerifier()
 {
-    // Allocate an arena to handle the majority of the allocations
-    UniquePLArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE));
-    if (!arena) {
-        return NS_ERROR_OUT_OF_MEMORY;
-    }
+  nsNSSShutDownPreventionLock locker;
+  if (isAlreadyShutDown()) {
+    return;
+  }
+
+  shutdown(calledFromObject);
+}
+
+NS_IMETHODIMP
+nsDataSignatureVerifier::VerifyData(const nsACString& aData,
+                                    const nsACString& aSignature,
+                                    const nsACString& aPublicKey,
+                                    bool* _retval)
+{
+  NS_ENSURE_ARG_POINTER(_retval);
 
-    // Base 64 decode the key
-    SECItem keyItem;
-    PORT_Memset(&keyItem, 0, sizeof(SECItem));
-    if (!NSSBase64_DecodeBuffer(arena.get(), &keyItem,
-                                nsPromiseFlatCString(aPublicKey).get(),
-                                aPublicKey.Length())) {
-        return NS_ERROR_FAILURE;
-    }
+  nsNSSShutDownPreventionLock locker;
+  if (isAlreadyShutDown()) {
+    return NS_ERROR_NOT_AVAILABLE;
+  }
 
-    // Extract the public key from the data
-    CERTSubjectPublicKeyInfo *pki = SECKEY_DecodeDERSubjectPublicKeyInfo(&keyItem);
-    if (!pki) {
-        return NS_ERROR_FAILURE;
-    }
-    SECKEYPublicKey *publicKey = SECKEY_ExtractPublicKey(pki);
-    SECKEY_DestroySubjectPublicKeyInfo(pki);
-    pki = nullptr;
+  // Allocate an arena to handle the majority of the allocations
+  UniquePLArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE));
+  if (!arena) {
+    return NS_ERROR_OUT_OF_MEMORY;
+  }
 
-    if (!publicKey) {
-        return NS_ERROR_FAILURE;
-    }
+  // Base 64 decode the key
+  SECItem keyItem;
+  PORT_Memset(&keyItem, 0, sizeof(SECItem));
+  if (!NSSBase64_DecodeBuffer(arena.get(), &keyItem,
+                              PromiseFlatCString(aPublicKey).get(),
+                              aPublicKey.Length())) {
+    return NS_ERROR_FAILURE;
+  }
 
-    // Base 64 decode the signature
-    SECItem signatureItem;
-    PORT_Memset(&signatureItem, 0, sizeof(SECItem));
-    if (!NSSBase64_DecodeBuffer(arena.get(), &signatureItem,
-                                nsPromiseFlatCString(aSignature).get(),
-                                aSignature.Length())) {
-        SECKEY_DestroyPublicKey(publicKey);
-        return NS_ERROR_FAILURE;
-    }
+  // Extract the public key from the data
+  UniqueCERTSubjectPublicKeyInfo pki(
+    SECKEY_DecodeDERSubjectPublicKeyInfo(&keyItem));
+  if (!pki) {
+    return NS_ERROR_FAILURE;
+  }
+
+  UniqueSECKEYPublicKey publicKey(SECKEY_ExtractPublicKey(pki.get()));
+  if (!publicKey) {
+    return NS_ERROR_FAILURE;
+  }
+
+  // Base 64 decode the signature
+  SECItem signatureItem;
+  PORT_Memset(&signatureItem, 0, sizeof(SECItem));
+  if (!NSSBase64_DecodeBuffer(arena.get(), &signatureItem,
+                              PromiseFlatCString(aSignature).get(),
+                              aSignature.Length())) {
+    return NS_ERROR_FAILURE;
+  }
 
-    // Decode the signature and algorithm
-    CERTSignedData sigData;
-    PORT_Memset(&sigData, 0, sizeof(CERTSignedData));
-    SECStatus ss = SEC_QuickDERDecodeItem(arena.get(), &sigData,
-                                          CERT_SignatureDataTemplate,
-                                          &signatureItem);
-    if (ss != SECSuccess) {
-        SECKEY_DestroyPublicKey(publicKey);
-        return NS_ERROR_FAILURE;
-    }
+  // Decode the signature and algorithm
+  CERTSignedData sigData;
+  PORT_Memset(&sigData, 0, sizeof(CERTSignedData));
+  SECStatus srv = SEC_QuickDERDecodeItem(arena.get(), &sigData,
+                                         CERT_SignatureDataTemplate,
+                                         &signatureItem);
+  if (srv != SECSuccess) {
+    return NS_ERROR_FAILURE;
+  }
 
-    // Perform the final verification
-    DER_ConvertBitString(&(sigData.signature));
-    ss = VFY_VerifyDataWithAlgorithmID((const unsigned char*)nsPromiseFlatCString(aData).get(),
-                                       aData.Length(), publicKey,
-                                       &(sigData.signature),
-                                       &(sigData.signatureAlgorithm),
-                                       nullptr, nullptr);
+  // Perform the final verification
+  DER_ConvertBitString(&(sigData.signature));
+  srv = VFY_VerifyDataWithAlgorithmID(
+    reinterpret_cast<const unsigned char*>(PromiseFlatCString(aData).get()),
+    aData.Length(), publicKey.get(), &(sigData.signature),
+    &(sigData.signatureAlgorithm), nullptr, nullptr);
 
-    // Clean up remaining objects
-    SECKEY_DestroyPublicKey(publicKey);
+  *_retval = (srv == SECSuccess);
 
-    *_retval = (ss == SECSuccess);
-
-    return NS_OK;
+  return NS_OK;
 }
 
 namespace mozilla {
 
 nsresult
 VerifyCMSDetachedSignatureIncludingCertificate(
   const SECItem& buffer, const SECItem& detachedDigest,
   nsresult (*verifyCertificate)(CERTCertificate* cert, void* context,
                                 void* pinArg),
-  void *verifyCertificateContext, void* pinArg)
+  void* verifyCertificateContext, void* pinArg,
+  const nsNSSShutDownPreventionLock& /*proofOfLock*/)
 {
   // XXX: missing pinArg is tolerated.
   if (NS_WARN_IF(!buffer.data && buffer.len > 0) ||
       NS_WARN_IF(!detachedDigest.data && detachedDigest.len > 0) ||
       (!verifyCertificate) ||
       NS_WARN_IF(!verifyCertificateContext)) {
     return NS_ERROR_INVALID_ARG;
   }
@@ -150,34 +162,36 @@ VerifyCMSDetachedSignatureIncludingCerti
   // Set digest value.
   if (NSS_CMSSignedData_SetDigestValue(signedData, SEC_OID_SHA1,
                                        const_cast<SECItem*>(&detachedDigest))) {
     return NS_ERROR_CMS_VERIFY_BAD_DIGEST;
   }
 
   // Parse the certificates into CERTCertificate objects held in memory so
   // verifyCertificate will be able to find them during path building.
-  ScopedCERTCertList certs(CERT_NewCertList());
+  UniqueCERTCertList certs(CERT_NewCertList());
   if (!certs) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
   if (signedData->rawCerts) {
     for (size_t i = 0; signedData->rawCerts[i]; ++i) {
-      ScopedCERTCertificate
+      UniqueCERTCertificate
         cert(CERT_NewTempCertificate(CERT_GetDefaultCertDB(),
                                      signedData->rawCerts[i], nullptr, false,
                                      true));
       // Skip certificates that fail to parse
-      if (cert) {
-        if (CERT_AddCertToListTail(certs.get(), cert.get()) == SECSuccess) {
-          cert.forget(); // ownership transfered
-        } else {
-          return NS_ERROR_OUT_OF_MEMORY;
-        }
+      if (!cert) {
+        continue;
       }
+
+      if (CERT_AddCertToListTail(certs.get(), cert.get()) != SECSuccess) {
+        return NS_ERROR_OUT_OF_MEMORY;
+      }
+
+      Unused << cert.release(); // Ownership transferred to the cert list.
     }
   }
 
   // Get the end-entity certificate.
   int numSigners = NSS_CMSSignedData_SignerInfoCount(signedData);
   if (NS_WARN_IF(numSigners != 1)) {
     return NS_ERROR_CMS_VERIFY_ERROR_PROCESSING;
   }
@@ -252,44 +266,46 @@ VerifyCertificate(CERTCertificate* cert,
 NS_IMETHODIMP
 nsDataSignatureVerifier::VerifySignature(const char* aRSABuf,
                                          uint32_t aRSABufLen,
                                          const char* aPlaintext,
                                          uint32_t aPlaintextLen,
                                          int32_t* aErrorCode,
                                          nsIX509Cert** aSigningCert)
 {
-  if (!aPlaintext || !aSigningCert || !aErrorCode) {
+  if (!aRSABuf || !aPlaintext || !aErrorCode || !aSigningCert) {
     return NS_ERROR_INVALID_ARG;
   }
 
+  nsNSSShutDownPreventionLock locker;
+  if (isAlreadyShutDown()) {
+    return NS_ERROR_NOT_AVAILABLE;
+  }
+
   *aErrorCode = VERIFY_ERROR_OTHER;
   *aSigningCert = nullptr;
 
-  nsNSSShutDownPreventionLock locker;
-
   Digest digest;
-  nsresult rv = digest.DigestBuf(SEC_OID_SHA1,
-                                 reinterpret_cast<const uint8_t*>(aPlaintext),
+  nsresult rv = digest.DigestBuf(SEC_OID_SHA1, uint8_t_ptr_cast(aPlaintext),
                                  aPlaintextLen);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return rv;
   }
 
   SECItem buffer = {
     siBuffer,
     reinterpret_cast<uint8_t*>(const_cast<char*>(aRSABuf)),
     aRSABufLen
   };
 
   VerifyCertificateContext context;
   // XXX: pinArg is missing
   rv = VerifyCMSDetachedSignatureIncludingCertificate(buffer, digest.get(),
                                                       VerifyCertificate,
-                                                      &context, nullptr);
+                                                      &context, nullptr, locker);
   if (NS_SUCCEEDED(rv)) {
     *aErrorCode = VERIFY_OK;
   } else if (NS_ERROR_GET_MODULE(rv) == NS_ERROR_MODULE_SECURITY) {
     if (rv == GetXPCOMFromNSSError(SEC_ERROR_UNKNOWN_ISSUER)) {
       *aErrorCode = VERIFY_ERROR_UNKNOWN_ISSUER;
     } else {
       *aErrorCode = VERIFY_ERROR_OTHER;
     }
--- a/security/manager/ssl/nsDataSignatureVerifier.h
+++ b/security/manager/ssl/nsDataSignatureVerifier.h
@@ -1,48 +1,47 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-#ifndef _NS_DATASIGNATUREVERIFIER_H_
-#define _NS_DATASIGNATUREVERIFIER_H_
-
-#include "nsIDataSignatureVerifier.h"
-#include "mozilla/Attributes.h"
+#ifndef nsDataSignatureVerifier_h
+#define nsDataSignatureVerifier_h
 
-#include "keythi.h"
+#include "certt.h"
+#include "nsIDataSignatureVerifier.h"
+#include "nsNSSShutDown.h"
 
-typedef struct CERTCertificateStr CERTCertificate;
-
-// 296d76aa-275b-4f3c-af8a-30a4026c18fc
 #define NS_DATASIGNATUREVERIFIER_CID \
     { 0x296d76aa, 0x275b, 0x4f3c, \
     { 0xaf, 0x8a, 0x30, 0xa4, 0x02, 0x6c, 0x18, 0xfc } }
 #define NS_DATASIGNATUREVERIFIER_CONTRACTID \
     "@mozilla.org/security/datasignatureverifier;1"
 
 class nsDataSignatureVerifier final : public nsIDataSignatureVerifier
+                                    , public nsNSSShutDownObject
 {
 public:
   NS_DECL_ISUPPORTS
   NS_DECL_NSIDATASIGNATUREVERIFIER
 
   nsDataSignatureVerifier()
   {
   }
 
 private:
-  ~nsDataSignatureVerifier()
-  {
-  }
+  ~nsDataSignatureVerifier();
+
+  // Nothing to release.
+  virtual void virtualDestroyNSSReference() override {}
 };
 
 namespace mozilla {
 
 nsresult VerifyCMSDetachedSignatureIncludingCertificate(
   const SECItem& buffer, const SECItem& detachedDigest,
   nsresult (*verifyCertificate)(CERTCertificate* cert, void* context,
                                 void* pinArg),
-  void* verifyCertificateContext, void* pinArg);
+  void* verifyCertificateContext, void* pinArg,
+  const nsNSSShutDownPreventionLock& proofOfLock);
 
 } // namespace mozilla
 
-#endif // _NS_DATASIGNATUREVERIFIER_H_
+#endif // nsDataSignatureVerifier_h