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>
diff --git a/include/openssl/x509.h b/include/openssl/x509.h
index 69d1e00..9e9e5b1 100644
--- a/include/openssl/x509.h
+++ b/include/openssl/x509.h
@@ -2565,7 +2565,7 @@
#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 @@
#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(
