Bug 1667153 - Add PK11_ImportDataKey API. r=rrelyea
authorKevin Jacobs <kjacobs@mozilla.com>
Thu, 24 Sep 2020 19:25:32 +0000
changeset 15764 8fdbec414ce239ab243b929df9c0c9724b7daa20
parent 15763 8ebee3cec9cfb4d5b4355173f193b8e5f5bb9e73
child 15765 c7d3b214dd4199fc7ab6040a9e7ef14149ca2151
push id3847
push userkjacobs@mozilla.com
push dateThu, 24 Sep 2020 19:40:49 +0000
reviewersrrelyea
bugs1667153
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. Differential Revision: https://phabricator.services.mozilla.com/D91316
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
--- a/automation/abi-check/expected-report-libnss3.so.txt
+++ b/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}
--- a/gtests/pk11_gtest/pk11_hkdf_unittest.cc
+++ b/gtests/pk11_gtest/pk11_hkdf_unittest.cc
@@ -21,17 +21,22 @@ namespace nss_test {
 typedef int HkdfTestType;
 const int kNSSHkdfLegacy = 0;
 const int kPkcs11HkdfDerive = 1;
 const int kPkcs11HkdfDeriveDataKey = 2;
 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) {
       case CKM_NSS_HKDF_SHA1:
         return CKM_SHA_1;
       case CKM_NSS_HKDF_SHA256:
         return CKM_SHA256;
       case CKM_NSS_HKDF_SHA384:
@@ -40,39 +45,48 @@ class Pkcs11HkdfTest : public ::testing:
         return CKM_SHA512;
       default:
         break;
     }
 
     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;
       case CKK_HKDF:
         mech = CKM_HKDF_KEY_GEN;
         break;
     }
 
-    ScopedPK11SymKey ikm(PK11_ImportSymKey(slot.get(), mech, PK11_OriginUnwrap,
-                                           CKA_SIGN, ikm_item, nullptr));
+    ScopedPK11SymKey ikm;
+
+    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(),
                          static_cast<unsigned int>(vec.salt.size())};
     CK_MECHANISM_TYPE derive_mech = vec.mech;
     CK_NSS_HKDFParams nss_hkdf_params = {
         true, vec.salt.data(), static_cast<unsigned int>(vec.salt.size()),
         true, vec.info.data(), static_cast<unsigned int>(vec.info.size())};
@@ -83,60 +97,50 @@ class Pkcs11HkdfTest : public ::testing:
                                   vec.salt.data(),
                                   static_cast<unsigned int>(vec.salt.size()),
                                   CK_INVALID_HANDLE,
                                   vec.info.data(),
                                   static_cast<unsigned int>(vec.info.size())};
     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(
-            PK11_SymKeyFromHandle(slot.get(), NULL, PK11_OriginUnwrap,
-                                  CKM_HKDF_DERIVE, handle, PR_TRUE, NULL));
+        SECItem data_item = {siBuffer, vec.ikm.data(),
+                             static_cast<unsigned int>(vec.ikm.size())};
+        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:
       case kPkcs11HkdfDerive:
         if (hkdf_params.ulSaltLen == 0) {
           hkdf_params.ulSaltType = CKF_HKDF_SALT_NULL;
           printf("kPkcs11HkdfNullSaltDerive\n");
-        } else if (type == kPkcs11HkdfSaltDerive) {
-          salt_key = ImportKey(keyType, &salt_item);
+        } else if (test_type == kPkcs11HkdfSaltDerive) {
+          salt_key = ImportKey(key_type, &salt_item, import_type);
           hkdf_params.ulSaltType = CKF_HKDF_SALT_KEY;
           hkdf_params.ulSaltLen = 0;
           hkdf_params.pSalt = NULL;
           hkdf_params.hSaltKey = PK11_GetSymKeyHandle(salt_key.get());
           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;
         params_item.len = sizeof(hkdf_params);
         derive_mech = CKM_HKDF_DERIVE;
         break;
       case kPkcs11HkdfData:
         printf("kPkcs11HkdfData\n");
@@ -161,81 +165,85 @@ class Pkcs11HkdfTest : public ::testing:
         SECItem vec_okm_item = {siBuffer, vec.okm.data(),
                                 static_cast<unsigned int>(vec.okm.size())};
         ASSERT_EQ(0, SECITEM_CompareItem(&vec_okm_item, act_okm_item));
       }
     } else {
       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,
-                        ::testing::ValuesIn(kHkdfTestVectors));
+INSTANTIATE_TEST_CASE_P(
+    Pkcs11HkdfTests, Pkcs11HkdfTest,
+    ::testing::Combine(::testing::ValuesIn(kHkdfTestVectors),
+                       ::testing::ValuesIn(kImportTypesAll)));
 
 TEST_F(Pkcs11HkdfTest, OkmLimits) {
   hkdf_vector vector{
       0,
       CKM_NSS_HKDF_SHA1,
       255 * SHA1_LENGTH /* per rfc5869 */,
       {0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b,
        0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b},
       {},
       {},
       {},
       {SECSuccess, false} /* Only looking at return value */
   };
 
   // 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
--- a/lib/nss/nss.def
+++ b/lib/nss/nss.def
@@ -1182,8 +1182,14 @@ SECMOD_GetSystemFIPSEnabled;
 ;+       *;
 ;+};
 ;+NSS_3.55 { 	# NSS 3.55 release
 ;+    global:
 PK11_FindEncodedCertInSlot;
 ;+    local:
 ;+       *;
 ;+};
