The branch master has been updated
via e29c73c93b88a4b7f492c7c8c7343223e7548612 (commit)
from 0e7abc903799a32d9d4eb34316cb72f1f4b28a52 (commit)
- Log -----------------------------------------------------------------
commit e29c73c93b88a4b7f492c7c8c7343223e7548612
Author: Viktor Dukhovni <openssl-us...@dukhovni.org>
Date: Fri Jan 1 00:51:12 2016 -0500
Fix X509_STORE_CTX_cleanup()
Reviewed-by: Dr. Stephen Henson <st...@openssl.org>
-----------------------------------------------------------------------
Summary of changes:
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 3d566f3..33a58df 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 c79db38..c03f6ac 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 9b88031..57fcf91 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 b78b59c..5e65998 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
_____
openssl-commits mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-commits
