Bug 1529813 - Expose Hkdf-Expand-Label with mechanism, r=ekrNSS_3_43_BETA4

Summary:
It turns out that leaf keys sometimes need to be exposed with different
mechanisms and sizes.  The default function provides something good enough for
use with the AEAD functions that were exposed, but if you want to use the key
directly, that isn't enough.  So here we are: new arguments for specifying the
mechanism and key size are needed.

Reviewers: ekr

Tags: #secure-revision

Bug #: 1529813

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

5 files changed, 97 insertions, 12 deletions

diff --git a/gtests/ssl_gtest/tls_hkdf_unittest.cc b/gtests/ssl_gtest/tls_hkdf_unittest.cc
index fe2038664..e1ad9e9f0 100644
--- a/gtests/ssl_gtest/tls_hkdf_unittest.cc
+++ b/gtests/ssl_gtest/tls_hkdf_unittest.cc
@@ -68,6 +68,18 @@ size_t GetHashLength(SSLHashType hash) {
   return 0;
 }
 
+CK_MECHANISM_TYPE GetHkdfMech(SSLHashType hash) {
+  switch (hash) {
+    case ssl_hash_sha256:
+      return CKM_NSS_HKDF_SHA256;
+    case ssl_hash_sha384:
+      return CKM_NSS_HKDF_SHA384;
+    default:
+      ADD_FAILURE() << "Unknown hash: " << hash;
+  }
+  return CKM_INVALID_MECHANISM;
+}
+
 PRUint16 GetSomeCipherSuiteForHash(SSLHashType hash) {
   switch (hash) {
     case ssl_hash_sha256:
@@ -136,15 +148,19 @@ class TlsHkdfTest : public ::testing::Test,
     ImportKey(&k2_, kKey2, hash_type_, slot_.get());
   }
 
-  void VerifyKey(const ScopedPK11SymKey& key, const DataBuffer& expected) {
+  void VerifyKey(const ScopedPK11SymKey& key, CK_MECHANISM_TYPE expected_mech,
+                 const DataBuffer& expected_value) {
+    EXPECT_EQ(expected_mech, PK11_GetMechanism(key.get()));
+
     SECStatus rv = PK11_ExtractKeyValue(key.get());
     ASSERT_EQ(SECSuccess, rv);
 
     SECItem* key_data = PK11_GetKeyData(key.get());
     ASSERT_NE(nullptr, key_data);
 
-    EXPECT_EQ(expected.len(), key_data->len);
-    EXPECT_EQ(0, memcmp(expected.data(), key_data->data, expected.len()));
+    EXPECT_EQ(expected_value.len(), key_data->len);
+    EXPECT_EQ(
+        0, memcmp(expected_value.data(), key_data->data, expected_value.len()));
   }
 
   void HkdfExtract(const ScopedPK11SymKey& ikmk1, const ScopedPK11SymKey& ikmk2,
@@ -157,7 +173,7 @@ class TlsHkdfTest : public ::testing::Test,
     ScopedPK11SymKey prkk(prk);
 
     DumpKey("Output", prkk);
-    VerifyKey(prkk, expected);
+    VerifyKey(prkk, GetHkdfMech(base_hash), expected);
 
     // Now test the public wrapper.
     PRUint16 cs = GetSomeCipherSuiteForHash(base_hash);
@@ -165,7 +181,7 @@ class TlsHkdfTest : public ::testing::Test,
                          ikmk2.get(), &prk);
     ASSERT_EQ(SECSuccess, rv);
     ASSERT_NE(nullptr, prk);
-    VerifyKey(ScopedPK11SymKey(prk), expected);
+    VerifyKey(ScopedPK11SymKey(prk), GetHkdfMech(base_hash), expected);
   }
 
   void HkdfExpandLabel(ScopedPK11SymKey* prk, SSLHashType base_hash,
@@ -191,7 +207,23 @@ class TlsHkdfTest : public ::testing::Test,
                              &secret);
     EXPECT_EQ(SECSuccess, rv);
     ASSERT_NE(nullptr, prk);
-    VerifyKey(ScopedPK11SymKey(secret), expected);
+    VerifyKey(ScopedPK11SymKey(secret), GetHkdfMech(base_hash), expected);
+
+    // Verify that a key can be created with a different key type and size.
+    rv = SSL_HkdfExpandLabelWithMech(
+        SSL_LIBRARY_VERSION_TLS_1_3, cs, prk->get(), session_hash,
+        session_hash_len, label, label_len, CKM_DES3_CBC_PAD, 24, &secret);
+    EXPECT_EQ(SECSuccess, rv);
+    ASSERT_NE(nullptr, prk);
+    ScopedPK11SymKey with_mech(secret);
+    EXPECT_EQ(static_cast<CK_MECHANISM_TYPE>(CKM_DES3_CBC_PAD),
+              PK11_GetMechanism(with_mech.get()));
+    // Just verify that the key is the right size.
+    rv = PK11_ExtractKeyValue(with_mech.get());
+    ASSERT_EQ(SECSuccess, rv);
+    SECItem* key_data = PK11_GetKeyData(with_mech.get());
+    ASSERT_NE(nullptr, key_data);
+    EXPECT_EQ(24U, key_data->len);
   }
 
  protected:
