commit 029d0e77fb64625469cc02c8df26767c72081dfd
author David Benjamin <davidben@google.com> Mon Dec 26 19:38:40 2022 -0500
committer Boringssl LUCI CQ <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> Thu Jan 19 23:01:04 2023 +0000
tree 335109944ebf6cab76fca10a3a1d6adc8c47458d
parent 45b8d7bbd771cbf7e116db2ba1f1cc7af959497e

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>

crypto/x509/x509_vfy.c

diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index ce110e6..7638d0d 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -1689,39 +1689,19 @@
 }
 
 static int check_policy(X509_STORE_CTX *ctx) {
-  int ret;
   if (ctx->parent) {
     return 1;
   }
-  // TODO(davidben): Historically, outputs of the |X509_policy_check| were saved
-  // on |ctx| and accessible via the public API. This has since been removed, so
-  // remove the fields from |X509_STORE_CTX|.
-  ret = X509_policy_check(&ctx->tree, &ctx->explicit_policy, ctx->chain,
-                          ctx->param->policies, ctx->param->flags);
-  if (ret == 0) {
-    OPENSSL_PUT_ERROR(X509, ERR_R_MALLOC_FAILURE);
-    ctx->error = X509_V_ERR_OUT_OF_MEM;
-    return 0;
-  }
-  // Invalid or inconsistent extensions
-  if (ret == -1) {
-    // Locate certificates with bad extensions and notify callback.
-    for (size_t i = 0; i < sk_X509_num(ctx->chain); i++) {
-      X509 *x = sk_X509_value(ctx->chain, i);
-      if (!(x->ex_flags & EXFLAG_INVALID_POLICY)) {
-        continue;
-      }
-      ctx->current_cert = x;
-      ctx->error = X509_V_ERR_INVALID_POLICY_EXTENSION;
-      if (!ctx->verify_cb(0, ctx)) {
-        return 0;
-      }
+
+  X509 *current_cert = NULL;
+  int ret = X509_policy_check(ctx->chain, ctx->param->policies,
+                              ctx->param->flags, &current_cert);
+  if (ret != X509_V_OK) {
+    ctx->current_cert = current_cert;
+    ctx->error = ret;
+    if (ret == X509_V_ERR_OUT_OF_MEM) {
+      return 0;
     }
-    return 1;
-  }
-  if (ret == -2) {
-    ctx->current_cert = NULL;
-    ctx->error = X509_V_ERR_NO_EXPLICIT_POLICY;
     return ctx->verify_cb(0, ctx);
   }
 
@@ -2311,10 +2291,6 @@
     }
     ctx->param = NULL;
   }
-  if (ctx->tree != NULL) {
-    X509_policy_tree_free(ctx->tree);
-    ctx->tree = NULL;
-  }
   if (ctx->chain != NULL) {
     sk_X509_pop_free(ctx->chain, X509_free);
     ctx->chain = NULL;