+;+NSS_3.58 { 	# NSS 3.58 release
+;+    global:
+PK11_ImportDataKey;
+;+    local:
+;+       *;
+;+};
--- a/lib/pk11wrap/pk11pub.h
+++ b/lib/pk11wrap/pk11pub.h
@@ -262,16 +262,18 @@ CK_RV PK11_MapPBEMechanismToCryptoMechan
 CK_MECHANISM_TYPE PK11_GetPadMechanism(CK_MECHANISM_TYPE);
 CK_MECHANISM_TYPE PK11_MapSignKeyType(KeyType keyType);
 
 /**********************************************************************
  *                   Symmetric, Public, and Private Keys
  **********************************************************************/
 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,
                                        CK_MECHANISM_TYPE type, PK11Origin origin, CK_ATTRIBUTE_TYPE operation,
                                        SECItem *key, CK_FLAGS flags, PRBool isPerm, void *wincx);
 PK11SymKey *PK11_SymKeyFromHandle(PK11SlotInfo *slot, PK11SymKey *parent,
                                   PK11Origin origin, CK_MECHANISM_TYPE type, CK_OBJECT_HANDLE keyID,
                                   PRBool owner, void *wincx);
--- a/lib/pk11wrap/pk11skey.c
+++ b/lib/pk11wrap/pk11skey.c
@@ -501,20 +501,47 @@ PK11_ImportSymKey(PK11SlotInfo *slot, CK
     templateCount = attrs - keyTemplate;
     PR_ASSERT(templateCount + 1 <= sizeof(keyTemplate) / sizeof(CK_ATTRIBUTE));
 
     keyType = PK11_GetKeyType(type, key->len);
     symKey = pk11_ImportSymKeyWithTempl(slot, type, origin, PR_FALSE,
                                         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;
 
-/*
- * turn key bits into an appropriate key object
- */
+    genObject = PK11_CreateGenericObject(slot, template, PR_ARRAY_SIZE(template), PR_FALSE);
+    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,
                            CK_FLAGS flags, PRBool isPerm, void *wincx)
 {
     PK11SymKey *symKey;
     unsigned int templateCount = 0;
     CK_OBJECT_CLASS keyClass = CKO_SECRET_KEY;
--- a/lib/ssl/tls13hkdf.c
+++ b/lib/ssl/tls13hkdf.c
@@ -33,16 +33,17 @@ static const struct {
 SECStatus
 tls13_HkdfExtract(PK11SymKey *ikm1, PK11SymKey *ikm2, SSLHashType baseHash,
                   PK11SymKey **prkp)
 {
     CK_HKDF_PARAMS params;
     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;
     SECStatus rv;
 
     params.bExtract = CK_TRUE;
     params.bExpand = CK_FALSE;
     params.prfHashMechanism = kTlsHkdfInfo[baseHash].pkcs11Mech;
@@ -97,54 +98,24 @@ tls13_HkdfExtract(PK11SymKey *ikm1, PK11
     paramsi.len = sizeof(params);
 
     PORT_Assert(kTlsHkdfInfo[baseHash].pkcs11Mech);
     PORT_Assert(kTlsHkdfInfo[baseHash].hashSize);
     PORT_Assert(kTlsHkdfInfo[baseHash].hash == 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
-         * we moved the keys (because this block requires ikm2 to be NULL and
-         * 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);
+        newIkm2 = PK11_ImportDataKey(slot, CKM_HKDF_DERIVE, PK11_OriginUnwrap,
+                                     CKA_DERIVE, &zeroKeyItem, NULL);
         if (!newIkm2) {
             return SECFailure;
         }
         ikm2 = newIkm2;
     }
     PORT_Assert(ikm2);
 
     PRINT_BUF(50, (NULL, "HKDF Extract: IKM1/Salt", params.pSalt, params.ulSaltLen));