diff --git a/lib/ssl/sslexp.h b/lib/ssl/sslexp.h
index 4f325e385..d8f050ea5 100644
--- a/lib/ssl/sslexp.h
+++ b/lib/ssl/sslexp.h
@@ -682,7 +682,10 @@ typedef struct SSLAeadContextStr SSLAeadContext;
 
 /* SSL_HkdfExtract and SSL_HkdfExpandLabel implement the functions from TLS,
  * using the version and ciphersuite to set parameters. This allows callers to
- * use these TLS functions as a KDF. This is only supported for TLS 1.3. */
+ * use these TLS functions as a KDF. This is only supported for TLS 1.3.
+ *
+ * SSL_HkdfExtract produces a key with a mechanism that is suitable for input to
+ * SSL_HkdfExpandLabel (and SSL_HkdfExpandLabelWithMech). */
 #define SSL_HkdfExtract(version, cipherSuite, salt, ikm, keyp)      \
     SSL_EXPERIMENTAL_API("SSL_HkdfExtract",                         \
                          (PRUint16 _version, PRUint16 _cipherSuite, \
@@ -690,6 +693,8 @@ typedef struct SSLAeadContextStr SSLAeadContext;
                           PK11SymKey * *_keyp),                     \
                          (version, cipherSuite, salt, ikm, keyp))
 
+/* SSL_HkdfExpandLabel produces a key with a mechanism that is suitable for
+ * input to SSL_HkdfExpandLabel or SSL_MakeAead. */
 #define SSL_HkdfExpandLabel(version, cipherSuite, prk,                     \
                             hsHash, hsHashLen, label, labelLen, keyp)      \
     SSL_EXPERIMENTAL_API("SSL_HkdfExpandLabel",                            \
@@ -701,6 +706,23 @@ typedef struct SSLAeadContextStr SSLAeadContext;
                          (version, cipherSuite, prk,                       \
                           hsHash, hsHashLen, label, labelLen, keyp))
 
+/* SSL_HkdfExpandLabelWithMech uses the KDF from the selected TLS version and
+ * cipher suite, as with the other calls, but the provided mechanism and key
+ * size. This allows the key to be used more widely. */
+#define SSL_HkdfExpandLabelWithMech(version, cipherSuite, prk,             \
+                                    hsHash, hsHashLen, label, labelLen,    \
+                                    mech, keySize, keyp)                   \
+    SSL_EXPERIMENTAL_API("SSL_HkdfExpandLabelWithMech",                    \
+                         (PRUint16 _version, PRUint16 _cipherSuite,        \
+                          PK11SymKey * _prk,                               \
+                          const PRUint8 *_hsHash, unsigned int _hsHashLen, \
+                          const char *_label, unsigned int _labelLen,      \
+                          CK_MECHANISM_TYPE _mech, unsigned int _keySize,  \
+                          PK11SymKey **_keyp),                             \
+                         (version, cipherSuite, prk,                       \
+                          hsHash, hsHashLen, label, labelLen,              \
+                          mech, keySize, keyp))
+
 /* Deprecated experimental APIs */
 #define SSL_UseAltServerHelloType(fd, enable) SSL_DEPRECATED_EXPERIMENTAL_API
 
