Bug 1693206 - Implement PKCS8 export of ECDSA keys

patch by Christoph Walcher
r=rrelyea, bbeurdouche

8 files changed, 179 insertions, 54 deletions

diff --git a/gtests/pk11_gtest/pk11_aeskeywrappad_unittest.cc b/gtests/pk11_gtest/pk11_aeskeywrappad_unittest.cc
index ef78f7b0e..9ab172bed 100644
--- a/gtests/pk11_gtest/pk11_aeskeywrappad_unittest.cc
+++ b/gtests/pk11_gtest/pk11_aeskeywrappad_unittest.cc
@@ -420,4 +420,4 @@ TEST_F(Pkcs11AESKeyWrapPadTest, WrapUnwrapRandom_ShortValidPadding) {
   ASSERT_EQ(0, memcmp(buf, unwrapped_key.data(), out_len));
 }
 
-} /* nss_test */
+}  // namespace nss_test
diff --git a/gtests/pk11_gtest/pk11_ecdsa_unittest.cc b/gtests/pk11_gtest/pk11_ecdsa_unittest.cc
index 9f4bf6a2b..2d0cf6823 100644
--- a/gtests/pk11_gtest/pk11_ecdsa_unittest.cc
+++ b/gtests/pk11_gtest/pk11_ecdsa_unittest.cc
@@ -14,6 +14,7 @@
 
 #include "pk11_ecdsa_vectors.h"
 #include "pk11_signature_test.h"
+#include "pk11_keygen.h"
 #include "testvectors/p256ecdsa-sha256-vectors.h"
 #include "testvectors/p384ecdsa-sha384-vectors.h"
 #include "testvectors/p521ecdsa-sha512-vectors.h"
@@ -63,6 +64,10 @@ TEST_P(Pkcs11EcdsaTest, SignAndVerify) {
   SignAndVerify(GetParam().sig_params_);
 }
 
