diff --git a/src/crypto/pem/pem_info.cc b/src/crypto/pem/pem_info.cc index 27d4edc50..873c21ba1 100644 --- a/src/crypto/pem/pem_info.cc +++ b/src/crypto/pem/pem_info.cc @@ -168,7 +168,8 @@ STACK_OF(X509_INFO) *PEM_X509_INFO_read_bio(BIO *bp, STACK_OF(X509_INFO) *sk, key_type = EVP_PKEY_EC; } - // If a private key has a header, assume it is encrypted. + // If a private key has a header, assume it is encrypted. This function does + // not decrypt private keys. if (key_type != EVP_PKEY_NONE && strlen(header) > 10) { if (info->x_pkey != NULL) { if (!sk_X509_INFO_push(ret, info)) { @@ -179,7 +180,7 @@ STACK_OF(X509_INFO) *PEM_X509_INFO_read_bio(BIO *bp, STACK_OF(X509_INFO) *sk, goto err; } } - // Historically, raw entries pushed an empty key. + // Use an empty key as a placeholder. info->x_pkey = X509_PKEY_new(); if (info->x_pkey == NULL || !PEM_get_EVP_CIPHER_INFO(header, &info->enc_cipher)) { diff --git a/src/crypto/x509/x509_test.cc b/src/crypto/x509/x509_test.cc index ad1297af4..27facf032 100644 --- a/src/crypto/x509/x509_test.cc +++ b/src/crypto/x509/x509_test.cc @@ -2974,6 +2974,8 @@ TEST(X509Test, MismatchAlgorithms) { X509_R_SIGNATURE_ALGORITHM_MISMATCH)); } +// TODO(crbug.com/387737061): Test that this function can decrypt certificates +// and CRLs, even though it leaves encrypted private keys alone. TEST(X509Test, PEMX509Info) { std::string cert = kRootCAPEM; auto cert_obj = CertFromPEM(kRootCAPEM); @@ -2987,6 +2989,9 @@ TEST(X509Test, PEMX509Info) { auto crl_obj = CRLFromPEM(kBasicCRL); ASSERT_TRUE(crl_obj); + bssl::UniquePtr placeholder_key(EVP_PKEY_new()); + ASSERT_TRUE(placeholder_key); + std::string unknown = "-----BEGIN UNKNOWN-----\n" "AAAA\n" @@ -2997,6 +3002,16 @@ TEST(X509Test, PEMX509Info) { "AAAA\n" "-----END CERTIFICATE-----\n"; + std::string encrypted_key = R"(-----BEGIN EC PRIVATE KEY----- +Proc-Type: 4,ENCRYPTED +DEK-Info: AES-128-CBC,B3B2988AECAE6EAB0D043105994C1123 + +RK7DUIGDHWTFh2rpTX+dR88hUyC1PyDlIULiNCkuWFwHrJbc1gM6hMVOKmU196XC +iITrIKmilFm9CPD6Tpfk/NhI/QPxyJlk1geIkxpvUZ2FCeMuYI1To14oYOUKv14q +wr6JtaX2G+pOmwcSPymZC4u2TncAP7KHgS8UGcMw8CE= +-----END EC PRIVATE KEY----- +)"; + // Each X509_INFO contains at most one certificate, CRL, etc. The format // creates a new X509_INFO when a repeated type is seen. std::string pem = @@ -3012,7 +3027,12 @@ TEST(X509Test, PEMX509Info) { // Doubled keys also start new entries. rsa + rsa + rsa + rsa + crl + // As do CRLs. - crl + crl; + crl + crl + + // Encrypted private keys are not decrypted (decryption failures would be + // fatal) and just returned as placeholder. + crl + cert + encrypted_key + + // Placeholder keys are still keys, so a new key starts a new entry. + rsa; const struct ExpectedInfo { const X509 *cert; @@ -3032,6 +3052,8 @@ TEST(X509Test, PEMX509Info) { {nullptr, rsa_obj.get(), crl_obj.get()}, {nullptr, nullptr, crl_obj.get()}, {nullptr, nullptr, crl_obj.get()}, + {cert_obj.get(), placeholder_key.get(), crl_obj.get()}, + {nullptr, rsa_obj.get(), nullptr}, }; auto check_info = [](const ExpectedInfo *expected, const X509_INFO *info) { @@ -3047,8 +3069,14 @@ TEST(X509Test, PEMX509Info) { } if (expected->key != nullptr) { ASSERT_NE(nullptr, info->x_pkey); - // EVP_PKEY_cmp returns one if the keys are equal. - EXPECT_EQ(1, EVP_PKEY_cmp(expected->key, info->x_pkey->dec_pkey)); + if (EVP_PKEY_id(expected->key) == EVP_PKEY_NONE) { + // Expect a placeholder key. + EXPECT_FALSE(info->x_pkey->dec_pkey); + } else { + // EVP_PKEY_cmp returns one if the keys are equal. + ASSERT_TRUE(info->x_pkey->dec_pkey); + EXPECT_EQ(1, EVP_PKEY_cmp(expected->key, info->x_pkey->dec_pkey)); + } } else { EXPECT_EQ(nullptr, info->x_pkey); } diff --git a/src/include/openssl/pem.h b/src/include/openssl/pem.h index 28bf3f286..3711d8a0e 100644 --- a/src/include/openssl/pem.h +++ b/src/include/openssl/pem.h @@ -307,6 +307,14 @@ OPENSSL_EXPORT int PEM_ASN1_write_bio(i2d_of_void *i2d, const char *name, // on success. In this case, the caller retains ownership of |sk| in both // success and failure. // +// This function will decrypt any encrypted certificates in |bp|, using |cb|, +// but it will not decrypt encrypted private keys. Encrypted private keys are +// instead represented as placeholder |X509_INFO| objects with an empty |x_pkey| +// field. This allows this function to be used with inputs with unencrypted +// certificates, but encrypted passwords, without knowing the password. However, +// it also means that this function cannot be used to decrypt the private key +// when the password is known. +// // WARNING: If the input contains "TRUSTED CERTIFICATE" PEM blocks, this // function parses auxiliary properties as in |d2i_X509_AUX|. Passing untrusted // input to this function allows an attacker to influence those properties. See diff --git a/src/rust/bssl-crypto/src/lib.rs b/src/rust/bssl-crypto/src/lib.rs index d1abb93da..db03b5599 100644 --- a/src/rust/bssl-crypto/src/lib.rs +++ b/src/rust/bssl-crypto/src/lib.rs @@ -476,8 +476,7 @@ fn cbb_to_vec(len: usize, func: F) -> Vec { bssl_sys::CBB_init_fixed(cbb, boxed.as_mut_ptr() as *mut u8, len) == 1 }) } - // `CBB_init` only fails if out of memory, which isn't something that this - // crate handles. + // `CBB_init_fixed` never fails and does not allocate. .unwrap(); func(&mut cbb);