Bug 1577822 - land NSS dc86215aea17 UPGRADE_NSS_RELEASE, r=kjacobs
authorJ.C. Jones <jc@mozilla.com>
Thu, 03 Oct 2019 20:05:41 +0000
changeset 496238 cdeda4226ef5bc59d1e8f9e03932d9b47635f222
parent 496237 d32973602aa9d04cd88baba4ddb9efba5bcc0937
child 496239 f4dd9d524447fc0c34020e12d4f6a05fd42a6754
push id97118
push userjjones@mozilla.com
push dateThu, 03 Oct 2019 20:22:24 +0000
treeherderautoland@cdeda4226ef5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskjacobs
bugs1577822, 1576307
milestone71.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 1577822 - land NSS dc86215aea17 UPGRADE_NSS_RELEASE, r=kjacobs 2019-10-03 Kevin Jacobs <kjacobs@mozilla.com> * gtests/pk11_gtest/pk11_cbc_unittest.cc, lib/softoken/pkcs11c.c: Bug 1576307 - Fixup for fips tests, permit NULL iv as necessary. r=jcj ECB mode should not require an IV. [dc86215aea17] [tip] 2019-09-30 Kevin Jacobs <kjacobs@mozilla.com> * gtests/pk11_gtest/pk11_cbc_unittest.cc, lib/softoken/pkcs11c.c: Bug 1576307 - Check mechanism param and param length before casting to mechanism-specific structs. r=jcj This patch adds missing PKCS11 input parameter checks, which are needed prior to casting to mechanism-specific structs. [53d92a324080] Differential Revision: https://phabricator.services.mozilla.com/D48109
security/nss/TAG-INFO
security/nss/coreconf/coreconf.dep
security/nss/gtests/pk11_gtest/pk11_cbc_unittest.cc
security/nss/lib/softoken/pkcs11c.c
--- a/security/nss/TAG-INFO
+++ b/security/nss/TAG-INFO
@@ -1,1 +1,1 @@
-c0913ad7a560
\ No newline at end of file
+dc86215aea17
\ No newline at end of file
--- a/security/nss/coreconf/coreconf.dep
+++ b/security/nss/coreconf/coreconf.dep
@@ -5,8 +5,9 @@
 
 /*
  * A dummy header file that is a dependency for all the object files.
  * Used to force a full recompilation of NSS in Mozilla's Tinderbox
  * depend builds.  See comments in rules.mk.
  */
 
 #error "Do not include this header file."
+
--- a/security/nss/gtests/pk11_gtest/pk11_cbc_unittest.cc
+++ b/security/nss/gtests/pk11_gtest/pk11_cbc_unittest.cc
@@ -228,16 +228,89 @@ TEST_P(Pkcs11CbcPadTest, FailEncryptSimp
 
   SECStatus rv =
       PK11_Encrypt(ek.get(), GetParam(), GetIv(), output, &output_len,
                    sizeof(output), kInput, GetInputLen(CKA_ENCRYPT));
   EXPECT_EQ(SECFailure, rv);
   EXPECT_EQ(333U, output_len);
 }
 
