Fix X509_STORE_CTX_cleanup()

Reviewed-by: Dr. Stephen Henson <steve@openssl.org>

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