Bug 1294011 - Obviate manual calls to SECITEM_FreeItem() in PSM. r=keeler
authorCykesiopka <cykesiopka.bmo@gmail.com>
Sat, 13 Aug 2016 21:45:00 +0800
changeset 337939 a1bcaa3640e51d8ab2b24faf7d28933ab6b33b37
parent 337938 f51506eaa0485b2ab149f24dd930e5f38b0784d7
child 337940 a70440c20f48f29bc3939c8ffa97ed6b3c2e96c0
push idunknown
push userunknown
push dateunknown
reviewerskeeler
bugs1294011
milestone51.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 1294011 - Obviate manual calls to SECITEM_FreeItem() in PSM. r=keeler MozReview-Commit-ID: 7RNV0YNraBx
security/certverifier/ExtendedValidation.cpp
security/manager/ssl/nsKeygenHandler.cpp
security/manager/ssl/nsKeygenHandler.h
security/manager/ssl/nsNSSCertificateFakeTransport.cpp
security/manager/ssl/nsNSSCertificateFakeTransport.h
security/manager/ssl/nsNTLMAuthModule.cpp
security/manager/ssl/nsPKCS12Blob.cpp
--- a/security/certverifier/ExtendedValidation.cpp
+++ b/security/certverifier/ExtendedValidation.cpp
@@ -1253,26 +1253,23 @@ static struct nsMyTrustedEVInfo myTruste
     "PFZlcmlTaWduIENsYXNzIDMgUHVibGljIFByaW1hcnkgQ2VydGlmaWNhdGlvbiBB"
     "dXRob3JpdHkgLSBHNA==",
     "L4D+I4wOIg9IZxIokYessw==",
     nullptr
   },
 };
 
 static SECOidTag
