Rewrite X.509 policy tree logic.
This reimplements policy handling using a similar DAG structure as in
https://chromium-review.googlesource.com/c/chromium/src/+/4111415. The
main difference is that, being C, we don't have std::set or std::map
easily available. But the algorithm can be implemented purely with
sorted lists, while remaining subquadratic.
This implementation relies on two assumptions:
1. We do not return the policy tree. This was removed in
https://boringssl-review.googlesource.com/c/boringssl/+/53327
2. We do not return the final set of certificate policies. I.e.,
certificate policy checking is only used for evaluating policy
constraints and X509_V_FLAG_EXPLICIT_POLICY.
The second assumption is not very important. It mostly simplifies
has_explicit_policy slightly.
In addition, this new implementation removes the per-certificate policy
cache. Instead, we just process the policy extensions anew on
certificate verification. This avoids a mess of threading complexity,
including a race condition in the old logic. See
https://boringssl-review.googlesource.com/c/boringssl/+/55762 for a
description of the race condition.
Change-Id: Ifba9037588ecff5eb6ed3c34c8bd7611f60013a6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56036
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index aba608e..e4af13e 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -5518,6 +5518,90 @@
}));
}
+#if defined(OPENSSL_THREADS)
+// A similar test to the above, but ensures the various bits of intermediate
+// state are computed safely.
+TEST(X509Test, PolicyThreads) {
+ const size_t kNumThreads = 10;
+
+ bssl::UniquePtr<ASN1_OBJECT> oid1(
+ OBJ_txt2obj("1.2.840.113554.4.1.72585.2.1", /*dont_search_names=*/1));
+ ASSERT_TRUE(oid1);
+ bssl::UniquePtr<ASN1_OBJECT> oid2(
+ OBJ_txt2obj("1.2.840.113554.4.1.72585.2.2", /*dont_search_names=*/1));
+ ASSERT_TRUE(oid2);
+ bssl::UniquePtr<ASN1_OBJECT> oid3(
+ OBJ_txt2obj("1.2.840.113554.4.1.72585.2.3", /*dont_search_names=*/1));
+ ASSERT_TRUE(oid3);
+
+ auto set_policies = [](X509_VERIFY_PARAM *param,
+ std::vector<const ASN1_OBJECT *> oids) {
+ for (const ASN1_OBJECT *oid : oids) {
+ bssl::UniquePtr<ASN1_OBJECT> copy(OBJ_dup(oid));
+ ASSERT_TRUE(copy);
+ ASSERT_TRUE(X509_VERIFY_PARAM_add0_policy(param, copy.get()));
+ copy.release(); // |X509_VERIFY_PARAM_add0_policy| takes ownership on
+ // success.
+ }
+ };
+
+ {
+ bssl::UniquePtr<X509> root(
+ CertFromPEM(GetTestData("crypto/x509/test/policy_root.pem").c_str()));
+ ASSERT_TRUE(root);
+ bssl::UniquePtr<X509> intermediate(CertFromPEM(
+ GetTestData("crypto/x509/test/policy_intermediate.pem").c_str()));
+ ASSERT_TRUE(intermediate);
+ bssl::UniquePtr<X509> leaf(
+ CertFromPEM(GetTestData("crypto/x509/test/policy_leaf.pem").c_str()));
+ ASSERT_TRUE(leaf);
+
+ std::vector<std::thread> threads;
+ for (size_t i = 0; i < kNumThreads; i++) {
+ threads.emplace_back([&] {
+ EXPECT_EQ(
+ X509_V_OK,
+ Verify(leaf.get(), {root.get()}, {intermediate.get()}, /*crls=*/{},
+ X509_V_FLAG_EXPLICIT_POLICY, [&](X509_VERIFY_PARAM *param) {
+ set_policies(param, {oid1.get()});
+ }));
+ });
+ }
+ for (auto &thread : threads) {
+ thread.join();
+ }
+ }
+
+ {
+ bssl::UniquePtr<X509> root(
+ CertFromPEM(GetTestData("crypto/x509/test/policy_root.pem").c_str()));
+ ASSERT_TRUE(root);
+ bssl::UniquePtr<X509> intermediate(CertFromPEM(
+ GetTestData("crypto/x509/test/policy_intermediate.pem").c_str()));
+ ASSERT_TRUE(intermediate);
+ bssl::UniquePtr<X509> leaf_invalid(CertFromPEM(
+ GetTestData("crypto/x509/test/policy_leaf_invalid.pem").c_str()));
+ ASSERT_TRUE(leaf_invalid);
+
+
+ std::vector<std::thread> threads;
+ for (size_t i = 0; i < kNumThreads; i++) {
+ threads.emplace_back([&] {
+ EXPECT_EQ(X509_V_ERR_INVALID_POLICY_EXTENSION,
+ Verify(leaf_invalid.get(), {root.get()}, {intermediate.get()},
+ /*crls=*/{}, X509_V_FLAG_EXPLICIT_POLICY,
+ [&](X509_VERIFY_PARAM *param) {
+ set_policies(param, {oid1.get()});
+ }));
+ });
+ }
+ for (auto &thread : threads) {
+ thread.join();
+ }
+ }
+}
+#endif // OPENSSL_THREADS
+
TEST(X509Test, ExtensionFromConf) {
static const char kTestOID[] = "1.2.840.113554.4.1.72585.2";
const struct {
