Bug 1666567 - land NSS 8fdbec414ce2 UPGRADE_NSS_RELEASE, r=kjacobs

2020-09-24  Kevin Jacobs  <kjacobs@mozilla.com>

	* automation/abi-check/expected-report-libnss3.so.txt,
	gtests/pk11_gtest/pk11_hkdf_unittest.cc, lib/nss/nss.def,
	lib/pk11wrap/pk11pub.h, lib/pk11wrap/pk11skey.c,
	lib/ssl/tls13hkdf.c:
	Bug 1667153 - Add PK11_ImportDataKey API. r=rrelyea

	This patch adds and exports `PK11_ImportDataKey`, and refactors the
	null PSK TLS 1.3 code to use it.

	[8fdbec414ce2] [tip]

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

security/nss/TAG-INFO

@@ -1 +1 @@
-8ebee3cec9cf
+8fdbec414ce2

security/nss/automation/abi-check/expected-report-libnss3.so.txt

@@ -0,0 +1,4 @@
+
+1 Added function:
+
+  [A] 'function PK11SymKey* PK11_ImportDataKey(PK11SlotInfo*, CK_MECHANISM_TYPE, PK11Origin, CK_ATTRIBUTE_TYPE, SECItem*, void*)'    {PK11_ImportDataKey@@NSS_3.58}

security/nss/coreconf/coreconf.dep

@@ -10,3 +10,4 @@
  */
 
 #error "Do not include this header file."
+

security/nss/gtests/pk11_gtest/pk11_hkdf_unittest.cc

@@ -26,7 +26,12 @@ const int kPkcs11HkdfSaltDerive = 3;
 const int kPkcs11HkdfData = 4;
 const int kPKCS11NumTypes = 5;
 
