From e498bf2d5f803457d9476b97c3c125b2c81f4b2e Mon Sep 17 00:00:00 2001 From: tb Date: Sat, 12 Aug 2023 06:14:36 +0000 Subject: [PATCH] Convert {DH,DSA}_new_method() to using calloc() Due to OPENSSL_NO_ENGINE the engine member of dh and dsa is currently uninitialized. As a consequence, {DH,DSA}_get0_engine() will return a garbage pointer, which is particularly bad because the only reason we kept them in the first place is that they are used by some software... A side effect of freeing with {DH,DSA}_free() instead of a hand-rolled version is that we may call ->meth->finish() before ->meth->init() was called. We need a NULL check for ->meth to be on the safe side in case we should need to bring ENGINE back. with nits from djm ok deraadt djm --- lib/libcrypto/dh/dh_lib.c | 72 +++++++++++++++---------------------- lib/libcrypto/dsa/dsa_lib.c | 65 +++++++++++++-------------------- 2 files changed, 54 insertions(+), 83 deletions(-) diff --git a/lib/libcrypto/dh/dh_lib.c b/lib/libcrypto/dh/dh_lib.c index 987f0b1f7ab..6e53df91773 100644 --- a/lib/libcrypto/dh/dh_lib.c +++ b/lib/libcrypto/dh/dh_lib.c @@ -1,4 +1,4 @@ -/* $OpenBSD: dh_lib.c,v 1.39 2023/07/08 15:29:03 beck Exp $ */ +/* $OpenBSD: dh_lib.c,v 1.40 2023/08/12 06:14:36 tb Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -122,61 +122,47 @@ LCRYPTO_ALIAS(DH_new); DH * DH_new_method(ENGINE *engine) { - DH *ret; + DH *dh; - ret = malloc(sizeof(DH)); - if (ret == NULL) { + if ((dh = calloc(1, sizeof(*dh))) == NULL) { DHerror(ERR_R_MALLOC_FAILURE); - return NULL; + goto err; } - ret->meth = DH_get_default_method(); + dh->meth = DH_get_default_method(); + dh->flags = dh->meth->flags & ~DH_FLAG_NON_FIPS_ALLOW; + dh->references = 1; + #ifndef OPENSSL_NO_ENGINE - if (engine) { + if (engine != NULL) { if (!ENGINE_init(engine)) { DHerror(ERR_R_ENGINE_LIB); - free(ret); - return NULL; + goto err; } - ret->engine = engine; + dh->engine = engine; } else - ret->engine = ENGINE_get_default_DH(); - if(ret->engine) { - ret->meth = ENGINE_get_DH(ret->engine); - if (ret->meth == NULL) { + dh->engine = ENGINE_get_default_DH(); + if (dh->engine != NULL) { + if ((dh->meth = ENGINE_get_DH(dh->engine)) == NULL) { DHerror(ERR_R_ENGINE_LIB); - ENGINE_finish(ret->engine); - free(ret); - return NULL; + goto err; } + dh->flags = dh->meth->flags & ~DH_FLAG_NON_FIPS_ALLOW; } #endif - ret->pad = 0; - ret->version = 0; - ret->p = NULL; - ret->g = NULL; - ret->length = 0; - ret->pub_key = NULL; - ret->priv_key = NULL; - ret->q = NULL; - ret->j = NULL; - ret->seed = NULL; - ret->seedlen = 0; - ret->counter = NULL; - ret->method_mont_p=NULL; - ret->references = 1; - ret->flags = ret->meth->flags & ~DH_FLAG_NON_FIPS_ALLOW; - CRYPTO_new_ex_data(CRYPTO_EX_INDEX_DH, ret, &ret->ex_data); - if (ret->meth->init != NULL && !ret->meth->init(ret)) { -#ifndef OPENSSL_NO_ENGINE - ENGINE_finish(ret->engine); -#endif - CRYPTO_free_ex_data(CRYPTO_EX_INDEX_DH, ret, &ret->ex_data); - free(ret); - ret = NULL; - } - return ret; + if (!CRYPTO_new_ex_data(CRYPTO_EX_INDEX_DH, dh, &dh->ex_data)) + goto err; + + if (dh->meth->init != NULL && !dh->meth->init(dh)) + goto err; + + return dh; + + err: + DH_free(dh); + + return NULL; } LCRYPTO_ALIAS(DH_new_method); @@ -191,7 +177,7 @@ DH_free(DH *r) if (i > 0) return; - if (r->meth->finish) + if (r->meth != NULL && r->meth->finish != NULL) r->meth->finish(r); #ifndef OPENSSL_NO_ENGINE ENGINE_finish(r->engine); diff --git a/lib/libcrypto/dsa/dsa_lib.c b/lib/libcrypto/dsa/dsa_lib.c index 46a7dbcfbed..a9d2179aeb2 100644 --- a/lib/libcrypto/dsa/dsa_lib.c +++ b/lib/libcrypto/dsa/dsa_lib.c @@ -1,4 +1,4 @@ -/* $OpenBSD: dsa_lib.c,v 1.43 2023/07/08 14:28:15 beck Exp $ */ +/* $OpenBSD: dsa_lib.c,v 1.44 2023/08/12 06:14:36 tb Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -127,61 +127,46 @@ LCRYPTO_ALIAS(DSA_set_method); DSA * DSA_new_method(ENGINE *engine) { - DSA *ret; + DSA *dsa; - ret = malloc(sizeof(DSA)); - if (ret == NULL) { + if ((dsa = calloc(1, sizeof(DSA))) == NULL) { DSAerror(ERR_R_MALLOC_FAILURE); - return NULL; + goto err; } - ret->meth = DSA_get_default_method(); + + dsa->meth = DSA_get_default_method(); + dsa->flags = dsa->meth->flags & ~DSA_FLAG_NON_FIPS_ALLOW; + dsa->references = 1; + #ifndef OPENSSL_NO_ENGINE if (engine) { if (!ENGINE_init(engine)) { DSAerror(ERR_R_ENGINE_LIB); - free(ret); - return NULL; + goto err; } - ret->engine = engine; + dsa->engine = engine; } else - ret->engine = ENGINE_get_default_DSA(); - if (ret->engine) { - ret->meth = ENGINE_get_DSA(ret->engine); - if (ret->meth == NULL) { + dsa->engine = ENGINE_get_default_DSA(); + if (dsa->engine != NULL) { + if ((dsa->meth = ENGINE_get_DSA(dsa->engine)) == NULL) { DSAerror(ERR_R_ENGINE_LIB); - ENGINE_finish(ret->engine); - free(ret); - return NULL; + goto err; } + dsa->flags = dsa->meth->flags & ~DSA_FLAG_NON_FIPS_ALLOW; } #endif - ret->pad = 0; - ret->version = 0; - ret->p = NULL; - ret->q = NULL; - ret->g = NULL; - - ret->pub_key = NULL; - ret->priv_key = NULL; + if (!CRYPTO_new_ex_data(CRYPTO_EX_INDEX_DSA, dsa, &dsa->ex_data)) + goto err; + if (dsa->meth->init != NULL && !dsa->meth->init(dsa)) + goto err; - ret->kinv = NULL; - ret->r = NULL; - ret->method_mont_p = NULL; + return dsa; - ret->references = 1; - ret->flags = ret->meth->flags & ~DSA_FLAG_NON_FIPS_ALLOW; - CRYPTO_new_ex_data(CRYPTO_EX_INDEX_DSA, ret, &ret->ex_data); - if (ret->meth->init != NULL && !ret->meth->init(ret)) { -#ifndef OPENSSL_NO_ENGINE - ENGINE_finish(ret->engine); -#endif - CRYPTO_free_ex_data(CRYPTO_EX_INDEX_DSA, ret, &ret->ex_data); - free(ret); - ret = NULL; - } + err: + DSA_free(dsa); - return ret; + return NULL; } LCRYPTO_ALIAS(DSA_new_method); @@ -197,7 +182,7 @@ DSA_free(DSA *r) if (i > 0) return; - if (r->meth->finish) + if (r->meth != NULL && r->meth->finish != NULL) r->meth->finish(r); #ifndef OPENSSL_NO_ENGINE ENGINE_finish(r->engine); -- 2.20.1