-register_oid(const SECItem* oid_item, const char* oid_name)
+RegisterOID(const SECItem& oidItem, const char* oidName)
 {
-  if (!oid_item)
-    return SEC_OID_UNKNOWN;
-
   SECOidData od;
-  od.oid.len = oid_item->len;
-  od.oid.data = oid_item->data;
+  od.oid.len = oidItem.len;
+  od.oid.data = oidItem.data;
   od.offset = SEC_OID_UNKNOWN;
-  od.desc = oid_name;
+  od.desc = oidName;
   od.mechanism = CKM_INVALID_MECHANISM;
   od.supportedExtension = INVALID_CERT_EXTENSION;
   return SECOID_AddEntry(&od);
 }
 
 static bool
 isEVPolicy(SECOidTag policyOIDTag)
 {
@@ -1312,39 +1309,39 @@ CertIsAuthoritativeForEVPolicy(const Uni
 }
 
 static PRStatus
 IdentityInfoInit()
 {
   for (size_t iEV = 0; iEV < PR_ARRAY_SIZE(myTrustedEVInfos); ++iEV) {
     nsMyTrustedEVInfo& entry = myTrustedEVInfos[iEV];
 
-    SECStatus rv;
-    CERTIssuerAndSN ias;
-
-    rv = ATOB_ConvertAsciiToItem(&ias.derIssuer, const_cast<char*>(entry.issuer_base64));
+    mozilla::ScopedAutoSECItem derIssuer;
+    SECStatus rv = ATOB_ConvertAsciiToItem(&derIssuer, entry.issuer_base64);
     PR_ASSERT(rv == SECSuccess);
     if (rv != SECSuccess) {
       return PR_FAILURE;
     }
-    rv = ATOB_ConvertAsciiToItem(&ias.serialNumber,
-                                 const_cast<char*>(entry.serial_base64));
+
+    mozilla::ScopedAutoSECItem serialNumber;
+    rv = ATOB_ConvertAsciiToItem(&serialNumber, entry.serial_base64);
     PR_ASSERT(rv == SECSuccess);
     if (rv != SECSuccess) {
-      SECITEM_FreeItem(&ias.derIssuer, false);
       return PR_FAILURE;
     }
 
+    CERTIssuerAndSN ias;
+    ias.derIssuer.data = derIssuer.data;
+    ias.derIssuer.len = derIssuer.len;
+    ias.serialNumber.data = serialNumber.data;
+    ias.serialNumber.len = serialNumber.len;
     ias.serialNumber.type = siUnsignedInteger;
 
     entry.cert = UniqueCERTCertificate(CERT_FindCertByIssuerAndSN(nullptr, &ias));
 
-    SECITEM_FreeItem(&ias.derIssuer, false);
-    SECITEM_FreeItem(&ias.serialNumber, false);
-
     // If an entry is missing in the NSS root database, it may be because the
     // root database is out of sync with what we expect (e.g. a different
     // version of system NSS is installed). We assert on debug builds, but
     // silently continue on release builds. In both cases, the root cert does
     // not get EV treatment.
     if (!entry.cert) {
 #ifdef DEBUG
       // The debug CA structs are at positions 0 to NUM_TEST_EV_ROOTS - 1, and
@@ -1362,28 +1359,24 @@ IdentityInfoInit()
                       entry.cert->derCert.data,
                       static_cast<int32_t>(entry.cert->derCert.len));
     PR_ASSERT(rv == SECSuccess);
     if (rv == SECSuccess) {
       bool same = !memcmp(certFingerprint, entry.ev_root_sha256_fingerprint,
                           sizeof(certFingerprint));
       PR_ASSERT(same);
       if (same) {
-
-        SECItem ev_oid_item;
-        ev_oid_item.data = nullptr;
-        ev_oid_item.len = 0;
-        rv = SEC_StringToOID(nullptr, &ev_oid_item, entry.dotted_oid, 0);
+        mozilla::ScopedAutoSECItem evOIDItem;
+        rv = SEC_StringToOID(nullptr, &evOIDItem, entry.dotted_oid, 0);
         PR_ASSERT(rv == SECSuccess);
         if (rv == SECSuccess) {
-          entry.oid_tag = register_oid(&ev_oid_item, entry.oid_name);
+          entry.oid_tag = RegisterOID(evOIDItem, entry.oid_name);
           if (entry.oid_tag == SEC_OID_UNKNOWN) {
             rv = SECFailure;
           }
-          SECITEM_FreeItem(&ev_oid_item, false);
         }
       } else {
         PR_SetError(SEC_ERROR_BAD_DATA, 0);
         rv = SECFailure;
       }
     }
 
     if (rv != SECSuccess) {
--- a/security/manager/ssl/nsKeygenHandler.cpp
+++ b/security/manager/ssl/nsKeygenHandler.cpp
@@ -1,38 +1,36 @@
 /* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
  *
  * 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/. */
 
-#include "secdert.h"
-#include "nspr.h"
-#include "nsNSSComponent.h" // for PIPNSS string bundle calls.
+#include "base64.h"
+#include "cryptohi.h"
 #include "keyhi.h"
-#include "secder.h"
-#include "cryptohi.h"
-#include "base64.h"
-#include "secasn1.h"
+#include "mozilla/Assertions.h"
+#include "mozilla/Telemetry.h"
+#include "nsIContent.h"
+#include "nsIDOMHTMLSelectElement.h"
+#include "nsIGenKeypairInfoDlg.h"
+#include "nsIServiceManager.h"
+#include "nsITokenDialogs.h"
 #include "nsKeygenHandler.h"
 #include "nsKeygenHandlerContent.h"
-#include "nsIServiceManager.h"
-#include "nsIDOMHTMLSelectElement.h"
-#include "nsIContent.h"
 #include "nsKeygenThread.h"
+#include "nsNSSComponent.h" // for PIPNSS string bundle calls.
 #include "nsNSSHelper.h"
 #include "nsReadableUtils.h"
 #include "nsUnicharUtils.h"
-#include "nsCRT.h"
-#include "nsITokenDialogs.h"
-#include "nsIGenKeypairInfoDlg.h"
-#include "nsNSSShutDown.h"
 #include "nsXULAppAPI.h"
-
-#include "mozilla/Telemetry.h"
+#include "nspr.h"
+#include "secasn1.h"
+#include "secder.h"
+#include "secdert.h"
 
 //These defines are taken from the PKCS#11 spec
 #define CKM_RSA_PKCS_KEY_PAIR_GEN     0x00000000
 #define CKM_DH_PKCS_KEY_PAIR_GEN      0x00000020
 
 DERTemplate SECAlgorithmIDTemplate[] = {
     { DER_SEQUENCE,
           0, nullptr, sizeof(SECAlgorithmID) },
@@ -145,20 +143,19 @@ static CurveNameTagPair nameTagPair[] =
   { "nistb409", SEC_OID_SECG_EC_SECT409R1},
   { "sect571k1", SEC_OID_SECG_EC_SECT571K1},
   { "nistk571", SEC_OID_SECG_EC_SECT571K1},
   { "sect571r1", SEC_OID_SECG_EC_SECT571R1},
   { "nistb571", SEC_OID_SECG_EC_SECT571R1},
 
 };
 
-SECKEYECParams * 
-decode_ec_params(const char *curve)
+mozilla::UniqueSECItem
+DecodeECParams(const char* curve)
 {
-    SECKEYECParams *ecparams;
     SECOidData *oidData = nullptr;
     SECOidTag curveOidTag = SEC_OID_UNKNOWN; /* default */
     int i, numCurves;
 
     if (curve && *curve) {
         numCurves = sizeof(nameTagPair)/sizeof(CurveNameTagPair);
         for (i = 0; ((i < numCurves) && (curveOidTag == SEC_OID_UNKNOWN)); 
              i++) {
@@ -168,20 +165,21 @@ decode_ec_params(const char *curve)
     }
 
     /* Return nullptr if curve name is not recognized */
     if ((curveOidTag == SEC_OID_UNKNOWN) || 
         (oidData = SECOID_FindOIDByTag(curveOidTag)) == nullptr) {
         return nullptr;
     }
 
-    ecparams = SECITEM_AllocItem(nullptr, nullptr, (2 + oidData->oid.len));
-
-    if (!ecparams)
-      return nullptr;
+    mozilla::UniqueSECItem ecparams(SECITEM_AllocItem(nullptr, nullptr,
+                                                      2 + oidData->oid.len));
+    if (!ecparams) {
+        return nullptr;
+    }
 
     /* 
      * ecparams->data needs to contain the ASN encoding of an object ID (OID)
      * representing the named curve. The actual OID is in 
      * oidData->oid.data so we simply prepend 0x06 and OID length
      */
     ecparams->data[0] = SEC_ASN1_OBJECT_ID;
     ecparams->data[1] = oidData->oid.len;
@@ -409,31 +407,30 @@ GatherKeygenTelemetry(uint32_t keyGenMec
     nsCString telemetryValue("rsa");
     telemetryValue.AppendPrintf("%d", keysize);
     mozilla::Telemetry::Accumulate(
         mozilla::Telemetry::KEYGEN_GENERATED_KEY_TYPE, telemetryValue);
   } else if (keyGenMechanism == CKM_EC_KEY_PAIR_GEN) {
     nsCString secp384r1 = NS_LITERAL_CSTRING("secp384r1");
     nsCString secp256r1 = NS_LITERAL_CSTRING("secp256r1");
 
-    SECKEYECParams* decoded = decode_ec_params(curve);
+    mozilla::UniqueSECItem decoded = DecodeECParams(curve);
     if (!decoded) {
       switch (keysize) {
         case 2048:
           mozilla::Telemetry::Accumulate(
               mozilla::Telemetry::KEYGEN_GENERATED_KEY_TYPE, secp384r1);
           break;
         case 1024:
         case 512:
           mozilla::Telemetry::Accumulate(
               mozilla::Telemetry::KEYGEN_GENERATED_KEY_TYPE, secp256r1);
           break;
       }
     } else {
-      SECITEM_FreeItem(decoded, true);
       if (secp384r1.EqualsIgnoreCase(curve, secp384r1.Length())) {
           mozilla::Telemetry::Accumulate(
               mozilla::Telemetry::KEYGEN_GENERATED_KEY_TYPE, secp384r1);
       } else if (secp256r1.EqualsIgnoreCase(curve, secp256r1.Length())) {
           mozilla::Telemetry::Accumulate(
               mozilla::Telemetry::KEYGEN_GENERATED_KEY_TYPE, secp256r1);
       } else {
         mozilla::Telemetry::Accumulate(
@@ -459,19 +456,20 @@ nsKeygenFormProcessor::GetPublicKey(cons
     }
 
     nsresult rv = NS_ERROR_FAILURE;
     UniquePORTString keystring;
     char *keyparamsString = nullptr;
     uint32_t keyGenMechanism;
     PK11SlotInfo *slot = nullptr;
     PK11RSAGenParams rsaParams;
+    mozilla::UniqueSECItem ecParams;
     SECOidTag algTag;
     int keysize = 0;
-    void *params = nullptr;
+    void *params = nullptr; // Non-owning.
     SECKEYPrivateKey *privateKey = nullptr;
     SECKEYPublicKey *publicKey = nullptr;
     CERTSubjectPublicKeyInfo *spkInfo = nullptr;
     SECStatus srv = SECFailure;
     SECItem spkiItem;
     SECItem pkacItem;
     SECItem signedItem;
     CERTPublicKeyAndChallenge pkac;
@@ -540,34 +538,37 @@ nsKeygenFormProcessor::GetPublicKey(cons
              * respectively depending on the user's selection
              * (High, Medium, Low). 
              * (RSA uses RSA-2048, RSA-1024 and RSA-512 for historical
              * reasons, while ECC choices represent a stronger mapping)
              * NOTE: The user's selection
              * is silently ignored when a valid curve is presented
              * in keyparams.
              */
-            if ((params = decode_ec_params(keyparamsString)) == nullptr) {
+            ecParams = DecodeECParams(keyparamsString);
+            if (!ecParams) {
                 /* The keyparams attribute did not specify a valid
                  * curve name so use a curve based on the keysize.
                  * NOTE: Here keysize is used only as an indication of
                  * High/Medium/Low strength; elliptic curve
                  * cryptography uses smaller keys than RSA to provide
                  * equivalent security.
                  */
                 switch (keysize) {
                 case 2048:
-                    params = decode_ec_params("secp384r1");
+                    ecParams = DecodeECParams("secp384r1");
                     break;
                 case 1024:
                 case 512:
-                    params = decode_ec_params("secp256r1");
+                    ecParams = DecodeECParams("secp256r1");
                     break;
                 } 
             }
+            MOZ_ASSERT(ecParams);
+            params = ecParams.get();
             /* XXX The signature algorithm ought to choose the hashing
              * algorithm based on key size once ECDSA variations based
              * on SHA256 SHA384 and SHA512 are standardized.
              */
             algTag = SEC_OID_ANSIX962_ECDSA_SIGNATURE_WITH_SHA1_DIGEST;
             break;
       default:
           goto loser;
@@ -706,22 +707,16 @@ loser:
         NS_RELEASE(KeygenRunnable);
     }
     if (keyparamsString) {
         free(keyparamsString);
     }
     if (pkac.challenge.data) {
         free(pkac.challenge.data);
     }
-    // If params is non-null and doesn't point to rsaParams, it was allocated
-    // in decode_ec_params. We have to free this memory.
-    if (params && params != &rsaParams) {
-        SECITEM_FreeItem(static_cast<SECItem*>(params), true);
-        params = nullptr;
-    }
     return rv;
 }
 
 // static
 void
 nsKeygenFormProcessor::ExtractParams(nsIDOMHTMLElement* aElement,
                                      nsAString& challengeValue,
                                      nsAString& keyTypeValue,
--- a/security/manager/ssl/nsKeygenHandler.h
+++ b/security/manager/ssl/nsKeygenHandler.h
@@ -2,16 +2,17 @@
  *
  * 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 nsKeygenHandler_h
 #define nsKeygenHandler_h
 
+#include "ScopedNSSTypes.h"
 #include "keythi.h"
 #include "nsCOMPtr.h"
 #include "nsError.h"
 #include "nsIFormProcessor.h"
 #include "nsIInterfaceRequestor.h"
 #include "nsNSSShutDown.h"
 #include "nsTArray.h"
 #include "secmodt.h"
@@ -19,17 +20,17 @@
 nsresult GetSlotWithMechanism(uint32_t mechanism,
                               nsIInterfaceRequestor* ctx,
                               PK11SlotInfo** retSlot,
                               nsNSSShutDownPreventionLock& /*proofOfLock*/);
 
 #define DEFAULT_RSA_KEYGEN_PE 65537L
 #define DEFAULT_RSA_KEYGEN_ALG SEC_OID_PKCS1_MD5_WITH_RSA_ENCRYPTION
 
-SECKEYECParams *decode_ec_params(const char *curve);
+mozilla::UniqueSECItem DecodeECParams(const char* curve);
 
 class nsKeygenFormProcessor : public nsIFormProcessor
                             , public nsNSSShutDownObject
 {
 public:
   nsKeygenFormProcessor();
   nsresult Init();
 
--- a/security/manager/ssl/nsNSSCertificateFakeTransport.cpp
+++ b/security/manager/ssl/nsNSSCertificateFakeTransport.cpp
@@ -20,19 +20,17 @@ NS_IMPL_ISUPPORTS(nsNSSCertificateFakeTr
 
 nsNSSCertificateFakeTransport::nsNSSCertificateFakeTransport()
   : mCertSerialization(nullptr)
 {
 }
 
 nsNSSCertificateFakeTransport::~nsNSSCertificateFakeTransport()
 {
-  if (mCertSerialization) {
-    SECITEM_FreeItem(mCertSerialization, true);
-  }
+  mCertSerialization = nullptr;
 }
 
 NS_IMETHODIMP
 nsNSSCertificateFakeTransport::GetDbKey(nsACString&)
 {
   NS_NOTREACHED("Unimplemented on content process");
   return NS_ERROR_NOT_IMPLEMENTED;
 }
@@ -258,20 +256,21 @@ nsNSSCertificateFakeTransport::Read(nsIO
   rv = aStream->ReadBytes(len, getter_Copies(str));
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   // On a non-chrome process we cannot instatiate mCert because we lack
   // nsNSSComponent. nsNSSCertificateFakeTransport object is used only to
   // carry the certificate serialization.
-
-  mCertSerialization = SECITEM_AllocItem(nullptr, nullptr, len);
-  if (!mCertSerialization)
-      return NS_ERROR_OUT_OF_MEMORY;
+  mCertSerialization =
+    mozilla::UniqueSECItem(SECITEM_AllocItem(nullptr, nullptr, len));
+  if (!mCertSerialization) {
+    return NS_ERROR_OUT_OF_MEMORY;
+  }
   PORT_Memcpy(mCertSerialization->data, str.Data(), len);
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsNSSCertificateFakeTransport::GetInterfaces(uint32_t* count, nsIID*** array)
 {
--- a/security/manager/ssl/nsNSSCertificateFakeTransport.h
+++ b/security/manager/ssl/nsNSSCertificateFakeTransport.h
@@ -1,16 +1,17 @@
 /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* 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 nsNSSCertificateFakeTransport_h
 #define nsNSSCertificateFakeTransport_h
 
+#include "ScopedNSSTypes.h"
 #include "mozilla/Vector.h"
 #include "nsCOMPtr.h"
 #include "nsIClassInfo.h"
 #include "nsISerializable.h"
 #include "nsIX509Cert.h"
 #include "nsIX509CertList.h"
 #include "secitem.h"
 
@@ -25,17 +26,17 @@ public:
   NS_DECL_NSICLASSINFO
 
   nsNSSCertificateFakeTransport();
 
 protected:
   virtual ~nsNSSCertificateFakeTransport();
 
 private:
-  SECItem* mCertSerialization;
+  mozilla::UniqueSECItem mCertSerialization;
 };
 
 class nsNSSCertListFakeTransport : public nsIX509CertList,
                                    public nsISerializable
 {
 public:
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_NSIX509CERTLIST
--- a/security/manager/ssl/nsNTLMAuthModule.cpp
+++ b/security/manager/ssl/nsNTLMAuthModule.cpp
@@ -1113,48 +1113,49 @@ des_makekey(const uint8_t *raw, uint8_t 
 
 // run des encryption algorithm (using NSS)
 static void
 des_encrypt(const uint8_t *key, const uint8_t *src, uint8_t *hash)
 {
   CK_MECHANISM_TYPE cipherMech = CKM_DES_ECB;
   PK11SymKey *symkey = nullptr;
   PK11Context *ctxt = nullptr;
-  SECItem keyItem, *param = nullptr;
+  SECItem keyItem;
+  mozilla::UniqueSECItem param;
   SECStatus rv;
   unsigned int n;
 
   mozilla::UniquePK11SlotInfo slot(PK11_GetBestSlot(cipherMech, nullptr));
   if (!slot)
   {
     NS_ERROR("no slot");
     goto done;
   }
 
-  keyItem.data = (uint8_t *) key;
+  keyItem.data = const_cast<uint8_t*>(key);
   keyItem.len = 8;
   symkey = PK11_ImportSymKey(slot.get(), cipherMech,
                              PK11_OriginUnwrap, CKA_ENCRYPT,
                              &keyItem, nullptr);
   if (!symkey)
   {
     NS_ERROR("no symkey");
     goto done;
   }
 
   // no initialization vector required
-  param = PK11_ParamFromIV(cipherMech, nullptr);
+  param = mozilla::UniqueSECItem(PK11_ParamFromIV(cipherMech, nullptr));
   if (!param)
   {
     NS_ERROR("no param");
     goto done;
   }
 
   ctxt = PK11_CreateContextBySymKey(cipherMech, CKA_ENCRYPT,
-                                    symkey, param);
+                                    symkey, param.get());
   if (!ctxt) {
     NS_ERROR("no context");
     goto done;
   }
 
   rv = PK11_CipherOp(ctxt, hash, (int *) &n, 8, (uint8_t *) src, 8);
   if (rv != SECSuccess) {
     NS_ERROR("des failure");
@@ -1167,11 +1168,9 @@ des_encrypt(const uint8_t *key, const ui
     goto done;
   }
 
 done:
   if (ctxt)
     PK11_DestroyContext(ctxt, true);
   if (symkey)
     PK11_FreeSymKey(symkey);
-  if (param)
-    SECITEM_FreeItem(param, true);
 }
--- a/security/manager/ssl/nsPKCS12Blob.cpp
+++ b/security/manager/ssl/nsPKCS12Blob.cpp
@@ -237,31 +237,30 @@ finish:
     SEC_PKCS12DecoderFinish(dcx);
   SECITEM_ZfreeItem(&unicodePw, false);
   return NS_OK;
 }
 
 static bool
 isExtractable(SECKEYPrivateKey *privKey)
 {
-  SECItem value;
-  bool    isExtractable = false;
-  SECStatus rv;
-
-  rv=PK11_ReadRawAttribute(PK11_TypePrivKey, privKey, CKA_EXTRACTABLE, &value);
+  ScopedAutoSECItem value;
+  SECStatus rv = PK11_ReadRawAttribute(PK11_TypePrivKey, privKey,
+                                       CKA_EXTRACTABLE, &value);
   if (rv != SECSuccess) {
     return false;
   }
+
+  bool isExtractable = false;
   if ((value.len == 1) && value.data) {
     isExtractable = !!(*(CK_BBOOL*)value.data);
   }
-  SECITEM_FreeItem(&value, false);
   return isExtractable;
 }
-  
+
 // nsPKCS12Blob::ExportToFile
 //
 // Having already loaded the certs, form them into a blob (loading the keys
 // also), encode the blob, and stuff it into the file.
 //
 // TODO: handle slots correctly
 //       mirror "slotToUse" behavior from PSM 1.x
 //       verify the cert array to start off with?