From e29c73c93b88a4b7f492c7c8c7343223e7548612 Mon Sep 17 00:00:00 2001 From: Viktor Dukhovni Date: Fri, 1 Jan 2016 00:51:12 -0500 Subject: [PATCH] Fix X509_STORE_CTX_cleanup() Reviewed-by: Dr. Stephen Henson --- apps/pkcs12.c | 42 +++++++++++++++----------------------- crypto/ts/ts_rsp_verify.c | 3 ++- crypto/x509/x509_vfy.c | 38 +++++++++++++++++++--------------- include/openssl/x509_vfy.h | 2 +- 4 files changed, 40 insertions(+), 45 deletions(-) diff --git a/apps/pkcs12.c b/apps/pkcs12.c index 3d566f3df2..33a58df524 100644 --- a/apps/pkcs12.c +++ b/apps/pkcs12.c @@ -74,7 +74,8 @@ # define CLCERTS 0x8 # define CACERTS 0x10 -int get_cert_chain(X509 *cert, X509_STORE *store, STACK_OF(X509) **chain); +static int get_cert_chain(X509 *cert, X509_STORE *store, + STACK_OF(X509) **chain); int dump_certs_keys_p12(BIO *out, PKCS12 *p12, char *pass, int passlen, int options, char *pempass, const EVP_CIPHER *enc); int dump_certs_pkeys_bags(BIO *out, STACK_OF(PKCS12_SAFEBAG) *bags, @@ -445,7 +446,7 @@ int pkcs12_main(int argc, char **argv) vret = get_cert_chain(ucert, store, &chain2); X509_STORE_free(store); - if (!vret) { + if (vret == X509_V_OK) { /* Exclude verified certificate */ for (i = 1; i < sk_X509_num(chain2); i++) sk_X509_push(certs, sk_X509_value(chain2, i)); @@ -453,7 +454,7 @@ int pkcs12_main(int argc, char **argv) X509_free(sk_X509_value(chain2, 0)); sk_X509_free(chain2); } else { - if (vret >= 0) + if (vret != X509_V_ERR_UNSPECIFIED) BIO_printf(bio_err, "Error %s getting chain.\n", X509_verify_cert_error_string(vret)); else @@ -718,36 +719,25 @@ int dump_certs_pkeys_bag(BIO *out, PKCS12_SAFEBAG *bag, char *pass, /* Given a single certificate return a verified chain or NULL if error */ -/* Hope this is OK .... */ - -int get_cert_chain(X509 *cert, X509_STORE *store, STACK_OF(X509) **chain) +static int get_cert_chain(X509 *cert, X509_STORE *store, + STACK_OF(X509) **chain) { X509_STORE_CTX store_ctx; - STACK_OF(X509) *chn; + STACK_OF(X509) *chn = NULL; int i = 0; - /* - * FIXME: Should really check the return status of X509_STORE_CTX_init - * for an error, but how that fits into the return value of this function - * is less obvious. - */ - X509_STORE_CTX_init(&store_ctx, store, cert, NULL); - if (X509_verify_cert(&store_ctx) <= 0) { - i = X509_STORE_CTX_get_error(&store_ctx); - if (i == 0) - /* - * avoid returning 0 if X509_verify_cert() did not set an - * appropriate error value in the context - */ - i = -1; - chn = NULL; - goto err; - } else + if (!X509_STORE_CTX_init(&store_ctx, store, cert, NULL)) { + *chain = NULL; + return X509_V_ERR_UNSPECIFIED; + } + + if (X509_verify_cert(&store_ctx) > 0) chn = X509_STORE_CTX_get1_chain(&store_ctx); - err: + else if ((i = X509_STORE_CTX_get_error(&store_ctx)) == 0) + i = X509_V_ERR_UNSPECIFIED; + X509_STORE_CTX_cleanup(&store_ctx); *chain = chn; - return i; } diff --git a/crypto/ts/ts_rsp_verify.c b/crypto/ts/ts_rsp_verify.c index c79db38ab9..c03f6aced2 100644 --- a/crypto/ts/ts_rsp_verify.c +++ b/crypto/ts/ts_rsp_verify.c @@ -217,7 +217,8 @@ static int ts_verify_cert(X509_STORE *store, STACK_OF(X509) *untrusted, int ret = 1; *chain = NULL; - X509_STORE_CTX_init(&cert_ctx, store, signer, untrusted); + if (!X509_STORE_CTX_init(&cert_ctx, store, signer, untrusted)) + return 0; X509_STORE_CTX_set_purpose(&cert_ctx, X509_PURPOSE_TIMESTAMP_SIGN); i = X509_verify_cert(&cert_ctx); if (i <= 0) { diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index 9b8803176e..57fcf91b30 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -2072,9 +2072,12 @@ int X509_STORE_CTX_init(X509_STORE_CTX *ctx, X509_STORE *store, X509 *x509, ctx->current_reasons = 0; ctx->tree = NULL; ctx->parent = NULL; + /* Zero ex_data to make sure we're cleanup-safe */ + memset(&ctx->ex_data, 0, sizeof(ctx->ex_data)); if (store) { ctx->verify_cb = store->verify_cb; + /* Seems to always be 0 in OpenSSL, else must be idempotent */ ctx->cleanup = store->cleanup; } else ctx->cleanup = 0; @@ -2106,8 +2109,6 @@ int X509_STORE_CTX_init(X509_STORE_CTX *ctx, X509_STORE *store, X509 *x509, if (store && store->get_crl) ctx->get_crl = store->get_crl; - else - ctx->get_crl = NULL; if (store && store->check_crl) ctx->check_crl = store->check_crl; @@ -2131,10 +2132,6 @@ int X509_STORE_CTX_init(X509_STORE_CTX *ctx, X509_STORE *store, X509 *x509, ctx->check_policy = check_policy; - /* - * For ctx->cleanup running well in X509_STORE_CTX_cleanup , - * initial all ctx before exceptional handling. - */ ctx->param = X509_VERIFY_PARAM_new(); if (ctx->param == NULL) { X509err(X509_F_X509_STORE_CTX_INIT, ERR_R_MALLOC_FAILURE); @@ -2158,18 +2155,16 @@ int X509_STORE_CTX_init(X509_STORE_CTX *ctx, X509_STORE *store, X509 *x509, goto err; } - /* - * Since X509_STORE_CTX_cleanup does a proper "free" on the ex_data, we - * put a corresponding "new" here. - */ - if (!CRYPTO_new_ex_data(CRYPTO_EX_INDEX_X509_STORE_CTX, ctx, - &(ctx->ex_data))) { - X509err(X509_F_X509_STORE_CTX_INIT, ERR_R_MALLOC_FAILURE); - goto err; - } - return 1; + if (CRYPTO_new_ex_data(CRYPTO_EX_INDEX_X509_STORE_CTX, ctx, + &ctx->ex_data)) + return 1; + X509err(X509_F_X509_STORE_CTX_INIT, ERR_R_MALLOC_FAILURE); err: + /* + * On error clean up allocated storage, if the store context was not + * allocated with X509_STORE_CTX_new() this is our last chance to do so. + */ X509_STORE_CTX_cleanup(ctx); return 0; } @@ -2187,8 +2182,17 @@ void X509_STORE_CTX_trusted_stack(X509_STORE_CTX *ctx, STACK_OF(X509) *sk) void X509_STORE_CTX_cleanup(X509_STORE_CTX *ctx) { - if (ctx->cleanup) + /* + * We need to be idempotent because, unfortunately, free() also calls + * cleanup(), so the natural call sequence new(), init(), cleanup(), free() + * calls cleanup() for the same object twice! Thus we must zero the + * pointers below after they're freed! + */ + /* Seems to always be 0 in OpenSSL, do this at most once. */ + if (ctx->cleanup != NULL) { ctx->cleanup(ctx); + ctx->cleanup = NULL; + } if (ctx->param != NULL) { if (ctx->parent == NULL) X509_VERIFY_PARAM_free(ctx->param); diff --git a/include/openssl/x509_vfy.h b/include/openssl/x509_vfy.h index b78b59c8f5..5e65998b35 100644 --- a/include/openssl/x509_vfy.h +++ b/include/openssl/x509_vfy.h @@ -282,7 +282,7 @@ void X509_STORE_CTX_set_depth(X509_STORE_CTX *ctx, int depth); X509_LOOKUP_ctrl((x),X509_L_ADD_DIR,(name),(long)(type),NULL) # define X509_V_OK 0 -/* illegal error (for uninitialized values, to avoid X509_V_OK): 1 */ +# define X509_V_ERR_UNSPECIFIED 1 # define X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT 2 # define X509_V_ERR_UNABLE_TO_GET_CRL 3 -- 2.34.1