From 8cae3d04ed7264ae99024caff4da79e9a796b915 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 12 Nov 2023 14:42:35 -0500 Subject: [PATCH] Remove the now no-op CRL reasons loop Now that we only process CRLs that cover all reasons: 1. A successful get_crl will always set current_reasons to CRLDP_ALL_REASONS. 2. The last_reasons == current_reasons will never happen. 3. The loop always makes exactly one iteration. A footnote on point 1: it is also possible for the caller to override get_crl. In that case, the caller's get_crl was previously responsible for setting current_reasons, but there was no way to do so. In reality, that callback was actually impossible to use correctly. See https://github.com/openssl/openssl/issues/21679 and https://github.com/openssl/openssl/issues/10211. I previously attempted to remove those first, but gRPC did not notice it was unusable and use it anyway. Instead, they're suppressing X509_V_ERR_UNABLE_TO_GET_CRL via the callback, which is probably working around the bug in their get_crl implementation. Later, when we tackle the callback, we'll probably need to unwind the gRPC mess and, in the process, add a X509_STORE_CTX_set_current_reasons for them to call for OpenSSL compatibility. For now, this change has the side effect of removing the need for them to call that. Bug: 601 Change-Id: Icc5c0fb195d9f66991d0e560911f304e82afa5fd Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63936 Reviewed-by: Bob Beck Commit-Queue: David Benjamin --- crypto/x509/internal.h | 1 - crypto/x509/x509_vfy.c | 63 +++++++++++++++--------------------------- 2 files changed, 22 insertions(+), 42 deletions(-) diff --git a/crypto/x509/internal.h b/crypto/x509/internal.h index e56f243b0..47701a8de 100644 --- a/crypto/x509/internal.h +++ b/crypto/x509/internal.h @@ -349,7 +349,6 @@ struct x509_store_ctx_st { X509_CRL *current_crl; // current CRL int current_crl_score; // score of current CRL - unsigned int current_reasons; // Reason mask X509_STORE_CTX *parent; // For CRL path validation: parent context diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index 1c6529d82..9210dcc89 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -825,49 +825,33 @@ static int check_revocation(X509_STORE_CTX *ctx) { static int check_cert(X509_STORE_CTX *ctx) { X509_CRL *crl = NULL; - X509 *x; - int ok = 0, cnum; - unsigned int last_reasons; - cnum = ctx->error_depth; - x = sk_X509_value(ctx->chain, cnum); + int ok = 0, cnum = ctx->error_depth; + X509 *x = sk_X509_value(ctx->chain, cnum); ctx->current_cert = x; ctx->current_issuer = NULL; ctx->current_crl_score = 0; - ctx->current_reasons = 0; - while (ctx->current_reasons != CRLDP_ALL_REASONS) { - last_reasons = ctx->current_reasons; - // Try to retrieve relevant CRL - if (ctx->get_crl) { - ok = ctx->get_crl(ctx, &crl, x); - } else { - ok = get_crl(ctx, &crl, x); - } - // If error looking up CRL, nothing we can do except notify callback - if (!ok) { - ctx->error = X509_V_ERR_UNABLE_TO_GET_CRL; - ok = ctx->verify_cb(0, ctx); - goto err; - } - ctx->current_crl = crl; - ok = ctx->check_crl(ctx, crl); - if (!ok) { - goto err; - } - ok = ctx->cert_crl(ctx, crl, x); - if (!ok) { - goto err; - } + // Try to retrieve relevant CRL + if (ctx->get_crl) { + ok = ctx->get_crl(ctx, &crl, x); + } else { + ok = get_crl(ctx, &crl, x); + } + // If error looking up CRL, nothing we can do except notify callback + if (!ok) { + ctx->error = X509_V_ERR_UNABLE_TO_GET_CRL; + ok = ctx->verify_cb(0, ctx); + goto err; + } + ctx->current_crl = crl; + ok = ctx->check_crl(ctx, crl); + if (!ok) { + goto err; + } - X509_CRL_free(crl); - crl = NULL; - // If reasons not updated we wont get anywhere by another iteration, - // so exit loop. - if (last_reasons == ctx->current_reasons) { - ctx->error = X509_V_ERR_UNABLE_TO_GET_CRL; - ok = ctx->verify_cb(0, ctx); - goto err; - } + ok = ctx->cert_crl(ctx, crl, x); + if (!ok) { + goto err; } err: @@ -1264,7 +1248,6 @@ static int get_crl(X509_STORE_CTX *ctx, X509_CRL **pcrl, X509 *x) { STACK_OF(X509_CRL) *skcrl; X509_NAME *nm = X509_get_issuer_name(x); ok = get_crl_sk(ctx, &crl, &issuer, &crl_score, ctx->crls); - if (ok) { goto done; } @@ -1287,8 +1270,6 @@ done: if (crl) { ctx->current_issuer = issuer; ctx->current_crl_score = crl_score; - // TODO(crbug.com/boringssl/601): Clean up remnants of partitioned CRLs. - ctx->current_reasons = CRLDP_ALL_REASONS; *pcrl = crl; return 1; }