Bug 1591363 - PBKDF2 memory leaks in NSC_GenerateKey. r=jcj

A memory leak was reported and confirmed in this bug. However, during the "manual" analysis of the flow, another possible leak was found.
I created a patch for both leaks, added gtests for unexpected keySizes and adjusted the general syntax of the gtest file.

Differential Revision: https://phabricator.services.mozilla.com/D52536

--HG--
extra : moz-landing-system : lando

gtests/pk11_gtest/pk11_pbkdf2_unittest.cc

@@ -22,53 +22,102 @@ class Pkcs11Pbkdf2Test : public ::testing::Test {
  public:
   void Derive(std::vector<uint8_t>& derived, SECOidTag hash_alg) {
     // Shared between test vectors.
-    const unsigned int iterations = 4096;
+    const unsigned int kIterations = 4096;
     std::string pass("passwordPASSWORDpassword");
     std::string salt("saltSALTsaltSALTsaltSALTsaltSALTsalt");
 
     // Derivation must succeed with the right values.
-    EXPECT_TRUE(DeriveBytes(pass, salt, derived, hash_alg, iterations));
+    EXPECT_TRUE(DeriveBytes(pass, salt, derived, hash_alg, kIterations));
 
     // Derivation must fail when the password is bogus.
-    std::string bogusPass("PasswordPASSWORDpassword");
-    EXPECT_FALSE(DeriveBytes(bogusPass, salt, derived, hash_alg, iterations));
+    std::string bogus_pass("PasswordPASSWORDpassword");
+    EXPECT_FALSE(DeriveBytes(bogus_pass, salt, derived, hash_alg, kIterations));
 
     // Derivation must fail when the salt is bogus.
-    std::string bogusSalt("SaltSALTsaltSALTsaltSALTsaltSALTsalt");
-    EXPECT_FALSE(DeriveBytes(pass, bogusSalt, derived, hash_alg, iterations));
+    std::string bogus_salt("SaltSALTsaltSALTsaltSALTsaltSALTsalt");
+    EXPECT_FALSE(DeriveBytes(pass, bogus_salt, derived, hash_alg, kIterations));
 
     // Derivation must fail when using the wrong hash function.
     SECOidTag next_hash_alg = static_cast<SECOidTag>(hash_alg + 1);
-    EXPECT_FALSE(DeriveBytes(pass, salt, derived, next_hash_alg, iterations));
+    EXPECT_FALSE(DeriveBytes(pass, salt, derived, next_hash_alg, kIterations));
 
-    // Derivation must fail when using the wrong number of iterations.
-    EXPECT_FALSE(DeriveBytes(pass, salt, derived, hash_alg, iterations + 1));
+    // Derivation must fail when using the wrong number of kIterations.
+    EXPECT_FALSE(DeriveBytes(pass, salt, derived, hash_alg, kIterations + 1));
+  }
+
+  void KeySizes(SECOidTag hash_alg) {
+    // These tests will only validate the controls around the key sizes.
+    // The resulting key is tested above, with valid key sizes.
+    const unsigned int kIterations = 10;
+    std::string pass("passwordPASSWORDpassword");
+    std::string salt("saltSALTsaltSALTsaltSALTsaltSALTsalt");
+
+    // Derivation must fail when using key sizes bigger than MAX_KEY_LEN.
+    const int big_key_size = 768;
+    EXPECT_FALSE(KeySizeParam(pass, salt, big_key_size, hash_alg, kIterations));
+
+    // Zero is acceptable as key size and will be managed internally.
+    const int zero_key_size = 0;
+    EXPECT_TRUE(KeySizeParam(pass, salt, zero_key_size, hash_alg, kIterations));
+
+    // -1 will be set to 0 internally and this means that the key size will be
+    // obtained from the template. If the template doesn't have this defined,
+    // it must fail.
+    const int minus_key_size = -1;
+    EXPECT_FALSE(
+        KeySizeParam(pass, salt, minus_key_size, hash_alg, kIterations));
+
+    // Lower than -1 is not allowed, as -1 means no keyLen defined.
+    const int negative_key_size = -10;
+    EXPECT_FALSE(
+        KeySizeParam(pass, salt, negative_key_size, hash_alg, kIterations));
   }
 
  private:
   bool DeriveBytes(std::string& pass, std::string& salt,
                    std::vector<uint8_t>& derived, SECOidTag hash_alg,
-                   unsigned int iterations) {
-    SECItem passItem = {siBuffer, ToUcharPtr(pass),
-                        static_cast<unsigned int>(pass.length())};
-    SECItem saltItem = {siBuffer, ToUcharPtr(salt),
-                        static_cast<unsigned int>(salt.length())};
+                   unsigned int kIterations) {
+    SECItem pass_item = {siBuffer, ToUcharPtr(pass),
+                         static_cast<unsigned int>(pass.length())};
+    SECItem salt_item = {siBuffer, ToUcharPtr(salt),
+                         static_cast<unsigned int>(salt.length())};
 
     // Set up PBKDF2 params.
     ScopedSECAlgorithmID alg_id(
         PK11_CreatePBEV2AlgorithmID(SEC_OID_PKCS5_PBKDF2, hash_alg, hash_alg,
-                                    derived.size(), iterations, &saltItem));
+                                    derived.size(), kIterations, &salt_item));
 
     // Derive.
     ScopedPK11SlotInfo slot(PK11_GetInternalSlot());
