From ee730c8683b0223ab9a3c4a29c27b9b3bdd58432 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 12 Nov 2023 13:43:33 -0500 Subject: [PATCH] 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 Auto-Submit: David Benjamin Commit-Queue: David Benjamin (cherry picked from commit f86149982323e57050f853c278ce8aa955b681dc) --- crypto/x509/x509_test.cc | 8 ++ crypto/x509/x509_vfy.c | 158 +++++---------------------------------- include/openssl/x509.h | 7 +- 3 files changed, 31 insertions(+), 142 deletions(-) diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index e0283dc70f6..e7d925baf27 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -1931,6 +1931,14 @@ TEST(X509Test, TestCRL) { 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 dadf594b8d4..a26fdcce1ac 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -128,8 +128,6 @@ static int get_crl_score(X509_STORE_CTX *ctx, X509 **pissuer, 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 @@ int X509_verify_cert(X509_STORE_CTX *ctx) { 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 @@ int X509_verify_cert(X509_STORE_CTX *ctx) { 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(); @@ -1014,11 +1023,11 @@ static int get_crl_sk(X509_STORE_CTX *ctx, X509_CRL **pcrl, X509_CRL **pdcrl, *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) { @@ -1028,109 +1037,6 @@ static int get_crl_sk(X509_STORE_CTX *ctx, X509_CRL **pcrl, X509_CRL **pdcrl, 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 @@ -1148,19 +1054,14 @@ static int get_crl_score(X509_STORE_CTX *ctx, X509 **pissuer, 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 @@ -1211,7 +1112,6 @@ static void crl_akid_check(X509_STORE_CTX *ctx, X509_CRL *crl, X509 **pissuer, 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++; @@ -1238,26 +1138,6 @@ static void crl_akid_check(X509_STORE_CTX *ctx, X509_CRL *crl, X509 **pissuer, 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 d3711b41637..fa9c5ee073e 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -2842,10 +2842,11 @@ OPENSSL_EXPORT void X509_STORE_CTX_set_depth(X509_STORE_CTX *ctx, int depth); #define X509_V_FLAG_INHIBIT_MAP 0x400 // X509_V_FLAG_NOTIFY_POLICY notifies the callback that the policy is OK #define X509_V_FLAG_NOTIFY_POLICY 0x800 -// X509_V_FLAG_EXTENDED_CRL_SUPPORT enables extended CRL features such as -// indirect CRLs, alternate CRL signing keys. +// X509_V_FLAG_EXTENDED_CRL_SUPPORT causes all verifications to fail. Extended +// CRL features have been removed. #define X509_V_FLAG_EXTENDED_CRL_SUPPORT 0x1000 -// X509_V_FLAG_USE_DELTAS enables Delta CRL support. +// X509_V_FLAG_USE_DELTAS causes all verifications to fail. Delta CRL support +// has been removed. #define X509_V_FLAG_USE_DELTAS 0x2000 // X509_V_FLAG_CHECK_SS_SIGNATURE enables checking the self signed CA signature. #define X509_V_FLAG_CHECK_SS_SIGNATURE 0x4000