From: Viktor Dukhovni Date: Fri, 1 Jan 2016 05:51:12 +0000 (-0500) Subject: Fix X509_STORE_CTX_cleanup() X-Git-Tag: OpenSSL_1_0_1r~13 X-Git-Url: https://git.librecmc.org/?a=commitdiff_plain;h=737d57d0725551e473d4da176c3c431f1f9d36df;p=oweals%2Fopenssl.git Fix X509_STORE_CTX_cleanup() Reviewed-by: Dr. Stephen Henson --- diff --git a/apps/pkcs12.c b/apps/pkcs12.c index e41b445a50..cbb75b7d5f 100644 --- a/apps/pkcs12.c +++ b/apps/pkcs12.c @@ -79,7 +79,8 @@ const EVP_CIPHER *enc; # 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); int dump_certs_pkeys_bags(BIO *out, STACK_OF(PKCS12_SAFEBAG) *bags, @@ -594,7 +595,7 @@ int 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)); @@ -602,7 +603,7 @@ int 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 @@ -906,36 +907,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 1a3a7c5248..e24b2d5388 100644 --- a/crypto/ts/ts_rsp_verify.c +++ b/crypto/ts/ts_rsp_verify.c @@ -255,7 +255,8 @@ static int TS_verify_cert(X509_STORE *store, STACK_OF(X509) *untrusted, /* chain is an out argument. */ *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 7009ae6307..3bad523f71 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -2026,9 +2026,10 @@ 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)); ctx->param = X509_VERIFY_PARAM_new(); - if (!ctx->param) { X509err(X509_F_X509_STORE_CTX_INIT, ERR_R_MALLOC_FAILURE); return 0; @@ -2037,7 +2038,6 @@ int X509_STORE_CTX_init(X509_STORE_CTX *ctx, X509_STORE *store, X509 *x509, /* * Inherit callbacks and flags from X509_STORE if not set use defaults. */ - if (store) ret = X509_VERIFY_PARAM_inherit(ctx->param, store->param); else @@ -2045,6 +2045,7 @@ int X509_STORE_CTX_init(X509_STORE_CTX *ctx, X509_STORE *store, X509 *x509, 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; @@ -2055,7 +2056,7 @@ int X509_STORE_CTX_init(X509_STORE_CTX *ctx, X509_STORE *store, X509 *x509, if (ret == 0) { X509err(X509_F_X509_STORE_CTX_INIT, ERR_R_MALLOC_FAILURE); - return 0; + goto err; } if (store && store->check_issued) @@ -2110,19 +2111,18 @@ int X509_STORE_CTX_init(X509_STORE_CTX *ctx, X509_STORE *store, X509 *x509, ctx->check_policy = check_policy; + 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: /* - * This memset() can't make any sense anyway, so it's removed. As - * X509_STORE_CTX_cleanup does a proper "free" on the ex_data, we put a - * corresponding "new" here and remove this bogus initialisation. + * 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. */ - /* memset(&(ctx->ex_data),0,sizeof(CRYPTO_EX_DATA)); */ - if (!CRYPTO_new_ex_data(CRYPTO_EX_INDEX_X509_STORE_CTX, ctx, - &(ctx->ex_data))) { - OPENSSL_free(ctx); - X509err(X509_F_X509_STORE_CTX_INIT, ERR_R_MALLOC_FAILURE); - return 0; - } - return 1; + X509_STORE_CTX_cleanup(ctx); + return 0; } /* @@ -2138,8 +2138,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/crypto/x509/x509_vfy.h b/crypto/x509/x509_vfy.h index aacdf55aa2..b7d8b2472e 100644 --- a/crypto/x509/x509_vfy.h +++ b/crypto/x509/x509_vfy.h @@ -310,7 +310,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