-    ScopedPK11SymKey symKey(
-        PK11_PBEKeyGen(slot.get(), alg_id.get(), &passItem, false, nullptr));
+    ScopedPK11SymKey sym_key(
+        PK11_PBEKeyGen(slot.get(), alg_id.get(), &pass_item, false, nullptr));
 
-    SECStatus rv = PK11_ExtractKeyValue(symKey.get());
+    SECStatus rv = PK11_ExtractKeyValue(sym_key.get());
     EXPECT_EQ(rv, SECSuccess);
 
-    SECItem* keyData = PK11_GetKeyData(symKey.get());
-    return !memcmp(&derived[0], keyData->data, keyData->len);
+    SECItem* key_data = PK11_GetKeyData(sym_key.get());
+    return !memcmp(&derived[0], key_data->data, key_data->len);
+  }
+
+  bool KeySizeParam(std::string& pass, std::string& salt, const int key_size,
+                    SECOidTag hash_alg, unsigned int kIterations) {
+    SECItem pass_item = {siBuffer, ToUcharPtr(pass),
+                         static_cast<unsigned int>(pass.length())};
+    SECItem salt_item = {siBuffer, ToUcharPtr(salt),
+                         static_cast<unsigned int>(salt.length())};
+
+    // Set up PBKDF2 params.
+    ScopedSECAlgorithmID alg_id(
+        PK11_CreatePBEV2AlgorithmID(SEC_OID_PKCS5_PBKDF2, hash_alg, hash_alg,
+                                    key_size, kIterations, &salt_item));
+
+    // Try to generate a key with the defined params.
+    ScopedPK11SlotInfo slot(PK11_GetInternalSlot());
+    ScopedPK11SymKey sym_key(
+        PK11_PBEKeyGen(slot.get(), alg_id.get(), &pass_item, false, nullptr));
+
+    // Should be nullptr if fail.
+    return sym_key.get();
   }
 };
 
@@ -93,4 +142,9 @@ TEST_F(Pkcs11Pbkdf2Test, DeriveKnown2) {
   Derive(derived, SEC_OID_HMAC_SHA256);
 }
 
+TEST_F(Pkcs11Pbkdf2Test, KeyLenSizes) {
+  // The size controls are regardless of the algorithms.
+  KeySizes(SEC_OID_HMAC_SHA256);
+}
+
 }  // namespace nss_test

lib/pk11wrap/pk11pbe.c

@@ -654,17 +654,17 @@ sec_pkcs5CreateAlgorithmID(SECOidTag algorithm,
         pbeAlgorithm = SEC_OID_PKCS5_PBKDF2;
         /*
          * 'algorithm' is the overall algorithm oid tag used to wrap the
-         * entire algoithm ID block. For PKCS5v1 and PKCS12, this
+         * entire algorithm ID block. For PKCS5v1 and PKCS12, this
          * algorithm OID has encoded in it both the PBE KDF function
          * and the encryption algorithm. For PKCS 5v2, PBE KDF and
          * encryption/macing oids are encoded as parameters in
          * the algorithm ID block.
          *
          * Thus in PKCS5 v1 and PKCS12, this algorithm maps to a pkcs #11
-         * mechanism, where as in PKCS 5v2, this alogithm tag does not map
+         * mechanism, where as in PKCS 5v2, this algorithm tag does not map
          * directly to a PKCS #11 mechanim, instead the 2 oids in the
          * algorithm ID block map the the actual PKCS #11 mechanism.
-         * gorithm is). We use choose this algorithm oid based on the
+         * algorithm is). We use choose this algorithm oid based on the
          * cipherAlgorithm to determine what this should be (MAC1 or PBES2).
          */
         if (algorithm == SEC_OID_PKCS5_PBKDF2) {

lib/pk11wrap/pk11skey.c

@@ -630,14 +630,19 @@ PK11_GetWindow(PK11SymKey *key)
 }
 
 /*
- * extract a symetric key value. NOTE: if the key is sensitive, we will
+ * extract a symmetric key value. NOTE: if the key is sensitive, we will
  * not be able to do this operation. This function is used to move
  * keys from one token to another */
 SECStatus
 PK11_ExtractKeyValue(PK11SymKey *symKey)
 {
     SECStatus rv;
 
+    if (symKey == NULL) {
+        PORT_SetError(SEC_ERROR_INVALID_ARGS);
+        return SECFailure;
+    }
+
     if (symKey->data.data != NULL) {
         if (symKey->size == 0) {
             symKey->size = symKey->data.len;

lib/softoken/pkcs11c.c

@@ -4403,6 +4403,8 @@ nsc_SetupPBEKeyGen(CK_MECHANISM_PTR pMechanism, NSSPKCS5PBEParameter **pbe,
     }
     if (crv == CKR_OK) {
         *pbe = params;
+    } else {
+        nsspkcs5_DestroyPBEParameter(params);
     }
     return crv;
 }
@@ -4467,8 +4469,9 @@ NSC_GenerateKey(CK_SESSION_HANDLE hSession,
         }
 
         crv = sftk_AddAttributeType(key, sftk_attr_expand(&pTemplate[i]));
-        if (crv != CKR_OK)
+        if (crv != CKR_OK) {
             break;
+        }
     }
     if (crv != CKR_OK) {
         goto loser;
@@ -4584,6 +4587,9 @@ NSC_GenerateKey(CK_SESSION_HANDLE hSession,
     }
 
     if (crv != CKR_OK) {
+        if (pbe_param) {
+            nsspkcs5_DestroyPBEParameter(pbe_param);
+        }
         goto loser;
     }