Bug 1515342 - Tests for invalid DH public keys, r=jcj

Summary:
This prevents crashes on invalid, particularly NULL, keys for DH and ECDH.

I factored out test code already landed for this.

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

6 files changed, 289 insertions, 144 deletions

diff --git a/gtests/pk11_gtest/manifest.mn b/gtests/pk11_gtest/manifest.mn
index e556fa9b6..52b4c06aa 100644
--- a/gtests/pk11_gtest/manifest.mn
+++ b/gtests/pk11_gtest/manifest.mn
@@ -12,18 +12,20 @@ CPPSRCS = \
       pk11_cbc_unittest.cc \
       pk11_chacha20poly1305_unittest.cc \
       pk11_curve25519_unittest.cc \
+      pk11_der_private_key_import_unittest.cc \
       pk11_ecdsa_unittest.cc \
       pk11_encrypt_derive_unittest.cc \
       pk11_export_unittest.cc \
       pk11_find_certs_unittest.cc \
       pk11_import_unittest.cc \
+      pk11_keygen.cc \
+      pk11_key_unittest.cc \
       pk11_pbkdf2_unittest.cc \
       pk11_prf_unittest.cc \
       pk11_prng_unittest.cc \
       pk11_rsapkcs1_unittest.cc \
       pk11_rsapss_unittest.cc \
       pk11_seed_cbc_unittest.cc \
-      pk11_der_private_key_import_unittest.cc \
       $(NULL)
 
 INCLUDES += -I$(CORE_DEPTH)/gtests/google_test/gtest/include \