-class Pkcs11HkdfTest : public ::testing::TestWithParam<hkdf_vector> {
+enum class Pk11ImportType { data = 0, key = 1 };
+static const Pk11ImportType kImportTypesAll[] = {Pk11ImportType::data,
+                                                 Pk11ImportType::key};
+
+class Pkcs11HkdfTest
+    : public ::testing::TestWithParam<std::tuple<hkdf_vector, Pk11ImportType>> {
  protected:
   CK_MECHANISM_TYPE Pk11HkdfToHash(CK_MECHANISM_TYPE nssHkdfMech) {
     switch (nssHkdfMech) {
@@ -45,14 +50,15 @@ class Pkcs11HkdfTest : public ::testing::TestWithParam<hkdf_vector> {
     return CKM_INVALID_MECHANISM;
   }
 
-  ScopedPK11SymKey ImportKey(CK_KEY_TYPE keyType, SECItem *ikm_item) {
+  ScopedPK11SymKey ImportKey(CK_KEY_TYPE key_type, SECItem *ikm_item,
+                             Pk11ImportType import_type) {
     ScopedPK11SlotInfo slot(PK11_GetInternalSlot());
     CK_MECHANISM_TYPE mech = CKM_HKDF_KEY_GEN;
     if (!slot) {
       ADD_FAILURE() << "Can't get slot";
       return nullptr;
     }
-    switch (keyType) {
+    switch (key_type) {
       case CKK_GENERIC_SECRET:
         mech = CKM_GENERIC_SECRET_KEY_GEN;
         break;
@@ -61,13 +67,21 @@ class Pkcs11HkdfTest : public ::testing::TestWithParam<hkdf_vector> {
         break;
     }
 
-    ScopedPK11SymKey ikm(PK11_ImportSymKey(slot.get(), mech, PK11_OriginUnwrap,
+    ScopedPK11SymKey ikm;
-                                           CKA_SIGN, ikm_item, nullptr));
+
+    if (import_type == Pk11ImportType::key) {
+      ikm.reset(PK11_ImportSymKey(slot.get(), mech, PK11_OriginUnwrap, CKA_SIGN,
+                                  ikm_item, nullptr));
+    } else {
+      ikm.reset(PK11_ImportDataKey(slot.get(), mech, PK11_OriginUnwrap,
+                                   CKA_SIGN, ikm_item, nullptr));
+    }
 
     return ikm;
   }
 
-  void RunTest(hkdf_vector vec, HkdfTestType type, CK_KEY_TYPE keyType) {
+  void RunTest(hkdf_vector vec, HkdfTestType test_type, CK_KEY_TYPE key_type,
+               Pk11ImportType import_type) {
     SECItem ikm_item = {siBuffer, vec.ikm.data(),
                         static_cast<unsigned int>(vec.ikm.size())};
     SECItem salt_item = {siBuffer, vec.salt.data(),
@@ -88,33 +102,23 @@ class Pkcs11HkdfTest : public ::testing::TestWithParam<hkdf_vector> {
     SECItem params_item = {siBuffer, (unsigned char *)&nss_hkdf_params,
                            sizeof(nss_hkdf_params)};
 
-    ScopedPK11SymKey ikm = ImportKey(keyType, &ikm_item);
+    ScopedPK11SymKey ikm = ImportKey(key_type, &ikm_item, import_type);
     ScopedPK11SymKey salt_key = NULL;
     ASSERT_NE(nullptr, ikm.get());
 
-    switch (type) {
+    switch (test_type) {
       case kNSSHkdfLegacy:
         printf("kNSSHkdfLegacy\n");
         break; /* already set up */
       case kPkcs11HkdfDeriveDataKey: {
         ScopedPK11SlotInfo slot(PK11_GetSlotFromKey(ikm.get()));
-        CK_OBJECT_CLASS ckoData = CKO_DATA;
-        CK_OBJECT_HANDLE handle;
-        CK_ATTRIBUTE pk11template[2] = {
-            {CKA_CLASS, (CK_BYTE_PTR)&ckoData, sizeof(ckoData)},
-            {CKA_VALUE, vec.ikm.data(), static_cast<CK_ULONG>(vec.ikm.size())}};
-
-        ScopedPK11GenericObject dataKey(PK11_CreateGenericObject(
-            slot.get(), pk11template, PR_ARRAY_SIZE(pk11template), PR_FALSE));
-        ASSERT_NE(nullptr, dataKey.get());
-        handle = PK11_GetObjectHandle(PK11_TypeGeneric, dataKey.get(), NULL);
-        ASSERT_NE((CK_ULONG)CK_INVALID_HANDLE, (CK_ULONG)handle);
         /* replaces old key with our new data key */
-        ikm = ScopedPK11SymKey(
+        SECItem data_item = {siBuffer, vec.ikm.data(),
-            PK11_SymKeyFromHandle(slot.get(), NULL, PK11_OriginUnwrap,
+                             static_cast<unsigned int>(vec.ikm.size())};
-                                  CKM_HKDF_DERIVE, handle, PR_TRUE, NULL));
+        ikm = ScopedPK11SymKey(PK11_ImportDataKey(slot.get(), CKM_HKDF_DERIVE,
+                                                  PK11_OriginUnwrap, CKA_DERIVE,
+                                                  &data_item, NULL));
         ASSERT_NE(nullptr, ikm.get());
-        /* generic object is freed, ikm owns the handle */
       }
       /* fall through */
       case kPkcs11HkdfSaltDerive:
@@ -122,8 +126,8 @@ class Pkcs11HkdfTest : public ::testing::TestWithParam<hkdf_vector> {
         if (hkdf_params.ulSaltLen == 0) {
           hkdf_params.ulSaltType = CKF_HKDF_SALT_NULL;
           printf("kPkcs11HkdfNullSaltDerive\n");
-        } else if (type == kPkcs11HkdfSaltDerive) {
+        } else if (test_type == kPkcs11HkdfSaltDerive) {
-          salt_key = ImportKey(keyType, &salt_item);
+          salt_key = ImportKey(key_type, &salt_item, import_type);
           hkdf_params.ulSaltType = CKF_HKDF_SALT_KEY;
           hkdf_params.ulSaltLen = 0;
           hkdf_params.pSalt = NULL;
@@ -131,7 +135,7 @@ class Pkcs11HkdfTest : public ::testing::TestWithParam<hkdf_vector> {
           printf("kPkcs11HkdfSaltDerive\n");
         } else {
           printf("kPkcs11HkdfDerive%s\n",
-                 (type == kPkcs11HkdfDeriveDataKey) ? "DataKey" : "");
+                 (test_type == kPkcs11HkdfDeriveDataKey) ? "DataKey" : "");
         }
         hkdf_params.prfHashMechanism = Pk11HkdfToHash(vec.mech);
         params_item.data = (unsigned char *)&hkdf_params;
@@ -166,23 +170,27 @@ class Pkcs11HkdfTest : public ::testing::TestWithParam<hkdf_vector> {
       ASSERT_EQ(nullptr, okm.get());
     }
   }
-  void RunTest(hkdf_vector vec) {
+  void RunTest(hkdf_vector vec, Pk11ImportType import_type) {
     HkdfTestType test_type;
 
     for (test_type = kNSSHkdfLegacy; test_type < kPKCS11NumTypes; test_type++) {
-      RunTest(vec, test_type, CKK_GENERIC_SECRET);
+      RunTest(vec, test_type, CKK_GENERIC_SECRET, import_type);
       if (test_type == kPkcs11HkdfDeriveDataKey) {
         continue;
       }
-      RunTest(vec, test_type, CKK_HKDF);
+      RunTest(vec, test_type, CKK_HKDF, import_type);
     }
   }
 };
 
-TEST_P(Pkcs11HkdfTest, TestVectors) { RunTest(GetParam()); }
+TEST_P(Pkcs11HkdfTest, TestVectors) {
+  RunTest(std::get<0>(GetParam()), std::get<1>(GetParam()));
+}
 
-INSTANTIATE_TEST_CASE_P(Pkcs11HkdfTests, Pkcs11HkdfTest,
+INSTANTIATE_TEST_CASE_P(
-                        ::testing::ValuesIn(kHkdfTestVectors));
+    Pkcs11HkdfTests, Pkcs11HkdfTest,
+    ::testing::Combine(::testing::ValuesIn(kHkdfTestVectors),
+                       ::testing::ValuesIn(kImportTypesAll)));
 
 TEST_F(Pkcs11HkdfTest, OkmLimits) {
   hkdf_vector vector{
@@ -198,44 +206,44 @@ TEST_F(Pkcs11HkdfTest, OkmLimits) {
   };
 
   // SHA1 limit
-  RunTest(vector);
+  RunTest(vector, Pk11ImportType::key);
 
   // SHA1 limit + 1
   vector.l += 1;
   vector.res.expect_rv = SECFailure;
-  RunTest(vector);
+  RunTest(vector, Pk11ImportType::key);
 
   // SHA256 limit
   vector.mech = CKM_NSS_HKDF_SHA256;
   vector.l = 255 * SHA256_LENGTH; /* per rfc5869 */
   vector.res.expect_rv = SECSuccess;
-  RunTest(vector);
+  RunTest(vector, Pk11ImportType::data);
 
   // SHA256 limit + 1
   vector.l += 1;
   vector.res.expect_rv = SECFailure;
-  RunTest(vector);
+  RunTest(vector, Pk11ImportType::data);
 
   // SHA384 limit
   vector.mech = CKM_NSS_HKDF_SHA384;
   vector.l = 255 * SHA384_LENGTH; /* per rfc5869 */
   vector.res.expect_rv = SECSuccess;
-  RunTest(vector);
+  RunTest(vector, Pk11ImportType::key);
 
   // SHA384 limit + 1
   vector.l += 1;
   vector.res.expect_rv = SECFailure;
-  RunTest(vector);
+  RunTest(vector, Pk11ImportType::key);
 
   // SHA512 limit
   vector.mech = CKM_NSS_HKDF_SHA512;
   vector.l = 255 * SHA512_LENGTH; /* per rfc5869 */
   vector.res.expect_rv = SECSuccess;
-  RunTest(vector);
+  RunTest(vector, Pk11ImportType::data);
 
   // SHA512 limit + 1
   vector.l += 1;
   vector.res.expect_rv = SECFailure;
-  RunTest(vector);
+  RunTest(vector, Pk11ImportType::data);
 }
 }  // namespace nss_test

security/nss/lib/nss/nss.def

@@ -1187,3 +1187,9 @@ PK11_FindEncodedCertInSlot;
 ;+    local:
 ;+       *;
 ;+};
+;+NSS_3.58 { 	# NSS 3.58 release
+;+    global:
+PK11_ImportDataKey;
+;+    local:
+;+       *;
+;+};

security/nss/lib/pk11wrap/pk11pub.h

@@ -267,6 +267,8 @@ CK_MECHANISM_TYPE PK11_MapSignKeyType(KeyType keyType);
  **********************************************************************/
 void PK11_FreeSymKey(PK11SymKey *key);
 PK11SymKey *PK11_ReferenceSymKey(PK11SymKey *symKey);
+PK11SymKey *PK11_ImportDataKey(PK11SlotInfo *slot, CK_MECHANISM_TYPE type, PK11Origin origin,
+                               CK_ATTRIBUTE_TYPE operation, SECItem *key, void *wincx);
 PK11SymKey *PK11_ImportSymKey(PK11SlotInfo *slot, CK_MECHANISM_TYPE type,
                               PK11Origin origin, CK_ATTRIBUTE_TYPE operation, SECItem *key, void *wincx);
 PK11SymKey *PK11_ImportSymKeyWithFlags(PK11SlotInfo *slot,

security/nss/lib/pk11wrap/pk11skey.c

@@ -506,10 +506,37 @@ PK11_ImportSymKey(PK11SlotInfo *slot, CK_MECHANISM_TYPE type,
                                         keyTemplate, templateCount, key, wincx);
     return symKey;
 }
+/* Import a PKCS #11 data object and return it as a key. This key is
+ * only useful in a limited number of mechanisms, such as HKDF. */
+PK11SymKey *
+PK11_ImportDataKey(PK11SlotInfo *slot, CK_MECHANISM_TYPE type, PK11Origin origin,
+                   CK_ATTRIBUTE_TYPE operation, SECItem *key, void *wincx)
+{
+    CK_OBJECT_CLASS ckoData = CKO_DATA;
+    CK_ATTRIBUTE template[2] = { { CKA_CLASS, (CK_BYTE_PTR)&ckoData, sizeof(ckoData) },
+                                 { CKA_VALUE, (CK_BYTE_PTR)key->data, key->len } };
+    CK_OBJECT_HANDLE handle;
+    PK11GenericObject *genObject;
 
-/*
+    genObject = PK11_CreateGenericObject(slot, template, PR_ARRAY_SIZE(template), PR_FALSE);
- * turn key bits into an appropriate key object
+    if (genObject == NULL) {
- */
+        return NULL;
+    }
+    handle = PK11_GetObjectHandle(PK11_TypeGeneric, genObject, NULL);
+    if (handle == CK_INVALID_HANDLE) {
+        return NULL;
+    }
+    /* A note about ownership of the PKCS #11 handle:
+     * PK11_CreateGenericObject() will not destroy the object it creates
+     * on Free, For that you want PK11_CreateManagedGenericObject().
+     * Below we import the handle into the symKey structure. We pass
+     * PR_TRUE as the owner so that the symKey will destroy the object
+     * once it's freed. This is way it's safe to free now. */
+    PK11_DestroyGenericObject(genObject);
+    return PK11_SymKeyFromHandle(slot, NULL, origin, type, handle, PR_TRUE, wincx);
+}
+
+/* turn key bits into an appropriate key object */
 PK11SymKey *
 PK11_ImportSymKeyWithFlags(PK11SlotInfo *slot, CK_MECHANISM_TYPE type,
                            PK11Origin origin, CK_ATTRIBUTE_TYPE operation, SECItem *key,

security/nss/lib/ssl/tls13hkdf.c

@@ -38,6 +38,7 @@ tls13_HkdfExtract(PK11SymKey *ikm1, PK11SymKey *ikm2, SSLHashType baseHash,
     SECItem paramsi;
     PK11SymKey *prk;
     static const PRUint8 zeroKeyBuf[HASH_LENGTH_MAX];
+    SECItem zeroKeyItem = { siBuffer, CONST_CAST(PRUint8, zeroKeyBuf), kTlsHkdfInfo[baseHash].hashSize };
     PK11SlotInfo *slot = NULL;
     PK11SymKey *newIkm2 = NULL;
     PK11SymKey *newIkm1 = NULL;
@@ -102,44 +103,14 @@ tls13_HkdfExtract(PK11SymKey *ikm1, PK11SymKey *ikm2, SSLHashType baseHash,
 
     /* A zero ikm2 is a key of hash-length 0s. */
     if (!ikm2) {
-        CK_OBJECT_CLASS ckoData = CKO_DATA;
-        CK_ATTRIBUTE template[2] = {
-            { CKA_CLASS, (CK_BYTE_PTR)&ckoData, sizeof(ckoData) },
-            { CKA_VALUE, (CK_BYTE_PTR)zeroKeyBuf, kTlsHkdfInfo[baseHash].hashSize }
-        };
-        CK_OBJECT_HANDLE handle;
-        PK11GenericObject *genObject;
-
         /* if we have ikm1, put the zero key in the same slot */
         slot = ikm1 ? PK11_GetSlotFromKey(ikm1) : PK11_GetBestSlot(CKM_HKDF_DERIVE, NULL);
         if (!slot) {
             return SECFailure;
         }
-        genObject = PK11_CreateGenericObject(slot, template,
-                                             PR_ARRAY_SIZE(template), PR_FALSE);
-        if (genObject == NULL) {
-            return SECFailure;
-        }
-        handle = PK11_GetObjectHandle(PK11_TypeGeneric, genObject, NULL);
-        if (handle == CK_INVALID_HANDLE) {
-            return SECFailure;
-        }
-        /* A note about ownership of the PKCS #11 handle.
-         * PK11_CreateGenericObject() will not destroy the object it creates
-         * on Free, For that you want PK11_CreateManagedGenericObject().
-         * Below we import the handle into the symKey structure. We pass
-         * PR_TRUE as the owner so that the symKey will destroy the object
-         * once it's freed. This is way it's safe to free now */
-        PK11_DestroyGenericObject(genObject);
 
-        /* NOTE: we can't both be in this block, and the block above where
+        newIkm2 = PK11_ImportDataKey(slot, CKM_HKDF_DERIVE, PK11_OriginUnwrap,
-         * we moved the keys (because this block requires ikm2 to be NULL and
+                                     CKA_DERIVE, &zeroKeyItem, NULL);
-         * the other requires ikm2 to be non-NULL. It's therefore safe to
-         * use newIkm2 in both cases and have a single free at the end for
-         * both */
-        PORT_Assert(newIkm2 == NULL); /* verify logic of the above statement */
-        newIkm2 = PK11_SymKeyFromHandle(slot, NULL, PK11_OriginUnwrap,
-                                        CKM_HKDF_DERIVE, handle, PR_TRUE, NULL);
         if (!newIkm2) {
             return SECFailure;
         }