Bug 1294011 - Obviate manual calls to SECITEM_FreeItem() in PSM. r=keeler draft
authorCykesiopka <cykesiopka.bmo@gmail.com>
Sat, 13 Aug 2016 21:45:00 +0800
changeset 400443 bd4c8981b52e3f5a504fc09958872415cf757eff
parent 400442 49a53da6979c0139592e018b3e0fc25030ee2180
child 528201 8c886b45c3cfb7aab99c89c2ef662e35a220cc64
push id26142
push usercykesiopka.bmo@gmail.com
push dateSat, 13 Aug 2016 14:28:57 +0000
reviewerskeeler
bugs1294011
milestone51.0a1
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?