From: tb <tb@openbsd.org>
Date: Sat, 12 Aug 2023 06:14:36 +0000 (+0000)
Subject: Convert {DH,DSA}_new_method() to using calloc()
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=e498bf2d5f803457d9476b97c3c125b2c81f4b2e;p=openbsd

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
---

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);