Bug 1029173 - Clean up nsDataSignatureVerifier. r=keeler
authorCykesiopka <cykesiopka.bmo@gmail.com>
Tue, 12 Apr 2016 18:09:06 -0700
changeset 293006 2c9daa36cfd7d8ba0a75220cb9453817f816acde
parent 293005 aeb302aeff162e1d0f7b80359a9e9a8de1ce086b
child 293007 f88bfa3062821076a4aabef319422ce705d9715c
push id30172
push userkwierso@gmail.com
push dateWed, 13 Apr 2016 21:18:48 +0000
treeherdermozilla-central@bc2373295e31 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskeeler
bugs1029173
milestone48.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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