Bug 1281569 - Remove unnecessary step of converting nsIX509Certs to Raw DER just to create a CERTCertificate in nsNSSCertificateDB. r=keeler
authorCykesiopka <cykesiopka.bmo@gmail.com>
Tue, 05 Jul 2016 02:59:18 -0700
changeset 304153 8ae66227417f77a77459047cd0d48ac612ed7e7c
parent 304152 0e79ff93d0515ce5f830851f97cb6b2fdbb58e9d
child 304154 8e94b77f805bcad09be127117c7407c3d783b67b
push id30414
push usercbook@mozilla.com
push dateFri, 08 Jul 2016 09:59:01 +0000
treeherdermozilla-central@45682df2d2d4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskeeler
bugs1281569
milestone50.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 1281569 - Remove unnecessary step of converting nsIX509Certs to Raw DER just to create a CERTCertificate in nsNSSCertificateDB. r=keeler There are a few places in nsNSSCertificateDB.cpp where the following is done: 1. GetRawDER() is called on a nsIX509Cert to obtain the DER representation of the cert. 2. The DER is used to construct a CERTCertificate for use with NSS functions. This step of converting to the DER is unnecessary, since GetCert() will provide an already constructed CERTCertificate. MozReview-Commit-ID: 35KMYI7dCXc
security/manager/ssl/nsNSSCertificateDB.cpp
security/manager/ssl/nsNSSCertificateDB.h
--- a/security/manager/ssl/nsNSSCertificateDB.cpp
+++ b/security/manager/ssl/nsNSSCertificateDB.cpp
@@ -254,17 +254,17 @@ nsNSSCertificateDB::getCertsFromPackage(
                              collectArgs) != SECSuccess) {
     return nullptr;
   }
 
   return collectArgs;
 }
 
 nsresult
