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>
chromium-stable
David Benjamin 1 year ago committed by Boringssl LUCI CQ
parent 580c04109e
commit 8cae3d04ed
  1. 1
      crypto/x509/internal.h
  2. 63
      crypto/x509/x509_vfy.c

@ -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

@ -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;
}

Loading…
Cancel
Save