From 66120783125830884db6730ee22715e85f73a0c9 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 5 Jan 2025 11:59:58 -0500 Subject: [PATCH 1/2] Document and test PEM_X509_INFO_read_bio's odd decryption behavior PEM_X509_INFO_read_bio is weird. It decrypts certificates and CRLs, but not private keys. We had some comments just saying we were trying to preserve historical (untested) behavior, but I think I've figured out why. It's so you can inspect a bundle of certs + encrypted keys without knowing the password. Attempting but failing to decrypt is fatal. On the flip side, this means that you cannot use this to decrypt the private key even if you wanted to! This was probably a mistake in SSLeay, but probably not worth fixing since this function's grouping behavior doesn't handle certificate chains right anyway. But we should at least document and test the intended behavior. This tests that encrypted private keys are left as placeholders, though I haven't filled in an encrypted certificate or CRL. (The main nuisance there is assembling a test input because OpenSSL's APIs don't even let you make them.) Bug: 387737061 Change-Id: Iebcafdba4924bbcb6298bde24013a508aecc716a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74810 Reviewed-by: Adam Langley Commit-Queue: David Benjamin --- crypto/pem/pem_info.cc | 5 +++-- crypto/x509/x509_test.cc | 34 +++++++++++++++++++++++++++++++--- include/openssl/pem.h | 8 ++++++++ 3 files changed, 42 insertions(+), 5 deletions(-) diff --git a/crypto/pem/pem_info.cc b/crypto/pem/pem_info.cc index 27d4edc50..873c21ba1 100644 --- a/crypto/pem/pem_info.cc +++ b/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/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index ad1297af4..27facf032 100644 --- a/crypto/x509/x509_test.cc +++ b/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/include/openssl/pem.h b/include/openssl/pem.h index 28bf3f286..3711d8a0e 100644 --- a/include/openssl/pem.h +++ b/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 From d4b6eb542d4fd109baacd550935efd00c521e674 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 7 Jan 2025 13:42:37 -0500 Subject: [PATCH 2/2] Fix unwrap comment for CBB_init_fixed Change-Id: I893930a8d23f49968883e4c9b8425ebcc5a2d23b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/75007 Auto-Submit: David Benjamin Reviewed-by: Adam Langley Commit-Queue: David Benjamin --- rust/bssl-crypto/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/rust/bssl-crypto/src/lib.rs b/rust/bssl-crypto/src/lib.rs index d1abb93da..db03b5599 100644 --- a/rust/bssl-crypto/src/lib.rs +++ b/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);