OERV-RUNTIME/boringssl
2
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Test X509_verify_cert with CAs that share a name

Browse Source 
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>
This commit is contained in:
David Benjamin
2024-02-26 22:08:59 +00:00
committed by Boringssl LUCI CQ
parent db614a5677
commit 89134daffd
1 changed files with 147 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crypto/x509/x509_test.cc
+147
View File
@@ -1780,6 +1780,30 @@ static bool AddKeyUsage(X509 *x509, const std::vector<KeyUsage> usages) {
/*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 @@ static bool AddRevokedSerialU64(X509_CRL *crl, uint64_t serial,
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 @@ TEST(X509Test, DirHashThreads) {
}
}
#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()));
}
}
}
}