+TEST_P(Pkcs11EcdsaTest, ImportExport) {
+  ImportExport(GetParam().sig_params_.pkcs8_);
+}
+
 static const Pkcs11EcdsaTestParams kEcdsaVectors[] = {
     {SEC_OID_SHA256,
      {DataBuffer(kP256Pkcs8, sizeof(kP256Pkcs8)),
@@ -243,4 +248,41 @@ INSTANTIATE_TEST_SUITE_P(WycheproofP521SignatureSha512Test,
                          Pkcs11EcdsaWycheproofTest,
                          ::testing::ValuesIn(kP521EcdsaSha512Vectors));
 
+class Pkcs11EcdsaRoundtripTest
+    : public Pkcs11EcdsaTestBase,
+      public ::testing::WithParamInterface<SECOidTag> {
+ public:
+  Pkcs11EcdsaRoundtripTest() : Pkcs11EcdsaTestBase(SEC_OID_SHA256) {}
+
+ protected:
+  void GenerateExportImportSignVerify(SECOidTag tag) {
+    Pkcs11KeyPairGenerator generator(CKM_EC_KEY_PAIR_GEN, tag);
+    ScopedSECKEYPrivateKey priv;
+    ScopedSECKEYPublicKey pub;
+    generator.GenerateKey(&priv, &pub, false);
+
+    DataBuffer exported;
+    ExportPrivateKey(&priv, exported);
+
+    if (tag != SEC_OID_CURVE25519) {
+      DataBuffer sig;
+      DataBuffer sig2;
+      DataBuffer data(kP256Data, sizeof(kP256Data));
+      ASSERT_TRUE(
+          ImportPrivateKeyAndSignHashedData(exported, data, &sig, &sig2));
+
+      Verify(pub, data, sig);
+    }
+  }
+};
+
+TEST_P(Pkcs11EcdsaRoundtripTest, GenerateExportImportSignVerify) {
+  GenerateExportImportSignVerify(GetParam());
+}
+INSTANTIATE_TEST_SUITE_P(Pkcs11EcdsaRoundtripTest, Pkcs11EcdsaRoundtripTest,
+                         ::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
index d96cd38f6..5b4de29de 100644
--- a/gtests/pk11_gtest/pk11_keygen.cc
+++ b/gtests/pk11_gtest/pk11_keygen.cc
@@ -22,7 +22,8 @@ class ParamHolder {
 };
 
 void Pkcs11KeyPairGenerator::GenerateKey(ScopedSECKEYPrivateKey* priv_key,
-                                         ScopedSECKEYPublicKey* pub_key) const {
+                                         ScopedSECKEYPublicKey* pub_key,
+                                         bool sensitive) const {
   // This function returns if an assertion fails, so don't leak anything.
   priv_key->reset(nullptr);
   pub_key->reset(nullptr);
@@ -34,8 +35,9 @@ void Pkcs11KeyPairGenerator::GenerateKey(ScopedSECKEYPrivateKey* priv_key,
   ASSERT_TRUE(slot);
 
   SECKEYPublicKey* pub_tmp;
-  ScopedSECKEYPrivateKey priv_tmp(PK11_GenerateKeyPair(
-      slot.get(), mech_, params->get(), &pub_tmp, PR_FALSE, PR_TRUE, nullptr));
+  ScopedSECKEYPrivateKey priv_tmp(
+      PK11_GenerateKeyPair(slot.get(), mech_, params->get(), &pub_tmp, PR_FALSE,
+                           sensitive ? PR_TRUE : PR_FALSE, nullptr));
   ASSERT_NE(nullptr, priv_tmp) << "PK11_GenerateKeyPair failed: "
                                << PORT_ErrorToName(PORT_GetError());
   ASSERT_NE(nullptr, pub_tmp);
diff --git a/gtests/pk11_gtest/pk11_keygen.h b/gtests/pk11_gtest/pk11_keygen.h
index 05ff97210..2c1ec5249 100644
--- a/gtests/pk11_gtest/pk11_keygen.h
+++ b/gtests/pk11_gtest/pk11_keygen.h
@@ -22,7 +22,7 @@ class Pkcs11KeyPairGenerator {
   SECOidTag curve() const { return curve_; }
 
   void GenerateKey(ScopedSECKEYPrivateKey* priv_key,
-                   ScopedSECKEYPublicKey* pub_key) const;
+                   ScopedSECKEYPublicKey* pub_key, bool sensitive = true) const;
 
  private:
   std::unique_ptr<ParamHolder> MakeParams() const;
diff --git a/gtests/pk11_gtest/pk11_signature_test.cc b/gtests/pk11_gtest/pk11_signature_test.cc
index 7ef51812c..c9700707f 100644
--- a/gtests/pk11_gtest/pk11_signature_test.cc
+++ b/gtests/pk11_gtest/pk11_signature_test.cc
@@ -134,11 +134,9 @@ bool Pk11SignatureTest::ImportPrivateKeyAndSignHashedData(
   return true;
 }
 
-void Pk11SignatureTest::Verify(const Pkcs11SignatureTestParams& params,
-                               const DataBuffer& sig, bool valid) {
-  ScopedSECKEYPublicKey pubKey(ImportPublicKey(params.spki_));
-  ASSERT_TRUE(pubKey);
-
+void Pk11SignatureTest::Verify(ScopedSECKEYPublicKey& pubKey,
+                               const DataBuffer& data, const DataBuffer& sig,
+                               bool valid) {
   SECStatus rv;
   DataBuffer hash;
 
@@ -150,7 +148,7 @@ void Pk11SignatureTest::Verify(const Pkcs11SignatureTestParams& params,
    * with the VFY_ interface, so just do the combined hash/Verify
    * in that case */
   if (!skip_raw_) {
-    ASSERT_TRUE(ComputeHash(params.data_, &hash));
+    ASSERT_TRUE(ComputeHash(data, &hash));
 
     // Verify.
     SECItem hashItem = {siBuffer, toUcharPtr(hash.data()),
@@ -168,7 +166,7 @@ void Pk11SignatureTest::Verify(const Pkcs11SignatureTestParams& params,
   ASSERT_NE((void*)context, (void*)NULL)
       << "CreateContext failed Error:" << PORT_ErrorToString(PORT_GetError())
       << "\n";
-  rv = PK11_DigestOp(context, params.data_.data(), params.data_.len());
+  rv = PK11_DigestOp(context, data.data(), data.len());
   /* expect success unconditionally here */
   EXPECT_EQ(rv, SECSuccess);
   unsigned int len;
diff --git a/gtests/pk11_gtest/pk11_signature_test.h b/gtests/pk11_gtest/pk11_signature_test.h
index 82ef03896..c4a8c52c3 100644
--- a/gtests/pk11_gtest/pk11_signature_test.h
+++ b/gtests/pk11_gtest/pk11_signature_test.h
@@ -34,6 +34,16 @@ class Pk11SignatureTest : public ::testing::Test {
   CK_MECHANISM_TYPE mechanism() const { return mechanism_; }
   void setSkipRaw(bool skip_raw) { skip_raw_ = true; }
 
+  bool ExportPrivateKey(ScopedSECKEYPrivateKey* key, DataBuffer& pkcs8) {
+    SECItem* pkcs8Item = PK11_ExportDERPrivateKeyInfo(key->get(), nullptr);
+    if (!pkcs8Item) {
+      return false;
+    }
+    pkcs8.Assign(pkcs8Item->data, pkcs8Item->len);
+    SECITEM_ZfreeItem(pkcs8Item, PR_TRUE);
+    return true;
+  }
+
   ScopedSECKEYPrivateKey ImportPrivateKey(const DataBuffer& pkcs8);
   ScopedSECKEYPublicKey ImportPublicKey(const DataBuffer& spki);
 
@@ -51,8 +61,23 @@ class Pk11SignatureTest : public ::testing::Test {
   bool ImportPrivateKeyAndSignHashedData(const DataBuffer& pkcs8,
                                          const DataBuffer& data,
                                          DataBuffer* sig, DataBuffer* sig2);
+
+  /* most primitive verify implemented in pk11_signature_test.cpp */
+  void Verify(ScopedSECKEYPublicKey& pubKey, const DataBuffer& data,
+              const DataBuffer& sig, bool valid);
+
+  /* quick helper functions that use the primitive verify */
+  void Verify(ScopedSECKEYPublicKey& pubKey, const DataBuffer& data,
+              const DataBuffer& sig) {
+    Verify(pubKey, data, sig, true);
+  }
+
   void Verify(const Pkcs11SignatureTestParams& params, const DataBuffer& sig,
-              bool valid);
+              bool valid) {
+    ScopedSECKEYPublicKey pubKey(ImportPublicKey(params.spki_));
+    ASSERT_TRUE(pubKey);
+    Verify(pubKey, params.data_, sig, valid);
+  }
 
   void Verify(const Pkcs11SignatureTestParams& params, bool valid) {
     Verify(params, params.signature_, valid);
@@ -71,6 +96,15 @@ class Pk11SignatureTest : public ::testing::Test {
     Verify(params, sig2, true);
   }
 
+  // Importing a private key in PKCS#8 format and reexporting it should
+  // result in the same binary representation.
+  void ImportExport(const DataBuffer& k) {
+    DataBuffer exported;
+    ScopedSECKEYPrivateKey key = ImportPrivateKey(k);
+    ExportPrivateKey(&key, exported);
+    EXPECT_EQ(k, exported);
+  }
+
  private:
   CK_MECHANISM_TYPE mechanism_;
   SECOidTag hash_oid_;
diff --git a/gtests/ssl_gtest/libssl_internals.c b/gtests/ssl_gtest/libssl_internals.c
index 963299659..c6b03c530 100644
--- a/gtests/ssl_gtest/libssl_internals.c
+++ b/gtests/ssl_gtest/libssl_internals.c
@@ -498,6 +498,4 @@ SECStatus SSLInt_SetRawEchConfigForRetry(PRFileDesc *fd, const uint8_t *buf,
   return SECSuccess;
 }
 
-PRBool SSLInt_IsIp(PRUint8 *s, unsigned int len) {
-  return tls13_IsIp(s, len);
-}
+PRBool SSLInt_IsIp(PRUint8 *s, unsigned int len) { return tls13_IsIp(s, len); }
diff --git a/lib/pk11wrap/pk11pk12.c b/lib/pk11wrap/pk11pk12.c
index c6d2f3c87..917b7f0f6 100644
--- a/lib/pk11wrap/pk11pk12.c
+++ b/lib/pk11wrap/pk11pk12.c
@@ -11,6 +11,8 @@
 #include "seccomon.h"
 #include "secmod.h"
 #include "secmodi.h"
+#include "secmodti.h"
+#include "secmodt.h"
 #include "pkcs11.h"
 #include "pk11func.h"
 #include "secitem.h"
@@ -702,61 +704,110 @@ PK11_ExportPrivKeyInfo(SECKEYPrivateKey *pk, void *wincx)
     const unsigned char pkiVersion = 0;
     /* RSAPrivateKey version (always zero) */
     const unsigned char rsaVersion = 0;
+    /* ECPrivateKey version (always one) */
+    const unsigned char ecVersion = 1;
     PLArenaPool *arena = NULL;
     SECKEYRawPrivateKey rawKey;
     SECKEYPrivateKeyInfo *pki;
     SECItem *encoded;
+    const SEC_ASN1Template *keyTemplate;
     SECStatus rv;
 
-    if (pk->keyType != rsaKey) {
-        PORT_SetError(PR_NOT_IMPLEMENTED_ERROR);
-        goto loser;
-    }
-
     arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
     if (!arena) {
         goto loser;
     }
     memset(&rawKey, 0, sizeof(rawKey));
     rawKey.keyType = pk->keyType;
-    rawKey.u.rsa.version.type = siUnsignedInteger;
-    rawKey.u.rsa.version.data = (unsigned char *)PORT_ArenaAlloc(arena, 1);
-    if (!rawKey.u.rsa.version.data) {
-        goto loser;
-    }
-    rawKey.u.rsa.version.data[0] = rsaVersion;
-    rawKey.u.rsa.version.len = 1;
-
-    /* Read the component attributes of the private key */
-    prepare_rsa_priv_key_export_for_asn1(&rawKey);
-    if (!ReadAttribute(pk, CKA_MODULUS, arena, &rawKey.u.rsa.modulus) ||
-        !ReadAttribute(pk, CKA_PUBLIC_EXPONENT, arena,
-                       &rawKey.u.rsa.publicExponent) ||
-        !ReadAttribute(pk, CKA_PRIVATE_EXPONENT, arena,
-                       &rawKey.u.rsa.privateExponent) ||
-        !ReadAttribute(pk, CKA_PRIME_1, arena, &rawKey.u.rsa.prime1) ||
-        !ReadAttribute(pk, CKA_PRIME_2, arena, &rawKey.u.rsa.prime2) ||
-        !ReadAttribute(pk, CKA_EXPONENT_1, arena,
-                       &rawKey.u.rsa.exponent1) ||
-        !ReadAttribute(pk, CKA_EXPONENT_2, arena,
-                       &rawKey.u.rsa.exponent2) ||
-        !ReadAttribute(pk, CKA_COEFFICIENT, arena,
-                       &rawKey.u.rsa.coefficient)) {
-        goto loser;
-    }
-
     pki = PORT_ArenaZNew(arena, SECKEYPrivateKeyInfo);
     if (!pki) {
         goto loser;
     }
-    encoded = SEC_ASN1EncodeItem(arena, &pki->privateKey, &rawKey,
-                                 SECKEY_RSAPrivateKeyExportTemplate);
-    if (!encoded) {
-        goto loser;
+
+    switch (pk->keyType) {
+        case rsaKey: {
+            rawKey.u.rsa.version.type = siUnsignedInteger;
+            rawKey.u.rsa.version.data = (unsigned char *)PORT_ArenaAlloc(arena, 1);
+            if (!rawKey.u.rsa.version.data) {
+                goto loser;
+            }
+
+            rawKey.u.rsa.version.data[0] = rsaVersion;
+            rawKey.u.rsa.version.len = 1;
+
+            /* Read the component attributes of the private key */
+            prepare_rsa_priv_key_export_for_asn1(&rawKey);
+            if (!ReadAttribute(pk, CKA_MODULUS, arena, &rawKey.u.rsa.modulus) ||
+                !ReadAttribute(pk, CKA_PUBLIC_EXPONENT, arena,
+                               &rawKey.u.rsa.publicExponent) ||
+                !ReadAttribute(pk, CKA_PRIVATE_EXPONENT, arena,
+                               &rawKey.u.rsa.privateExponent) ||
+                !ReadAttribute(pk, CKA_PRIME_1, arena, &rawKey.u.rsa.prime1) ||
+                !ReadAttribute(pk, CKA_PRIME_2, arena, &rawKey.u.rsa.prime2) ||
+                !ReadAttribute(pk, CKA_EXPONENT_1, arena,
+                               &rawKey.u.rsa.exponent1) ||
+                !ReadAttribute(pk, CKA_EXPONENT_2, arena,
+                               &rawKey.u.rsa.exponent2) ||
+                !ReadAttribute(pk, CKA_COEFFICIENT, arena,
+                               &rawKey.u.rsa.coefficient)) {
+                goto loser;
+            }
+
+            keyTemplate = SECKEY_RSAPrivateKeyExportTemplate;
+
+            rv = SECOID_SetAlgorithmID(arena, &pki->algorithm, SEC_OID_PKCS1_RSA_ENCRYPTION, NULL);
+            if (rv != SECSuccess) {
+                goto loser;
+            }
+
+        } break;
+        case ecKey: {
+            rawKey.u.ec.version.type = siUnsignedInteger;
+            rawKey.u.ec.version.data = (unsigned char *)PORT_ArenaAlloc(arena, 1);
+            if (!rawKey.u.ec.version.data) {
+                goto loser;
+            }
+            rawKey.u.ec.version.data[0] = ecVersion;
+            rawKey.u.ec.version.len = 1;
+
+            SECItem curveOID;
+            /* Read the component attributes of the private key */
+            prepare_ec_priv_key_export_for_asn1(&rawKey);
+            if (!ReadAttribute(pk, CKA_VALUE, arena,
+                               &rawKey.u.ec.privateValue) ||
+                !ReadAttribute(pk, CKA_EC_PARAMS, arena, &curveOID)) {
+                goto loser;
+            }
+            if (!ReadAttribute(pk, CKA_EC_POINT, arena,
+                               &rawKey.u.ec.publicValue)) {
+                SECKEYPublicKey *pubk = SECKEY_ConvertToPublicKey(pk);
+                if (pubk == NULL)
+                    goto loser;
+                rv = SECITEM_CopyItem(arena, &rawKey.u.ec.publicValue, &pubk->u.ec.publicValue);
+                SECKEY_DestroyPublicKey(pubk);
+                if (rv != SECSuccess) {
+                    goto loser;
+                }
+            }
+
+            keyTemplate = SECKEY_ECPrivateKeyExportTemplate;
+            /* Convert length in bytes to length in bits. */
+            rawKey.u.ec.publicValue.len <<= 3;
+
+            rv = SECOID_SetAlgorithmID(arena, &pki->algorithm, SEC_OID_ANSIX962_EC_PUBLIC_KEY, &curveOID);
+            if (rv != SECSuccess) {
+                goto loser;
+            }
+
+        } break;
+        default: {
+            PORT_SetError(PR_NOT_IMPLEMENTED_ERROR);
+            goto loser;
+        }
     }
-    rv = SECOID_SetAlgorithmID(arena, &pki->algorithm,
-                               SEC_OID_PKCS1_RSA_ENCRYPTION, NULL);
-    if (rv != SECSuccess) {
+
+    encoded = SEC_ASN1EncodeItem(arena, &pki->privateKey, &rawKey, keyTemplate);
+    if (!encoded) {
         goto loser;
     }
     pki->version.type = siUnsignedInteger;