-nsNSSCertificateDB::handleCACertDownload(nsIArray *x509Certs,
+nsNSSCertificateDB::handleCACertDownload(NotNull<nsIArray*> x509Certs,
                                          nsIInterfaceRequestor *ctx,
                                          const nsNSSShutDownPreventionLock &proofOfLock)
 {
   // First thing we have to do is figure out which certificate we're 
   // gonna present to the user.  The CA may have sent down a list of 
   // certs which may or may not be a chained list of certs.  Until
   // the day we can design some solid UI for the general case, we'll
   // code to the > 90% case.  That case is where a CA sends down a
@@ -279,17 +279,16 @@ nsNSSCertificateDB::handleCACertDownload
   uint32_t numCerts;
 
   x509Certs->GetLength(&numCerts);
   NS_ASSERTION(numCerts > 0, "Didn't get any certs to import.");
   if (numCerts == 0)
     return NS_OK; // Nothing to import, so nothing to do.
 
   nsCOMPtr<nsIX509Cert> certToShow;
-  nsCOMPtr<nsISupports> isupports;
   uint32_t selCertIndex;
   if (numCerts == 1) {
     // There's only one cert, so let's show it.
     selCertIndex = 0;
     certToShow = do_QueryElementAt(x509Certs, selCertIndex);
   } else {
     nsCOMPtr<nsIX509Cert> cert0;    // first cert
     nsCOMPtr<nsIX509Cert> cert1;    // second cert
@@ -332,38 +331,22 @@ nsNSSCertificateDB::handleCACertDownload
 
   if (!certToShow)
     return NS_ERROR_FAILURE;
 
   nsCOMPtr<nsICertificateDialogs> dialogs;
   nsresult rv = ::getNSSDialogs(getter_AddRefs(dialogs), 
                                 NS_GET_IID(nsICertificateDialogs),
                                 NS_CERTIFICATEDIALOGS_CONTRACTID);
-                       
-  if (NS_FAILED(rv))
+  if (NS_FAILED(rv)) {
     return rv;
- 
-  SECItem der;
-  rv=certToShow->GetRawDER(&der.len, (uint8_t **)&der.data);
-
-  if (NS_FAILED(rv))
-    return rv;
+  }
 
-  MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("Creating temp cert\n"));
-  CERTCertDBHandle *certdb = CERT_GetDefaultCertDB();
-  UniqueCERTCertificate tmpCert(CERT_FindCertByDERCert(certdb, &der));
+  UniqueCERTCertificate tmpCert(certToShow->GetCert());
   if (!tmpCert) {
-    tmpCert.reset(CERT_NewTempCertificate(certdb, &der, nullptr, false, true));
-  }
-  free(der.data);
-  der.data = nullptr;
-  der.len = 0;
-
-  if (!tmpCert) {
-    NS_ERROR("Couldn't create cert from DER blob");
     return NS_ERROR_FAILURE;
   }
 
   if (!CERT_IsCACert(tmpCert.get(), nullptr)) {
     DisplayCertificateAlert(ctx, "NotACACert", certToShow, proofOfLock);
     return NS_ERROR_FAILURE;
   }
 
@@ -387,22 +370,20 @@ nsNSSCertificateDB::handleCACertDownload
   MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("Created nick \"%s\"\n", nickname.get()));
 
   nsNSSCertTrust trust;
   trust.SetValidCA();
   trust.AddCATrust(!!(trustBits & nsIX509CertDB::TRUSTED_SSL),
                    !!(trustBits & nsIX509CertDB::TRUSTED_EMAIL),
                    !!(trustBits & nsIX509CertDB::TRUSTED_OBJSIGN));
 
-  SECStatus srv = __CERT_AddTempCertToPerm(tmpCert.get(),
-                                           const_cast<char*>(nickname.get()),
-                                           trust.GetTrust());
-
-  if (srv != SECSuccess)
+  if (CERT_AddTempCertToPerm(tmpCert.get(), nickname.get(),
+                             trust.GetTrust()) != SECSuccess) {
     return NS_ERROR_FAILURE;
+  }
 
   // Import additional delivered certificates that can be verified.
 
   // build a CertList for filtering
   UniqueCERTCertList certList(CERT_NewCertList());
   if (!certList) {
     return NS_ERROR_FAILURE;
   }
@@ -410,32 +391,31 @@ nsNSSCertificateDB::handleCACertDownload
   // get all remaining certs into temp store
 
   for (uint32_t i=0; i<numCerts; i++) {
     if (i == selCertIndex) {
       // we already processed that one
       continue;
     }
 
-    certToShow = do_QueryElementAt(x509Certs, i);
-    certToShow->GetRawDER(&der.len, (uint8_t **)&der.data);
-
-    CERTCertificate *tmpCert2 = 
-      CERT_NewTempCertificate(certdb, &der, nullptr, false, true);
+    nsCOMPtr<nsIX509Cert> remainingCert = do_QueryElementAt(x509Certs, i);
+    if (!remainingCert) {
+      continue;
+    }
 
-    free(der.data);
-    der.data = nullptr;
-    der.len = 0;
-
+    UniqueCERTCertificate tmpCert2(remainingCert->GetCert());
     if (!tmpCert2) {
-      NS_ERROR("Couldn't create temp cert from DER blob");
       continue;  // Let's try to import the rest of 'em
     }
 
-    CERT_AddCertToListTail(certList.get(), tmpCert2);
+    if (CERT_AddCertToListTail(certList.get(), tmpCert2.get()) != SECSuccess) {
+      continue;
+    }
+
+    Unused << tmpCert2.release();
   }
 
   return ImportValidCACertsInList(certList, ctx, proofOfLock);
 }
 
 NS_IMETHODIMP
 nsNSSCertificateDB::ImportCertificates(uint8_t* data, uint32_t length,
                                        uint32_t type,
@@ -476,17 +456,17 @@ nsNSSCertificateDB::ImportCertificates(u
       return NS_ERROR_FAILURE;
     }
     nsresult rv = array->AppendElement(cert, false);
     if (NS_FAILED(rv)) {
       return rv;
     }
   }
 
