ANY PRIVATE KEY is not a real PEM type `PEM_STRING_EVP_PKEY` is not a real PEM type. It's just a placeholder used by `PEM_read_bio_PrivateKey` and `PEM_bytes_read_bio` to match one of several names. It's already rejected by `PEM_read_bio_PrivateKey`, but not in the expected way: - We get a confusing ERR_LIB_ASN1 error out of the error queue. - We don't correctly skip over it as an unrecognized type to keep parsing. Fix this. Since we were already not parsing ANY PRIVATE KEY, this is unlikely to impact anything. We're more likely to make inputs start parsing than not. Update-Note: PEM blocks with the unrecognized PEM type "ANY PRIVATE KEY" will now be cleanly skipped over by `PEM_read_bio_PrivateKey`, like other unknown types, rather than fail with a surprising error code. Change-Id: Ibb44b630300487146a25372212b4e204f6d2db09 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/97327 Reviewed-by: Xiangfei Ding <xfding@google.com> Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/pem/pem_lib.cc b/crypto/pem/pem_lib.cc index f59c42b..5385049 100644 --- a/crypto/pem/pem_lib.cc +++ b/crypto/pem/pem_lib.cc
@@ -43,7 +43,7 @@ using namespace bssl; static int load_iv(const char **fromp, unsigned char *to, size_t num); -static int check_pem(const std::string_view nm, const std::string_view name); +static bool check_pem(std::string_view name, std::string_view expected); // PEM_proc_type appends a Proc-Type header to `buf`, determined by `type`. static void PEM_proc_type(char buf[PEM_BUFSIZE], int type) { @@ -100,58 +100,45 @@ return ret; } -static int check_pem(const std::string_view nm, const std::string_view name) { - // Normal matching nm and name - if (nm == name) { - return 1; +static bool check_pem(std::string_view name, std::string_view expected) { + // `PEM_STRING_EVP_PKEY` is not a real PEM type, but a placeholder that + // matches one of several private key types. + if (expected == PEM_STRING_EVP_PKEY) { + return name == PEM_STRING_PKCS8 || name == PEM_STRING_PKCS8INF || + name == PEM_STRING_RSA || name == PEM_STRING_EC || + name == PEM_STRING_DSA; } - // Make PEM_STRING_EVP_PKEY match any private key - - if (name == PEM_STRING_EVP_PKEY) { - return nm == PEM_STRING_PKCS8 || nm == PEM_STRING_PKCS8INF || - nm == PEM_STRING_RSA || nm == PEM_STRING_EC || nm == PEM_STRING_DSA; + // Normal name matching. + if (name == expected) { + return true; } - // Permit older strings - - if (nm == PEM_STRING_X509_OLD && name == PEM_STRING_X509) { - return 1; + // Permit older strings. + if (name == PEM_STRING_X509_OLD && expected == PEM_STRING_X509) { + return true; + } + if (name == PEM_STRING_X509_REQ_OLD && expected == PEM_STRING_X509_REQ) { + return true; } - if (nm == PEM_STRING_X509_REQ_OLD && name == PEM_STRING_X509_REQ) { - return 1; + // Allow normal certs to be read as trusted certs. + if (name == PEM_STRING_X509 && expected == PEM_STRING_X509_TRUSTED) { + return true; + } + if (name == PEM_STRING_X509_OLD && expected == PEM_STRING_X509_TRUSTED) { + return true; } - // Allow normal certs to be read as trusted certs - if (nm == PEM_STRING_X509 && name == PEM_STRING_X509_TRUSTED) { - return 1; + // Some CAs use PKCS#7 with CERTIFICATE headers. + if (name == PEM_STRING_X509 && expected == PEM_STRING_PKCS7) { + return true; + } + if (name == PEM_STRING_PKCS7_SIGNED && expected == PEM_STRING_PKCS7) { + return true; } - if (nm == PEM_STRING_X509_OLD && name == PEM_STRING_X509_TRUSTED) { - return 1; - } - - // Some CAs use PKCS#7 with CERTIFICATE headers - if (nm == PEM_STRING_X509 && name == PEM_STRING_PKCS7) { - return 1; - } - - if (nm == PEM_STRING_PKCS7_SIGNED && name == PEM_STRING_PKCS7) { - return 1; - } - -#ifndef OPENSSL_NO_CMS - if (nm == PEM_STRING_X509 && name == PEM_STRING_CMS) { - return 1; - } - // Allow CMS to be read from PKCS#7 headers - if (nm == PEM_STRING_PKCS7 && name == PEM_STRING_CMS) { - return 1; - } -#endif - - return 0; + return false; } static const EVP_CIPHER *cipher_by_name(const std::string_view name) { @@ -174,7 +161,7 @@ } int PEM_bytes_read_bio(unsigned char **pdata, long *plen, char **pnm, - const char *name, BIO *bp, pem_password_cb *cb, + const char *expected_name, BIO *bp, pem_password_cb *cb, void *u) { EVP_CIPHER_INFO cipher; UniquePtr<char> nm; @@ -186,7 +173,7 @@ for (;;) { if (!PEM_read_bio_inner(bp, &nm, &header, &data)) { if (ERR_equals(ERR_peek_error(), ERR_LIB_PEM, PEM_R_NO_START_LINE)) { - ERR_add_error_data(2, "Expecting: ", name); + ERR_add_error_data(2, "Expecting: ", expected_name); } return 0; } @@ -194,7 +181,7 @@ OPENSSL_PUT_ERROR(PEM, ERR_R_OVERFLOW); return 0; } - if (check_pem(nm.get(), name)) { + if (check_pem(nm.get(), expected_name)) { break; } }
diff --git a/crypto/pem/pem_pkey.cc b/crypto/pem/pem_pkey.cc index 9a45d39..58eb720 100644 --- a/crypto/pem/pem_pkey.cc +++ b/crypto/pem/pem_pkey.cc
@@ -96,7 +96,13 @@ ret = d2i_PrivateKey(EVP_PKEY_EC, x, &p, len); } else if (strcmp(nm, PEM_STRING_DSA) == 0) { ret = d2i_PrivateKey(EVP_PKEY_DSA, x, &p, len); + } else { + // `PEM_bytes_read_bio` should not have returned a PEM type this function + // does not recognized. + OPENSSL_PUT_ERROR(PEM, ERR_R_INTERNAL_ERROR); + goto err; } + p8err: if (ret == nullptr) { OPENSSL_PUT_ERROR(PEM, ERR_R_ASN1_LIB);
diff --git a/crypto/pem/pem_test.cc b/crypto/pem/pem_test.cc index 55e65ef..e0fa52b 100644 --- a/crypto/pem/pem_test.cc +++ b/crypto/pem/pem_test.cc
@@ -514,4 +514,44 @@ } } +// The placeholder ANY PRIVATE KEY value is not a valid key type. +TEST(PEMTest, AnyPrivateKey) { + { + static const char kInput[] = R"( +-----BEGIN ANY PRIVATE KEY----- +MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgBw8IcnrUoEqc3VnJ +TYlodwi1b8ldMHcO6NHJzgqLtGqhRANCAATmK2niv2Wfl74vHg2UikzVl2u3qR4N +Rvvdqakendy6WgHn1peoChj5w8SjHlbifINI2xYaHPUdfvGULUvPciLB +-----END ANY PRIVATE KEY----- +)"; + bssl::UniquePtr<BIO> bio(BIO_new_mem_buf(kInput, -1)); + ASSERT_TRUE(bio); + bssl::UniquePtr<EVP_PKEY> pkey( + PEM_read_bio_PrivateKey(bio.get(), nullptr, nullptr, nullptr)); + EXPECT_FALSE(pkey); + EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_PEM, PEM_R_NO_START_LINE)); + } + + { + static const char kInput[] = R"( +-----BEGIN ANY PRIVATE KEY----- +MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgBw8IcnrUoEqc3VnJ +TYlodwi1b8ldMHcO6NHJzgqLtGqhRANCAATmK2niv2Wfl74vHg2UikzVl2u3qR4N +Rvvdqakendy6WgHn1peoChj5w8SjHlbifINI2xYaHPUdfvGULUvPciLB +-----END ANY PRIVATE KEY----- +-----BEGIN PRIVATE KEY----- +MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgBw8IcnrUoEqc3VnJ +TYlodwi1b8ldMHcO6NHJzgqLtGqhRANCAATmK2niv2Wfl74vHg2UikzVl2u3qR4N +Rvvdqakendy6WgHn1peoChj5w8SjHlbifINI2xYaHPUdfvGULUvPciLB +-----END PRIVATE KEY----- +)"; + bssl::UniquePtr<BIO> bio(BIO_new_mem_buf(kInput, -1)); + ASSERT_TRUE(bio); + bssl::UniquePtr<EVP_PKEY> pkey( + PEM_read_bio_PrivateKey(bio.get(), nullptr, nullptr, nullptr)); + ASSERT_TRUE(pkey); + EXPECT_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_EC); + } +} + } // namespace
diff --git a/include/openssl/pem.h b/include/openssl/pem.h index 3f36013..7dc418a 100644 --- a/include/openssl/pem.h +++ b/include/openssl/pem.h
@@ -140,8 +140,8 @@ const unsigned char *data, long len); OPENSSL_EXPORT int PEM_bytes_read_bio(unsigned char **pdata, long *plen, - char **pnm, const char *name, BIO *bp, - pem_password_cb *cb, void *u); + char **pnm, const char *expected_name, + BIO *bp, pem_password_cb *cb, void *u); OPENSSL_EXPORT void *PEM_ASN1_read_bio(d2i_of_void *d2i, const char *name, BIO *bp, void **x, pem_password_cb *cb, void *u);