From: tb <tb@openbsd.org>
Date: Thu, 10 Aug 2023 09:43:51 +0000 (+0000)
Subject: Various fixes in {dh,dsa}_priv_encode()
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=d46624175283f46a1ac475a3696b25e035298657;p=openbsd

Various fixes in {dh,dsa}_priv_encode()

Avoid creating an ASN1_STRING with negative length, set type, data
and length via ASN1_STRING_type_new() and ASN1_STRING_set0() instead
of doing this manually. Check return value for i2d_ASN1_INTEGER()
and use an intermediate ASN1_OBJECT instead of nested function calls.
Finally, clear sensitive data with freezero().

ok jsing
---

diff --git a/lib/libcrypto/dh/dh_ameth.c b/lib/libcrypto/dh/dh_ameth.c
index cc594cfd38e..12f2db7b8eb 100644
--- a/lib/libcrypto/dh/dh_ameth.c
+++ b/lib/libcrypto/dh/dh_ameth.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: dh_ameth.c,v 1.30 2023/07/08 15:29:03 beck Exp $ */
+/* $OpenBSD: dh_ameth.c,v 1.31 2023/08/10 09:43:51 tb Exp $ */
 /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL
  * project 2006.
  */
@@ -236,48 +236,51 @@ dherr:
 static int
 dh_priv_encode(PKCS8_PRIV_KEY_INFO *p8, const EVP_PKEY *pkey)
 {
+	const DH *dh = pkey->pkey.dh;
 	ASN1_STRING *params = NULL;
 	ASN1_INTEGER *prkey = NULL;
-	unsigned char *dp = NULL;
-	int dplen;
+	ASN1_OBJECT *aobj;
+	unsigned char *data = NULL, *dp = NULL;
+	int datalen = 0, dplen = 0;
 
-	params = ASN1_STRING_new();
-
-	if (!params) {
+	if ((datalen = i2d_DHparams(dh, &data)) <= 0) {
 		DHerror(ERR_R_MALLOC_FAILURE);
+		datalen = 0;
 		goto err;
 	}
-
-	params->length = i2d_DHparams(pkey->pkey.dh, &params->data);
-	if (params->length <= 0) {
+	if ((params = ASN1_STRING_type_new(V_ASN1_SEQUENCE)) == NULL) {
 		DHerror(ERR_R_MALLOC_FAILURE);
 		goto err;
 	}
-	params->type = V_ASN1_SEQUENCE;
-
-	/* Get private key into integer */
-	prkey = BN_to_ASN1_INTEGER(pkey->pkey.dh->priv_key, NULL);
+	ASN1_STRING_set0(params, data, datalen);
+	data = NULL;
+	datalen = 0;
 
-	if (!prkey) {
+	if ((prkey = BN_to_ASN1_INTEGER(dh->priv_key, NULL)) == NULL) {
 		DHerror(DH_R_BN_ERROR);
 		goto err;
 	}
-
-	dplen = i2d_ASN1_INTEGER(prkey, &dp);
-
+	if ((dplen = i2d_ASN1_INTEGER(prkey, &dp)) <= 0) {
+		DHerror(ERR_R_MALLOC_FAILURE);
+		dplen = 0;
+		goto err;
+	}
 	ASN1_INTEGER_free(prkey);
 	prkey = NULL;
 
-	if (!PKCS8_pkey_set0(p8, OBJ_nid2obj(NID_dhKeyAgreement), 0,
-	    V_ASN1_SEQUENCE, params, dp, dplen))
+	if ((aobj = OBJ_nid2obj(NID_dhKeyAgreement)) == NULL)
+		goto err;
+	if (!PKCS8_pkey_set0(p8, aobj, 0, V_ASN1_SEQUENCE, params, dp, dplen))
 		goto err;
 
 	return 1;
 
-err:
-	free(dp);
+ err:
 	ASN1_STRING_free(params);
 	ASN1_INTEGER_free(prkey);
+	freezero(data, datalen);
+	freezero(dp, dplen);
+
 	return 0;
 }
 
diff --git a/lib/libcrypto/dsa/dsa_ameth.c b/lib/libcrypto/dsa/dsa_ameth.c
index 5a0c3116aad..ad5aa09cd0c 100644
--- a/lib/libcrypto/dsa/dsa_ameth.c
+++ b/lib/libcrypto/dsa/dsa_ameth.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: dsa_ameth.c,v 1.43 2023/07/07 06:59:18 tb Exp $ */
+/* $OpenBSD: dsa_ameth.c,v 1.44 2023/08/10 09:43:51 tb Exp $ */
 /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL
  * project 2006.
  */
@@ -265,46 +265,51 @@ done:
 static int
 dsa_priv_encode(PKCS8_PRIV_KEY_INFO *p8, const EVP_PKEY *pkey)
 {
+	const DSA *dsa = pkey->pkey.dsa;
 	ASN1_STRING *params = NULL;
 	ASN1_INTEGER *prkey = NULL;
-	unsigned char *dp = NULL;
-	int dplen;
+	ASN1_OBJECT *aobj;
+	unsigned char *data = NULL, *dp = NULL;
+	int datalen = 0, dplen = 0;
 
-	params = ASN1_STRING_new();
-	if (!params) {
+	if ((datalen = i2d_DSAparams(dsa, &data)) <= 0) {
 		DSAerror(ERR_R_MALLOC_FAILURE);
+		datalen = 0;
 		goto err;
 	}
-
-	params->length = i2d_DSAparams(pkey->pkey.dsa, &params->data);
-	if (params->length <= 0) {
+	if ((params = ASN1_STRING_type_new(V_ASN1_SEQUENCE)) == NULL) {
 		DSAerror(ERR_R_MALLOC_FAILURE);
 		goto err;
 	}
-	params->type = V_ASN1_SEQUENCE;
+	ASN1_STRING_set0(params, data, datalen);
+	data = NULL;
+	datalen = 0;
 
-	/* Get private key into integer */
-	prkey = BN_to_ASN1_INTEGER(pkey->pkey.dsa->priv_key, NULL);
-	if (!prkey) {
+	if ((prkey = BN_to_ASN1_INTEGER(dsa->priv_key, NULL)) == NULL) {
 		DSAerror(DSA_R_BN_ERROR);
 		goto err;
 	}
-
-	dplen = i2d_ASN1_INTEGER(prkey, &dp);
-
+	if ((dplen = i2d_ASN1_INTEGER(prkey, &dp)) <= 0) {
+		DSAerror(ERR_R_MALLOC_FAILURE);
+		dplen = 0;
+		goto err;
+	}
 	ASN1_INTEGER_free(prkey);
 	prkey = NULL;
 
-	if (!PKCS8_pkey_set0(p8, OBJ_nid2obj(NID_dsa), 0, V_ASN1_SEQUENCE,
-	    params, dp, dplen))
+	if ((aobj = OBJ_nid2obj(NID_dsa)) == NULL)
+		goto err;
+	if (!PKCS8_pkey_set0(p8, aobj, 0, V_ASN1_SEQUENCE, params, dp, dplen))
 		goto err;
 
 	return 1;
 
-err:
-	free(dp);
+ err:
 	ASN1_STRING_free(params);
 	ASN1_INTEGER_free(prkey);
+	freezero(data, datalen);
+	freezero(dp, dplen);
+
 	return 0;
 }