Fix handling of critical X.509 policy constraints

If we see a critical policy constraints extension, we have two options:
We can either process it, which requires running policy validation, or
reject the certificate. We and OpenSSL do neither by default, which
means we may accept certificate chains that we weren't supposed to.

This fixes it by enabling X.509 policy validation unconditionally and
makes X509_V_FLAG_POLICY_CHECK moot. As a side effect, callers no longer
need to do anything for CVE-2023-0466.

This is the opposite of [0]'s advice, which instead recommends skipping
the feature and rejecting critical policy contraints. That would be a
good move for a new X.509 implementation. Policy validation is
badly-designed, even by X.509's standards. But we have OpenSSL's history
of previously accepting critical policy constraints (even though it
didn't check it). I also found at least one caller that tests a chain
with policy constraints, albeit a non-critical one.

We now have an efficient policy validation implementation, so just
enable it.

Of course, fixing this bug in either direction has compatibility risks:
either we take on the compat risk of being newly incompatible with
policyConstraints-using PKIs, or we take on the compat risk of newly
rejecting certificates that were invalid due to a policy validation
error, but no one noticed. The latter case seems safer because the chain
is unambiguously invalid.

Update-Note: X.509 certificate verification (not parsing) will now
notice policy-validation-related errors in the certificate chain. These
include syntax errors in policy-related extensions, and chains with a
requireExplicitPolicy policy constraint that are valid for no
certificate policies. Such chains are unambiguously invalid. We just did
not check it before by default. This is an obscure corner of X.509 and
not expected to come up in most PKIs.

[0] https://www.ietf.org/archive/id/draft-davidben-x509-policy-graph-01.html#section-3.4.4

Fixed: 557
Change-Id: Icc00c7797bb95fd3b14570eb068543fd83cda7b9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58426
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

crypto/x509/test/make_policy_certs.go

@@ -302,6 +302,10 @@ func main() {
 	leafSingle.template.PolicyIdentifiers = []asn1.ObjectIdentifier{testOID5}
 	mustGenerateCertificate("policy_leaf_oid5.pem", &leafSingle, &intermediate)
 
+	leafNone := leaf
+	leafNone.template.PolicyIdentifiers = nil
+	mustGenerateCertificate("policy_leaf_none.pem", &leafNone, &intermediate)
+
 	// Make version of Root, signed by Root 2, with policy mapping inhibited.
 	// This can be combined with intermediateMapped to test the combination.
 	b = cryptobyte.NewBuilder(nil)

crypto/x509/test/policy_leaf_none.pem

@@ -0,0 +1,10 @@
+-----BEGIN CERTIFICATE-----
+MIIBezCCASCgAwIBAgIBAzAKBggqhkjOPQQDAjAeMRwwGgYDVQQDExNQb2xpY3kg
+SW50ZXJtZWRpYXRlMCAXDTAwMDEwMTAwMDAwMFoYDzIxMDAwMTAxMDAwMDAwWjAa
+MRgwFgYDVQQDEw93d3cuZXhhbXBsZS5jb20wWTATBgcqhkjOPQIBBggqhkjOPQMB
+BwNCAASRKti8VW2Rkma+Kt9jQkMNitlCs0l5w8u3SSwm7HZREvmcBCJBjVIREacR
+qI0umhzR2V5NLzBBP9yPD/A+Ch5Xo1EwTzAOBgNVHQ8BAf8EBAMCAgQwEwYDVR0l
+BAwwCgYIKwYBBQUHAwEwDAYDVR0TAQH/BAIwADAaBgNVHREEEzARgg93d3cuZXhh
+bXBsZS5jb20wCgYIKoZIzj0EAwIDSQAwRgIhAIDFeeYJ8nmYo09OnJFpNS3A6fYO
+ZliHkAqOsg193DTnAiEA3OSHLCczcvRjMG+qd/FI61u2sKU1hhHh7uHtD/YO/dA=
+-----END CERTIFICATE-----

crypto/x509/x509_test.cc

@@ -5313,6 +5313,9 @@ TEST(X509Test, Policy) {
   bssl::UniquePtr<X509> leaf_invalid(CertFromPEM(
       GetTestData("crypto/x509/test/policy_leaf_invalid.pem").c_str()));
   ASSERT_TRUE(leaf_invalid);
+  bssl::UniquePtr<X509> leaf_none(CertFromPEM(
+      GetTestData("crypto/x509/test/policy_leaf_none.pem").c_str()));
+  ASSERT_TRUE(leaf_none);
   bssl::UniquePtr<X509> leaf_oid1(CertFromPEM(
       GetTestData("crypto/x509/test/policy_leaf_oid1.pem").c_str()));
   ASSERT_TRUE(leaf_oid1);
@@ -5335,14 +5338,6 @@ TEST(X509Test, Policy) {
       GetTestData("crypto/x509/test/policy_leaf_require1.pem").c_str()));
   ASSERT_TRUE(leaf_require1);
 
-  // By default, OpenSSL does not check policies, so even syntax errors in the
-  // certificatePolicies extension go unnoticed. (This is probably not
-  // important.)
-  EXPECT_EQ(X509_V_OK, Verify(leaf.get(), {root.get()},
-                              {intermediate.get()}, /*crls=*/{}));
-  EXPECT_EQ(X509_V_OK, Verify(leaf_invalid.get(), {root.get()},
-                              {intermediate.get()}, /*crls=*/{}));
-
   auto set_policies = [](X509_VERIFY_PARAM *param,
                          std::vector<const ASN1_OBJECT *> oids) {
     for (const ASN1_OBJECT *oid : oids) {
@@ -5351,9 +5346,6 @@ TEST(X509Test, Policy) {
       ASSERT_TRUE(X509_VERIFY_PARAM_add0_policy(param, copy.get()));
       copy.release();  // |X509_VERIFY_PARAM_add0_policy| takes ownership on
                        // success.
-      // TODO(davidben): |X509_VERIFY_PARAM_add0_policy| does not set this flag,
-      // while |X509_VERIFY_PARAM_set1_policies| does. Is this a bug?
-      X509_VERIFY_PARAM_set_flags(param, X509_V_FLAG_POLICY_CHECK);
     }
   };
 
@@ -5399,6 +5391,9 @@ TEST(X509Test, Policy) {
                    [&](X509_VERIFY_PARAM *param) {
                      set_policies(param, {oid1.get()});
                    }));
+  EXPECT_EQ(X509_V_ERR_INVALID_POLICY_EXTENSION,
+            Verify(leaf_invalid.get(), {root.get()}, {intermediate.get()},
+                   /*crls=*/{}));
 
   // There is a duplicate policy in the policy extension.
   EXPECT_EQ(X509_V_ERR_INVALID_POLICY_EXTENSION,
@@ -5443,10 +5438,17 @@ TEST(X509Test, Policy) {
                      set_policies(param, {oid3.get()});
                    }));
 
+  // requireExplicitPolicy applies even if the application does not configure a
+  // user-initial-policy-set. If the validation results in no policies, the
+  // chain is invalid.
+  EXPECT_EQ(X509_V_ERR_NO_EXPLICIT_POLICY,
+            Verify(leaf_none.get(), {root.get()}, {intermediate_require.get()},
+                   /*crls=*/{}));
+
   // A leaf can also set requireExplicitPolicy.
   EXPECT_EQ(X509_V_OK,
             Verify(leaf_require.get(), {root.get()}, {intermediate.get()},
-                   /*crls=*/{}, X509_V_FLAG_POLICY_CHECK));
+                   /*crls=*/{}, /*flags=*/0));
   EXPECT_EQ(X509_V_OK, Verify(leaf_require.get(), {root.get()},
                               {intermediate.get()}, /*crls=*/{},
                               /*flags=*/0, [&](X509_VERIFY_PARAM *param) {

crypto/x509/x509_vfy.c

@@ -447,7 +447,6 @@ int X509_verify_cert(X509_STORE_CTX *ctx) {
 
   // Check revocation status: we do this after copying parameters because
   // they may be needed for CRL signature verification.
-
   ok = ctx->check_revocation(ctx);
   if (!ok) {
     goto end;
@@ -464,14 +463,13 @@ int X509_verify_cert(X509_STORE_CTX *ctx) {
   }
 
   // Check name constraints
-
   ok = check_name_constraints(ctx);
   if (!ok) {
     goto end;
   }
 
-  // If we get this far evaluate policies
-  if (!bad_chain && (ctx->param->flags & X509_V_FLAG_POLICY_CHECK)) {
+  // If we get this far, evaluate policies.
+  if (!bad_chain) {
     ok = ctx->check_policy(ctx);
   }
 
@@ -1612,6 +1610,7 @@ static int cert_crl(X509_STORE_CTX *ctx, X509_CRL *crl, X509 *x) {
 }
 
 static int check_policy(X509_STORE_CTX *ctx) {
+  // TODO(davidben): Why do we disable policy validation for CRL paths?
   if (ctx->parent) {
     return 1;
   }

crypto/x509/x509_vpm.c

@@ -350,9 +350,6 @@ int X509_VERIFY_PARAM_set1_name(X509_VERIFY_PARAM *param, const char *name) {
 
 int X509_VERIFY_PARAM_set_flags(X509_VERIFY_PARAM *param, unsigned long flags) {
   param->flags |= flags;
-  if (flags & X509_V_FLAG_POLICY_MASK) {
-    param->flags |= X509_V_FLAG_POLICY_CHECK;
-  }
   return 1;
 }
 
@@ -398,8 +395,6 @@ int X509_VERIFY_PARAM_add0_policy(X509_VERIFY_PARAM *param,
   if (!sk_ASN1_OBJECT_push(param->policies, policy)) {
     return 0;
   }
-  // TODO(davidben): This does not set |X509_V_FLAG_POLICY_CHECK|, while
-  // |X509_VERIFY_PARAM_set1_policies| does. Is this a bug?
   return 1;
 }
 
@@ -421,7 +416,6 @@ int X509_VERIFY_PARAM_set1_policies(X509_VERIFY_PARAM *param,
     return 0;
   }
 
-  param->flags |= X509_V_FLAG_POLICY_CHECK;
   return 1;
 }
 

include/openssl/x509.h

@@ -2565,7 +2565,7 @@ OPENSSL_EXPORT void X509_STORE_CTX_set_depth(X509_STORE_CTX *ctx, int depth);
 #define X509_V_FLAG_X509_STRICT 0x00
 // This flag does nothing as proxy certificate support has been removed.
 #define X509_V_FLAG_ALLOW_PROXY_CERTS 0x40
-// Enable policy checking
+// Does nothing as its functionality has been enabled by default.
 #define X509_V_FLAG_POLICY_CHECK 0x80
 // Policy variable require-explicit-policy
 #define X509_V_FLAG_EXPLICIT_POLICY 0x100
@@ -2602,11 +2602,6 @@ OPENSSL_EXPORT void X509_STORE_CTX_set_depth(X509_STORE_CTX *ctx, int depth);
 #define X509_VP_FLAG_LOCKED 0x8
 #define X509_VP_FLAG_ONCE 0x10
 
-// Internal use: mask of policy related options
-#define X509_V_FLAG_POLICY_MASK                             \
-  (X509_V_FLAG_POLICY_CHECK | X509_V_FLAG_EXPLICIT_POLICY | \
-   X509_V_FLAG_INHIBIT_ANY | X509_V_FLAG_INHIBIT_MAP)
-
 OPENSSL_EXPORT int X509_OBJECT_idx_by_subject(STACK_OF(X509_OBJECT) *h,
                                               int type, X509_NAME *name);
 OPENSSL_EXPORT X509_OBJECT *X509_OBJECT_retrieve_by_subject(

sources.cmake

@@ -127,6 +127,7 @@ set(
   crypto/x509/test/policy_leaf_any.pem
   crypto/x509/test/policy_leaf_duplicate.pem
   crypto/x509/test/policy_leaf_invalid.pem
+  crypto/x509/test/policy_leaf_none.pem
   crypto/x509/test/policy_leaf_oid1.pem
   crypto/x509/test/policy_leaf_oid2.pem
   crypto/x509/test/policy_leaf_oid3.pem