diff --git a/crypto/x509/internal.h b/crypto/x509/internal.h index 420e61cd67..5de3ed8daa 100644 --- a/crypto/x509/internal.h +++ b/crypto/x509/internal.h @@ -185,8 +185,6 @@ struct x509_revoked_st { ASN1_INTEGER *serialNumber; ASN1_TIME *revocationDate; STACK_OF(X509_EXTENSION) /* optional */ *extensions; - // Set up if indirect CRL - STACK_OF(GENERAL_NAME) *issuer; // Revocation reason int reason; } /* X509_REVOKED */; @@ -206,6 +204,22 @@ typedef struct { // an |X509_NAME|. DECLARE_ASN1_FUNCTIONS(X509_CRL_INFO) +// Values in idp_flags field +// IDP present +#define IDP_PRESENT 0x1 +// IDP values inconsistent +#define IDP_INVALID 0x2 +// onlyuser true +#define IDP_ONLYUSER 0x4 +// onlyCA true +#define IDP_ONLYCA 0x8 +// onlyattr true +#define IDP_ONLYATTR 0x10 +// indirectCRL true +#define IDP_INDIRECT 0x20 +// onlysomereasons present +#define IDP_REASONS 0x40 + struct X509_crl_st { // actual signature X509_CRL_INFO *crl; @@ -218,12 +232,7 @@ struct X509_crl_st { ISSUING_DIST_POINT *idp; // 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 */; struct X509_VERIFY_PARAM_st { @@ -353,9 +362,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 CRYPTO_EX_DATA ex_data; } /* X509_STORE_CTX */; diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index a26fdcce1a..2213a139dd 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -77,42 +77,30 @@ static CRYPTO_EX_DATA_CLASS g_ex_data_class = // CRL score values // No unhandled critical extensions - #define CRL_SCORE_NOCRITICAL 0x100 // certificate is within CRL scope - #define CRL_SCORE_SCOPE 0x080 // CRL times valid - #define CRL_SCORE_TIME 0x040 // Issuer name matches certificate - #define CRL_SCORE_ISSUER_NAME 0x020 // If this score or above CRL is probably valid - #define CRL_SCORE_VALID \ (CRL_SCORE_NOCRITICAL | CRL_SCORE_TIME | CRL_SCORE_SCOPE) // CRL issuer is certificate issuer - #define CRL_SCORE_ISSUER_CERT 0x018 // CRL issuer is on certificate path - #define CRL_SCORE_SAME_PATH 0x008 // CRL issuer matches CRL AKID - #define CRL_SCORE_AKID 0x004 -// Have a delta CRL with valid times - -#define CRL_SCORE_TIME_DELTA 0x002 - static int null_callback(int ok, X509_STORE_CTX *e); static int check_issued(X509_STORE_CTX *ctx, X509 *x, X509 *issuer); static X509 *find_issuer(X509_STORE_CTX *ctx, STACK_OF(X509) *sk, X509 *x); @@ -124,17 +112,12 @@ static int check_revocation(X509_STORE_CTX *ctx); static int check_cert(X509_STORE_CTX *ctx); static int check_policy(X509_STORE_CTX *ctx); -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, +static int get_crl_score(X509_STORE_CTX *ctx, X509 **pissuer, X509_CRL *crl, X509 *x); -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, - unsigned int *preasons); -static int check_crl_path(X509_STORE_CTX *ctx, X509 *x); -static int check_crl_chain(X509_STORE_CTX *ctx, STACK_OF(X509) *cert_path, - STACK_OF(X509) *crl_path); +static int get_crl(X509_STORE_CTX *ctx, X509_CRL **pcrl, X509 *x); +static int 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); static int internal_verify(X509_STORE_CTX *ctx); @@ -559,10 +542,7 @@ static int get_issuer_sk(X509 **issuer, X509_STORE_CTX *ctx, X509 *x) { static int check_chain_extensions(X509_STORE_CTX *ctx) { int ok = 0, plen = 0; - - // If |ctx->parent| is set, this is CRL path validation. - int purpose = - ctx->parent == NULL ? ctx->param->purpose : X509_PURPOSE_CRL_SIGN; + int purpose = ctx->param->purpose; // Check all untrusted certificates for (int i = 0; i < ctx->last_untrusted; i++) { @@ -821,10 +801,6 @@ static int check_revocation(X509_STORE_CTX *ctx) { if (ctx->param->flags & X509_V_FLAG_CRL_CHECK_ALL) { last = (int)sk_X509_num(ctx->chain) - 1; } else { - // If checking CRL paths this isn't the EE certificate - if (ctx->parent) { - return 1; - } last = 0; } for (int i = 0; i <= last; i++) { @@ -838,79 +814,43 @@ static int check_revocation(X509_STORE_CTX *ctx) { } static int check_cert(X509_STORE_CTX *ctx) { - X509_CRL *crl = NULL, *dcrl = NULL; - X509 *x; - int ok = 0, cnum; - unsigned int last_reasons; - cnum = ctx->error_depth; - x = sk_X509_value(ctx->chain, cnum); + X509_CRL *crl = NULL; + 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_delta(ctx, &crl, &dcrl, 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; - } - - if (dcrl) { - ok = ctx->check_crl(ctx, dcrl); - if (!ok) { - goto err; - } - ok = ctx->cert_crl(ctx, dcrl, x); - if (!ok) { - goto err; - } - } else { - ok = 1; - } - // Don't look in full CRL if delta reason is removefromCRL - if (ok != 2) { - 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); - X509_CRL_free(dcrl); - crl = NULL; - dcrl = 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: X509_CRL_free(crl); - X509_CRL_free(dcrl); - ctx->current_crl = NULL; return ok; } // Check CRL times against values in X509_STORE_CTX - static int check_crl_time(X509_STORE_CTX *ctx, X509_CRL *crl, int notify) { if (ctx->param->flags & X509_V_FLAG_NO_CHECK_TIME) { return 1; @@ -959,8 +899,7 @@ static int check_crl_time(X509_STORE_CTX *ctx, X509_CRL *crl, int notify) { return 0; } } - // Ignore expiry of base CRL is delta is valid - if ((i < 0) && !(ctx->current_crl_score & CRL_SCORE_TIME_DELTA)) { + if (i < 0) { if (!notify) { return 0; } @@ -978,20 +917,16 @@ static int check_crl_time(X509_STORE_CTX *ctx, X509_CRL *crl, int notify) { return 1; } -static int get_crl_sk(X509_STORE_CTX *ctx, X509_CRL **pcrl, X509_CRL **pdcrl, - X509 **pissuer, int *pscore, unsigned int *preasons, - STACK_OF(X509_CRL) *crls) { +static int get_crl_sk(X509_STORE_CTX *ctx, X509_CRL **pcrl, X509 **pissuer, + int *pscore, STACK_OF(X509_CRL) *crls) { int crl_score, best_score = *pscore; - size_t i; - unsigned int reasons, best_reasons = 0; X509 *x = ctx->current_cert; - X509_CRL *crl, *best_crl = NULL; + X509_CRL *best_crl = NULL; X509 *crl_issuer = NULL, *best_crl_issuer = NULL; - for (i = 0; i < sk_X509_CRL_num(crls); i++) { - crl = sk_X509_CRL_value(crls, i); - reasons = *preasons; - crl_score = get_crl_score(ctx, &crl_issuer, &reasons, crl, x); + for (size_t i = 0; i < sk_X509_CRL_num(crls); i++) { + X509_CRL *crl = sk_X509_CRL_value(crls, i); + crl_score = get_crl_score(ctx, &crl_issuer, crl, x); if (crl_score < best_score || crl_score == 0) { continue; } @@ -1011,7 +946,6 @@ static int get_crl_sk(X509_STORE_CTX *ctx, X509_CRL **pcrl, X509_CRL **pdcrl, best_crl = crl; best_crl_issuer = crl_issuer; best_score = crl_score; - best_reasons = reasons; } if (best_crl) { @@ -1021,13 +955,7 @@ static int get_crl_sk(X509_STORE_CTX *ctx, X509_CRL **pcrl, X509_CRL **pdcrl, *pcrl = best_crl; *pissuer = best_crl_issuer; *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; - } } if (best_score >= CRL_SCORE_VALID) { @@ -1039,14 +967,10 @@ static int get_crl_sk(X509_STORE_CTX *ctx, X509_CRL **pcrl, X509_CRL **pdcrl, // 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 -// also used to determine if the CRL is suitable: if no new reasons the CRL -// is rejected, otherwise reasons is updated. - -static int get_crl_score(X509_STORE_CTX *ctx, X509 **pissuer, - unsigned int *preasons, X509_CRL *crl, X509 *x) { +// the certificate issuer this is returned in *pissuer. +static int get_crl_score(X509_STORE_CTX *ctx, X509 **pissuer, X509_CRL *crl, + X509 *x) { int crl_score = 0; - unsigned int tmp_reasons = *preasons, crl_reasons; // First see if we can reject CRL straight away @@ -1058,20 +982,11 @@ static int get_crl_score(X509_STORE_CTX *ctx, X509 **pissuer, 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 + // We do not support indirect CRLs, so the issuer names must match. if (X509_NAME_cmp(X509_get_issuer_name(x), X509_CRL_get_issuer(crl))) { - if (!(crl->idp_flags & IDP_INDIRECT)) { - return 0; - } - } else { - crl_score |= CRL_SCORE_ISSUER_NAME; + return 0; } + crl_score |= CRL_SCORE_ISSUER_NAME; if (!(crl->flags & EXFLAG_CRITICAL)) { crl_score |= CRL_SCORE_NOCRITICAL; @@ -1083,32 +998,21 @@ static int get_crl_score(X509_STORE_CTX *ctx, X509 **pissuer, } // Check authority key ID and locate certificate issuer - crl_akid_check(ctx, crl, pissuer, &crl_score); - - // If we can't locate certificate issuer at this point forget it - - if (!(crl_score & CRL_SCORE_AKID)) { + if (!crl_akid_check(ctx, crl, pissuer, &crl_score)) { + // If we can't locate certificate issuer at this point forget it return 0; } // Check cert for matching CRL distribution points - - if (crl_crldp_check(x, crl, crl_score, &crl_reasons)) { - // If no new reasons reject - if (!(crl_reasons & ~tmp_reasons)) { - return 0; - } - tmp_reasons |= crl_reasons; + if (crl_crldp_check(x, crl, crl_score)) { crl_score |= CRL_SCORE_SCOPE; } - *preasons = tmp_reasons; - return crl_score; } -static void crl_akid_check(X509_STORE_CTX *ctx, X509_CRL *crl, X509 **pissuer, - int *pcrl_score) { +static int crl_akid_check(X509_STORE_CTX *ctx, X509_CRL *crl, X509 **pissuer, + int *pcrl_score) { X509 *crl_issuer = NULL; X509_NAME *cnm = X509_CRL_get_issuer(crl); int cidx = ctx->error_depth; @@ -1120,11 +1024,9 @@ static void crl_akid_check(X509_STORE_CTX *ctx, X509_CRL *crl, X509 **pissuer, crl_issuer = sk_X509_value(ctx->chain, cidx); if (X509_check_akid(crl_issuer, crl->akid) == X509_V_OK) { - if (*pcrl_score & CRL_SCORE_ISSUER_NAME) { - *pcrl_score |= CRL_SCORE_AKID | CRL_SCORE_ISSUER_CERT; - *pissuer = crl_issuer; - return; - } + *pcrl_score |= CRL_SCORE_AKID | CRL_SCORE_ISSUER_CERT; + *pissuer = crl_issuer; + return 1; } for (cidx++; cidx < (int)sk_X509_num(ctx->chain); cidx++) { @@ -1135,64 +1037,10 @@ static void crl_akid_check(X509_STORE_CTX *ctx, X509_CRL *crl, X509 **pissuer, if (X509_check_akid(crl_issuer, crl->akid) == X509_V_OK) { *pcrl_score |= CRL_SCORE_AKID | CRL_SCORE_SAME_PATH; *pissuer = crl_issuer; - return; + return 1; } } -} -// Check the path of a CRL issuer certificate. This creates a new -// X509_STORE_CTX and populates it with most of the parameters from the -// parent. This could be optimised somewhat since a lot of path checking will -// be duplicated by the parent, but this will rarely be used in practice. - -static int check_crl_path(X509_STORE_CTX *ctx, X509 *x) { - X509_STORE_CTX crl_ctx; - int ret; - // Don't allow recursive CRL path validation - if (ctx->parent) { - return 0; - } - if (!X509_STORE_CTX_init(&crl_ctx, ctx->ctx, x, ctx->untrusted)) { - return -1; - } - - crl_ctx.crls = ctx->crls; - // Copy verify params across - X509_STORE_CTX_set0_param(&crl_ctx, ctx->param); - - crl_ctx.parent = ctx; - crl_ctx.verify_cb = ctx->verify_cb; - - // Verify CRL issuer - ret = X509_verify_cert(&crl_ctx); - - if (ret <= 0) { - goto err; - } - - // Check chain is acceptable - - ret = check_crl_chain(ctx, ctx->chain, crl_ctx.chain); -err: - X509_STORE_CTX_cleanup(&crl_ctx); - return ret; -} - -// RFC 3280 says nothing about the relationship between CRL path and -// certificate path, which could lead to situations where a certificate could -// be revoked or validated by a CA not authorised to do so. RFC 5280 is more -// strict and states that the two paths must end in the same trust anchor, -// though some discussions remain... until this is resolved we use the -// RFC 5280 version - -static int check_crl_chain(X509_STORE_CTX *ctx, STACK_OF(X509) *cert_path, - STACK_OF(X509) *crl_path) { - X509 *cert_ta, *crl_ta; - cert_ta = sk_X509_value(cert_path, sk_X509_num(cert_path) - 1); - crl_ta = sk_X509_value(crl_path, sk_X509_num(crl_path) - 1); - if (!X509_cmp(cert_ta, crl_ta)) { - return 1; - } return 0; } @@ -1200,7 +1048,6 @@ static int check_crl_chain(X509_STORE_CTX *ctx, STACK_OF(X509) *cert_path, // Both are relative names and compare X509_NAME types. 2. One full, one // relative. Compare X509_NAME to GENERAL_NAMES. 3. Both are full names and // compare two GENERAL_NAMES. 4. One is NULL: automatic match. - static int idp_check_dp(DIST_POINT_NAME *a, DIST_POINT_NAME *b) { X509_NAME *nm = NULL; GENERAL_NAMES *gens = NULL; @@ -1265,30 +1112,8 @@ static int idp_check_dp(DIST_POINT_NAME *a, DIST_POINT_NAME *b) { return 0; } -static int crldp_check_crlissuer(DIST_POINT *dp, X509_CRL *crl, int crl_score) { - size_t i; - X509_NAME *nm = X509_CRL_get_issuer(crl); - // If no CRLissuer return is successful iff don't need a match - if (!dp->CRLissuer) { - return !!(crl_score & CRL_SCORE_ISSUER_NAME); - } - for (i = 0; i < sk_GENERAL_NAME_num(dp->CRLissuer); i++) { - GENERAL_NAME *gen = sk_GENERAL_NAME_value(dp->CRLissuer, i); - if (gen->type != GEN_DIRNAME) { - continue; - } - if (!X509_NAME_cmp(gen->d.directoryName, nm)) { - return 1; - } - } - return 0; -} - // Check CRLDP and IDP - -static int crl_crldp_check(X509 *x, X509_CRL *crl, int crl_score, - unsigned int *preasons) { - size_t i; +static int crl_crldp_check(X509 *x, X509_CRL *crl, int crl_score) { if (crl->idp_flags & IDP_ONLYATTR) { return 0; } @@ -1301,44 +1126,44 @@ static int crl_crldp_check(X509 *x, X509_CRL *crl, int crl_score, return 0; } } - *preasons = crl->idp_reasons; - for (i = 0; i < sk_DIST_POINT_num(x->crldp); i++) { + for (size_t i = 0; i < sk_DIST_POINT_num(x->crldp); i++) { DIST_POINT *dp = sk_DIST_POINT_value(x->crldp, i); - if (crldp_check_crlissuer(dp, crl, crl_score)) { - if (!crl->idp || idp_check_dp(dp->distpoint, crl->idp->distpoint)) { - *preasons &= dp->dp_reasons; - return 1; - } + // Skip distribution points with a reasons field or a CRL issuer: + // + // We do not support CRLs partitioned by reason code. RFC 5280 requires CAs + // include at least one DistributionPoint that covers all reasons. + // + // We also do not support indirect CRLs, and a CRL issuer can only match + // indirect CRLs (RFC 5280, section 6.3.3, step b.1). + // support. + if (dp->reasons != NULL && dp->CRLissuer != NULL && + (!crl->idp || idp_check_dp(dp->distpoint, crl->idp->distpoint))) { + return 1; } } - if ((!crl->idp || !crl->idp->distpoint) && - (crl_score & CRL_SCORE_ISSUER_NAME)) { - return 1; - } - return 0; -} -// Retrieve CRL corresponding to current certificate. If deltas enabled try -// to find a delta CRL too + // If the CRL does not specify an issuing distribution point, allow it to + // match anything. + // + // TODO(davidben): Does this match RFC 5280? It's hard to follow because RFC + // 5280 starts from distribution points, while this starts from CRLs. + return !crl->idp || !crl->idp->distpoint; +} -static int get_crl_delta(X509_STORE_CTX *ctx, X509_CRL **pcrl, X509_CRL **pdcrl, - X509 *x) { +// Retrieve CRL corresponding to current certificate. +static int get_crl(X509_STORE_CTX *ctx, X509_CRL **pcrl, X509 *x) { int ok; X509 *issuer = NULL; int crl_score = 0; - unsigned int reasons; - X509_CRL *crl = NULL, *dcrl = NULL; + X509_CRL *crl = NULL; STACK_OF(X509_CRL) *skcrl; X509_NAME *nm = X509_get_issuer_name(x); - reasons = ctx->current_reasons; - ok = get_crl_sk(ctx, &crl, &dcrl, &issuer, &crl_score, &reasons, ctx->crls); - + ok = get_crl_sk(ctx, &crl, &issuer, &crl_score, ctx->crls); if (ok) { goto done; } // Lookup CRLs from store - skcrl = ctx->lookup_crls(ctx, nm); // If no CRLs found and a near match from get_crl_sk use that @@ -1346,7 +1171,7 @@ static int get_crl_delta(X509_STORE_CTX *ctx, X509_CRL **pcrl, X509_CRL **pdcrl, goto done; } - get_crl_sk(ctx, &crl, &dcrl, &issuer, &crl_score, &reasons, skcrl); + get_crl_sk(ctx, &crl, &issuer, &crl_score, skcrl); sk_X509_CRL_pop_free(skcrl, X509_CRL_free); @@ -1356,9 +1181,7 @@ static int get_crl_delta(X509_STORE_CTX *ctx, X509_CRL **pcrl, X509_CRL **pdcrl, if (crl) { ctx->current_issuer = issuer; ctx->current_crl_score = crl_score; - ctx->current_reasons = reasons; *pcrl = crl; - *pdcrl = dcrl; return 1; } @@ -1394,42 +1217,29 @@ static int check_crl(X509_STORE_CTX *ctx, X509_CRL *crl) { } 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; - 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; - } + // 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_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 (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; } } @@ -1484,12 +1294,8 @@ static int cert_crl(X509_STORE_CTX *ctx, X509_CRL *crl, X509 *x) { return 0; } } - // Look for serial number of certificate in CRL If found make sure reason - // is not removeFromCRL. + // Look for serial number of certificate in CRL. if (X509_CRL_get0_by_cert(crl, &rev, x)) { - if (rev->reason == CRL_REASON_REMOVE_FROM_CRL) { - return 2; - } ctx->error = X509_V_ERR_CERT_REVOKED; ok = ctx->verify_cb(0, ctx); if (!ok) { @@ -1501,10 +1307,6 @@ static int cert_crl(X509_STORE_CTX *ctx, X509_CRL *crl, X509 *x) { } static int check_policy(X509_STORE_CTX *ctx) { - if (ctx->parent) { - return 1; - } - X509 *current_cert = NULL; int ret = X509_policy_check(ctx->chain, ctx->param->policies, ctx->param->flags, ¤t_cert); @@ -1773,7 +1575,10 @@ X509_CRL *X509_STORE_CTX_get0_current_crl(X509_STORE_CTX *ctx) { } X509_STORE_CTX *X509_STORE_CTX_get0_parent_ctx(X509_STORE_CTX *ctx) { - return ctx->parent; + // In OpenSSL, an |X509_STORE_CTX| sometimes has a parent context during CRL + // path validation for indirect CRLs. We require the CRL to be issued + // somewhere along the certificate path, so this is always NULL. + return NULL; } void X509_STORE_CTX_set_cert(X509_STORE_CTX *ctx, X509 *x) { ctx->cert = x; } @@ -2000,16 +1805,10 @@ void X509_STORE_CTX_cleanup(X509_STORE_CTX *ctx) { ctx->cleanup(ctx); ctx->cleanup = NULL; } - if (ctx->param != NULL) { - if (ctx->parent == NULL) { - X509_VERIFY_PARAM_free(ctx->param); - } - ctx->param = NULL; - } - if (ctx->chain != NULL) { - sk_X509_pop_free(ctx->chain, X509_free); - ctx->chain = NULL; - } + X509_VERIFY_PARAM_free(ctx->param); + ctx->param = NULL; + sk_X509_pop_free(ctx->chain, X509_free); + ctx->chain = NULL; CRYPTO_free_ex_data(&g_ex_data_class, ctx, &(ctx->ex_data)); OPENSSL_memset(&ctx->ex_data, 0, sizeof(CRYPTO_EX_DATA)); } diff --git a/crypto/x509/x_crl.c b/crypto/x509/x_crl.c index 8bab2b0569..fe78c0c6ed 100644 --- a/crypto/x509/x_crl.c +++ b/crypto/x509/x_crl.c @@ -115,45 +115,15 @@ ASN1_SEQUENCE_enc(X509_CRL_INFO, enc, crl_inf_cb) = { ASN1_EXP_SEQUENCE_OF_OPT(X509_CRL_INFO, extensions, X509_EXTENSION, 0), } ASN1_SEQUENCE_END_enc(X509_CRL_INFO, X509_CRL_INFO) -// Set CRL entry issuer according to CRL certificate issuer extension. Check -// for unhandled critical CRL entry extensions. - -static int crl_set_issuers(X509_CRL *crl) { - size_t i, k; - int j; - GENERAL_NAMES *gens, *gtmp; - STACK_OF(X509_REVOKED) *revoked; - - revoked = X509_CRL_get_REVOKED(crl); - - gens = NULL; - for (i = 0; i < sk_X509_REVOKED_num(revoked); i++) { +static int crl_parse_entry_extensions(X509_CRL *crl) { + STACK_OF(X509_REVOKED) *revoked = X509_CRL_get_REVOKED(crl); + for (size_t i = 0; i < sk_X509_REVOKED_num(revoked); i++) { X509_REVOKED *rev = sk_X509_REVOKED_value(revoked, i); - STACK_OF(X509_EXTENSION) *exts; - ASN1_ENUMERATED *reason; - X509_EXTENSION *ext; - gtmp = X509_REVOKED_get_ext_d2i(rev, NID_certificate_issuer, &j, NULL); - if (!gtmp && (j != -1)) { - crl->flags |= EXFLAG_INVALID; - return 1; - } - - if (gtmp) { - gens = gtmp; - if (!crl->issuers) { - crl->issuers = sk_GENERAL_NAMES_new_null(); - if (!crl->issuers) { - return 0; - } - } - if (!sk_GENERAL_NAMES_push(crl->issuers, gtmp)) { - return 0; - } - } - rev->issuer = gens; - reason = X509_REVOKED_get_ext_d2i(rev, NID_crl_reason, &j, NULL); - if (!reason && (j != -1)) { + int crit; + ASN1_ENUMERATED *reason = + X509_REVOKED_get_ext_d2i(rev, NID_crl_reason, &crit, NULL); + if (!reason && crit != -1) { crl->flags |= EXFLAG_INVALID; return 1; } @@ -165,17 +135,11 @@ static int crl_set_issuers(X509_CRL *crl) { rev->reason = CRL_REASON_NONE; } - // Check for critical CRL entry extensions - - exts = rev->extensions; - - for (k = 0; k < sk_X509_EXTENSION_num(exts); k++) { - ext = sk_X509_EXTENSION_value(exts, k); + // We do not support any critical CRL entry extensions. + const STACK_OF(X509_EXTENSION) *exts = rev->extensions; + for (size_t j = 0; j < sk_X509_EXTENSION_num(exts); j++) { + const X509_EXTENSION *ext = sk_X509_EXTENSION_value(exts, j); if (X509_EXTENSION_get_critical(ext)) { - if (OBJ_obj2nid(X509_EXTENSION_get_object(ext)) == - NID_certificate_issuer) { - continue; - } crl->flags |= EXFLAG_CRITICAL; break; } @@ -190,9 +154,6 @@ static int crl_set_issuers(X509_CRL *crl) { static int crl_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it, void *exarg) { X509_CRL *crl = (X509_CRL *)*pval; - STACK_OF(X509_EXTENSION) *exts; - X509_EXTENSION *ext; - size_t idx; int i; switch (operation) { @@ -201,10 +162,6 @@ static int crl_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it, crl->akid = NULL; crl->flags = 0; 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: { @@ -247,39 +204,17 @@ static int crl_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it, 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 // directly: applications shouldn't do this. - - exts = crl->crl->extensions; - - for (idx = 0; idx < sk_X509_EXTENSION_num(exts); idx++) { - int nid; - ext = sk_X509_EXTENSION_value(exts, idx); - nid = OBJ_obj2nid(X509_EXTENSION_get_object(ext)); - if (nid == NID_freshest_crl) { - crl->flags |= EXFLAG_FRESHEST; - } + const STACK_OF(X509_EXTENSION) *exts = crl->crl->extensions; + for (size_t idx = 0; idx < sk_X509_EXTENSION_num(exts); idx++) { + 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)) { + if (nid == NID_issuing_distribution_point || + nid == NID_authority_key_identifier) { continue; } crl->flags |= EXFLAG_CRITICAL; @@ -287,7 +222,7 @@ static int crl_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it, } } - if (!crl_set_issuers(crl)) { + if (!crl_parse_entry_extensions(crl)) { return 0; } @@ -297,16 +232,15 @@ static int crl_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it, 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; } return 1; } // Convert IDP into a more convenient form - +// +// TODO(davidben): Each of these flags are already booleans, so this is not +// really more convenient. We can probably remove |idp_flags|. static int setup_idp(X509_CRL *crl, ISSUING_DIST_POINT *idp) { int idp_only = 0; // Set various flags according to IDP @@ -324,6 +258,11 @@ static int setup_idp(X509_CRL *crl, ISSUING_DIST_POINT *idp) { crl->idp_flags |= IDP_ONLYATTR; } + // Per RFC 5280, section 5.2.5, at most one of onlyContainsUserCerts, + // onlyContainsCACerts, and onlyContainsAttributeCerts may be true. + // + // TODO(crbug.com/boringssl/443): Move this check to the |ISSUING_DIST_POINT| + // parser. if (idp_only > 1) { crl->idp_flags |= IDP_INVALID; } @@ -334,15 +273,10 @@ static int setup_idp(X509_CRL *crl, ISSUING_DIST_POINT *idp) { if (idp->onlysomereasons) { crl->idp_flags |= IDP_REASONS; - if (idp->onlysomereasons->length > 0) { - crl->idp_reasons = idp->onlysomereasons->data[0]; - } - if (idp->onlysomereasons->length > 1) { - crl->idp_reasons |= (idp->onlysomereasons->data[1] << 8); - } - crl->idp_reasons &= CRLDP_ALL_REASONS; } + // TODO(davidben): The new verifier does not support nameRelativeToCRLIssuer. + // Remove this? return DIST_POINT_set_dpname(idp->distpoint, X509_CRL_get_issuer(crl)); } @@ -402,32 +336,7 @@ int X509_CRL_get0_by_cert(X509_CRL *crl, X509_REVOKED **ret, X509 *x) { static int crl_revoked_issuer_match(X509_CRL *crl, X509_NAME *nm, X509_REVOKED *rev) { - size_t i; - - if (!rev->issuer) { - if (!nm) { - return 1; - } - if (!X509_NAME_cmp(nm, X509_CRL_get_issuer(crl))) { - return 1; - } - return 0; - } - - if (!nm) { - nm = X509_CRL_get_issuer(crl); - } - - for (i = 0; i < sk_GENERAL_NAME_num(rev->issuer); i++) { - GENERAL_NAME *gen = sk_GENERAL_NAME_value(rev->issuer, i); - if (gen->type != GEN_DIRNAME) { - continue; - } - if (!X509_NAME_cmp(nm, gen->d.directoryName)) { - return 1; - } - } - return 0; + return nm == NULL || X509_NAME_cmp(nm, X509_CRL_get_issuer(crl)) == 0; } static struct CRYPTO_STATIC_MUTEX g_crl_sort_lock = CRYPTO_STATIC_MUTEX_INIT; @@ -468,9 +377,6 @@ static int crl_lookup(X509_CRL *crl, X509_REVOKED **ret, if (ret) { *ret = rev; } - if (rev->reason == CRL_REASON_REMOVE_FROM_CRL) { - return 2; - } return 1; } } diff --git a/crypto/x509v3/v3_purp.c b/crypto/x509v3/v3_purp.c index c25a8c26a2..1696225f53 100644 --- a/crypto/x509v3/v3_purp.c +++ b/crypto/x509v3/v3_purp.c @@ -351,23 +351,11 @@ int X509_supported_extension(const X509_EXTENSION *ex) { } static int setup_dp(X509 *x, DIST_POINT *dp) { - X509_NAME *iname = NULL; - size_t i; - if (dp->reasons) { - if (dp->reasons->length > 0) { - dp->dp_reasons = dp->reasons->data[0]; - } - if (dp->reasons->length > 1) { - dp->dp_reasons |= (dp->reasons->data[1] << 8); - } - dp->dp_reasons &= CRLDP_ALL_REASONS; - } else { - dp->dp_reasons = CRLDP_ALL_REASONS; - } if (!dp->distpoint || (dp->distpoint->type != 1)) { return 1; } - for (i = 0; i < sk_GENERAL_NAME_num(dp->CRLissuer); i++) { + X509_NAME *iname = NULL; + for (size_t i = 0; i < sk_GENERAL_NAME_num(dp->CRLissuer); i++) { GENERAL_NAME *gen = sk_GENERAL_NAME_value(dp->CRLissuer, i); if (gen->type == GEN_DIRNAME) { iname = gen->d.directoryName; @@ -554,9 +542,6 @@ int x509v3_cache_extensions(X509 *x) { for (j = 0; j < X509_get_ext_count(x); j++) { const X509_EXTENSION *ex = X509_get_ext(x, j); - if (OBJ_obj2nid(X509_EXTENSION_get_object(ex)) == NID_freshest_crl) { - x->ex_flags |= EXFLAG_FRESHEST; - } if (!X509_EXTENSION_get_critical(ex)) { continue; } diff --git a/include/openssl/x509.h b/include/openssl/x509.h index 22868eebcf..5f7ea65dec 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -578,9 +578,8 @@ OPENSSL_EXPORT const ASN1_TIME *X509_CRL_get0_nextUpdate(const X509_CRL *crl); OPENSSL_EXPORT X509_NAME *X509_CRL_get_issuer(const X509_CRL *crl); // X509_CRL_get0_by_serial finds the entry in |crl| whose serial number is -// |serial|. If found, it sets |*out| to the entry. It then returns two if the -// reason code is removeFromCRL and one if it was revoked. If not found, it -// returns zero. +// |serial|. If found, it sets |*out| to the entry and returns one. If not +// found, it returns zero. // // On success, |*out| continues to be owned by |crl|. It is an error to free or // otherwise modify |*out|. @@ -588,9 +587,6 @@ OPENSSL_EXPORT X509_NAME *X509_CRL_get_issuer(const X509_CRL *crl); // TODO(crbug.com/boringssl/600): Ideally |crl| would be const. It is broadly // thread-safe, but changes the order of entries in |crl|. It cannot be called // concurrently with |i2d_X509_CRL|. -// -// TODO(crbug.com/boringssl/601): removeFromCRL is part of delta CRLs. Remove -// this special case. OPENSSL_EXPORT int X509_CRL_get0_by_serial(X509_CRL *crl, X509_REVOKED **out, const ASN1_INTEGER *serial); @@ -2396,6 +2392,10 @@ OPENSSL_EXPORT int X509_NAME_get_text_by_OBJ(const X509_NAME *name, OPENSSL_EXPORT int X509_NAME_get_text_by_NID(const X509_NAME *name, int nid, char *buf, int len); +// X509_STORE_CTX_get0_parent_ctx returns NULL. +OPENSSL_EXPORT X509_STORE_CTX *X509_STORE_CTX_get0_parent_ctx( + X509_STORE_CTX *ctx); + // Private structures. @@ -3057,8 +3057,6 @@ OPENSSL_EXPORT int X509_STORE_CTX_get_error_depth(X509_STORE_CTX *ctx); OPENSSL_EXPORT X509 *X509_STORE_CTX_get_current_cert(X509_STORE_CTX *ctx); OPENSSL_EXPORT X509 *X509_STORE_CTX_get0_current_issuer(X509_STORE_CTX *ctx); OPENSSL_EXPORT X509_CRL *X509_STORE_CTX_get0_current_crl(X509_STORE_CTX *ctx); -OPENSSL_EXPORT X509_STORE_CTX *X509_STORE_CTX_get0_parent_ctx( - X509_STORE_CTX *ctx); // X509_STORE_CTX_get_chain returns the pointer to the verified chain if // verification was successful. If unsuccessful it may return null or a partial diff --git a/include/openssl/x509v3.h b/include/openssl/x509v3.h index 529cd6025a..22533b0da5 100644 --- a/include/openssl/x509v3.h +++ b/include/openssl/x509v3.h @@ -237,7 +237,6 @@ struct DIST_POINT_st { DIST_POINT_NAME *distpoint; ASN1_BIT_STRING *reasons; GENERAL_NAMES *CRLissuer; - int dp_reasons; }; typedef STACK_OF(DIST_POINT) CRL_DIST_POINTS; @@ -309,31 +308,13 @@ typedef struct POLICY_CONSTRAINTS_st { struct ISSUING_DIST_POINT_st { DIST_POINT_NAME *distpoint; - int onlyuser; - int onlyCA; + ASN1_BOOLEAN onlyuser; + ASN1_BOOLEAN onlyCA; ASN1_BIT_STRING *onlysomereasons; - int indirectCRL; - int onlyattr; + ASN1_BOOLEAN indirectCRL; + ASN1_BOOLEAN onlyattr; }; -// Values in idp_flags field -// IDP present -#define IDP_PRESENT 0x1 -// IDP values inconsistent -#define IDP_INVALID 0x2 -// onlyuser true -#define IDP_ONLYUSER 0x4 -// onlyCA true -#define IDP_ONLYCA 0x8 -// onlyattr true -#define IDP_ONLYATTR 0x10 -// indirectCRL true -#define IDP_INDIRECT 0x20 -// onlysomereasons present -#define IDP_REASONS 0x40 - - - // X509_PURPOSE stuff #define EXFLAG_BCONS 0x1 @@ -349,7 +330,6 @@ struct ISSUING_DIST_POINT_st { #define EXFLAG_SET 0x100 #define EXFLAG_CRITICAL 0x200 -#define EXFLAG_FRESHEST 0x1000 // Self signed #define EXFLAG_SS 0x2000