Test X509_verify_cert with CAs that share a name
In that case, we rely on AKID/SKID matches to disambiguate. However,
OpenSSL's internal interfaces are not very good at handling this case
and often work around their own bugs. As a precursor to, hopefully,
cleaning that up someday, test this, with both direct adding and
hash_dir.
I've just tested the basic case here. Looking at the code, I think
there are bugs where, e.g., if CA1 was added directly and CA2 is only
accessible via hash_dir, X509_STORE_CTX_get1_issuer does not know to
check hash_dir for CA2, because internal interfaces get in the way.
Bug: 685
Change-Id: I32737661c84d6a006cf9d5ae1ec42b3f27437bf0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66010
Reviewed-by: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 8b20ade..c4fd0b8 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -1780,6 +1780,30 @@
/*flags=*/0);
}
+static bool AddSubjectKeyIdentifier(X509 *x509,
+ bssl::Span<const uint8_t> key_id) {
+ bssl::UniquePtr<ASN1_OCTET_STRING> oct(ASN1_OCTET_STRING_new());
+ return oct != nullptr &&
+ ASN1_STRING_set(oct.get(), key_id.data(), key_id.size()) &&
+ X509_add1_ext_i2d(x509, NID_subject_key_identifier, oct.get(),
+ /*crit=*/0, /*flags=*/0);
+}
+
+static bool AddAuthorityKeyIdentifer(X509 *x509, bssl::Span<const uint8_t> key_id) {
+ bssl::UniquePtr<AUTHORITY_KEYID> akid(AUTHORITY_KEYID_new());
+ if (akid == nullptr) {
+ return false;
+ }
+ akid->keyid = ASN1_OCTET_STRING_new();
+ if (akid->keyid == nullptr ||
+ !ASN1_STRING_set(akid->keyid, key_id.data(), key_id.size()) ||
+ !X509_add1_ext_i2d(x509, NID_authority_key_identifier, akid.get(),
+ /*crit=*/0, /*flags=*/0)) {
+ return false;
+ }
+ return true;
+}
+
static bssl::UniquePtr<X509_CRL> MakeTestCRL(const char *issuer,
int this_update_offset_day,
int next_update_offset_day) {
@@ -1819,6 +1843,22 @@
return true;
}
+static bool AddAuthorityKeyIdentifer(X509_CRL *crl,
+ bssl::Span<const uint8_t> key_id) {
+ bssl::UniquePtr<AUTHORITY_KEYID> akid(AUTHORITY_KEYID_new());
+ if (akid == nullptr) {
+ return false;
+ }
+ akid->keyid = ASN1_OCTET_STRING_new();
+ if (akid->keyid == nullptr ||
+ !ASN1_STRING_set(akid->keyid, key_id.data(), key_id.size()) ||
+ !X509_CRL_add1_ext_i2d(crl, NID_authority_key_identifier, akid.get(),
+ /*crit=*/0, /*flags=*/0)) {
+ return false;
+ }
+ return true;
+}
+
TEST(X509Test, NameConstraints) {
bssl::UniquePtr<EVP_PKEY> key = PrivateKeyFromPEM(kP256Key);
ASSERT_TRUE(key);
@@ -8372,3 +8412,110 @@
}
}
#endif // OPENSSL_THREADS
+
+// Test that, when there are two CAs with the same name, but different key
+// identifiers, certificate and CRL lookup can disambiguate correctly.
+TEST(X509Test, DuplicateName) {
+ // Make two certificate chains and empty CRLs, with the same names but
+ // different keys.
+ bssl::UniquePtr<EVP_PKEY> key1 = PrivateKeyFromPEM(kP256Key);
+ ASSERT_TRUE(key1);
+ uint8_t key_id1[] = {'K', 'e', 'y', '1'};
+ bssl::UniquePtr<X509> ca1 =
+ MakeTestCert("CA", "CA", key1.get(), /*is_ca=*/true);
+ ASSERT_TRUE(ca1);
+ ASSERT_TRUE(AddSubjectKeyIdentifier(ca1.get(), key_id1));
+ ASSERT_TRUE(X509_sign(ca1.get(), key1.get(), EVP_sha256()));
+ bssl::UniquePtr<X509> leaf1 =
+ MakeTestCert("CA", "Leaf", key1.get(), /*is_ca=*/false);
+ ASSERT_TRUE(leaf1);
+ ASSERT_TRUE(AddAuthorityKeyIdentifer(leaf1.get(), key_id1));
+ ASSERT_TRUE(X509_sign(leaf1.get(), key1.get(), EVP_sha256()));
+ bssl::UniquePtr<X509_CRL> crl1 = MakeTestCRL("CA", -1, 1);
+ ASSERT_TRUE(crl1);
+ ASSERT_TRUE(AddAuthorityKeyIdentifer(crl1.get(), key_id1));
+ ASSERT_TRUE(X509_CRL_sign(crl1.get(), key1.get(), EVP_sha256()));
+ // TODO(davidben): Some state in CRLs does not get correctly set up unless it
+ // is parsed from data. |X509_CRL_sign| should reset it internally.
+ crl1 = ReencodeCRL(crl1.get());
+ ASSERT_TRUE(crl1);
+
+ bssl::UniquePtr<EVP_PKEY> key2 = PrivateKeyFromPEM(kRSAKey);
+ ASSERT_TRUE(key2);
+ uint8_t key_id2[] = {'K', 'e', 'y', '2'};
+ bssl::UniquePtr<X509> ca2 =
+ MakeTestCert("CA", "CA", key2.get(), /*is_ca=*/true);
+ ASSERT_TRUE(ca2);
+ ASSERT_TRUE(AddSubjectKeyIdentifier(ca2.get(), key_id2));
+ ASSERT_TRUE(X509_sign(ca2.get(), key2.get(), EVP_sha256()));
+ bssl::UniquePtr<X509> leaf2 =
+ MakeTestCert("CA", "Leaf", key2.get(), /*is_ca=*/false);
+ ASSERT_TRUE(leaf2);
+ ASSERT_TRUE(AddAuthorityKeyIdentifer(leaf2.get(), key_id2));
+ ASSERT_TRUE(X509_sign(leaf2.get(), key2.get(), EVP_sha256()));
+ bssl::UniquePtr<X509_CRL> crl2 = MakeTestCRL("CA", -2, 2);
+ ASSERT_TRUE(crl2);
+ ASSERT_TRUE(AddAuthorityKeyIdentifer(crl2.get(), key_id2));
+ ASSERT_TRUE(X509_CRL_sign(crl2.get(), key2.get(), EVP_sha256()));
+ // TODO(davidben): Some state in CRLs does not get correctly set up unless it
+ // is parsed from data. |X509_CRL_sign| should reset it internally.
+ crl2 = ReencodeCRL(crl2.get());
+ ASSERT_TRUE(crl2);
+
+ for (bool key1_first : {false, true}) {
+ SCOPED_TRACE(key1_first);
+ X509 *first_leaf = leaf1.get();
+ X509 *second_leaf = leaf2.get();
+ if (!key1_first) {
+ std::swap(first_leaf, second_leaf);
+ }
+
+ for (bool use_dir : {false, true}) {
+ SCOPED_TRACE(use_dir);
+ bssl::UniquePtr<X509_STORE> store(X509_STORE_new());
+ ASSERT_TRUE(store);
+ TemporaryHashDir dir(X509_FILETYPE_PEM);
+ if (use_dir) {
+ ASSERT_TRUE(dir.Init());
+ ASSERT_TRUE(dir.AddCert(ca1.get(), kNewHash));
+ ASSERT_TRUE(dir.AddCert(ca2.get(), kNewHash));
+ ASSERT_TRUE(dir.AddCRL(crl1.get(), kNewHash));
+ ASSERT_TRUE(dir.AddCRL(crl2.get(), kNewHash));
+ ASSERT_EQ(dir.num_cert_hashes(), 1u);
+ ASSERT_EQ(dir.num_crl_hashes(), 1u);
+ ASSERT_TRUE(X509_STORE_load_locations(store.get(), /*file=*/nullptr,
+ dir.path().c_str()));
+ } else {
+ ASSERT_TRUE(X509_STORE_add_cert(store.get(), ca1.get()));
+ ASSERT_TRUE(X509_STORE_add_cert(store.get(), ca2.get()));
+ ASSERT_TRUE(X509_STORE_add_crl(store.get(), crl1.get()));
+ ASSERT_TRUE(X509_STORE_add_crl(store.get(), crl2.get()));
+ }
+
+ // Verify the two certificates. Whichever comes first, we should
+ // successfully find their CA and CRL.
+ {
+ bssl::UniquePtr<X509_STORE_CTX> ctx(X509_STORE_CTX_new());
+ ASSERT_TRUE(ctx);
+ ASSERT_TRUE(
+ X509_STORE_CTX_init(ctx.get(), store.get(), first_leaf, nullptr));
+ X509_STORE_CTX_set_flags(ctx.get(), X509_V_FLAG_CRL_CHECK);
+ X509_STORE_CTX_set_time_posix(ctx.get(), /*flags=*/0, kReferenceTime);
+ EXPECT_TRUE(X509_verify_cert(ctx.get()))
+ << X509_verify_cert_error_string(
+ X509_STORE_CTX_get_error(ctx.get()));
+ }
+ {
+ bssl::UniquePtr<X509_STORE_CTX> ctx(X509_STORE_CTX_new());
+ ASSERT_TRUE(ctx);
+ ASSERT_TRUE(
+ X509_STORE_CTX_init(ctx.get(), store.get(), second_leaf, nullptr));
+ X509_STORE_CTX_set_flags(ctx.get(), X509_V_FLAG_CRL_CHECK);
+ X509_STORE_CTX_set_time_posix(ctx.get(), /*flags=*/0, kReferenceTime);
+ EXPECT_TRUE(X509_verify_cert(ctx.get()))
+ << X509_verify_cert_error_string(
+ X509_STORE_CTX_get_error(ctx.get()));
+ }
+ }
+ }
+}
