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 <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/x509/internal.h b/crypto/x509/internal.h
index e56f243..47701a8 100644
--- a/crypto/x509/internal.h
+++ b/crypto/x509/internal.h
@@ -349,7 +349,6 @@
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 1c6529d..9210dcc 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -825,49 +825,33 @@
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 @@
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 @@
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;
}
