On Tue, Dec 22, 2015 at 04:33:45AM +, 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 clea
