Align PKCS12_parse closer to OpenSSL.
OpenSSL uses the private key to find the leaf certificate. cryptography.io's
tests rely on this.
Update-Note: PKCS12_parse's behavior changes slightly. Affected callers are
recommended to switch to PKCS12_get_key_and_certs, which has much more
predictable behavior and has no pressures from 3rd-party software to match
OpenSSL's quirks.
Change-Id: I4ee2befbd56a0882ee166f00e748f2fe58ac6a86
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/36125
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/pkcs8/pkcs12_test.cc b/crypto/pkcs8/pkcs12_test.cc
index 92a1bff..d345006 100644
--- a/crypto/pkcs8/pkcs12_test.cc
+++ b/crypto/pkcs8/pkcs12_test.cc
@@ -1826,3 +1826,152 @@
NID_pbe_WithSHA1And3_Key_TripleDES_CBC,
NID_pbe_WithSHA1And3_Key_TripleDES_CBC, 100, 100);
}
+
+static bssl::UniquePtr<EVP_PKEY> MakeTestKey() {
+ bssl::UniquePtr<EC_KEY> ec_key(
+ EC_KEY_new_by_curve_name(NID_X9_62_prime256v1));
+ if (!ec_key ||
+ !EC_KEY_generate_key(ec_key.get())) {
+ return nullptr;
+ }
+ bssl::UniquePtr<EVP_PKEY> evp_pkey(EVP_PKEY_new());
+ if (!evp_pkey ||
+ !EVP_PKEY_assign_EC_KEY(evp_pkey.get(), ec_key.release())) {
+ return nullptr;
+ }
+ return evp_pkey;
+}
+
+static bssl::UniquePtr<X509> MakeTestCert(EVP_PKEY *key) {
+ bssl::UniquePtr<X509> x509(X509_new());
+ if (!x509) {
+ return nullptr;
+ }
+ X509_NAME* subject = X509_get_subject_name(x509.get());
+ if (!X509_gmtime_adj(X509_get_notBefore(x509.get()), 0) ||
+ !X509_gmtime_adj(X509_get_notAfter(x509.get()), 60 * 60 * 24) ||
+ !X509_NAME_add_entry_by_txt(subject, "CN", MBSTRING_ASC,
+ reinterpret_cast<const uint8_t *>("Test"), -1,
+ -1, 0) ||
+ !X509_set_issuer_name(x509.get(), subject) ||
+ !X509_set_pubkey(x509.get(), key) ||
+ !X509_sign(x509.get(), key, EVP_sha256())) {
+ return nullptr;
+ }
+ return x509;
+}
+
+static bool PKCS12CreateVector(std::vector<uint8_t> *out, EVP_PKEY *pkey,
+ const std::vector<X509 *> &certs) {
+ bssl::UniquePtr<STACK_OF(X509)> chain(sk_X509_new_null());
+ if (!chain) {
+ return false;
+ }
+
+ for (X509 *cert : certs) {
+ if (!bssl::PushToStack(chain.get(), bssl::UpRef(cert))) {
+ return false;
+ }
+ }
+
+ bssl::UniquePtr<PKCS12> p12(PKCS12_create(kPassword, nullptr /* name */, pkey,
+ nullptr /* cert */, chain.get(), 0,
+ 0, 0, 0, 0));
+ if (!p12) {
+ return false;
+ }
+
+ int len = i2d_PKCS12(p12.get(), nullptr);
+ if (len < 0) {
+ return false;
+ }
+ out->resize(static_cast<size_t>(len));
+ uint8_t *ptr = out->data();
+ return i2d_PKCS12(p12.get(), &ptr) == len;
+}
+
+static void ExpectPKCS12Parse(bssl::Span<const uint8_t> in,
+ EVP_PKEY *expect_key, X509 *expect_cert,
+ const std::vector<X509 *> &expect_ca_certs) {
+ bssl::UniquePtr<BIO> bio(BIO_new_mem_buf(in.data(), in.size()));
+ ASSERT_TRUE(bio);
+
+ bssl::UniquePtr<PKCS12> p12(d2i_PKCS12_bio(bio.get(), nullptr));
+ ASSERT_TRUE(p12);
+
+ EVP_PKEY *key = nullptr;
+ X509 *cert = nullptr;
+ STACK_OF(X509) *ca_certs = nullptr;
+ ASSERT_TRUE(PKCS12_parse(p12.get(), kPassword, &key, &cert, &ca_certs));
+
+ bssl::UniquePtr<EVP_PKEY> delete_key(key);
+ bssl::UniquePtr<X509> delete_cert(cert);
+ bssl::UniquePtr<STACK_OF(X509)> delete_ca_certs(ca_certs);
+
+ if (expect_key == nullptr) {
+ EXPECT_FALSE(key);
+ } else {
+ ASSERT_TRUE(key);
+ EXPECT_EQ(1, EVP_PKEY_cmp(key, expect_key));
+ }
+
+ if (expect_cert == nullptr) {
+ EXPECT_FALSE(cert);
+ } else {
+ ASSERT_TRUE(cert);
+ EXPECT_EQ(0, X509_cmp(cert, expect_cert));
+ }
+
+ ASSERT_EQ(expect_ca_certs.size(), sk_X509_num(ca_certs));
+ for (size_t i = 0; i < expect_ca_certs.size(); i++) {
+ EXPECT_EQ(0, X509_cmp(expect_ca_certs[i], sk_X509_value(ca_certs, i)));
+ }
+}
+
+// Test that |PKCS12_parse| returns values in the expected order.
+TEST(PKCS12Test, Order) {
+ bssl::UniquePtr<EVP_PKEY> key1 = MakeTestKey();
+ ASSERT_TRUE(key1);
+ bssl::UniquePtr<X509> cert1 = MakeTestCert(key1.get());
+ ASSERT_TRUE(cert1);
+ bssl::UniquePtr<X509> cert1b = MakeTestCert(key1.get());
+ ASSERT_TRUE(cert1b);
+ bssl::UniquePtr<EVP_PKEY> key2 = MakeTestKey();
+ ASSERT_TRUE(key2);
+ bssl::UniquePtr<X509> cert2 = MakeTestCert(key2.get());
+ ASSERT_TRUE(cert2);
+ bssl::UniquePtr<EVP_PKEY> key3 = MakeTestKey();
+ ASSERT_TRUE(key3);
+ bssl::UniquePtr<X509> cert3 = MakeTestCert(key3.get());
+ ASSERT_TRUE(cert3);
+
+ // PKCS12_parse uses the key to select the main certificate.
+ std::vector<uint8_t> p12;
+ ASSERT_TRUE(PKCS12CreateVector(&p12, key1.get(),
+ {cert1.get(), cert2.get(), cert3.get()}));
+ ExpectPKCS12Parse(p12, key1.get(), cert1.get(), {cert2.get(), cert3.get()});
+
+ ASSERT_TRUE(PKCS12CreateVector(&p12, key1.get(),
+ {cert3.get(), cert1.get(), cert2.get()}));
+ ExpectPKCS12Parse(p12, key1.get(), cert1.get(), {cert3.get(), cert2.get()});
+
+ ASSERT_TRUE(PKCS12CreateVector(&p12, key1.get(),
+ {cert2.get(), cert3.get(), cert1.get()}));
+ ExpectPKCS12Parse(p12, key1.get(), cert1.get(), {cert2.get(), cert3.get()});
+
+ // In case of duplicates, the last one is selected. (It is unlikely anything
+ // depends on which is selected, but we match OpenSSL.)
+ ASSERT_TRUE(
+ PKCS12CreateVector(&p12, key1.get(), {cert1.get(), cert1b.get()}));
+ ExpectPKCS12Parse(p12, key1.get(), cert1b.get(), {cert1.get()});
+
+ // If there is no key, all certificates are returned as "CA" certificates.
+ ASSERT_TRUE(PKCS12CreateVector(&p12, nullptr,
+ {cert1.get(), cert2.get(), cert3.get()}));
+ ExpectPKCS12Parse(p12, nullptr, nullptr,
+ {cert1.get(), cert2.get(), cert3.get()});
+
+ // The same happens if there is a key, but it does not match any certificate.
+ ASSERT_TRUE(PKCS12CreateVector(&p12, key1.get(), {cert2.get(), cert3.get()}));
+ ExpectPKCS12Parse(p12, key1.get(), nullptr, {cert2.get(), cert3.get()});
+}
diff --git a/crypto/pkcs8/pkcs8_x509.c b/crypto/pkcs8/pkcs8_x509.c
index 1a09b9c..4458b56 100644
--- a/crypto/pkcs8/pkcs8_x509.c
+++ b/crypto/pkcs8/pkcs8_x509.c
@@ -910,9 +910,25 @@
return 0;
}
+ // OpenSSL selects the last certificate which matches the private key as
+ // |out_cert|.
+ //
+ // TODO(davidben): OpenSSL additionally reverses the order of the
+ // certificates, which was likely originally a bug, but may be a feature by
+ // now. See https://crbug.com/boringssl/250 and
+ // https://github.com/openssl/openssl/issues/6698.
*out_cert = NULL;
- if (sk_X509_num(ca_certs) > 0) {
- *out_cert = sk_X509_shift(ca_certs);
+ size_t num_certs = sk_X509_num(ca_certs);
+ if (*out_pkey != NULL && num_certs > 0) {
+ for (size_t i = num_certs - 1; i < num_certs; i--) {
+ X509 *cert = sk_X509_value(ca_certs, i);
+ if (X509_check_private_key(cert, *out_pkey)) {
+ *out_cert = cert;
+ sk_X509_delete(ca_certs, i);
+ break;
+ }
+ ERR_clear_error();
+ }
}
if (out_ca_certs) {
diff --git a/include/openssl/pkcs8.h b/include/openssl/pkcs8.h
index ee48f19..385b995 100644
--- a/include/openssl/pkcs8.h
+++ b/include/openssl/pkcs8.h
@@ -168,12 +168,18 @@
// PKCS12_parse calls |PKCS12_get_key_and_certs| on the ASN.1 data stored in
// |p12|. The |out_pkey| and |out_cert| arguments must not be NULL and, on
-// successful exit, the private key and first certificate will be stored in
+// successful exit, the private key and matching certificate will be stored in
// them. The |out_ca_certs| argument may be NULL but, if not, then any extra
// certificates will be appended to |*out_ca_certs|. If |*out_ca_certs| is NULL
// then it will be set to a freshly allocated stack containing the extra certs.
//
+// Note if |p12| does not contain a private key, both |*out_pkey| and
+// |*out_cert| will be set to NULL and all certificates will be returned via
+// |*out_ca_certs|.
+//
// It returns one on success and zero on error.
+//
+// Use |PKCS12_get_key_and_certs| instead.
OPENSSL_EXPORT int PKCS12_parse(const PKCS12 *p12, const char *password,
EVP_PKEY **out_pkey, X509 **out_cert,
STACK_OF(X509) **out_ca_certs);