diff --git a/lib/ssl/sslimpl.h b/lib/ssl/sslimpl.h
index 2dbf60436..247d1107f 100644
--- a/lib/ssl/sslimpl.h
+++ b/lib/ssl/sslimpl.h
@@ -1779,6 +1779,12 @@ SECStatus SSLExp_HkdfExpandLabel(PRUint16 version, PRUint16 cipherSuite, PK11Sym
                                  const PRUint8 *hsHash, unsigned int hsHashLen,
                                  const char *label, unsigned int labelLen,
                                  PK11SymKey **key);
+SECStatus
+SSLExp_HkdfExpandLabelWithMech(PRUint16 version, PRUint16 cipherSuite, PK11SymKey *prk,
+                               const PRUint8 *hsHash, unsigned int hsHashLen,
+                               const char *label, unsigned int labelLen,
+                               CK_MECHANISM_TYPE mech, unsigned int keySize,
+                               PK11SymKey **keyp);
 
 SEC_END_PROTOS
 
diff --git a/lib/ssl/sslprimitive.c b/lib/ssl/sslprimitive.c
index 811827c7d..540c17840 100644
--- a/lib/ssl/sslprimitive.c
+++ b/lib/ssl/sslprimitive.c
@@ -45,7 +45,9 @@ tls13_GetHashAndCipher(PRUint16 version, PRUint16 cipherSuite,
         return SECFailure;
     }
     *hash = suiteDef->prf_hash;
-    *cipher = cipherDef;
+    if (cipher != NULL) {
+        *cipher = cipherDef;
+    }
     return SECSuccess;
 }
 
@@ -216,9 +218,8 @@ SSLExp_HkdfExtract(PRUint16 version, PRUint16 cipherSuite,
     }
 
     SSLHashType hash;
-    const ssl3BulkCipherDef *cipher; /* Unused here. */
     SECStatus rv = tls13_GetHashAndCipher(version, cipherSuite,
-                                          &hash, &cipher);
+                                          &hash, NULL);
     if (rv != SECSuccess) {
         return SECFailure; /* Code already set. */
     }
@@ -238,9 +239,8 @@ SSLExp_HkdfExpandLabel(PRUint16 version, PRUint16 cipherSuite, PK11SymKey *prk,
     }
 
     SSLHashType hash;
-    const ssl3BulkCipherDef *cipher; /* Unused here. */
     SECStatus rv = tls13_GetHashAndCipher(version, cipherSuite,
-                                          &hash, &cipher);
+                                          &hash, NULL);
     if (rv != SECSuccess) {
         return SECFailure; /* Code already set. */
     }
@@ -248,3 +248,27 @@ SSLExp_HkdfExpandLabel(PRUint16 version, PRUint16 cipherSuite, PK11SymKey *prk,
                                  tls13_GetHkdfMechanismForHash(hash),
                                  tls13_GetHashSizeForHash(hash), keyp);
 }
+
+SECStatus
+SSLExp_HkdfExpandLabelWithMech(PRUint16 version, PRUint16 cipherSuite, PK11SymKey *prk,
+                               const PRUint8 *hsHash, unsigned int hsHashLen,
+                               const char *label, unsigned int labelLen,
+                               CK_MECHANISM_TYPE mech, unsigned int keySize,
+                               PK11SymKey **keyp)
+{
+    if (prk == NULL || keyp == NULL ||
+        label == NULL || labelLen == 0 ||
+        mech == CKM_INVALID_MECHANISM || keySize == 0) {
+        PORT_SetError(SEC_ERROR_INVALID_ARGS);
+        return SECFailure;
+    }
+
+    SSLHashType hash;
+    SECStatus rv = tls13_GetHashAndCipher(version, cipherSuite,
+                                          &hash, NULL);
+    if (rv != SECSuccess) {
+        return SECFailure; /* Code already set. */
+    }
+    return tls13_HkdfExpandLabel(prk, hash, hsHash, hsHashLen, label, labelLen,
+                                 mech, keySize, keyp);
+}
diff --git a/lib/ssl/sslsock.c b/lib/ssl/sslsock.c
index 735710531..96b963bc2 100644
--- a/lib/ssl/sslsock.c
+++ b/lib/ssl/sslsock.c
@@ -4054,6 +4054,7 @@ struct {
     EXP(InstallExtensionHooks),
     EXP(HkdfExtract),
     EXP(HkdfExpandLabel),
+    EXP(HkdfExpandLabelWithMech),
     EXP(KeyUpdate),
     EXP(MakeAead),
     EXP(RecordLayerData),