-  return handleCACertDownload(array, ctx, locker);
+  return handleCACertDownload(WrapNotNull(array), ctx, locker);
 }
 
 /**
  * Filters an array of certs by usage and imports them into temporary storage.
  *
  * @param numcerts
  *        Size of the |certs| array.
  * @param certs
@@ -1355,76 +1335,59 @@ nsNSSCertificateDB::get_default_nickname
     }
     if (!dummycert) {
       break;
     }
     count++;
   }
 }
 
-NS_IMETHODIMP nsNSSCertificateDB::AddCertFromBase64(const char* aBase64,
-                                                    const char* aTrust,
-                                                    const char* aName)
+NS_IMETHODIMP
+nsNSSCertificateDB::AddCertFromBase64(const char* aBase64, const char* aTrust,
+                                      const char* /*aName*/)
 {
   NS_ENSURE_ARG_POINTER(aBase64);
-  nsCOMPtr <nsIX509Cert> newCert;
+  NS_ENSURE_ARG_POINTER(aTrust);
 
   nsNSSShutDownPreventionLock locker;
   if (isAlreadyShutDown()) {
     return NS_ERROR_NOT_AVAILABLE;
   }
 
   nsNSSCertTrust trust;
+  if (CERT_DecodeTrustString(trust.GetTrust(), aTrust) != SECSuccess) {
+    return NS_ERROR_FAILURE;
+  }
 
-  // need to calculate the trust bits from the aTrust string.
-  SECStatus stat = CERT_DecodeTrustString(trust.GetTrust(),
-    /* this is const, but not declared that way */(char *) aTrust);
-  NS_ENSURE_STATE(stat == SECSuccess); // if bad trust passed in, return error.
-
-
+  nsCOMPtr<nsIX509Cert> newCert;
   nsresult rv = ConstructX509FromBase64(aBase64, getter_AddRefs(newCert));
   NS_ENSURE_SUCCESS(rv, rv);
 
-  SECItem der;
-  rv = newCert->GetRawDER(&der.len, (uint8_t **)&der.data);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("Creating temp cert\n"));
-  CERTCertDBHandle *certdb = CERT_GetDefaultCertDB();
-  UniqueCERTCertificate tmpCert(CERT_FindCertByDERCert(certdb, &der));
+  UniqueCERTCertificate tmpCert(newCert->GetCert());
   if (!tmpCert) {
-    tmpCert.reset(CERT_NewTempCertificate(certdb, &der, nullptr, false, true));
-  }
-  free(der.data);
-  der.data = nullptr;
-  der.len = 0;
-
-  if (!tmpCert) {
-    NS_ERROR("Couldn't create cert from DER blob");
-    return MapSECStatus(SECFailure);
+    return NS_ERROR_FAILURE;
   }
 
-   // If there's already a certificate that matches this one in the database,
-   // we still want to set its trust to the given value.
+  // If there's already a certificate that matches this one in the database, we
+  // still want to set its trust to the given value.
   if (tmpCert->isperm) {
     return SetCertTrustFromString(newCert, aTrust);
   }
 
   UniquePORTString nickname(CERT_MakeCANickname(tmpCert.get()));
 
   MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("Created nick \"%s\"\n", nickname.get()));
 
   rv = attemptToLogInWithDefaultPassword();
   if (NS_WARN_IF(rv != NS_OK)) {
     return rv;
   }
 
-  SECStatus srv = __CERT_AddTempCertToPerm(tmpCert.get(),
-                                           const_cast<char*>(nickname.get()),
-                                           trust.GetTrust());
+  SECStatus srv = CERT_AddTempCertToPerm(tmpCert.get(), nickname.get(),
+                                         trust.GetTrust());
   return MapSECStatus(srv);
 }
 
 NS_IMETHODIMP
 nsNSSCertificateDB::AddCert(const nsACString & aCertDER, const char *aTrust,
                             const char *aName)
 {
   nsCString base64;
--- a/security/manager/ssl/nsNSSCertificateDB.h
+++ b/security/manager/ssl/nsNSSCertificateDB.h
@@ -3,16 +3,17 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef nsNSSCertificateDB_h
 #define nsNSSCertificateDB_h
 
 #include "ScopedNSSTypes.h"
 #include "certt.h"
 #include "mozilla/Mutex.h"
+#include "mozilla/NotNull.h"
 #include "mozilla/RefPtr.h"
 #include "mozilla/UniquePtr.h"
 #include "nsIX509CertDB.h"
 #include "nsNSSShutDown.h"
 #include "nsString.h"
 
 class nsCString;
 class nsIArray;
@@ -53,17 +54,17 @@ private:
 
   static void DisplayCertificateAlert(nsIInterfaceRequestor *ctx, 
                                       const char *stringID, nsIX509Cert *certToShow,
                                       const nsNSSShutDownPreventionLock &proofOfLock);
 
   CERTDERCerts* getCertsFromPackage(const mozilla::UniquePLArenaPool& arena,
                                     uint8_t* data, uint32_t length,
                                     const nsNSSShutDownPreventionLock& proofOfLock);
-  nsresult handleCACertDownload(nsIArray *x509Certs, 
+  nsresult handleCACertDownload(mozilla::NotNull<nsIArray*> x509Certs,
                                 nsIInterfaceRequestor *ctx,
                                 const nsNSSShutDownPreventionLock &proofOfLock);
 
   // We don't own any NSS objects here, so no need to clean up
   virtual void virtualDestroyNSSReference() override { };
 };
 
 #define NS_X509CERTDB_CID { /* fb0bbc5c-452e-4783-b32c-80124693d871 */ \