Remove delta and extended CRL support
Update-Note: The X509_V_FLAG_EXTENDED_CRL_SUPPORT and
X509_V_FLAG_USE_DELTAS flags now cause verification to fail. They
weren't enabled by any caller.
This broadly is meant to disable:
- Delta CRLs
- Indirect CRLs (When the CRL's issuer is somehow different from the
certificate. The security properties for this is very interesting,
since it refers to just any other random name under the same trust
anchor. Very clearly a remnant of when X.509 was meant to authenticate
a global directory. See the rather worrisome comment over
check_crl_chain.)
- Merging together multiple CRLs that are partitioned by reasons
There's some other code we can now unwind, which will be handled in
follow-up changes. This CL is meant to be a minimal change to disable
them. Though even this minimal change requires we delete a bunch of
functions.
Bug: 601
Change-Id: I319ab793f480c6b99de86da6077b616f18edf06b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63929
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 9699b5a..fda412f 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -1497,6 +1497,14 @@
param, kReferenceTime + 2 * 30 * 24 * 3600);
}));
+ // We no longer support indirect or delta CRLs.
+ EXPECT_EQ(X509_V_ERR_INVALID_CALL,
+ Verify(leaf.get(), {root.get()}, {root.get()}, {basic_crl.get()},
+ X509_V_FLAG_CRL_CHECK | X509_V_FLAG_EXTENDED_CRL_SUPPORT));
+ EXPECT_EQ(X509_V_ERR_INVALID_CALL,
+ Verify(leaf.get(), {root.get()}, {root.get()}, {basic_crl.get()},
+ X509_V_FLAG_CRL_CHECK | X509_V_FLAG_USE_DELTAS));
+
// Parsing kBadExtensionCRL should fail.
EXPECT_FALSE(CRLFromPEM(kBadExtensionCRL));
}
diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index f5e7733..f203da9 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -128,8 +128,6 @@
unsigned int *preasons, X509_CRL *crl, X509 *x);
static int get_crl_delta(X509_STORE_CTX *ctx, X509_CRL **pcrl, X509_CRL **pdcrl,
X509 *x);
-static void get_delta_sk(X509_STORE_CTX *ctx, X509_CRL **dcrl, int *pcrl_score,
- X509_CRL *base, STACK_OF(X509_CRL) *crls);
static void crl_akid_check(X509_STORE_CTX *ctx, X509_CRL *crl, X509 **pissuer,
int *pcrl_score);
static int crl_crldp_check(X509 *x, X509_CRL *crl, int crl_score,
@@ -193,6 +191,7 @@
ctx->error = X509_V_ERR_INVALID_CALL;
return -1;
}
+
if (ctx->chain != NULL) {
// This X509_STORE_CTX has already been used to verify a cert. We
// cannot do another one.
@@ -201,6 +200,16 @@
return -1;
}
+ if (ctx->param->flags &
+ (X509_V_FLAG_EXTENDED_CRL_SUPPORT | X509_V_FLAG_USE_DELTAS)) {
+ // We do not support indirect or delta CRLs. The flags still exist for
+ // compatibility with bindings libraries, but to ensure we do not
+ // inadvertently skip a CRL check that the caller expects, fail closed.
+ OPENSSL_PUT_ERROR(X509, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
+ ctx->error = X509_V_ERR_INVALID_CALL;
+ return -1;
+ }
+
// first we make sure the chain we are going to build is present and that
// the first entry is in place
ctx->chain = sk_X509_new_null();
@@ -1002,11 +1011,11 @@
*pscore = best_score;
*preasons = best_reasons;
X509_CRL_up_ref(best_crl);
+ // TODO(crbug.com/boringssl/601): Remove remnants of delta CRL support.
if (*pdcrl) {
X509_CRL_free(*pdcrl);
*pdcrl = NULL;
}
- get_delta_sk(ctx, pdcrl, pscore, best_crl, crls);
}
if (best_score >= CRL_SCORE_VALID) {
@@ -1016,109 +1025,6 @@
return 0;
}
-// Compare two CRL extensions for delta checking purposes. They should be
-// both present or both absent. If both present all fields must be identical.
-
-static int crl_extension_match(X509_CRL *a, X509_CRL *b, int nid) {
- const ASN1_OCTET_STRING *exta, *extb;
- int i;
- i = X509_CRL_get_ext_by_NID(a, nid, -1);
- if (i >= 0) {
- // Can't have multiple occurrences
- if (X509_CRL_get_ext_by_NID(a, nid, i) != -1) {
- return 0;
- }
- exta = X509_EXTENSION_get_data(X509_CRL_get_ext(a, i));
- } else {
- exta = NULL;
- }
-
- i = X509_CRL_get_ext_by_NID(b, nid, -1);
-
- if (i >= 0) {
- if (X509_CRL_get_ext_by_NID(b, nid, i) != -1) {
- return 0;
- }
- extb = X509_EXTENSION_get_data(X509_CRL_get_ext(b, i));
- } else {
- extb = NULL;
- }
-
- if (!exta && !extb) {
- return 1;
- }
-
- if (!exta || !extb) {
- return 0;
- }
-
- if (ASN1_OCTET_STRING_cmp(exta, extb)) {
- return 0;
- }
-
- return 1;
-}
-
-// See if a base and delta are compatible
-
-static int check_delta_base(X509_CRL *delta, X509_CRL *base) {
- // Delta CRL must be a delta
- if (!delta->base_crl_number) {
- return 0;
- }
- // Base must have a CRL number
- if (!base->crl_number) {
- return 0;
- }
- // Issuer names must match
- if (X509_NAME_cmp(X509_CRL_get_issuer(base), X509_CRL_get_issuer(delta))) {
- return 0;
- }
- // AKID and IDP must match
- if (!crl_extension_match(delta, base, NID_authority_key_identifier)) {
- return 0;
- }
- if (!crl_extension_match(delta, base, NID_issuing_distribution_point)) {
- return 0;
- }
- // Delta CRL base number must not exceed Full CRL number.
- if (ASN1_INTEGER_cmp(delta->base_crl_number, base->crl_number) > 0) {
- return 0;
- }
- // Delta CRL number must exceed full CRL number
- if (ASN1_INTEGER_cmp(delta->crl_number, base->crl_number) > 0) {
- return 1;
- }
- return 0;
-}
-
-// For a given base CRL find a delta... maybe extend to delta scoring or
-// retrieve a chain of deltas...
-
-static void get_delta_sk(X509_STORE_CTX *ctx, X509_CRL **dcrl, int *pscore,
- X509_CRL *base, STACK_OF(X509_CRL) *crls) {
- X509_CRL *delta;
- size_t i;
- if (!(ctx->param->flags & X509_V_FLAG_USE_DELTAS)) {
- return;
- }
- if (!((ctx->current_cert->ex_flags | base->flags) & EXFLAG_FRESHEST)) {
- return;
- }
- for (i = 0; i < sk_X509_CRL_num(crls); i++) {
- delta = sk_X509_CRL_value(crls, i);
- if (check_delta_base(delta, base)) {
- if (check_crl_time(ctx, delta, 0)) {
- *pscore |= CRL_SCORE_TIME_DELTA;
- }
- X509_CRL_up_ref(delta);
- *dcrl = delta;
- return;
- }
- }
- *dcrl = NULL;
-}
-
// For a given CRL return how suitable it is for the supplied certificate
// 'x'. The return value is a mask of several criteria. If the issuer is not
// the certificate issuer this is returned in *pissuer. The reasons mask is
@@ -1136,19 +1042,14 @@
if (crl->idp_flags & IDP_INVALID) {
return 0;
}
- // Reason codes or indirect CRLs need extended CRL support
- if (!(ctx->param->flags & X509_V_FLAG_EXTENDED_CRL_SUPPORT)) {
- if (crl->idp_flags & (IDP_INDIRECT | IDP_REASONS)) {
- return 0;
- }
- } else if (crl->idp_flags & IDP_REASONS) {
- // If no new reasons reject
- if (!(crl->idp_reasons & ~tmp_reasons)) {
- return 0;
- }
+ // Reason codes and indirect CRLs are not supported.
+ if (crl->idp_flags & (IDP_INDIRECT | IDP_REASONS)) {
+ return 0;
}
- // Don't process deltas at this stage
- else if (crl->base_crl_number) {
+ if (crl->base_crl_number) {
+ // Don't process deltas at this stage
+ //
+ // TODO(crbug.com/boringssl/601): Clean up remnants of delta CRL support.
return 0;
}
// If issuer name doesn't match certificate need indirect CRL
@@ -1199,7 +1100,6 @@
X509 *crl_issuer = NULL;
X509_NAME *cnm = X509_CRL_get_issuer(crl);
int cidx = ctx->error_depth;
- size_t i;
if ((size_t)cidx != sk_X509_num(ctx->chain) - 1) {
cidx++;
@@ -1226,26 +1126,6 @@
return;
}
}
-
- // Anything else needs extended CRL support
-
- if (!(ctx->param->flags & X509_V_FLAG_EXTENDED_CRL_SUPPORT)) {
- return;
- }
-
- // Otherwise the CRL issuer is not on the path. Look for it in the set of
- // untrusted certificates.
- for (i = 0; i < sk_X509_num(ctx->untrusted); i++) {
- crl_issuer = sk_X509_value(ctx->untrusted, i);
- if (X509_NAME_cmp(X509_get_subject_name(crl_issuer), cnm)) {
- continue;
- }
- if (X509_check_akid(crl_issuer, crl->akid) == X509_V_OK) {
- *pissuer = crl_issuer;
- *pcrl_score |= CRL_SCORE_AKID;
- return;
- }
- }
}
// Check the path of a CRL issuer certificate. This creates a new
diff --git a/include/openssl/x509.h b/include/openssl/x509.h
index 6c34f41..ebbf1ff 100644
--- a/include/openssl/x509.h
+++ b/include/openssl/x509.h
@@ -2806,9 +2806,9 @@
#define X509_V_FLAG_INHIBIT_MAP 0x400
// Notify callback that policy is OK
#define X509_V_FLAG_NOTIFY_POLICY 0x800
-// Extended CRL features such as indirect CRLs, alternate CRL signing keys
+// Causes all verifications to fail. Extended CRL features have been removed.
#define X509_V_FLAG_EXTENDED_CRL_SUPPORT 0x1000
-// Delta CRL support
+// Causes all verifications to fail. Delta CRL support has been removed.
#define X509_V_FLAG_USE_DELTAS 0x2000
// Check selfsigned CA signature
#define X509_V_FLAG_CHECK_SS_SIGNATURE 0x4000
