Bug 1649648 - Fix null pointers passed as argument in pk11wrap/pk11pb…

…e.c:886 r=kjacobs Differential Revision: https://phabricator.services.mozilla.com/D81824 --HG-- extra : moz-landing-system : lando
sailfishos-mirror · Jul 9, 2020 · 5440fbb · 5440fbb
1 parent fc0a5e9
commit 5440fbb
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 9 deletions.
diff --git a/gtests/pk11_gtest/pk11_pbkdf2_unittest.cc b/gtests/pk11_gtest/pk11_pbkdf2_unittest.cc
@@ -51,6 +51,7 @@ class Pkcs11Pbkdf2Test : public ::testing::Test {
     const unsigned int kIterations = 10;
     std::string pass("passwordPASSWORDpassword");
     std::string salt("saltSALTsaltSALTsaltSALTsaltSALTsalt");
+    std::string salt_empty("");
 
     // Derivation must fail when using key sizes bigger than MAX_KEY_LEN.
     const int big_key_size = 768;
@@ -60,6 +61,10 @@ class Pkcs11Pbkdf2Test : public ::testing::Test {
     const int zero_key_size = 0;
     EXPECT_TRUE(KeySizeParam(pass, salt, zero_key_size, hash_alg, kIterations));
 
+    // Zero is acceptable as salt size and will be managed internally.
+    EXPECT_TRUE(
+        KeySizeParam(pass, salt_empty, 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.
@@ -71,6 +76,12 @@ class Pkcs11Pbkdf2Test : public ::testing::Test {
     const int negative_key_size = -10;
     EXPECT_FALSE(
         KeySizeParam(pass, salt, negative_key_size, hash_alg, kIterations));
+
+    // Malformed inputs are handled without crashing
+    EXPECT_FALSE(
+        MalformedPass(pass, salt, big_key_size, hash_alg, kIterations));
+    EXPECT_FALSE(
+        MalformedSalt(pass, salt, big_key_size, hash_alg, kIterations));
   }
 
  private:
@@ -99,13 +110,8 @@ class Pkcs11Pbkdf2Test : public ::testing::Test {
     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())};
-
+  bool GenerateKey(SECItem pass_item, SECItem salt_item, const int key_size,
+                   SECOidTag hash_alg, unsigned int kIterations) {
     // Set up PBKDF2 params.
     ScopedSECAlgorithmID alg_id(
         PK11_CreatePBEV2AlgorithmID(SEC_OID_PKCS5_PBKDF2, hash_alg, hash_alg,
@@ -119,6 +125,34 @@ class Pkcs11Pbkdf2Test : public ::testing::Test {
     // Should be nullptr if fail.
     return sym_key.get();
   }
+
+  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())};
+
+    return GenerateKey(pass_item, salt_item, key_size, hash_alg, kIterations);
+  }
+
+  bool MalformedSalt(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, nullptr, 0};
+
+    return GenerateKey(pass_item, salt_item, key_size, hash_alg, kIterations);
+  }
+
+  bool MalformedPass(std::string& pass, std::string& salt, const int key_size,
+                     SECOidTag hash_alg, unsigned int kIterations) {
+    SECItem pass_item = {siBuffer, nullptr, 0};
+    SECItem salt_item = {siBuffer, ToUcharPtr(salt),
+                         static_cast<unsigned int>(salt.length())};
+
+    return GenerateKey(pass_item, salt_item, key_size, hash_alg, kIterations);
+  }
 };
 
 // RFC 6070 <http://tools.ietf.org/html/rfc6070>

diff --git a/lib/pk11wrap/pk11pbe.c b/lib/pk11wrap/pk11pbe.c
@@ -883,7 +883,9 @@ pbe_PK11AlgidToParam(SECAlgorithmID *algid, SECItem *mech)
         pbeV2_params->ulPrfDataLen = 0;
         pbeV2_params->saltSource = CKZ_SALT_SPECIFIED;
         pSalt = ((CK_CHAR_PTR)pbeV2_params) + sizeof(CK_PKCS5_PBKD2_PARAMS);
-        PORT_Memcpy(pSalt, salt->data, salt->len);
+        if (salt->data) {
+            PORT_Memcpy(pSalt, salt->data, salt->len);
+        }
         pbeV2_params->pSaltSourceData = pSalt;
         pbeV2_params->ulSaltSourceDataLen = salt->len;
         pbeV2_params->iterations = iterations;
@@ -899,7 +901,9 @@ pbe_PK11AlgidToParam(SECAlgorithmID *algid, SECItem *mech)
 
         pSalt = ((CK_CHAR_PTR)pbe_params) + sizeof(CK_PBE_PARAMS);
         pbe_params->pSalt = pSalt;
-        PORT_Memcpy(pSalt, salt->data, salt->len);
+        if (salt->data) {
+            PORT_Memcpy(pSalt, salt->data, salt->len);
+        }
         pbe_params->ulSaltLen = salt->len;
         if (iv_len) {
             pbe_params->pInitVector =