From 1c02c94c34e1c57154914d51c44e818aa290f7a0 Mon Sep 17 00:00:00 2001 From: davidben Date: Wed, 4 Jan 2017 05:54:32 -0800 Subject: [PATCH] Remove the password parameter for ECPrivateKey::ExportEncryptedPrivateKey. 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 | 4 +- crypto/ec_private_key.cc | 32 +++++---------- crypto/ec_private_key.h | 13 ++---- crypto/ec_private_key_unittest.cc | 40 ++----------------- crypto/ec_signature_creator_unittest.cc | 16 +++----- net/extras/sqlite/sqlite_channel_id_store.cc | 6 +-- .../sqlite_channel_id_store_unittest.cc | 4 +- net/ssl/channel_id_service.cc | 3 -- net/ssl/channel_id_service.h | 5 --- net/ssl/ssl_platform_key_nss_unittest.cc | 2 +- 10 files changed, 30 insertions(+), 95 deletions(-) diff --git a/components/gcm_driver/crypto/p256_key_util.cc b/components/gcm_driver/crypto/p256_key_util.cc index daf7e0ff720458..18f927d27f56de 100644 --- a/components/gcm_driver/crypto/p256_key_util.cc +++ b/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 local_key_pair( crypto::ECPrivateKey::CreateFromEncryptedPrivateKeyInfo( - "" /* no password */, std::vector(private_key.data(), private_key.data() + private_key.size()), std::vector( diff --git a/crypto/ec_private_key.cc b/crypto/ec_private_key.cc index 0a2344c2a6661c..9914c5dde866e9 100644 --- a/crypto/ec_private_key.cc +++ b/crypto/ec_private_key.cc @@ -95,7 +95,6 @@ std::unique_ptr ECPrivateKey::CreateFromPrivateKeyInfo( // static std::unique_ptr ECPrivateKey::CreateFromEncryptedPrivateKeyInfo( - const std::string& password, const std::vector& encrypted_private_key_info, const std::vector& 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::CreateFromEncryptedPrivateKeyInfo( if (!p8_encrypted || ptr != data + encrypted_private_key_info.size()) return nullptr; - bssl::UniquePtr 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("\0\0"), 2)); - } - if (!p8_decrypted) { - p8_decrypted.reset(PKCS8_decrypt_pbe( - p8_encrypted.get(), - reinterpret_cast(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 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* output) const { } bool ECPrivateKey::ExportEncryptedPrivateKey( - const std::string& password, - int iterations, std::vector* 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 encrypted( - PKCS8_encrypt_pbe(NID_pbe_WithSHA1And3_Key_TripleDES_CBC, nullptr, - reinterpret_cast(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; diff --git a/crypto/ec_private_key.h b/crypto/ec_private_key.h index 8e5fe5e99bb89e..432019be5d7eb9 100644 --- a/crypto/ec_private_key.h +++ b/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 CreateFromEncryptedPrivateKeyInfo( - const std::string& password, const std::vector& encrypted_private_key_info, const std::vector& subject_public_key_info); @@ -60,16 +59,12 @@ class CRYPTO_EXPORT ECPrivateKey { bool ExportPrivateKey(std::vector* 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* output) const; + bool ExportEncryptedPrivateKey(std::vector* output) const; // Exports the public key to an X.509 SubjectPublicKeyInfo block. bool ExportPublicKey(std::vector* output) const; diff --git a/crypto/ec_private_key_unittest.cc b/crypto/ec_private_key_unittest.cc index 386844cd5a65b2..534564703df040 100644 --- a/crypto/ec_private_key_unittest.cc +++ b/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 keypair(crypto::ECPrivateKey::Create()); ASSERT_TRUE(keypair); @@ -58,19 +55,10 @@ TEST(ECPrivateKeyUnitTest, InitRandomTest) { // Re-import as an EncryptedPrivateKeyInfo with kPassword1. std::vector encrypted_privkey; std::vector 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 keypair1( - crypto::ECPrivateKey::Create()); - ASSERT_TRUE(keypair1); - - std::vector privkey1; - std::vector pubkey1; - ASSERT_TRUE(keypair1->ExportEncryptedPrivateKey( - password1, 1, &privkey1)); - ASSERT_TRUE(keypair1->ExportPublicKey(&pubkey1)); - - std::unique_ptr 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 keypair_nss( crypto::ECPrivateKey::CreateFromEncryptedPrivateKeyInfo( - "", std::vector(std::begin(kNSSKey), std::end(kNSSKey)), + std::vector(std::begin(kNSSKey), std::end(kNSSKey)), std::vector(std::begin(kNSSPublicKey), std::end(kNSSPublicKey)))); @@ -298,7 +266,6 @@ TEST(ECPrivateKeyUnitTest, LoadOpenSSLKeyTest) { std::unique_ptr keypair_openssl( crypto::ECPrivateKey::CreateFromEncryptedPrivateKeyInfo( - "", std::vector(std::begin(kOpenSSLKey), std::end(kOpenSSLKey)), std::vector(std::begin(kOpenSSLPublicKey), std::end(kOpenSSLPublicKey)))); @@ -393,7 +360,6 @@ TEST(ECPrivateKeyUnitTest, LoadOldOpenSSLKeyTest) { std::unique_ptr keypair_openssl( crypto::ECPrivateKey::CreateFromEncryptedPrivateKeyInfo( - "", std::vector(std::begin(kOpenSSLKey), std::end(kOpenSSLKey)), std::vector(std::begin(kOpenSSLPublicKey), std::end(kOpenSSLPublicKey)))); diff --git a/crypto/ec_signature_creator_unittest.cc b/crypto/ec_signature_creator_unittest.cc index 018ba54a23cab1..1bfd89d0a764d1 100644 --- a/crypto/ec_signature_creator_unittest.cc +++ b/crypto/ec_signature_creator_unittest.cc @@ -21,23 +21,19 @@ TEST(ECSignatureCreatorTest, BasicTest) { // Do a verify round trip. std::unique_ptr key_original( crypto::ECPrivateKey::Create()); - ASSERT_TRUE(key_original.get()); + ASSERT_TRUE(key_original); std::vector key_info; - ASSERT_TRUE( - key_original->ExportEncryptedPrivateKey(std::string(), 1000, &key_info)); - std::vector pubkey_info; - ASSERT_TRUE(key_original->ExportPublicKey(&pubkey_info)); + ASSERT_TRUE(key_original->ExportPrivateKey(&key_info)); std::unique_ptr 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 signer( crypto::ECSignatureCreator::Create(key.get())); - ASSERT_TRUE(signer.get()); + ASSERT_TRUE(signer); std::string data("Hello, World!"); std::vector signature; diff --git a/net/extras/sqlite/sqlite_channel_id_store.cc b/net/extras/sqlite/sqlite_channel_id_store.cc index 238577fd18d9dd..ebcf7ec74ba251 100644 --- a/net/extras/sqlite/sqlite_channel_id_store.cc +++ b/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 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 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 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; diff --git a/net/extras/sqlite/sqlite_channel_id_store_unittest.cc b/net/extras/sqlite/sqlite_channel_id_store_unittest.cc index 8cb2bb12062ad7..7c9b223e3e1275 100644 --- a/net/extras/sqlite/sqlite_channel_id_store_unittest.cc +++ b/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 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() { diff --git a/net/ssl/channel_id_service.cc b/net/ssl/channel_id_service.cc index 53c3c636cd775f..74ab38169585c1 100644 --- a/net/ssl/channel_id_service.cc +++ b/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) { } diff --git a/net/ssl/channel_id_service.h b/net/ssl/channel_id_service.h index d9de87964e6d71..617a297e84e068 100644 --- a/net/ssl/channel_id_service.h +++ b/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. diff --git a/net/ssl/ssl_platform_key_nss_unittest.cc b/net/ssl/ssl_platform_key_nss_unittest.cc index 85799cae131deb..350c46273b7c5f 100644 --- a/net/ssl/ssl_platform_key_nss_unittest.cc +++ b/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 encrypted; - ASSERT_TRUE(ec_private_key->ExportEncryptedPrivateKey("", 1, &encrypted)); + ASSERT_TRUE(ec_private_key->ExportEncryptedPrivateKey(&encrypted)); SECItem encrypted_item = {siBuffer, encrypted.data(), static_cast(encrypted.size())};