Remove a pile of unused X509_STORE callbacks Other than the verify callback, these are practically unused. The one exception is that gRPC have used a couple of the CRL functions, though in somewhat questionable ways. Keep those for now, but we need to work with them to fix their code. There's also a bit of mess around check_issued, largely due to further rust-openssl misunderstandings. Update-Note: Removed a bunch of unused X509_STORE callback functions. We can restore them if someone was using them. Change-Id: I9c47581784c56a4c3b762e603a20ad7d5d612c65 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64133 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 574c560..68919b6 100644 --- a/crypto/x509/internal.h +++ b/crypto/x509/internal.h
@@ -335,18 +335,10 @@ X509_VERIFY_PARAM *param; // Callbacks for various operations - X509_STORE_CTX_verify_fn verify; // called to verify a certificate X509_STORE_CTX_verify_cb verify_cb; // error callback X509_STORE_CTX_get_issuer_fn get_issuer; // get issuers cert from ctx - X509_STORE_CTX_check_issued_fn check_issued; // check issued - X509_STORE_CTX_check_revocation_fn - check_revocation; // Check revocation status of chain - X509_STORE_CTX_get_crl_fn get_crl; // retrieve CRL - X509_STORE_CTX_check_crl_fn check_crl; // Check CRL validity - X509_STORE_CTX_cert_crl_fn cert_crl; // Check certificate against CRL - X509_STORE_CTX_lookup_certs_fn lookup_certs; - X509_STORE_CTX_lookup_crls_fn lookup_crls; - X509_STORE_CTX_cleanup_fn cleanup; + X509_STORE_CTX_get_crl_fn get_crl; // retrieve CRL + X509_STORE_CTX_check_crl_fn check_crl; // Check CRL validity CRYPTO_refcount_t references; } /* X509_STORE */; @@ -375,19 +367,10 @@ void *other_ctx; // Other info for use with get_issuer() // Callbacks for various operations - X509_STORE_CTX_verify_fn verify; // called to verify a certificate X509_STORE_CTX_verify_cb verify_cb; // error callback X509_STORE_CTX_get_issuer_fn get_issuer; // get issuers cert from ctx - X509_STORE_CTX_check_issued_fn check_issued; // check issued - X509_STORE_CTX_check_revocation_fn - check_revocation; // Check revocation status of chain - X509_STORE_CTX_get_crl_fn get_crl; // retrieve CRL - X509_STORE_CTX_check_crl_fn check_crl; // Check CRL validity - X509_STORE_CTX_cert_crl_fn cert_crl; // Check certificate against CRL - X509_STORE_CTX_check_policy_fn check_policy; - X509_STORE_CTX_lookup_certs_fn lookup_certs; - X509_STORE_CTX_lookup_crls_fn lookup_crls; - X509_STORE_CTX_cleanup_fn cleanup; + X509_STORE_CTX_get_crl_fn get_crl; // retrieve CRL + X509_STORE_CTX_check_crl_fn check_crl; // Check CRL validity // The following is built up int valid; // if 0, rebuild chain @@ -460,6 +443,15 @@ const STACK_OF(ASN1_OBJECT) *user_policies, unsigned long flags, X509 **out_current_cert); +// x509_check_issued_with_callback calls |X509_check_issued|, but allows the +// verify callback to override the result. It returns one on success and zero on +// error. +// +// TODO(davidben): Reduce the scope of the verify callback and remove this. The +// callback only runs with |X509_V_FLAG_CB_ISSUER_CHECK|, which is only used by +// one internal project and rust-openssl, who use it by mistake. +int x509_check_issued_with_callback(X509_STORE_CTX *ctx, X509 *x, X509 *issuer); + #if defined(__cplusplus) } // extern C
diff --git a/crypto/x509/x509_lu.c b/crypto/x509/x509_lu.c index 4b59fc8..31e92bb 100644 --- a/crypto/x509/x509_lu.c +++ b/crypto/x509/x509_lu.c
@@ -563,13 +563,14 @@ return 0; } // If certificate matches all OK - if (ctx->check_issued(ctx, x, obj.data.x509)) { + if (x509_check_issued_with_callback(ctx, x, obj.data.x509)) { *issuer = obj.data.x509; return 1; } X509_OBJECT_free_contents(&obj); - // Else find index of first cert accepted by 'check_issued' + // Else find index of first cert accepted by + // |x509_check_issued_with_callback|. ret = 0; CRYPTO_MUTEX_lock_write(&ctx->ctx->objs_lock); idx = X509_OBJECT_idx_by_subject(ctx->ctx->objs, X509_LU_X509, xn); @@ -585,7 +586,7 @@ if (X509_NAME_cmp(xn, X509_get_subject_name(pobj->data.x509))) { break; } - if (ctx->check_issued(ctx, x, pobj->data.x509)) { + if (x509_check_issued_with_callback(ctx, x, pobj->data.x509)) { *issuer = pobj->data.x509; X509_OBJECT_up_ref_count(pobj); ret = 1; @@ -620,14 +621,6 @@ X509_VERIFY_PARAM *X509_STORE_get0_param(X509_STORE *ctx) { return ctx->param; } -void X509_STORE_set_verify(X509_STORE *ctx, X509_STORE_CTX_verify_fn verify) { - ctx->verify = verify; -} - -X509_STORE_CTX_verify_fn X509_STORE_get_verify(X509_STORE *ctx) { - return ctx->verify; -} - void X509_STORE_set_verify_cb(X509_STORE *ctx, X509_STORE_CTX_verify_cb verify_cb) { ctx->verify_cb = verify_cb; @@ -646,25 +639,6 @@ return ctx->get_issuer; } -void X509_STORE_set_check_issued(X509_STORE *ctx, - X509_STORE_CTX_check_issued_fn check_issued) { - ctx->check_issued = check_issued; -} - -X509_STORE_CTX_check_issued_fn X509_STORE_get_check_issued(X509_STORE *ctx) { - return ctx->check_issued; -} - -void X509_STORE_set_check_revocation( - X509_STORE *ctx, X509_STORE_CTX_check_revocation_fn check_revocation) { - ctx->check_revocation = check_revocation; -} - -X509_STORE_CTX_check_revocation_fn X509_STORE_get_check_revocation( - X509_STORE *ctx) { - return ctx->check_revocation; -} - void X509_STORE_set_get_crl(X509_STORE *ctx, X509_STORE_CTX_get_crl_fn get_crl) { ctx->get_crl = get_crl; @@ -683,40 +657,4 @@ return ctx->check_crl; } -void X509_STORE_set_cert_crl(X509_STORE *ctx, - X509_STORE_CTX_cert_crl_fn cert_crl) { - ctx->cert_crl = cert_crl; -} - -X509_STORE_CTX_cert_crl_fn X509_STORE_get_cert_crl(X509_STORE *ctx) { - return ctx->cert_crl; -} - -void X509_STORE_set_lookup_certs(X509_STORE *ctx, - X509_STORE_CTX_lookup_certs_fn lookup_certs) { - ctx->lookup_certs = lookup_certs; -} - -X509_STORE_CTX_lookup_certs_fn X509_STORE_get_lookup_certs(X509_STORE *ctx) { - return ctx->lookup_certs; -} - -void X509_STORE_set_lookup_crls(X509_STORE *ctx, - X509_STORE_CTX_lookup_crls_fn lookup_crls) { - ctx->lookup_crls = lookup_crls; -} - -X509_STORE_CTX_lookup_crls_fn X509_STORE_get_lookup_crls(X509_STORE *ctx) { - return ctx->lookup_crls; -} - -void X509_STORE_set_cleanup(X509_STORE *ctx, - X509_STORE_CTX_cleanup_fn ctx_cleanup) { - ctx->cleanup = ctx_cleanup; -} - -X509_STORE_CTX_cleanup_fn X509_STORE_get_cleanup(X509_STORE *ctx) { - return ctx->cleanup; -} - X509_STORE *X509_STORE_CTX_get0_store(X509_STORE_CTX *ctx) { return ctx->ctx; }
diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index 5eca5c7..e604bb9 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c
@@ -102,7 +102,6 @@ #define CRL_SCORE_AKID 0x004 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); static int check_chain_extensions(X509_STORE_CTX *ctx); static int check_name_constraints(X509_STORE_CTX *ctx); @@ -118,6 +117,7 @@ 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 cert_crl(X509_STORE_CTX *ctx, X509_CRL *crl, X509 *x); static int internal_verify(X509_STORE_CTX *ctx); @@ -141,7 +141,7 @@ X509 *xtmp = NULL; size_t i; // Lookup all certs with matching subject name - certs = ctx->lookup_certs(ctx, X509_get_subject_name(x)); + certs = X509_STORE_get1_certs(ctx, X509_get_subject_name(x)); if (certs == NULL) { return NULL; } @@ -400,7 +400,8 @@ // self signed certificate in which case we've indicated an error already // and set bad_chain == 1 if (trust != X509_TRUST_TRUSTED && !bad_chain) { - if ((chain_ss == NULL) || !ctx->check_issued(ctx, x, chain_ss)) { + if (chain_ss == NULL || + !x509_check_issued_with_callback(ctx, x, chain_ss)) { if (ctx->last_untrusted >= num) { ctx->error = X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY; } else { @@ -439,17 +440,13 @@ // Check revocation status: we do this after copying parameters because // they may be needed for CRL signature verification. - ok = ctx->check_revocation(ctx); + ok = check_revocation(ctx); if (!ok) { goto end; } // At this point, we have a chain and need to verify it - if (ctx->verify != NULL) { - ok = ctx->verify(ctx); - } else { - ok = internal_verify(ctx); - } + ok = internal_verify(ctx); if (!ok) { goto end; } @@ -462,7 +459,7 @@ // If we get this far, evaluate policies. if (!bad_chain) { - ok = ctx->check_policy(ctx); + ok = check_policy(ctx); } end: @@ -487,7 +484,7 @@ X509 *issuer; for (i = 0; i < sk_X509_num(sk); i++) { issuer = sk_X509_value(sk, i); - if (ctx->check_issued(ctx, x, issuer)) { + if (x509_check_issued_with_callback(ctx, x, issuer)) { return issuer; } } @@ -496,7 +493,8 @@ // Given a possible certificate and issuer check them -static int check_issued(X509_STORE_CTX *ctx, X509 *x, X509 *issuer) { +int x509_check_issued_with_callback(X509_STORE_CTX *ctx, X509 *x, + X509 *issuer) { int ret; ret = X509_check_issued(issuer, x); if (ret == X509_V_OK) { @@ -827,7 +825,7 @@ goto err; } - ok = ctx->cert_crl(ctx, crl, x); + ok = cert_crl(ctx, crl, x); if (!ok) { goto err; } @@ -1152,7 +1150,7 @@ } // Lookup CRLs from store - skcrl = ctx->lookup_crls(ctx, nm); + skcrl = X509_STORE_get1_crls(ctx, nm); // If no CRLs found and a near match from get_crl_sk use that if (!skcrl && crl) { @@ -1195,7 +1193,7 @@ } else { issuer = sk_X509_value(ctx->chain, chnum); // If not self signed, can't check signature - if (!ctx->check_issued(ctx, issuer, issuer)) { + if (!x509_check_issued_with_callback(ctx, issuer, issuer)) { ctx->error = X509_V_ERR_UNABLE_TO_GET_CRL_ISSUER; ok = ctx->verify_cb(0, ctx); if (!ok) { @@ -1380,7 +1378,7 @@ n--; xi = sk_X509_value(ctx->chain, n); - if (ctx->check_issued(ctx, xi, xi)) { + if (x509_check_issued_with_callback(ctx, xi, xi)) { xs = xi; } else { if (ctx->param->flags & X509_V_FLAG_PARTIAL_CHAIN) { @@ -1669,7 +1667,6 @@ // Inherit callbacks and flags from X509_STORE. ctx->verify_cb = store->verify_cb; - ctx->cleanup = store->cleanup; if (!X509_VERIFY_PARAM_inherit(ctx->param, store->param) || !X509_VERIFY_PARAM_inherit(ctx->param, @@ -1677,12 +1674,6 @@ goto err; } - if (store->check_issued) { - ctx->check_issued = store->check_issued; - } else { - ctx->check_issued = check_issued; - } - if (store->get_issuer) { ctx->get_issuer = store->get_issuer; } else { @@ -1695,18 +1686,6 @@ ctx->verify_cb = null_callback; } - if (store->verify) { - ctx->verify = store->verify; - } else { - ctx->verify = internal_verify; - } - - if (store->check_revocation) { - ctx->check_revocation = store->check_revocation; - } else { - ctx->check_revocation = check_revocation; - } - if (store->get_crl) { ctx->get_crl = store->get_crl; } else { @@ -1719,26 +1698,6 @@ ctx->check_crl = check_crl; } - if (store->cert_crl) { - ctx->cert_crl = store->cert_crl; - } else { - ctx->cert_crl = cert_crl; - } - - if (store->lookup_certs) { - ctx->lookup_certs = store->lookup_certs; - } else { - ctx->lookup_certs = X509_STORE_get1_certs; - } - - if (store->lookup_crls) { - ctx->lookup_crls = store->lookup_crls; - } else { - ctx->lookup_crls = X509_STORE_get1_crls; - } - - ctx->check_policy = check_policy; - return 1; err: @@ -1765,12 +1724,6 @@ } void X509_STORE_CTX_cleanup(X509_STORE_CTX *ctx) { - // We need to be idempotent because, unfortunately, |X509_STORE_CTX_free| - // also calls this function. - if (ctx->cleanup != NULL) { - ctx->cleanup(ctx); - ctx->cleanup = NULL; - } X509_VERIFY_PARAM_free(ctx->param); ctx->param = NULL; sk_X509_pop_free(ctx->chain, X509_free);
diff --git a/include/openssl/x509.h b/include/openssl/x509.h index 41305d8..4ef12d6 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h
@@ -2774,23 +2774,11 @@ DEFINE_STACK_OF(X509_VERIFY_PARAM) typedef int (*X509_STORE_CTX_verify_cb)(int, X509_STORE_CTX *); -typedef int (*X509_STORE_CTX_verify_fn)(X509_STORE_CTX *); typedef int (*X509_STORE_CTX_get_issuer_fn)(X509 **issuer, X509_STORE_CTX *ctx, X509 *x); -typedef int (*X509_STORE_CTX_check_issued_fn)(X509_STORE_CTX *ctx, X509 *x, - X509 *issuer); -typedef int (*X509_STORE_CTX_check_revocation_fn)(X509_STORE_CTX *ctx); typedef int (*X509_STORE_CTX_get_crl_fn)(X509_STORE_CTX *ctx, X509_CRL **crl, X509 *x); typedef int (*X509_STORE_CTX_check_crl_fn)(X509_STORE_CTX *ctx, X509_CRL *crl); -typedef int (*X509_STORE_CTX_cert_crl_fn)(X509_STORE_CTX *ctx, X509_CRL *crl, - X509 *x); -typedef int (*X509_STORE_CTX_check_policy_fn)(X509_STORE_CTX *ctx); -typedef STACK_OF(X509) *(*X509_STORE_CTX_lookup_certs_fn)(X509_STORE_CTX *ctx, - X509_NAME *nm); -typedef STACK_OF(X509_CRL) *(*X509_STORE_CTX_lookup_crls_fn)( - X509_STORE_CTX *ctx, X509_NAME *nm); -typedef int (*X509_STORE_CTX_cleanup_fn)(X509_STORE_CTX *ctx); OPENSSL_EXPORT int X509_STORE_set_depth(X509_STORE *store, int depth); @@ -2967,14 +2955,6 @@ X509_VERIFY_PARAM *pm); OPENSSL_EXPORT X509_VERIFY_PARAM *X509_STORE_get0_param(X509_STORE *ctx); -OPENSSL_EXPORT void X509_STORE_set_verify(X509_STORE *ctx, - X509_STORE_CTX_verify_fn verify); -#define X509_STORE_set_verify_func(ctx, func) \ - X509_STORE_set_verify((ctx), (func)) -OPENSSL_EXPORT void X509_STORE_CTX_set_verify(X509_STORE_CTX *ctx, - X509_STORE_CTX_verify_fn verify); -OPENSSL_EXPORT X509_STORE_CTX_verify_fn X509_STORE_get_verify(X509_STORE *ctx); - // X509_STORE_set_verify_cb acts like |X509_STORE_CTX_set_verify_cb| but sets // the verify callback for any |X509_STORE_CTX| created from this |X509_STORE| // @@ -2989,14 +2969,6 @@ X509_STORE *ctx, X509_STORE_CTX_get_issuer_fn get_issuer); OPENSSL_EXPORT X509_STORE_CTX_get_issuer_fn X509_STORE_get_get_issuer(X509_STORE *ctx); -OPENSSL_EXPORT void X509_STORE_set_check_issued( - X509_STORE *ctx, X509_STORE_CTX_check_issued_fn check_issued); -OPENSSL_EXPORT X509_STORE_CTX_check_issued_fn -X509_STORE_get_check_issued(X509_STORE *ctx); -OPENSSL_EXPORT void X509_STORE_set_check_revocation( - X509_STORE *ctx, X509_STORE_CTX_check_revocation_fn check_revocation); -OPENSSL_EXPORT X509_STORE_CTX_check_revocation_fn -X509_STORE_get_check_revocation(X509_STORE *ctx); OPENSSL_EXPORT void X509_STORE_set_get_crl(X509_STORE *ctx, X509_STORE_CTX_get_crl_fn get_crl); OPENSSL_EXPORT X509_STORE_CTX_get_crl_fn @@ -3005,24 +2977,6 @@ X509_STORE *ctx, X509_STORE_CTX_check_crl_fn check_crl); OPENSSL_EXPORT X509_STORE_CTX_check_crl_fn X509_STORE_get_check_crl(X509_STORE *ctx); -OPENSSL_EXPORT void X509_STORE_set_cert_crl( - X509_STORE *ctx, X509_STORE_CTX_cert_crl_fn cert_crl); -OPENSSL_EXPORT X509_STORE_CTX_cert_crl_fn -X509_STORE_get_cert_crl(X509_STORE *ctx); -OPENSSL_EXPORT void X509_STORE_set_lookup_certs( - X509_STORE *ctx, X509_STORE_CTX_lookup_certs_fn lookup_certs); -OPENSSL_EXPORT X509_STORE_CTX_lookup_certs_fn -X509_STORE_get_lookup_certs(X509_STORE *ctx); -OPENSSL_EXPORT void X509_STORE_set_lookup_crls( - X509_STORE *ctx, X509_STORE_CTX_lookup_crls_fn lookup_crls); -#define X509_STORE_set_lookup_crls_cb(ctx, func) \ - X509_STORE_set_lookup_crls((ctx), (func)) -OPENSSL_EXPORT X509_STORE_CTX_lookup_crls_fn -X509_STORE_get_lookup_crls(X509_STORE *ctx); -OPENSSL_EXPORT void X509_STORE_set_cleanup(X509_STORE *ctx, - X509_STORE_CTX_cleanup_fn cleanup); -OPENSSL_EXPORT X509_STORE_CTX_cleanup_fn -X509_STORE_get_cleanup(X509_STORE *ctx); OPENSSL_EXPORT X509_STORE_CTX *X509_STORE_CTX_new(void);