Bug 1281569 - Remove unnecessary step of converting nsIX509Certs to Raw DER just to create a CERTCertificate in nsNSSCertificateDB. draft
authorCykesiopka <cykesiopka.bmo@gmail.com>
Tue, 05 Jul 2016 02:59:18 -0700
changeset 383923 caedac2fe32944bb8d0fa95919b2e7611bbead76
parent 383922 7a504a73b6f26d2afe6e947f0e6309662091b1f5
child 524574 0a2621c8969faaccaed9dffafd9bde36149c825e
push id22127
push usercykesiopka.bmo@gmail.com
push dateTue, 05 Jul 2016 10:04:38 +0000
bugs1281569
milestone50.0a1
Bug 1281569 - Remove unnecessary step of converting nsIX509Certs to Raw DER just to create a CERTCertificate in nsNSSCertificateDB. 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
@@ -252,17 +252,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
@@ -277,17 +277,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
@@ -330,38 +329,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;
   }
 
@@ -385,22 +368,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;
   }
@@ -408,32 +389,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,
@@ -474,17 +454,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
@@ -1353,76 +1333,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 */ \