[openssl-dev] [openssl.org #4193] Minor Issue with X509_STORE_CTX_init and it's callers.
Viktor Dukhovni
openssl-users at dukhovni.org
Tue Dec 22 06:53:54 UTC 2015
On Tue, Dec 22, 2015 at 04:33:45AM +0000, Srinivas Koripella via RT wrote:
> There is a minor issue with X509_STORE_CTX_init and its usage. Most of
> the callers of X509_STORE_CTX_init use a stack variable and pass its
> address as the ctx argument to this function. However, X509_STORE_CTX_init
> in case of an error in the call to CRYPTO_new_ex_data does an OPENSSL_free
> on this stack variable. This in theory should be ok as the underlying
> free implementation should probably be a no-op as this address is from
> the stack.
Thanks for the report. The bug was introduced way back on 2001/09/01
by commit 79aa04ef27f69a1149d4d0e72d2d2953b6241ef0 and is present
in OpenSSL 0.9.8 through 1.0.2.
In the "master" development branch the extraneous "free" is gone,
but the code is still not quite right, because the memset removed
in 2001 really does belong (early) in X509_STORE_CTX_init() and
should have been removed from X509_STORE_CTX_cleanup() instead,
where zeroing data that is invalidated by cleanup is of course.
Try the (lightly tested) patch below my signature.
--
Viktor.
diff --git a/apps/pkcs12.c b/apps/pkcs12.c
index e41b445..cbb75b7 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 da89911..29aa5a4 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 ab94948..f44a4a0 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -2283,9 +2283,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(CRYPTO_EX_DATA));
ctx->param = X509_VERIFY_PARAM_new();
-
if (!ctx->param) {
X509err(X509_F_X509_STORE_CTX_INIT, ERR_R_MALLOC_FAILURE);
return 0;
@@ -2294,7 +2295,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
@@ -2302,6 +2302,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;
@@ -2312,7 +2313,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)
@@ -2367,19 +2368,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;
}
/*
@@ -2395,6 +2395,13 @@ void X509_STORE_CTX_trusted_stack(X509_STORE_CTX *ctx, STACK_OF(X509) *sk)
void X509_STORE_CTX_cleanup(X509_STORE_CTX *ctx)
{
+ /*
+ * 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, else must be idempotent */
if (ctx->cleanup)
ctx->cleanup(ctx);
if (ctx->param != NULL) {
diff --git a/crypto/x509/x509_vfy.h b/crypto/x509/x509_vfy.h
index bd8613c..2663e1c 100644
--- a/crypto/x509/x509_vfy.h
+++ b/crypto/x509/x509_vfy.h
@@ -313,7 +313,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
More information about the openssl-dev
mailing list