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 <[email protected]>
Commit-Queue: David Benjamin <[email protected]>

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)) {

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<EVP_PKEY> 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);
     }

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