+// It's a bit of a lie to put this in pk11_cbc_unittest, since we
+// also test bounds checking in other modes. There doesn't seem
+// to be an appropriately-generic place elsewhere.
+TEST_F(Pkcs11CbcPadTest, FailEncryptShortParam) {
+  SECStatus rv = SECFailure;
+  uint8_t encrypted[sizeof(kInput)];
+  unsigned int encrypted_len = 0;
+  size_t input_len = AES_BLOCK_SIZE;
+
+  // CK_GCM_PARAMS is the largest param struct used across AES modes
+  uint8_t param_buf[sizeof(CK_GCM_PARAMS)];
+  SECItem param = {siBuffer, param_buf, sizeof(param_buf)};
+  SECItem key_item = {siBuffer, const_cast<uint8_t*>(kKeyData), 16};
+
+  // Setup (we use the ECB key for other modes)
+  ScopedPK11SlotInfo slot(PK11_GetInternalSlot());
+  ASSERT_NE(nullptr, slot);
+  ScopedPK11SymKey key(PK11_ImportSymKey(slot.get(), CKM_AES_ECB,
+                                         PK11_OriginUnwrap, CKA_ENCRYPT,
+                                         &key_item, nullptr));
+  ASSERT_TRUE(key.get());
+
+  // CTR should have a CK_AES_CTR_PARAMS
+  param.len = sizeof(CK_AES_CTR_PARAMS) - 1;
+  rv = PK11_Encrypt(key.get(), CKM_AES_CTR, &param, encrypted, &encrypted_len,
+                    sizeof(encrypted), kInput, input_len);
+  EXPECT_EQ(SECFailure, rv);
+
+  param.len++;
+  reinterpret_cast<CK_AES_CTR_PARAMS*>(param.data)->ulCounterBits = 32;
+  rv = PK11_Encrypt(key.get(), CKM_AES_CTR, &param, encrypted, &encrypted_len,
+                    sizeof(encrypted), kInput, input_len);
+  EXPECT_EQ(SECSuccess, rv);
+
+  // GCM should have a CK_GCM_PARAMS
+  param.len = sizeof(CK_GCM_PARAMS) - 1;
+  rv = PK11_Encrypt(key.get(), CKM_AES_GCM, &param, encrypted, &encrypted_len,
+                    sizeof(encrypted), kInput, input_len);
+  EXPECT_EQ(SECFailure, rv);
+
+  param.len++;
+  reinterpret_cast<CK_GCM_PARAMS*>(param.data)->pIv = param_buf;
+  reinterpret_cast<CK_GCM_PARAMS*>(param.data)->ulIvLen = 12;
+  reinterpret_cast<CK_GCM_PARAMS*>(param.data)->pAAD = nullptr;
+  reinterpret_cast<CK_GCM_PARAMS*>(param.data)->ulAADLen = 0;
+  reinterpret_cast<CK_GCM_PARAMS*>(param.data)->ulTagBits = 128;
+  rv = PK11_Encrypt(key.get(), CKM_AES_GCM, &param, encrypted, &encrypted_len,
+                    sizeof(encrypted), kInput, input_len);
+  EXPECT_EQ(SECSuccess, rv);
+
+  // CBC should have a 16B IV
+  param.len = AES_BLOCK_SIZE - 1;
+  rv = PK11_Encrypt(key.get(), CKM_AES_CBC, &param, encrypted, &encrypted_len,
+                    sizeof(encrypted), kInput, input_len);
+  EXPECT_EQ(SECFailure, rv);
+
+  param.len++;
+  rv = PK11_Encrypt(key.get(), CKM_AES_CBC, &param, encrypted, &encrypted_len,
+                    sizeof(encrypted), kInput, input_len);
+  EXPECT_EQ(SECSuccess, rv);
+
+  // CTS
+  param.len = AES_BLOCK_SIZE - 1;
+  rv = PK11_Encrypt(key.get(), CKM_AES_CTS, &param, encrypted, &encrypted_len,
+                    sizeof(encrypted), kInput, input_len);
+  EXPECT_EQ(SECFailure, rv);
+
+  param.len++;
+  rv = PK11_Encrypt(key.get(), CKM_AES_CTS, &param, encrypted, &encrypted_len,
+                    sizeof(encrypted), kInput, input_len);
+  EXPECT_EQ(SECSuccess, rv);
+}
+
 TEST_P(Pkcs11CbcPadTest, ContextFailDecryptSimple) {
   ScopedPK11Context dctx = MakeContext(CKA_DECRYPT);
   uint8_t output[sizeof(kInput) + 64];
   int output_len = 77;
 
   SECStatus rv = PK11_CipherOp(dctx.get(), output, &output_len, sizeof(output),
                                kInput, GetInputLen(CKA_DECRYPT));
   EXPECT_EQ(SECSuccess, rv);
--- a/security/nss/lib/softoken/pkcs11c.c
+++ b/security/nss/lib/softoken/pkcs11c.c
@@ -35,17 +35,17 @@
 #include "softoken.h"
 #include "secasn1.h"
 #include "secerr.h"
 
 #include "prprf.h"
 #include "prenv.h"
 
 #define __PASTE(x, y) x##y
-
+#define BAD_PARAM_CAST(pMech, typeSize) (!pMech->pParameter || pMech->ulParameterLen < typeSize)
 /*
  * we renamed all our internal functions, get the correct
  * definitions for them...
  */
 #undef CK_PKCS11_FUNCTION_INFO
 #undef CK_NEED_ARG_LIST
 
 #define CK_EXTERN extern
@@ -778,16 +778,20 @@ sftk_CryptInit(CK_SESSION_HANDLE hSessio
 #endif
     CK_KEY_TYPE key_type;
     CK_RV crv = CKR_OK;
     unsigned effectiveKeyLength;
     unsigned char newdeskey[24];
     PRBool useNewKey = PR_FALSE;
     int t;
 
+    if (!pMechanism) {
+        return CKR_MECHANISM_PARAM_INVALID;
+    }
+
     crv = sftk_MechAllowsOperation(pMechanism->mechanism, mechUsage);
     if (crv != CKR_OK)
         return crv;
 
     session = sftk_SessionFromHandle(hSession);
     if (session == NULL)
         return CKR_SESSION_HANDLE_INVALID;
 
@@ -891,16 +895,21 @@ sftk_CryptInit(CK_SESSION_HANDLE hSessio
                 crv = CKR_KEY_TYPE_INCONSISTENT;
                 break;
             }
             att = sftk_FindAttribute(key, CKA_VALUE);
             if (att == NULL) {
                 crv = CKR_KEY_HANDLE_INVALID;
                 break;
             }
+
+            if (BAD_PARAM_CAST(pMechanism, sizeof(CK_RC2_CBC_PARAMS))) {
+                crv = CKR_MECHANISM_PARAM_INVALID;
+                break;
+            }
             rc2_param = (CK_RC2_CBC_PARAMS *)pMechanism->pParameter;
             effectiveKeyLength = (rc2_param->ulEffectiveBits + 7) / 8;
             context->cipherInfo =
                 RC2_CreateContext((unsigned char *)att->attrib.pValue,
                                   att->attrib.ulValueLen, rc2_param->iv,
                                   pMechanism->mechanism == CKM_RC2_ECB ? NSS_RC2 : NSS_RC2_CBC, effectiveKeyLength);
             sftk_FreeAttribute(att);
             if (context->cipherInfo == NULL) {
@@ -920,16 +929,21 @@ sftk_CryptInit(CK_SESSION_HANDLE hSessio
                 crv = CKR_KEY_TYPE_INCONSISTENT;
                 break;
             }
             att = sftk_FindAttribute(key, CKA_VALUE);
             if (att == NULL) {
                 crv = CKR_KEY_HANDLE_INVALID;
                 break;
             }
+
+            if (BAD_PARAM_CAST(pMechanism, sizeof(CK_RC5_CBC_PARAMS))) {
+                crv = CKR_MECHANISM_PARAM_INVALID;
+                break;
+            }
             rc5_param = (CK_RC5_CBC_PARAMS *)pMechanism->pParameter;
             context->blockSize = rc5_param->ulWordsize * 2;
             rc5Key.data = (unsigned char *)att->attrib.pValue;
             rc5Key.len = att->attrib.ulValueLen;
             context->cipherInfo = RC5_CreateContext(&rc5Key, rc5_param->ulRounds,
                                                     rc5_param->ulWordsize, rc5_param->pIv,
                                                     pMechanism->mechanism == CKM_RC5_ECB ? NSS_RC5 : NSS_RC5_CBC);
             sftk_FreeAttribute(att);
@@ -1117,16 +1131,23 @@ sftk_CryptInit(CK_SESSION_HANDLE hSessio
             context->doPad = PR_TRUE;
         /* fall thru */
         case CKM_AES_ECB:
         case CKM_AES_CBC:
             context->blockSize = 16;
         case CKM_AES_CTS:
         case CKM_AES_CTR:
         case CKM_AES_GCM:
+            if ((pMechanism->mechanism == CKM_AES_GCM && BAD_PARAM_CAST(pMechanism, sizeof(CK_GCM_PARAMS))) ||
+                (pMechanism->mechanism == CKM_AES_CTR && BAD_PARAM_CAST(pMechanism, sizeof(CK_AES_CTR_PARAMS))) ||
+                ((pMechanism->mechanism == CKM_AES_CBC || pMechanism->mechanism == CKM_AES_CTS) && BAD_PARAM_CAST(pMechanism, AES_BLOCK_SIZE))) {
+                crv = CKR_MECHANISM_PARAM_INVALID;
+                break;
+            }
+
             if (pMechanism->mechanism == CKM_AES_GCM) {
                 context->multi = PR_FALSE;
             }
             if (key_type != CKK_AES) {
                 crv = CKR_KEY_TYPE_INCONSISTENT;
                 break;
             }
             att = sftk_FindAttribute(key, CKA_VALUE);
@@ -2211,19 +2232,23 @@ sftk_InitCBCMac(CK_SESSION_HANDLE hSessi
     unsigned char ivBlock[SFTK_MAX_BLOCK_SIZE];
     unsigned char k2[SFTK_MAX_BLOCK_SIZE];
     unsigned char k3[SFTK_MAX_BLOCK_SIZE];
     SFTKSessionContext *context;
     CK_RV crv;
     unsigned int blockSize;
     PRBool isXCBC = PR_FALSE;
 
+    if (!pMechanism) {
+        return CKR_MECHANISM_PARAM_INVALID;
+    }
+
     switch (pMechanism->mechanism) {
         case CKM_RC2_MAC_GENERAL:
-            if (!pMechanism->pParameter) {
+            if (BAD_PARAM_CAST(pMechanism, sizeof(CK_RC2_MAC_GENERAL_PARAMS))) {
                 return CKR_MECHANISM_PARAM_INVALID;
             }
             mac_bytes =
                 ((CK_RC2_MAC_GENERAL_PARAMS *)pMechanism->pParameter)->ulMacLength;
         /* fall through */
         case CKM_RC2_MAC:
             /* this works because ulEffectiveBits is in the same place in both the
              * CK_RC2_MAC_GENERAL_PARAMS and CK_RC2_CBC_PARAMS */
@@ -2233,22 +2258,28 @@ sftk_InitCBCMac(CK_SESSION_HANDLE hSessi
             PORT_Memset(rc2_params.iv, 0, sizeof(rc2_params.iv));
             cbc_mechanism.mechanism = CKM_RC2_CBC;
             cbc_mechanism.pParameter = &rc2_params;
             cbc_mechanism.ulParameterLen = sizeof(rc2_params);
             blockSize = 8;
             break;
 #if NSS_SOFTOKEN_DOES_RC5
         case CKM_RC5_MAC_GENERAL:
+            if (BAD_PARAM_CAST(pMechanism, sizeof(CK_RC5_MAC_GENERAL_PARAMS))) {
+                return CKR_MECHANISM_PARAM_INVALID;
+            }
             mac_bytes =
                 ((CK_RC5_MAC_GENERAL_PARAMS *)pMechanism->pParameter)->ulMacLength;
         /* fall through */
         case CKM_RC5_MAC:
             /* this works because ulEffectiveBits is in the same place in both the
              * CK_RC5_MAC_GENERAL_PARAMS and CK_RC5_CBC_PARAMS */
+            if (BAD_PARAM_CAST(pMechanism, sizeof(CK_RC5_MAC_GENERAL_PARAMS))) {
+                return CKR_MECHANISM_PARAM_INVALID;
+            }
             rc5_mac = (CK_RC5_MAC_GENERAL_PARAMS *)pMechanism->pParameter;
             rc5_params.ulWordsize = rc5_mac->ulWordsize;
             rc5_params.ulRounds = rc5_mac->ulRounds;
             rc5_params.pIv = ivBlock;
             if ((blockSize = rc5_mac->ulWordsize * 2) > SFTK_MAX_BLOCK_SIZE)
                 return CKR_MECHANISM_PARAM_INVALID;
             rc5_params.ulIvLen = blockSize;
             PORT_Memset(ivBlock, 0, blockSize);
@@ -3842,21 +3873,27 @@ nsc_pbe_key_gen(NSSPKCS5PBEParameter *pk
     CK_PBE_PARAMS *pbe_params = NULL;
     CK_PKCS5_PBKD2_PARAMS *pbkd2_params = NULL;
 
     *key_length = 0;
     iv.data = NULL;
     iv.len = 0;
 
     if (pMechanism->mechanism == CKM_PKCS5_PBKD2) {
+        if (BAD_PARAM_CAST(pMechanism, sizeof(CK_PKCS5_PBKD2_PARAMS))) {
+            return CKR_MECHANISM_PARAM_INVALID;
+        }
         pbkd2_params = (CK_PKCS5_PBKD2_PARAMS *)pMechanism->pParameter;
         pwitem.data = (unsigned char *)pbkd2_params->pPassword;
         /* was this a typo in the PKCS #11 spec? */
         pwitem.len = *pbkd2_params->ulPasswordLen;
     } else {
+        if (BAD_PARAM_CAST(pMechanism, sizeof(CK_PBE_PARAMS))) {
+            return CKR_MECHANISM_PARAM_INVALID;
+        }
         pbe_params = (CK_PBE_PARAMS *)pMechanism->pParameter;
         pwitem.data = (unsigned char *)pbe_params->pPassword;
         pwitem.len = pbe_params->ulPasswordLen;
     }
     pbe_key = nsspkcs5_ComputeKeyAndIV(pkcs5_pbe, &pwitem, &iv, faulty3DES);
     if (pbe_key == NULL) {
         return CKR_HOST_MEMORY;
     }
@@ -4146,16 +4183,20 @@ nsc_SetupHMACKeyGen(CK_MECHANISM_PTR pMe
     }
 
     params = (NSSPKCS5PBEParameter *)PORT_ArenaZAlloc(arena,
                                                       sizeof(NSSPKCS5PBEParameter));
     if (params == NULL) {
         PORT_FreeArena(arena, PR_TRUE);
         return CKR_HOST_MEMORY;
     }
+    if (BAD_PARAM_CAST(pMechanism, sizeof(CK_PBE_PARAMS))) {
+        PORT_FreeArena(arena, PR_TRUE);
+        return CKR_MECHANISM_PARAM_INVALID;
+    }
 
     params->poolp = arena;
     params->ivLen = 0;
     params->pbeType = NSSPKCS5_PKCS12_V2;
     params->hashType = HASH_AlgSHA1;
     params->encAlg = SEC_OID_SHA1; /* any invalid value */
     params->is2KeyDES = PR_FALSE;
     params->keyID = pbeBitGenIntegrityKey;
@@ -4225,20 +4266,20 @@ nsc_SetupPBEKeyGen(CK_MECHANISM_PTR pMec
     *pbe = NULL;
 
     oid = SECOID_FindOIDByMechanism(pMechanism->mechanism);
     if (oid == NULL) {
         return CKR_MECHANISM_INVALID;
     }
 
     if (pMechanism->mechanism == CKM_PKCS5_PBKD2) {
-        pbkd2_params = (CK_PKCS5_PBKD2_PARAMS *)pMechanism->pParameter;
-        if (pbkd2_params == NULL) {
+        if (BAD_PARAM_CAST(pMechanism, sizeof(CK_PKCS5_PBKD2_PARAMS))) {
             return CKR_MECHANISM_PARAM_INVALID;
         }
+        pbkd2_params = (CK_PKCS5_PBKD2_PARAMS *)pMechanism->pParameter;
         switch (pbkd2_params->prf) {
             case CKP_PKCS5_PBKD2_HMAC_SHA1:
                 hashType = HASH_AlgSHA1;
                 break;
             case CKP_PKCS5_PBKD2_HMAC_SHA224:
                 hashType = HASH_AlgSHA224;
                 break;
             case CKP_PKCS5_PBKD2_HMAC_SHA256:
@@ -4255,16 +4296,19 @@ nsc_SetupPBEKeyGen(CK_MECHANISM_PTR pMec
         }
         if (pbkd2_params->saltSource != CKZ_SALT_SPECIFIED) {
             return CKR_MECHANISM_PARAM_INVALID;
         }
         salt.data = (unsigned char *)pbkd2_params->pSaltSourceData;
         salt.len = (unsigned int)pbkd2_params->ulSaltSourceDataLen;
         iteration = pbkd2_params->iterations;
     } else {
+        if (BAD_PARAM_CAST(pMechanism, sizeof(CK_PBE_PARAMS))) {
+            return CKR_MECHANISM_PARAM_INVALID;
+        }
         pbe_params = (CK_PBE_PARAMS *)pMechanism->pParameter;
         salt.data = (unsigned char *)pbe_params->pSalt;
         salt.len = (unsigned int)pbe_params->ulSaltLen;
         iteration = pbe_params->ulIteration;
     }
     params = nsspkcs5_NewParam(oid->offset, hashType, &salt, iteration);
     if (params == NULL) {
         return CKR_MECHANISM_INVALID;
@@ -4374,18 +4418,17 @@ NSC_GenerateKey(CK_SESSION_HANDLE hSessi
             continue;
         }
 
         crv = sftk_AddAttributeType(key, sftk_attr_expand(&pTemplate[i]));
         if (crv != CKR_OK)
             break;
     }
     if (crv != CKR_OK) {
-        sftk_FreeObject(key);
-        return crv;
+        goto loser;
     }
 
     /* make sure we don't have any class, key_type, or value fields */
     sftk_DeleteAttributeType(key, CKA_CLASS);
     sftk_DeleteAttributeType(key, CKA_KEY_TYPE);
     sftk_DeleteAttributeType(key, CKA_VALUE);
 
     /* Now Set up the parameters to generate the key (based on mechanism) */
@@ -4488,18 +4531,17 @@ NSC_GenerateKey(CK_SESSION_HANDLE hSessi
     /* make sure we aren't going to overflow the buffer */
     if (sizeof(buf) < key_length) {
         /* someone is getting pretty optimistic about how big their key can
          * be... */
         crv = CKR_TEMPLATE_INCONSISTENT;
     }
 
     if (crv != CKR_OK) {
-        sftk_FreeObject(key);
-        return crv;
+        goto loser;
     }
 
     /* if there was no error,
      * key_type *MUST* be set in the switch statement above */
     PORT_Assert(key_type != CKK_INVALID_KEY_TYPE);
 
     /*
      * now to the actual key gen.
@@ -4507,16 +4549,20 @@ NSC_GenerateKey(CK_SESSION_HANDLE hSessi
     switch (key_gen_type) {
         case nsc_pbe:
             crv = nsc_pbe_key_gen(pbe_param, pMechanism, buf, &key_length,
                                   faultyPBE3DES);
             nsspkcs5_DestroyPBEParameter(pbe_param);
             break;
         case nsc_ssl:
             rsa_pms = (SSL3RSAPreMasterSecret *)buf;
+            if (BAD_PARAM_CAST(pMechanism, sizeof(CK_VERSION))) {
+                crv = CKR_MECHANISM_PARAM_INVALID;
+                goto loser;
+            }
             version = (CK_VERSION *)pMechanism->pParameter;
             rsa_pms->client_version[0] = version->major;
             rsa_pms->client_version[1] = version->minor;
             crv =
                 NSC_GenerateRandom(0, &rsa_pms->random[0], sizeof(rsa_pms->random));
             break;
         case nsc_bulk:
             /* get the key, check for weak keys and repeat if found */
@@ -4525,67 +4571,68 @@ NSC_GenerateKey(CK_SESSION_HANDLE hSessi
             } while (crv == CKR_OK && checkWeak && sftk_IsWeakKey(buf, key_type));
             break;
         case nsc_param:
             /* generate parameters */
             *buf = 0;
             crv = nsc_parameter_gen(key_type, key);
             break;
         case nsc_jpake:
+            if (BAD_PARAM_CAST(pMechanism, sizeof(CK_NSS_JPAKERound1Params))) {
+                crv = CKR_MECHANISM_PARAM_INVALID;
+                goto loser;
+            }
             crv = jpake_Round1(hashType,
                                (CK_NSS_JPAKERound1Params *)pMechanism->pParameter,
                                key);
             break;
     }
 
     if (crv != CKR_OK) {
-        sftk_FreeObject(key);
-        return crv;
+        goto loser;
     }
 
     /* Add the class, key_type, and value */
     crv = sftk_AddAttributeType(key, CKA_CLASS, &objclass, sizeof(CK_OBJECT_CLASS));
     if (crv != CKR_OK) {
-        sftk_FreeObject(key);
-        return crv;
+        goto loser;
     }
     crv = sftk_AddAttributeType(key, CKA_KEY_TYPE, &key_type, sizeof(CK_KEY_TYPE));
     if (crv != CKR_OK) {
-        sftk_FreeObject(key);
-        return crv;
+        goto loser;
     }
     if (key_length != 0) {
         crv = sftk_AddAttributeType(key, CKA_VALUE, buf, key_length);
         if (crv != CKR_OK) {
-            sftk_FreeObject(key);
-            return crv;
+            goto loser;
         }
     }
 
     /* get the session */
     session = sftk_SessionFromHandle(hSession);
     if (session == NULL) {
-        sftk_FreeObject(key);
-        return CKR_SESSION_HANDLE_INVALID;
+        crv = CKR_SESSION_HANDLE_INVALID;
+        goto loser;
     }
 
     /*
      * handle the base object stuff
      */
     crv = sftk_handleObject(key, session);
     sftk_FreeSession(session);
     if (crv == CKR_OK && sftk_isTrue(key, CKA_SENSITIVE)) {
         crv = sftk_forceAttribute(key, CKA_ALWAYS_SENSITIVE, &cktrue, sizeof(CK_BBOOL));
     }
     if (crv == CKR_OK && !sftk_isTrue(key, CKA_EXTRACTABLE)) {
         crv = sftk_forceAttribute(key, CKA_NEVER_EXTRACTABLE, &cktrue, sizeof(CK_BBOOL));
     }
     if (crv == CKR_OK) {
         *phKey = key->handle;
     }
+loser:
     sftk_FreeObject(key);
     return crv;
 }
 
 #define PAIRWISE_DIGEST_LENGTH SHA1_LENGTH /* 160-bits */
 #define PAIRWISE_MESSAGE_LENGTH 20         /* 160-bits */
 
 /*
@@ -6605,17 +6652,16 @@ NSC_DeriveKey(CK_SESSION_HANDLE hSession
     CK_ULONG tmpKeySize;
     CK_ULONG IVSize;
     CK_ULONG keySize = 0;
     CK_RV crv = CKR_OK;
     CK_BBOOL cktrue = CK_TRUE;
     CK_KEY_TYPE keyType = CKK_GENERIC_SECRET;
     CK_OBJECT_CLASS classType = CKO_SECRET_KEY;
     CK_KEY_DERIVATION_STRING_DATA *stringPtr;
-    CK_MECHANISM_TYPE mechanism = pMechanism->mechanism;
     PRBool isTLS = PR_FALSE;
     PRBool isDH = PR_FALSE;
     HASH_HashType tlsPrfHash = HASH_AlgNULL;
     SECStatus rv;
     int i;
     unsigned int outLen;
     unsigned char sha_out[SHA1_LENGTH];
     unsigned char key_block[NUM_MIXERS * SFTK_MAX_MAC_LENGTH];
@@ -6623,16 +6669,21 @@ NSC_DeriveKey(CK_SESSION_HANDLE hSession
     HASH_HashType hashType;
     PRBool extractValue = PR_TRUE;
 
     CHECK_FORK();
 
     if (!slot) {
         return CKR_SESSION_HANDLE_INVALID;
     }
+    if (!pMechanism) {
+        return CKR_MECHANISM_PARAM_INVALID;
+    }
+    CK_MECHANISM_TYPE mechanism = pMechanism->mechanism;
+
     /*
      * now lets create an object to hang the attributes off of
      */
     if (phKey)
         *phKey = CK_INVALID_HANDLE;
 
     key = sftk_NewObject(slot); /* fill in the handle later */
     if (key == NULL) {
@@ -6796,16 +6847,20 @@ NSC_DeriveKey(CK_SESSION_HANDLE hSession
         case CKM_SSL3_MASTER_KEY_DERIVE:
         case CKM_SSL3_MASTER_KEY_DERIVE_DH: {
             CK_SSL3_MASTER_KEY_DERIVE_PARAMS *ssl3_master;
             SSL3RSAPreMasterSecret *rsa_pms;
             unsigned char crsrdata[SSL3_RANDOM_LENGTH * 2];
 
             if ((mechanism == CKM_TLS12_MASTER_KEY_DERIVE) ||
                 (mechanism == CKM_TLS12_MASTER_KEY_DERIVE_DH)) {
+                if (BAD_PARAM_CAST(pMechanism, sizeof(CK_TLS12_MASTER_KEY_DERIVE_PARAMS))) {
+                    crv = CKR_MECHANISM_PARAM_INVALID;
+                    break;
+                }
                 CK_TLS12_MASTER_KEY_DERIVE_PARAMS *tls12_master =
                     (CK_TLS12_MASTER_KEY_DERIVE_PARAMS *)pMechanism->pParameter;
                 tlsPrfHash = GetHashTypeFromMechanism(tls12_master->prfHashMechanism);
                 if (tlsPrfHash == HASH_AlgNULL) {
                     crv = CKR_MECHANISM_PARAM_INVALID;
                     break;
                 }
             } else if ((mechanism == CKM_NSS_TLS_MASTER_KEY_DERIVE_SHA256) ||
@@ -7063,16 +7118,20 @@ NSC_DeriveKey(CK_SESSION_HANDLE hSession
             CK_SSL3_KEY_MAT_PARAMS *ssl3_keys;
             CK_SSL3_KEY_MAT_OUT *ssl3_keys_out;
             CK_ULONG effKeySize;
             unsigned int block_needed;
             unsigned char srcrdata[SSL3_RANDOM_LENGTH * 2];
             unsigned char crsrdata[SSL3_RANDOM_LENGTH * 2];
 
             if (mechanism == CKM_TLS12_KEY_AND_MAC_DERIVE) {
+                if (BAD_PARAM_CAST(pMechanism, sizeof(CK_TLS12_KEY_MAT_PARAMS))) {
+                    crv = CKR_MECHANISM_PARAM_INVALID;
+                    break;
+                }
                 CK_TLS12_KEY_MAT_PARAMS *tls12_keys =
                     (CK_TLS12_KEY_MAT_PARAMS *)pMechanism->pParameter;
                 tlsPrfHash = GetHashTypeFromMechanism(tls12_keys->prfHashMechanism);
                 if (tlsPrfHash == HASH_AlgNULL) {
                     crv = CKR_MECHANISM_PARAM_INVALID;
                     break;
                 }
             } else if (mechanism == CKM_NSS_TLS_KEY_AND_MAC_DERIVE_SHA256) {
@@ -7106,16 +7165,23 @@ NSC_DeriveKey(CK_SESSION_HANDLE hSession
                 break;
             }
             sha = SHA1_NewContext();
             if (sha == NULL) {
                 MD5_DestroyContext(md5, PR_TRUE);
                 crv = CKR_HOST_MEMORY;
                 break;
             }
+
+            if (BAD_PARAM_CAST(pMechanism, sizeof(CK_SSL3_KEY_MAT_PARAMS))) {
+                MD5_DestroyContext(md5, PR_TRUE);
+                SHA1_DestroyContext(sha, PR_TRUE);
+                crv = CKR_MECHANISM_PARAM_INVALID;
+                break;
+            }
             ssl3_keys = (CK_SSL3_KEY_MAT_PARAMS *)pMechanism->pParameter;
 
             PORT_Memcpy(srcrdata,
                         ssl3_keys->RandomInfo.pServerRandom, SSL3_RANDOM_LENGTH);
             PORT_Memcpy(srcrdata + SSL3_RANDOM_LENGTH,
                         ssl3_keys->RandomInfo.pClientRandom, SSL3_RANDOM_LENGTH);
 
             PORT_Memcpy(crsrdata,
@@ -7300,16 +7366,20 @@ NSC_DeriveKey(CK_SESSION_HANDLE hSession
             unsigned char des3key[MAX_DES3_KEY_SIZE];
             CK_DES_CBC_ENCRYPT_DATA_PARAMS *desEncryptPtr;
             int mode;
             unsigned char *iv;
             unsigned char *data;
             CK_ULONG len;
 
             if (mechanism == CKM_DES3_ECB_ENCRYPT_DATA) {
+                if (BAD_PARAM_CAST(pMechanism, sizeof(CK_KEY_DERIVATION_STRING_DATA))) {
+                    crv = CKR_MECHANISM_PARAM_INVALID;
+                    break;
+                }
                 stringPtr = (CK_KEY_DERIVATION_STRING_DATA *)
                                 pMechanism->pParameter;
                 mode = NSS_DES_EDE3;
                 iv = NULL;
                 data = stringPtr->pData;
                 len = stringPtr->ulLen;
             } else {
                 mode = NSS_DES_EDE3_CBC;
@@ -7349,20 +7419,28 @@ NSC_DeriveKey(CK_SESSION_HANDLE hSession
             int mode;
             unsigned char *iv;
             unsigned char *data;
             CK_ULONG len;
 
             if (mechanism == CKM_AES_ECB_ENCRYPT_DATA) {
                 mode = NSS_AES;
                 iv = NULL;
+                if (BAD_PARAM_CAST(pMechanism, sizeof(CK_KEY_DERIVATION_STRING_DATA))) {
+                    crv = CKR_MECHANISM_PARAM_INVALID;
+                    break;
+                }
                 stringPtr = (CK_KEY_DERIVATION_STRING_DATA *)pMechanism->pParameter;
                 data = stringPtr->pData;
                 len = stringPtr->ulLen;
             } else {
+                if (BAD_PARAM_CAST(pMechanism, sizeof(CK_AES_CBC_ENCRYPT_DATA_PARAMS))) {
+                    crv = CKR_MECHANISM_PARAM_INVALID;
+                    break;
+                }
                 aesEncryptPtr =
                     (CK_AES_CBC_ENCRYPT_DATA_PARAMS *)pMechanism->pParameter;
                 mode = NSS_AES_CBC;
                 iv = aesEncryptPtr->iv;
                 data = aesEncryptPtr->pData;
                 len = aesEncryptPtr->length;
             }
 
@@ -7385,24 +7463,32 @@ NSC_DeriveKey(CK_SESSION_HANDLE hSession
             void *cipherInfo;
             CK_AES_CBC_ENCRYPT_DATA_PARAMS *aesEncryptPtr;
             int mode;
             unsigned char *iv;
             unsigned char *data;
             CK_ULONG len;
 
             if (mechanism == CKM_CAMELLIA_ECB_ENCRYPT_DATA) {
+                if (BAD_PARAM_CAST(pMechanism, sizeof(CK_KEY_DERIVATION_STRING_DATA))) {
+                    crv = CKR_MECHANISM_PARAM_INVALID;
+                    break;
+                }
                 stringPtr = (CK_KEY_DERIVATION_STRING_DATA *)
                                 pMechanism->pParameter;
                 aesEncryptPtr = NULL;
                 mode = NSS_CAMELLIA;
                 data = stringPtr->pData;
                 len = stringPtr->ulLen;
                 iv = NULL;
             } else {
+                if (BAD_PARAM_CAST(pMechanism, sizeof(CK_AES_CBC_ENCRYPT_DATA_PARAMS))) {
+                    crv = CKR_MECHANISM_PARAM_INVALID;
+                    break;
+                }
                 stringPtr = NULL;
                 aesEncryptPtr = (CK_AES_CBC_ENCRYPT_DATA_PARAMS *)
                                     pMechanism->pParameter;
                 mode = NSS_CAMELLIA_CBC;
                 iv = aesEncryptPtr->iv;
                 data = aesEncryptPtr->pData;
                 len = aesEncryptPtr->length;
             }
@@ -7426,24 +7512,32 @@ NSC_DeriveKey(CK_SESSION_HANDLE hSession
             void *cipherInfo;
             CK_AES_CBC_ENCRYPT_DATA_PARAMS *aesEncryptPtr;
             int mode;
             unsigned char *iv;
             unsigned char *data;
             CK_ULONG len;
 
             if (mechanism == CKM_SEED_ECB_ENCRYPT_DATA) {
+                if (BAD_PARAM_CAST(pMechanism, sizeof(CK_KEY_DERIVATION_STRING_DATA))) {
+                    crv = CKR_MECHANISM_PARAM_INVALID;
+                    break;
+                }
                 mode = NSS_SEED;
                 stringPtr = (CK_KEY_DERIVATION_STRING_DATA *)
                                 pMechanism->pParameter;
                 aesEncryptPtr = NULL;
                 data = stringPtr->pData;
                 len = stringPtr->ulLen;
                 iv = NULL;
             } else {
+                if (BAD_PARAM_CAST(pMechanism, sizeof(CK_AES_CBC_ENCRYPT_DATA_PARAMS))) {
+                    crv = CKR_MECHANISM_PARAM_INVALID;
+                    break;
+                }
                 mode = NSS_SEED_CBC;
                 aesEncryptPtr = (CK_AES_CBC_ENCRYPT_DATA_PARAMS *)
                                     pMechanism->pParameter;
                 iv = aesEncryptPtr->iv;
                 data = aesEncryptPtr->pData;
                 len = aesEncryptPtr->length;
             }
 
@@ -7525,16 +7619,20 @@ NSC_DeriveKey(CK_SESSION_HANDLE hSession
             break;
         }
 
         case CKM_CONCATENATE_BASE_AND_DATA:
             crv = sftk_DeriveSensitiveCheck(sourceKey, key);
             if (crv != CKR_OK)
                 break;
 
+            if (BAD_PARAM_CAST(pMechanism, sizeof(CK_KEY_DERIVATION_STRING_DATA))) {
+                crv = CKR_MECHANISM_PARAM_INVALID;
+                break;
+            }
             stringPtr = (CK_KEY_DERIVATION_STRING_DATA *)pMechanism->pParameter;
             tmpKeySize = att->attrib.ulValueLen + stringPtr->ulLen;
             if (keySize == 0)
                 keySize = tmpKeySize;
             if (keySize > tmpKeySize) {
                 crv = CKR_TEMPLATE_INCONSISTENT;
                 break;
             }
@@ -7551,16 +7649,20 @@ NSC_DeriveKey(CK_SESSION_HANDLE hSession
             crv = sftk_forceAttribute(key, CKA_VALUE, buf, keySize);
             PORT_ZFree(buf, tmpKeySize);
             break;
         case CKM_CONCATENATE_DATA_AND_BASE:
             crv = sftk_DeriveSensitiveCheck(sourceKey, key);
             if (crv != CKR_OK)
                 break;
 
+            if (BAD_PARAM_CAST(pMechanism, sizeof(CK_KEY_DERIVATION_STRING_DATA))) {
+                crv = CKR_MECHANISM_PARAM_INVALID;
+                break;
+            }
             stringPtr = (CK_KEY_DERIVATION_STRING_DATA *)pMechanism->pParameter;
             tmpKeySize = att->attrib.ulValueLen + stringPtr->ulLen;
             if (keySize == 0)
                 keySize = tmpKeySize;
             if (keySize > tmpKeySize) {
                 crv = CKR_TEMPLATE_INCONSISTENT;
                 break;
             }
@@ -7577,16 +7679,20 @@ NSC_DeriveKey(CK_SESSION_HANDLE hSession
             crv = sftk_forceAttribute(key, CKA_VALUE, buf, keySize);
             PORT_ZFree(buf, tmpKeySize);
             break;
         case CKM_XOR_BASE_AND_DATA:
             crv = sftk_DeriveSensitiveCheck(sourceKey, key);
             if (crv != CKR_OK)
                 break;
 
+            if (BAD_PARAM_CAST(pMechanism, sizeof(CK_KEY_DERIVATION_STRING_DATA))) {
+                crv = CKR_MECHANISM_PARAM_INVALID;
+                break;
+            }
             stringPtr = (CK_KEY_DERIVATION_STRING_DATA *)pMechanism->pParameter;
             tmpKeySize = PR_MIN(att->attrib.ulValueLen, stringPtr->ulLen);
             if (keySize == 0)
                 keySize = tmpKeySize;
             if (keySize > tmpKeySize) {
                 crv = CKR_TEMPLATE_INCONSISTENT;
                 break;
             }
@@ -7601,16 +7707,20 @@ NSC_DeriveKey(CK_SESSION_HANDLE hSession
                 buf[i] ^= stringPtr->pData[i];
             }
 
             crv = sftk_forceAttribute(key, CKA_VALUE, buf, keySize);
             PORT_ZFree(buf, keySize);
             break;
 
         case CKM_EXTRACT_KEY_FROM_KEY: {
+            if (BAD_PARAM_CAST(pMechanism, sizeof(CK_EXTRACT_PARAMS))) {
+                crv = CKR_MECHANISM_PARAM_INVALID;
+                break;
+            }
             /* the following assumes 8 bits per byte */
             CK_ULONG extract = *(CK_EXTRACT_PARAMS *)pMechanism->pParameter;
             CK_ULONG shift = extract & 0x7; /* extract mod 8 the fast way */
             CK_ULONG offset = extract >> 3; /* extract div 8 the fast way */
 
             crv = sftk_DeriveSensitiveCheck(sourceKey, key);
             if (crv != CKR_OK)
                 break;