Don't parse delta CRL and CRL number extensions These extensions were only parsed for delta CRL support. Instead, ignore CRL numbers and treat critical delta CRL extensions as unrecognized critical extensions. This trims a pile of dead, untested code from the verifier. Update-Note: While this is broadly a no-op, this may change behavior slightly at the edges. Invalid CRL number extensions will now be ignored instead of treated as a parse error. A delta CRL that incorrectly marks its delta CRL extension as non-critical will be interpreted as a normal CRL. (This is the expected behavior for an implementation which does not implement delta CRLs. Extensions like this are supposed to be marked critical.) Bug: 601 Change-Id: Iece9a8acce83a7c59e129499517fc8eb0d048e98 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63931 Auto-Submit: David Benjamin <davidben@google.com> 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 408698b..72d0f9b 100644 --- a/crypto/x509/internal.h +++ b/crypto/x509/internal.h
@@ -219,9 +219,6 @@ // Convenient breakdown of IDP int idp_flags; int idp_reasons; - // CRL and base CRL numbers for delta processing - ASN1_INTEGER *crl_number; - ASN1_INTEGER *base_crl_number; unsigned char crl_hash[SHA256_DIGEST_LENGTH]; STACK_OF(GENERAL_NAMES) *issuers; } /* X509_CRL */;
diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index f203da9..89e95aa 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c
@@ -1046,12 +1046,6 @@ if (crl->idp_flags & (IDP_INDIRECT | IDP_REASONS)) { return 0; } - 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 if (X509_NAME_cmp(X509_get_issuer_name(x), X509_CRL_get_issuer(crl))) { if (!(crl->idp_flags & IDP_INDIRECT)) { @@ -1306,9 +1300,7 @@ return 0; } -// Retrieve CRL corresponding to current certificate. If deltas enabled try -// to find a delta CRL too - +// Retrieve CRL corresponding to current certificate. static int get_crl_delta(X509_STORE_CTX *ctx, X509_CRL **pcrl, X509_CRL **pdcrl, X509 *x) { int ok; @@ -1326,7 +1318,6 @@ } // Lookup CRLs from store - skcrl = ctx->lookup_crls(ctx, nm); // If no CRLs found and a near match from get_crl_sk use that @@ -1382,42 +1373,39 @@ } if (issuer) { - // Skip most tests for deltas because they have already been done - if (!crl->base_crl_number) { - // Check for cRLSign bit if keyUsage present - if ((issuer->ex_flags & EXFLAG_KUSAGE) && - !(issuer->ex_kusage & KU_CRL_SIGN)) { - ctx->error = X509_V_ERR_KEYUSAGE_NO_CRL_SIGN; + // Check for cRLSign bit if keyUsage present + if ((issuer->ex_flags & EXFLAG_KUSAGE) && + !(issuer->ex_kusage & KU_CRL_SIGN)) { + ctx->error = X509_V_ERR_KEYUSAGE_NO_CRL_SIGN; + ok = ctx->verify_cb(0, ctx); + if (!ok) { + goto err; + } + } + + if (!(ctx->current_crl_score & CRL_SCORE_SCOPE)) { + ctx->error = X509_V_ERR_DIFFERENT_CRL_SCOPE; + ok = ctx->verify_cb(0, ctx); + if (!ok) { + goto err; + } + } + + if (!(ctx->current_crl_score & CRL_SCORE_SAME_PATH)) { + if (check_crl_path(ctx, ctx->current_issuer) <= 0) { + ctx->error = X509_V_ERR_CRL_PATH_VALIDATION_ERROR; ok = ctx->verify_cb(0, ctx); if (!ok) { goto err; } } + } - if (!(ctx->current_crl_score & CRL_SCORE_SCOPE)) { - ctx->error = X509_V_ERR_DIFFERENT_CRL_SCOPE; - ok = ctx->verify_cb(0, ctx); - if (!ok) { - goto err; - } - } - - if (!(ctx->current_crl_score & CRL_SCORE_SAME_PATH)) { - if (check_crl_path(ctx, ctx->current_issuer) <= 0) { - ctx->error = X509_V_ERR_CRL_PATH_VALIDATION_ERROR; - ok = ctx->verify_cb(0, ctx); - if (!ok) { - goto err; - } - } - } - - if (crl->idp_flags & IDP_INVALID) { - ctx->error = X509_V_ERR_INVALID_EXTENSION; - ok = ctx->verify_cb(0, ctx); - if (!ok) { - goto err; - } + if (crl->idp_flags & IDP_INVALID) { + ctx->error = X509_V_ERR_INVALID_EXTENSION; + ok = ctx->verify_cb(0, ctx); + if (!ok) { + goto err; } }
diff --git a/crypto/x509/x_crl.c b/crypto/x509/x_crl.c index 227867b..37aed82 100644 --- a/crypto/x509/x_crl.c +++ b/crypto/x509/x_crl.c
@@ -201,8 +201,6 @@ crl->idp_flags = 0; crl->idp_reasons = CRLDP_ALL_REASONS; crl->issuers = NULL; - crl->crl_number = NULL; - crl->base_crl_number = NULL; break; case ASN1_OP_D2I_POST: { @@ -245,21 +243,6 @@ return 0; } - crl->crl_number = X509_CRL_get_ext_d2i(crl, NID_crl_number, &i, NULL); - if (crl->crl_number == NULL && i != -1) { - return 0; - } - - crl->base_crl_number = X509_CRL_get_ext_d2i(crl, NID_delta_crl, &i, NULL); - if (crl->base_crl_number == NULL && i != -1) { - return 0; - } - // Delta CRLs must have CRL number - if (crl->base_crl_number && !crl->crl_number) { - OPENSSL_PUT_ERROR(X509, X509_R_DELTA_CRL_WITHOUT_CRL_NUMBER); - return 0; - } - // See if we have any unhandled critical CRL extensions and indicate // this in a flag. We only currently handle IDP so anything else // critical sets the flag. This code accesses the X509_CRL structure @@ -269,9 +252,8 @@ const X509_EXTENSION *ext = sk_X509_EXTENSION_value(exts, idx); int nid = OBJ_obj2nid(X509_EXTENSION_get_object(ext)); if (X509_EXTENSION_get_critical(ext)) { - // We handle IDP and deltas if (nid == NID_issuing_distribution_point || - nid == NID_authority_key_identifier || nid == NID_delta_crl) { + nid == NID_authority_key_identifier) { continue; } crl->flags |= EXFLAG_CRITICAL; @@ -289,8 +271,6 @@ case ASN1_OP_FREE_POST: AUTHORITY_KEYID_free(crl->akid); ISSUING_DIST_POINT_free(crl->idp); - ASN1_INTEGER_free(crl->crl_number); - ASN1_INTEGER_free(crl->base_crl_number); sk_GENERAL_NAMES_pop_free(crl->issuers, GENERAL_NAMES_free); break; }