diff --git a/gtests/pk11_gtest/pk11_gtest.gyp b/gtests/pk11_gtest/pk11_gtest.gyp
index f949ea27f..29c1a4a52 100644
--- a/gtests/pk11_gtest/pk11_gtest.gyp
+++ b/gtests/pk11_gtest/pk11_gtest.gyp
@@ -11,24 +11,26 @@
       'target_name': 'pk11_gtest',
       'type': 'executable',
       'sources': [
-        'pk11_aeskeywrap_unittest.cc',
         'pk11_aes_cmac_unittest.cc',
         'pk11_aes_gcm_unittest.cc',
+        'pk11_aeskeywrap_unittest.cc',
         'pk11_cbc_unittest.cc',
         'pk11_chacha20poly1305_unittest.cc',
         'pk11_cipherop_unittest.cc',
         'pk11_curve25519_unittest.cc',
+        'pk11_der_private_key_import_unittest.cc',
         'pk11_ecdsa_unittest.cc',
         'pk11_encrypt_derive_unittest.cc',
         'pk11_find_certs_unittest.cc',
         'pk11_import_unittest.cc',
+        'pk11_keygen.cc',
+        'pk11_key_unittest.cc',
         'pk11_pbkdf2_unittest.cc',
         'pk11_prf_unittest.cc',
         'pk11_prng_unittest.cc',
         'pk11_rsapkcs1_unittest.cc',
         'pk11_rsapss_unittest.cc',
         'pk11_seed_cbc_unittest.cc',
-        'pk11_der_private_key_import_unittest.cc',
         '<(DEPTH)/gtests/common/gtests.cc'
       ],
       'dependencies': [
diff --git a/gtests/pk11_gtest/pk11_import_unittest.cc b/gtests/pk11_gtest/pk11_import_unittest.cc
index 5ffa92e64..19ecb94a2 100644
--- a/gtests/pk11_gtest/pk11_import_unittest.cc
+++ b/gtests/pk11_gtest/pk11_import_unittest.cc
@@ -15,6 +15,7 @@
 #include "nss_scoped_ptrs.h"
 #include "gtest/gtest.h"
 #include "databuffer.h"
+#include "pk11_keygen.h"
 
 namespace nss_test {
 
@@ -30,7 +31,7 @@ struct PK11GenericObjectsDeleter {
 
 class Pk11KeyImportTestBase : public ::testing::Test {
  public:
-  Pk11KeyImportTestBase(CK_MECHANISM_TYPE mech) : mech_(mech) {}
+  Pk11KeyImportTestBase() = default;
   virtual ~Pk11KeyImportTestBase() = default;
 
   void SetUp() override {
@@ -42,20 +43,19 @@ class Pk11KeyImportTestBase : public ::testing::Test {
     password_.reset(SECITEM_DupItem(&pwItem));
   }
 
-  void Test() {
+  void Test(const Pkcs11KeyPairGenerator& generator) {
     // Generate a key and export it.
-    KeyType key_type;
+    KeyType key_type = nullKey;
     ScopedSECKEYEncryptedPrivateKeyInfo key_info;
     ScopedSECItem public_value;
-    GenerateAndExport(&key_type, &key_info, &public_value);
+    GenerateAndExport(generator, &key_type, &key_info, &public_value);
 
-    ASSERT_NE(nullptr, public_value);
     // Note: NSS is currently unable export wrapped DH keys, so this doesn't
-    // test
-    // CKM_DH_PKCS_KEY_PAIR_GEN beyond generate and verify
+    // test those beyond generate and verify.
     if (key_type == dhKey) {
       return;
     }
+    ASSERT_NE(nullptr, public_value);
     ASSERT_NE(nullptr, key_info);
 
     // Now import the encrypted key.
@@ -73,17 +73,6 @@ class Pk11KeyImportTestBase : public ::testing::Test {
     CheckForPublicKey(priv_key, public_value.get());
   }
 
- protected:
-  class ParamHolder {
-   public:
-    virtual ~ParamHolder() = default;
-    virtual void* get() = 0;
-  };
-
-  virtual std::unique_ptr<ParamHolder> MakeParams() = 0;
-
-  CK_MECHANISM_TYPE mech_;
-
  private:
   SECItem GetPublicComponent(ScopedSECKEYPublicKey& pub_key) {
     SECItem null = {siBuffer, NULL, 0};
@@ -202,20 +191,14 @@ class Pk11KeyImportTestBase : public ::testing::Test {
     }
   }
 
-  void GenerateAndExport(KeyType* key_type,
+  void GenerateAndExport(const Pkcs11KeyPairGenerator& generator,
+                         KeyType* key_type,
                          ScopedSECKEYEncryptedPrivateKeyInfo* key_info,
                          ScopedSECItem* public_value) {
-    auto params = MakeParams();
-    ASSERT_NE(nullptr, params);
-
-    SECKEYPublicKey* pub_tmp;
-    ScopedSECKEYPrivateKey priv_key(
-        PK11_GenerateKeyPair(slot_.get(), mech_, params->get(), &pub_tmp,
-                             PR_FALSE, PR_TRUE, nullptr));
-    ASSERT_NE(nullptr, priv_key) << "PK11_GenerateKeyPair failed: "
-                                 << PORT_ErrorToName(PORT_GetError());
-    ScopedSECKEYPublicKey pub_key(pub_tmp);
-    ASSERT_NE(nullptr, pub_key);
+    ScopedSECKEYPrivateKey priv_key;
+    ScopedSECKEYPublicKey pub_key;
+    generator.GenerateKey(&priv_key, &pub_key);
+    ASSERT_TRUE(priv_key);
 
     // Save the public value, which we will need on import */
     SECItem* pub_val;
@@ -240,14 +223,13 @@ class Pk11KeyImportTestBase : public ::testing::Test {
     CheckForPublicKey(priv_key, pub_val);
 
     *key_type = t;
-    public_value->reset(SECITEM_DupItem(pub_val));
-
     // Note: NSS is currently unable export wrapped DH keys, so this doesn't
-    // test
-    // CKM_DH_PKCS_KEY_PAIR_GEN beyond generate and verify
-    if (mech_ == CKM_DH_PKCS_KEY_PAIR_GEN) {
+    // test those beyond generate and verify.
+    if (t == dhKey) {
       return;
     }
+    public_value->reset(SECITEM_DupItem(pub_val));
+
     // Wrap and export the key.
     ScopedSECKEYEncryptedPrivateKeyInfo epki(PK11_ExportEncryptedPrivKeyInfo(
         slot_.get(), SEC_OID_AES_256_CBC, password_.get(), priv_key.get(), 1,
@@ -266,82 +248,13 @@ class Pk11KeyImportTest
     : public Pk11KeyImportTestBase,
       public ::testing::WithParamInterface<CK_MECHANISM_TYPE> {
  public:
-  Pk11KeyImportTest() : Pk11KeyImportTestBase(GetParam()) {}
+  Pk11KeyImportTest() = default;
   virtual ~Pk11KeyImportTest() = default;
-
- protected:
-  std::unique_ptr<ParamHolder> MakeParams() override {
-    switch (mech_) {
-      case CKM_RSA_PKCS_KEY_PAIR_GEN:
-        return std::unique_ptr<ParamHolder>(new RsaParamHolder());
-
-      case CKM_DSA_KEY_PAIR_GEN:
-      case CKM_DH_PKCS_KEY_PAIR_GEN: {
-        PQGParams* pqg_params = nullptr;
-        PQGVerify* pqg_verify = nullptr;
-        const unsigned int key_size = 1024;
-        SECStatus rv = PK11_PQG_ParamGenV2(key_size, 0, key_size / 16,
-                                           &pqg_params, &pqg_verify);
-        if (rv != SECSuccess) {
-          ADD_FAILURE() << "PK11_PQG_ParamGenV2 failed";
-          return nullptr;
-        }
-        EXPECT_NE(nullptr, pqg_verify);
-        EXPECT_NE(nullptr, pqg_params);
-        PK11_PQG_DestroyVerify(pqg_verify);
-        if (mech_ == CKM_DSA_KEY_PAIR_GEN) {
-          return std::unique_ptr<ParamHolder>(new PqgParamHolder(pqg_params));
-        }
-        return std::unique_ptr<ParamHolder>(new DhParamHolder(pqg_params));
-      }
-
-      default:
-        ADD_FAILURE() << "unknown OID " << mech_;
-    }
-    return nullptr;
-  }
-
- private:
-  class RsaParamHolder : public ParamHolder {
-   public:
-    RsaParamHolder()
-        : params_({/*.keySizeInBits = */ 1024, /*.pe = */ 0x010001}) {}
-    ~RsaParamHolder() = default;
-
-    void* get() override { return &params_; }
-
-   private:
-    PK11RSAGenParams params_;
-  };
-
-  class PqgParamHolder : public ParamHolder {
-   public:
-    PqgParamHolder(PQGParams* params) : params_(params) {}
-    ~PqgParamHolder() = default;
-
-    void* get() override { return params_.get(); }
-
-   private:
-    ScopedPQGParams params_;
-  };
-
-  class DhParamHolder : public PqgParamHolder {
-   public:
-    DhParamHolder(PQGParams* params)
-        : PqgParamHolder(params),
-          params_({/*.arena = */ nullptr,
-                   /*.prime = */ params->prime,
-                   /*.base = */ params->base}) {}
-    ~DhParamHolder() = default;
-
-    void* get() override { return &params_; }
-
-   private:
-    SECKEYDHParams params_;
-  };
 };
 
-TEST_P(Pk11KeyImportTest, GenerateExportImport) { Test(); }
+TEST_P(Pk11KeyImportTest, GenerateExportImport) {
+  Test(Pkcs11KeyPairGenerator(GetParam()));
+}
 
 INSTANTIATE_TEST_CASE_P(Pk11KeyImportTest, Pk11KeyImportTest,
                         ::testing::Values(CKM_RSA_PKCS_KEY_PAIR_GEN,
@@ -351,42 +264,13 @@ INSTANTIATE_TEST_CASE_P(Pk11KeyImportTest, Pk11KeyImportTest,
 class Pk11KeyImportTestEC : public Pk11KeyImportTestBase,
                             public ::testing::WithParamInterface<SECOidTag> {
  public:
-  Pk11KeyImportTestEC() : Pk11KeyImportTestBase(CKM_EC_KEY_PAIR_GEN) {}
+  Pk11KeyImportTestEC() = default;
   virtual ~Pk11KeyImportTestEC() = default;
-
- protected:
-  std::unique_ptr<ParamHolder> MakeParams() override {
-    return std::unique_ptr<ParamHolder>(new EcParamHolder(GetParam()));
-  }
-
- private:
-  class EcParamHolder : public ParamHolder {
-   public:
-    EcParamHolder(SECOidTag curve_oid) {
-      SECOidData* curve = SECOID_FindOIDByTag(curve_oid);
-      EXPECT_NE(nullptr, curve);
-
-      size_t plen = curve->oid.len + 2;
-      extra_.reset(new uint8_t[plen]);
-      extra_[0] = SEC_ASN1_OBJECT_ID;
-      extra_[1] = static_cast<uint8_t>(curve->oid.len);
-      memcpy(&extra_[2], curve->oid.data, curve->oid.len);
-
-      ec_params_ = {/*.type = */ siBuffer,
-                    /*.data = */ extra_.get(),
-                    /*.len = */ static_cast<unsigned int>(plen)};
-    }
-    ~EcParamHolder() = default;
-
-    void* get() override { return &ec_params_; }
-
-   private:
-    SECKEYECParams ec_params_;
-    std::unique_ptr<uint8_t[]> extra_;
-  };
 };
 
-TEST_P(Pk11KeyImportTestEC, GenerateExportImport) { Test(); }
+TEST_P(Pk11KeyImportTestEC, GenerateExportImport) {
+  Test(Pkcs11KeyPairGenerator(CKM_EC_KEY_PAIR_GEN, GetParam()));
+}
 
 INSTANTIATE_TEST_CASE_P(Pk11KeyImportTestEC, Pk11KeyImportTestEC,
                         ::testing::Values(SEC_OID_SECG_EC_SECP256R1,
diff --git a/gtests/pk11_gtest/pk11_key_unittest.cc b/gtests/pk11_gtest/pk11_key_unittest.cc
new file mode 100644
index 000000000..1351b53de
--- /dev/null
+++ b/gtests/pk11_gtest/pk11_key_unittest.cc
@@ -0,0 +1,80 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#include <memory>
+#include "nss.h"
+#include "pk11pub.h"
+#include "pk11pqg.h"
+#include "prerror.h"
+#include "secoid.h"
+
+#include "gtest/gtest.h"
+#include "nss_scoped_ptrs.h"
+#include "pk11_keygen.h"
+
+namespace nss_test {
+
+class Pkcs11NullKeyTestBase : public ::testing::Test {
+ protected:
+  // This constructs a key pair, then erases the public value from the public
+  // key.  NSS should reject this.
+  void Test(const Pkcs11KeyPairGenerator& generator,
+            CK_MECHANISM_TYPE dh_mech) {
+    ScopedSECKEYPrivateKey priv;
+    ScopedSECKEYPublicKey pub;
+    generator.GenerateKey(&priv, &pub);
+    ASSERT_TRUE(priv);
+
+    // These don't leak because they are allocated to the arena associated with
+    // the public key.
+    SECItem* pub_val = nullptr;
+    switch (SECKEY_GetPublicKeyType(pub.get())) {
+      case rsaKey:
+        pub_val = &pub->u.rsa.modulus;
+        break;
+
+      case dsaKey:
+        pub_val = &pub->u.dsa.publicValue;
+        break;
+
+      case dhKey:
+        pub_val = &pub->u.dh.publicValue;
+        break;
+
+      case ecKey:
+        pub_val = &pub->u.ec.publicValue;
+        break;
+
+      default:
+        FAIL() << "Unknown key type " << SECKEY_GetPublicKeyType(pub.get());
+    }
+    pub_val->data = nullptr;
+    pub_val->len = 0;
+
+    ScopedPK11SymKey symKey(PK11_PubDeriveWithKDF(
+        priv.get(), pub.get(), false, nullptr, nullptr, dh_mech,
+        CKM_SHA512_HMAC, CKA_DERIVE, 0, CKD_NULL, nullptr, nullptr));
+    ASSERT_FALSE(symKey);
+  }
+};
+
+class Pkcs11DhNullKeyTest : public Pkcs11NullKeyTestBase {};
+TEST_F(Pkcs11DhNullKeyTest, UseNullPublicValue) {
+  Test(Pkcs11KeyPairGenerator(CKM_DH_PKCS_KEY_PAIR_GEN), CKM_DH_PKCS_DERIVE);
+}
+
+class Pkcs11EcdhNullKeyTest : public Pkcs11NullKeyTestBase,
+                              public ::testing::WithParamInterface<SECOidTag> {
+};
+TEST_P(Pkcs11EcdhNullKeyTest, UseNullPublicValue) {
+  Test(Pkcs11KeyPairGenerator(CKM_EC_KEY_PAIR_GEN, GetParam()),
+       CKM_ECDH1_DERIVE);
+}
+INSTANTIATE_TEST_CASE_P(Pkcs11EcdhNullKeyTest, Pkcs11EcdhNullKeyTest,
+                        ::testing::Values(SEC_OID_SECG_EC_SECP256R1,
+                                          SEC_OID_SECG_EC_SECP384R1,
+                                          SEC_OID_SECG_EC_SECP521R1,
+                                          SEC_OID_CURVE25519));
+
+}  // namespace nss_test
diff --git a/gtests/pk11_gtest/pk11_keygen.cc b/gtests/pk11_gtest/pk11_keygen.cc
new file mode 100644
index 000000000..d96cd38f6
--- /dev/null
+++ b/gtests/pk11_gtest/pk11_keygen.cc
@@ -0,0 +1,143 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#include "pk11_keygen.h"
+
+#include "pk11pub.h"
+#include "pk11pqg.h"
+#include "prerror.h"
+
+#include "gtest/gtest.h"
+
+namespace nss_test {
+
+class ParamHolder {
+ public:
+  virtual void* get() = 0;
+  virtual ~ParamHolder() = default;
+
+ protected:
+  ParamHolder() = default;
+};
+
+void Pkcs11KeyPairGenerator::GenerateKey(ScopedSECKEYPrivateKey* priv_key,
+                                         ScopedSECKEYPublicKey* pub_key) const {
+  // This function returns if an assertion fails, so don't leak anything.
+  priv_key->reset(nullptr);
+  pub_key->reset(nullptr);
+
+  auto params = MakeParams();
+  ASSERT_NE(nullptr, params);
+
+  ScopedPK11SlotInfo slot(PK11_GetInternalKeySlot());
+  ASSERT_TRUE(slot);
+
+  SECKEYPublicKey* pub_tmp;
+  ScopedSECKEYPrivateKey priv_tmp(PK11_GenerateKeyPair(
+      slot.get(), mech_, params->get(), &pub_tmp, PR_FALSE, PR_TRUE, nullptr));
+  ASSERT_NE(nullptr, priv_tmp) << "PK11_GenerateKeyPair failed: "
+                               << PORT_ErrorToName(PORT_GetError());
+  ASSERT_NE(nullptr, pub_tmp);
+
+  priv_key->swap(priv_tmp);
+  pub_key->reset(pub_tmp);
+}
+
+class RsaParamHolder : public ParamHolder {
+ public:
+  RsaParamHolder() : params_({1024, 0x010001}) {}
+  ~RsaParamHolder() = default;
+
+  void* get() override { return &params_; }
+
+ private:
+  PK11RSAGenParams params_;
+};
+
+class PqgParamHolder : public ParamHolder {
+ public:
+  PqgParamHolder(PQGParams* params) : params_(params) {}
+  ~PqgParamHolder() = default;
+
+  void* get() override { return params_.get(); }
+
+ private:
+  ScopedPQGParams params_;
+};
+
+class DhParamHolder : public PqgParamHolder {
+ public:
+  DhParamHolder(PQGParams* params)
+      : PqgParamHolder(params),
+        params_({nullptr, params->prime, params->base}) {}
+  ~DhParamHolder() = default;
+
+  void* get() override { return &params_; }
+
+ private:
+  SECKEYDHParams params_;
+};
+
+class EcParamHolder : public ParamHolder {
+ public:
+  EcParamHolder(SECOidTag curve_oid) {
+    SECOidData* curve = SECOID_FindOIDByTag(curve_oid);
+    EXPECT_NE(nullptr, curve);
+
+    size_t plen = curve->oid.len + 2;
+    extra_.reset(new uint8_t[plen]);
+    extra_[0] = SEC_ASN1_OBJECT_ID;
+    extra_[1] = static_cast<uint8_t>(curve->oid.len);
+    memcpy(&extra_[2], curve->oid.data, curve->oid.len);
+
+    ec_params_ = {siBuffer, extra_.get(), static_cast<unsigned int>(plen)};
+  }
+  ~EcParamHolder() = default;
+
+  void* get() override { return &ec_params_; }
+
+ private:
+  SECKEYECParams ec_params_;
+  std::unique_ptr<uint8_t[]> extra_;
+};
+
+std::unique_ptr<ParamHolder> Pkcs11KeyPairGenerator::MakeParams() const {
+  switch (mech_) {
+    case CKM_RSA_PKCS_KEY_PAIR_GEN:
+      std::cerr << "Generate RSA pair" << std::endl;
+      return std::unique_ptr<ParamHolder>(new RsaParamHolder());
+
+    case CKM_DSA_KEY_PAIR_GEN:
+    case CKM_DH_PKCS_KEY_PAIR_GEN: {
+      PQGParams* pqg_params = nullptr;
+      PQGVerify* pqg_verify = nullptr;
+      const unsigned int key_size = 1024;
+      SECStatus rv = PK11_PQG_ParamGenV2(key_size, 0, key_size / 16,
+                                         &pqg_params, &pqg_verify);
+      if (rv != SECSuccess) {
+        ADD_FAILURE() << "PK11_PQG_ParamGenV2 failed";
+        return nullptr;
+      }
+      EXPECT_NE(nullptr, pqg_verify);
+      EXPECT_NE(nullptr, pqg_params);
+      PK11_PQG_DestroyVerify(pqg_verify);
+      if (mech_ == CKM_DSA_KEY_PAIR_GEN) {
+        std::cerr << "Generate DSA pair" << std::endl;
+        return std::unique_ptr<ParamHolder>(new PqgParamHolder(pqg_params));
+      }
+      std::cerr << "Generate DH pair" << std::endl;
+      return std::unique_ptr<ParamHolder>(new DhParamHolder(pqg_params));
+    }
+
+    case CKM_EC_KEY_PAIR_GEN:
+      std::cerr << "Generate EC pair on " << curve_ << std::endl;
+      return std::unique_ptr<ParamHolder>(new EcParamHolder(curve_));
+
+    default:
+      ADD_FAILURE() << "unknown OID " << mech_;
+  }
+  return nullptr;
+}
+
+}  // namespace nss_test
diff --git a/gtests/pk11_gtest/pk11_keygen.h b/gtests/pk11_gtest/pk11_keygen.h
new file mode 100644
index 000000000..05ff97210
--- /dev/null
+++ b/gtests/pk11_gtest/pk11_keygen.h
@@ -0,0 +1,34 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#include "nss.h"
+#include "secoid.h"
+
+#include "nss_scoped_ptrs.h"
+
+namespace nss_test {
+
+class ParamHolder;
+
+class Pkcs11KeyPairGenerator {
+ public:
+  Pkcs11KeyPairGenerator(CK_MECHANISM_TYPE mech, SECOidTag curve_oid)
+      : mech_(mech), curve_(curve_oid) {}
+  Pkcs11KeyPairGenerator(CK_MECHANISM_TYPE mech)
+      : Pkcs11KeyPairGenerator(mech, SEC_OID_UNKNOWN) {}
+
+  CK_MECHANISM_TYPE mechanism() const { return mech_; }
+  SECOidTag curve() const { return curve_; }
+
+  void GenerateKey(ScopedSECKEYPrivateKey* priv_key,
+                   ScopedSECKEYPublicKey* pub_key) const;
+
+ private:
+  std::unique_ptr<ParamHolder> MakeParams() const;
+
+  CK_MECHANISM_TYPE mech_;
+  SECOidTag curve_;
+};
+
+}  // namespace nss_test