Remove the password parameter for ECPrivateKey::ExportEncryptedPrivat…

…eKey.

Even with a password, the encryption scheme used here is really not what
we'd want people to use. This does two things:

1. Cut down on the number of ways to use ExportEncryptedPrivateKey and
   makes it less likely someone will mistakenly use it for security
   purposes.

2. When we ported to BoringSSL, we added "raw" versions of
   PKCS8_{encrypt,decrypt} to account for confusion about two ways to
   encode the empty password. But PKCS8_{encrypt,decrypt} already
   handled this by treating NULL and "" differently. Limiting to just
   the empty password lets us trim BoringSSL's API surface in
   preparation for decoupling it from crypto/asn1.

BUG=603319

Review-Url: https://codereview.chromium.org/2608453002
Cr-Commit-Position: refs/heads/master@{#441365}

components/gcm_driver/crypto/p256_key_util.cc

@@ -51,8 +51,7 @@ bool CreateP256KeyPair(std::string* out_private_key,
   // Export the encrypted private key with an empty password. This is not done
   // to provide any security, but rather to achieve a consistent private key
   // storage between the BoringSSL and NSS implementations.
-  if (!key_pair->ExportEncryptedPrivateKey(
-          "" /* password */, 1 /* iteration */, &private_key)) {
+  if (!key_pair->ExportEncryptedPrivateKey(&private_key)) {
     DLOG(ERROR) << "Unable to export the private key.";
     return false;
   }
@@ -100,7 +99,6 @@ bool ComputeSharedP256Secret(const base::StringPiece& private_key,
 
   std::unique_ptr<crypto::ECPrivateKey> local_key_pair(
       crypto::ECPrivateKey::CreateFromEncryptedPrivateKeyInfo(
-          "" /* no password */,
           std::vector<uint8_t>(private_key.data(),
                                private_key.data() + private_key.size()),
           std::vector<uint8_t>(

crypto/ec_private_key.cc

@@ -95,7 +95,6 @@ std::unique_ptr<ECPrivateKey> ECPrivateKey::CreateFromPrivateKeyInfo(
 
 // static
 std::unique_ptr<ECPrivateKey> ECPrivateKey::CreateFromEncryptedPrivateKeyInfo(
-    const std::string& password,
     const std::vector<uint8_t>& encrypted_private_key_info,
     const std::vector<uint8_t>& subject_public_key_info) {
   // NOTE: The |subject_public_key_info| can be ignored here, it is only
@@ -113,21 +112,15 @@ std::unique_ptr<ECPrivateKey> ECPrivateKey::CreateFromEncryptedPrivateKeyInfo(
   if (!p8_encrypted || ptr != data + encrypted_private_key_info.size())
     return nullptr;
 
-  bssl::UniquePtr<PKCS8_PRIV_KEY_INFO> p8_decrypted;
-  if (password.empty()) {
-    // Hack for reading keys generated by an older version of the OpenSSL
-    // code. OpenSSL used to use "\0\0" rather than the empty string because it
-    // would treat the password as an ASCII string to be converted to UCS-2
-    // while NSS used a byte string.
-    p8_decrypted.reset(PKCS8_decrypt_pbe(
-        p8_encrypted.get(), reinterpret_cast<const uint8_t*>("\0\0"), 2));
-  }
-  if (!p8_decrypted) {
-    p8_decrypted.reset(PKCS8_decrypt_pbe(
-        p8_encrypted.get(),
-        reinterpret_cast<const uint8_t*>(password.data()),
-        password.size()));
-  }
+  // Hack for reading keys generated by an older version of the OpenSSL code.
+  // Some implementations encode the empty password as "\0\0" (passwords are
+  // normally encoded in big-endian UCS-2 with a NUL terminator) and some
+  // encode as the empty string. PKCS8_decrypt distinguishes the two by whether
+  // the password is nullptr.
+  bssl::UniquePtr<PKCS8_PRIV_KEY_INFO> p8_decrypted(
+      PKCS8_decrypt(p8_encrypted.get(), "", 0));
+  if (!p8_decrypted)
+    p8_decrypted.reset(PKCS8_decrypt(p8_encrypted.get(), nullptr, 0));
 
   if (!p8_decrypted)
     return nullptr;
@@ -166,8 +159,6 @@ bool ECPrivateKey::ExportPrivateKey(std::vector<uint8_t>* output) const {
 }
 
 bool ECPrivateKey::ExportEncryptedPrivateKey(
-    const std::string& password,
-    int iterations,
     std::vector<uint8_t>* output) const {
   OpenSSLErrStackTracer err_tracer(FROM_HERE);
   // Convert into a PKCS#8 object.
@@ -180,9 +171,8 @@ bool ECPrivateKey::ExportEncryptedPrivateKey(
   // so use NID_pbe_WithSHA1And3_Key_TripleDES_CBC which should be the OpenSSL
   // equivalent.
   bssl::UniquePtr<X509_SIG> encrypted(
-      PKCS8_encrypt_pbe(NID_pbe_WithSHA1And3_Key_TripleDES_CBC, nullptr,
-                        reinterpret_cast<const uint8_t*>(password.data()),
-                        password.size(), nullptr, 0, iterations, pkcs8.get()));
+      PKCS8_encrypt(NID_pbe_WithSHA1And3_Key_TripleDES_CBC, nullptr, nullptr, 0,
+                    nullptr, 0, 1, pkcs8.get()));
   if (!encrypted)
     return false;
 

crypto/ec_private_key.h

@@ -41,13 +41,12 @@ class CRYPTO_EXPORT ECPrivateKey {
 
   // Creates a new instance by importing an existing key pair.
   // The key pair is given as an ASN.1-encoded PKCS #8 EncryptedPrivateKeyInfo
-  // block and an X.509 SubjectPublicKeyInfo block.
+  // block with empty password and an X.509 SubjectPublicKeyInfo block.
   // Returns nullptr if initialization fails.
   //
   // This function is deprecated. Use CreateFromPrivateKeyInfo for new code.
   // See https://crbug.com/603319.
   static std::unique_ptr<ECPrivateKey> CreateFromEncryptedPrivateKeyInfo(
-      const std::string& password,
       const std::vector<uint8_t>& encrypted_private_key_info,
       const std::vector<uint8_t>& subject_public_key_info);
 
@@ -60,16 +59,12 @@ class CRYPTO_EXPORT ECPrivateKey {
   bool ExportPrivateKey(std::vector<uint8_t>* output) const;
 
   // Exports the private key as an ASN.1-encoded PKCS #8 EncryptedPrivateKeyInfo
-  // block and the public key as an X.509 SubjectPublicKeyInfo block.
-  // The |password| and |iterations| are used as inputs to the key derivation
-  // function for generating the encryption key.  PKCS #5 recommends a minimum
-  // of 1000 iterations, on modern systems a larger value may be preferrable.
+  // block wth empty password. This was historically used as a workaround for
+  // NSS API deficiencies and does not provide security.
   //
   // This function is deprecated. Use ExportPrivateKey for new code. See
   // https://crbug.com/603319.
-  bool ExportEncryptedPrivateKey(const std::string& password,
-                                 int iterations,
-                                 std::vector<uint8_t>* output) const;
+  bool ExportEncryptedPrivateKey(std::vector<uint8_t>* output) const;
 
   // Exports the public key to an X.509 SubjectPublicKeyInfo block.
   bool ExportPublicKey(std::vector<uint8_t>* output) const;

crypto/ec_private_key_unittest.cc

@@ -41,9 +41,6 @@ void ExpectKeysEqual(const crypto::ECPrivateKey* keypair1,
 // should get back the same exact public key, and the private key should have
 // the same value and elliptic curve params.
 TEST(ECPrivateKeyUnitTest, InitRandomTest) {
-  static const char kPassword1[] = "";
-  static const char kPassword2[] = "test";
-
   std::unique_ptr<crypto::ECPrivateKey> keypair(crypto::ECPrivateKey::Create());
   ASSERT_TRUE(keypair);
 
@@ -58,19 +55,10 @@ TEST(ECPrivateKeyUnitTest, InitRandomTest) {
   // Re-import as an EncryptedPrivateKeyInfo with kPassword1.
   std::vector<uint8_t> encrypted_privkey;
   std::vector<uint8_t> pubkey;
-  EXPECT_TRUE(
-      keypair->ExportEncryptedPrivateKey(kPassword1, 1, &encrypted_privkey));
+  EXPECT_TRUE(keypair->ExportEncryptedPrivateKey(&encrypted_privkey));
   EXPECT_TRUE(keypair->ExportPublicKey(&pubkey));
   keypair_copy = crypto::ECPrivateKey::CreateFromEncryptedPrivateKeyInfo(
-      kPassword1, encrypted_privkey, pubkey);
-  ASSERT_TRUE(keypair_copy);
-  ExpectKeysEqual(keypair.get(), keypair_copy.get());
-
-  // Re-import as an EncryptedPrivateKeyInfo with kPassword2.
-  EXPECT_TRUE(
-      keypair->ExportEncryptedPrivateKey(kPassword2, 1, &encrypted_privkey));
-  keypair_copy = crypto::ECPrivateKey::CreateFromEncryptedPrivateKeyInfo(
-      kPassword2, encrypted_privkey, pubkey);
+      encrypted_privkey, pubkey);
   ASSERT_TRUE(keypair_copy);
   ExpectKeysEqual(keypair.get(), keypair_copy.get());
 }
@@ -200,26 +188,6 @@ TEST(ECPrivateKeyUnitTest, RSAPrivateKeyInfo) {
   EXPECT_FALSE(key);
 }
 
-TEST(ECPrivateKeyUnitTest, BadPasswordTest) {
-  const std::string password1;
-  const std::string password2 = "test";
-
-  std::unique_ptr<crypto::ECPrivateKey> keypair1(
-      crypto::ECPrivateKey::Create());
-  ASSERT_TRUE(keypair1);
-
-  std::vector<uint8_t> privkey1;
-  std::vector<uint8_t> pubkey1;
-  ASSERT_TRUE(keypair1->ExportEncryptedPrivateKey(
-      password1, 1, &privkey1));
-  ASSERT_TRUE(keypair1->ExportPublicKey(&pubkey1));
-
-  std::unique_ptr<crypto::ECPrivateKey> keypair2(
-      crypto::ECPrivateKey::CreateFromEncryptedPrivateKeyInfo(
-          password2, privkey1, pubkey1));
-  ASSERT_FALSE(keypair2);
-}
-
 TEST(ECPrivateKeyUnitTest, LoadNSSKeyTest) {
   static const uint8_t kNSSKey[] = {
       0x30, 0x81, 0xb8, 0x30, 0x23, 0x06, 0x0a, 0x2a, 0x86, 0x48, 0x86, 0xf7,
@@ -252,7 +220,7 @@ TEST(ECPrivateKeyUnitTest, LoadNSSKeyTest) {
 
   std::unique_ptr<crypto::ECPrivateKey> keypair_nss(
       crypto::ECPrivateKey::CreateFromEncryptedPrivateKeyInfo(
-          "", std::vector<uint8_t>(std::begin(kNSSKey), std::end(kNSSKey)),
+          std::vector<uint8_t>(std::begin(kNSSKey), std::end(kNSSKey)),
           std::vector<uint8_t>(std::begin(kNSSPublicKey),
                                std::end(kNSSPublicKey))));
 
@@ -298,7 +266,6 @@ TEST(ECPrivateKeyUnitTest, LoadOpenSSLKeyTest) {
 
   std::unique_ptr<crypto::ECPrivateKey> keypair_openssl(
       crypto::ECPrivateKey::CreateFromEncryptedPrivateKeyInfo(
-          "",
           std::vector<uint8_t>(std::begin(kOpenSSLKey), std::end(kOpenSSLKey)),
           std::vector<uint8_t>(std::begin(kOpenSSLPublicKey),
                                std::end(kOpenSSLPublicKey))));
@@ -393,7 +360,6 @@ TEST(ECPrivateKeyUnitTest, LoadOldOpenSSLKeyTest) {
 
   std::unique_ptr<crypto::ECPrivateKey> keypair_openssl(
       crypto::ECPrivateKey::CreateFromEncryptedPrivateKeyInfo(
-          "",
           std::vector<uint8_t>(std::begin(kOpenSSLKey), std::end(kOpenSSLKey)),
           std::vector<uint8_t>(std::begin(kOpenSSLPublicKey),
                                std::end(kOpenSSLPublicKey))));

crypto/ec_signature_creator_unittest.cc

@@ -21,23 +21,19 @@ TEST(ECSignatureCreatorTest, BasicTest) {
   // Do a verify round trip.
   std::unique_ptr<crypto::ECPrivateKey> key_original(
       crypto::ECPrivateKey::Create());
-  ASSERT_TRUE(key_original.get());
+  ASSERT_TRUE(key_original);
 
   std::vector<uint8_t> key_info;
-  ASSERT_TRUE(
-      key_original->ExportEncryptedPrivateKey(std::string(), 1000, &key_info));
-  std::vector<uint8_t> pubkey_info;
-  ASSERT_TRUE(key_original->ExportPublicKey(&pubkey_info));
+  ASSERT_TRUE(key_original->ExportPrivateKey(&key_info));
 
   std::unique_ptr<crypto::ECPrivateKey> key(
-      crypto::ECPrivateKey::CreateFromEncryptedPrivateKeyInfo(
-          std::string(), key_info, pubkey_info));
-  ASSERT_TRUE(key.get());
-  ASSERT_TRUE(key->key() != NULL);
+      crypto::ECPrivateKey::CreateFromPrivateKeyInfo(key_info));
+  ASSERT_TRUE(key);
+  ASSERT_TRUE(key->key());
 
   std::unique_ptr<crypto::ECSignatureCreator> signer(
       crypto::ECSignatureCreator::Create(key.get()));
-  ASSERT_TRUE(signer.get());
+  ASSERT_TRUE(signer);
 
   std::string data("Hello, World!");
   std::vector<uint8_t> signature;

net/extras/sqlite/sqlite_channel_id_store.cc

@@ -227,8 +227,7 @@ void SQLiteChannelIDStore::Backend::LoadInBackground(
     smt.ColumnBlobAsVector(2, &public_key_from_db);
     std::unique_ptr<crypto::ECPrivateKey> key(
         crypto::ECPrivateKey::CreateFromEncryptedPrivateKeyInfo(
-            ChannelIDService::kEPKIPassword, private_key_from_db,
-            public_key_from_db));
+            private_key_from_db, public_key_from_db));
     if (!key)
       continue;
     std::unique_ptr<DefaultChannelIDStore::ChannelID> channel_id(
@@ -498,8 +497,7 @@ void SQLiteChannelIDStore::Backend::Commit() {
         add_statement.Reset(true);
         add_statement.BindString(0, po->channel_id().server_identifier());
         std::vector<uint8_t> private_key, public_key;
-        if (!po->channel_id().key()->ExportEncryptedPrivateKey(
-                ChannelIDService::kEPKIPassword, 1, &private_key))
+        if (!po->channel_id().key()->ExportEncryptedPrivateKey(&private_key))
           continue;
         if (!po->channel_id().key()->ExportPublicKey(&public_key))
           continue;

net/extras/sqlite/sqlite_channel_id_store_unittest.cc

@@ -65,8 +65,8 @@ class SQLiteChannelIDStoreTest : public testing::Test {
     ASSERT_TRUE(asn1::ExtractSPKIFromDERCert(*cert_data, &spki));
     std::vector<uint8_t> public_key(spki.size());
     memcpy(public_key.data(), spki.data(), spki.size());
-    *key = crypto::ECPrivateKey::CreateFromEncryptedPrivateKeyInfo(
-        ChannelIDService::kEPKIPassword, private_key, public_key);
+    *key = crypto::ECPrivateKey::CreateFromEncryptedPrivateKeyInfo(private_key,
+                                                                   public_key);
   }
 
   static base::Time GetTestCertExpirationTime() {

net/ssl/channel_id_service.cc

@@ -201,9 +201,6 @@ class ChannelIDServiceJob {
   bool create_if_missing_;
 };
 
-// static
-const char ChannelIDService::kEPKIPassword[] = "";
-
 ChannelIDService::Request::Request() : service_(NULL) {
 }
 

net/ssl/channel_id_service.h

@@ -69,11 +69,6 @@ class NET_EXPORT ChannelIDService
     ChannelIDServiceJob* job_;
   };
 
-  // Password used on EncryptedPrivateKeyInfo data stored in EC private_key
-  // values.  (This is not used to provide any security, but to workaround NSS
-  // being unable to import unencrypted PrivateKeyInfo for EC keys.)
-  static const char kEPKIPassword[];
-
   // This object owns |channel_id_store|.  |task_runner| will
   // be used to post channel ID generation worker tasks.  The tasks are
   // safe for use with WorkerPool and SequencedWorkerPool::CONTINUE_ON_SHUTDOWN.

net/ssl/ssl_platform_key_nss_unittest.cc

@@ -74,7 +74,7 @@ TEST_P(SSLPlatformKeyNSSTest, KeyMatches) {
         crypto::ECPrivateKey::CreateFromPrivateKeyInfo(pkcs8_vector);
     ASSERT_TRUE(ec_private_key);
     std::vector<uint8_t> encrypted;
-    ASSERT_TRUE(ec_private_key->ExportEncryptedPrivateKey("", 1, &encrypted));
+    ASSERT_TRUE(ec_private_key->ExportEncryptedPrivateKey(&encrypted));
 
     SECItem encrypted_item = {siBuffer, encrypted.data(),
                               static_cast<unsigned>(encrypted.size())};