From: tb <tb@openbsd.org>
Date: Mon, 21 Aug 2023 09:52:30 +0000 (+0000)
Subject: ec_ameth: clean up eckey_{pub,priv}_encode()
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=2b293404fc7806435b127a250d5d80fa764f26d5;p=openbsd

ec_ameth: clean up eckey_{pub,priv}_encode()

Factor eckey_param_free() out of eckey_pub_encode(). ASN1_OBJECT_free()
is not actually needed. This will be addressed later.

i2o_ECPublicKey() allocates internally if *out == NULL, so no need to do
the two-call dance. Its return value is documented to be <= 0 on error,
which is wrong in the sense that only 0 is returned. Keep using the same
check for <= 0 as everywhere else.

Set of EC_PKEY_NO_PARAMETERS after the poorly named eckey_param2type() to
avoid potential underhanded side effects.

In eckey_priv_encode(), error exits would leak pval was leaked a few times.
Avoid this and simplify using i2d's internal allocation. Reinstate the
flags in a single error path.

ok jsing
---

diff --git a/lib/libcrypto/ec/ec_ameth.c b/lib/libcrypto/ec/ec_ameth.c
index 6adf7ce6714..f2ad5f60c68 100644
--- a/lib/libcrypto/ec/ec_ameth.c
+++ b/lib/libcrypto/ec/ec_ameth.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ec_ameth.c,v 1.42 2023/08/12 08:07:35 tb Exp $ */
+/* $OpenBSD: ec_ameth.c,v 1.43 2023/08/21 09:52:30 tb Exp $ */
 /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL
  * project 2006.
  */
@@ -75,6 +75,17 @@ static int ecdh_cms_decrypt(CMS_RecipientInfo *ri);
 static int ecdh_cms_encrypt(CMS_RecipientInfo *ri);
 #endif
 
+static void
+eckey_param_free(int ptype, void *pval)
+{
+	if (pval == NULL)
+		return;
+	if (ptype == V_ASN1_OBJECT)
+		ASN1_OBJECT_free(pval);		/* XXX - really necessary? */
+	else
+		ASN1_STRING_free(pval);
+}
+
 static int
 eckey_param2type(int *pptype, void **ppval, EC_KEY *ec_key)
 {
@@ -110,36 +121,37 @@ eckey_param2type(int *pptype, void **ppval, EC_KEY *ec_key)
 static int
 eckey_pub_encode(X509_PUBKEY *pk, const EVP_PKEY *pkey)
 {
-	EC_KEY *ec_key = pkey->pkey.ec;
+	EC_KEY *eckey = pkey->pkey.ec;
+	int ptype = V_ASN1_UNDEF;
 	void *pval = NULL;
-	int ptype;
-	unsigned char *penc = NULL, *p;
-	int penclen;
+	ASN1_OBJECT *aobj;
+	unsigned char *key = NULL;
+	int key_len = 0;
+	int ret = 0;
 
-	if (!eckey_param2type(&ptype, &pval, ec_key)) {
+	if (!eckey_param2type(&ptype, &pval, eckey)) {
 		ECerror(ERR_R_EC_LIB);
-		return 0;
+		goto err;
 	}
-	penclen = i2o_ECPublicKey(ec_key, NULL);
-	if (penclen <= 0)
+	if ((key_len = i2o_ECPublicKey(eckey, &key)) <= 0) {
+		key_len = 0;
 		goto err;
-	penc = malloc(penclen);
-	if (!penc)
+	}
+	if ((aobj = OBJ_nid2obj(EVP_PKEY_EC)) == NULL)
 		goto err;
-	p = penc;
-	penclen = i2o_ECPublicKey(ec_key, &p);
-	if (penclen <= 0)
+	if (!X509_PUBKEY_set0_param(pk, aobj, ptype, pval, key, key_len))
 		goto err;
-	if (X509_PUBKEY_set0_param(pk, OBJ_nid2obj(EVP_PKEY_EC),
-		ptype, pval, penc, penclen))
-		return 1;
+	pval = NULL;
+	key = NULL;
+	key_len = 0;
+
+	ret = 1;
+
  err:
-	if (ptype == V_ASN1_OBJECT)
-		ASN1_OBJECT_free(pval);
-	else
-		ASN1_STRING_free(pval);
-	free(penc);
-	return 0;
+	eckey_param_free(ptype, pval);
+	freezero(key, key_len);
+
+	return ret;
 }
 
 static EC_KEY *
@@ -308,54 +320,47 @@ eckey_priv_decode(EVP_PKEY *pkey, const PKCS8_PRIV_KEY_INFO *p8)
 static int
 eckey_priv_encode(PKCS8_PRIV_KEY_INFO *p8, const EVP_PKEY *pkey)
 {
-	EC_KEY *ec_key;
-	unsigned char *ep, *p;
-	int eplen, ptype;
-	void *pval;
-	unsigned int tmp_flags, old_flags;
+	EC_KEY *eckey = pkey->pkey.ec;
+	void *pval = NULL;
+	int ptype = V_ASN1_UNDEF;
+	ASN1_OBJECT *aobj;
+	unsigned char *key = NULL;
+	int key_len = 0;
+	unsigned int flags;
+	int ret = 0;
 
-	ec_key = pkey->pkey.ec;
+	flags = EC_KEY_get_enc_flags(eckey);
 
-	if (!eckey_param2type(&ptype, &pval, ec_key)) {
+	if (!eckey_param2type(&ptype, &pval, eckey)) {
 		ECerror(EC_R_DECODE_ERROR);
-		return 0;
+		goto err;
 	}
-	/* set the private key */
 
-	/*
-	 * do not include the parameters in the SEC1 private key see PKCS#11
-	 * 12.11
-	 */
-	old_flags = EC_KEY_get_enc_flags(ec_key);
-	tmp_flags = old_flags | EC_PKEY_NO_PARAMETERS;
-	EC_KEY_set_enc_flags(ec_key, tmp_flags);
-	eplen = i2d_ECPrivateKey(ec_key, NULL);
-	if (!eplen) {
-		EC_KEY_set_enc_flags(ec_key, old_flags);
-		ECerror(ERR_R_EC_LIB);
-		return 0;
-	}
-	ep = malloc(eplen);
-	if (!ep) {
-		EC_KEY_set_enc_flags(ec_key, old_flags);
-		ECerror(ERR_R_MALLOC_FAILURE);
-		return 0;
-	}
-	p = ep;
-	if (!i2d_ECPrivateKey(ec_key, &p)) {
-		EC_KEY_set_enc_flags(ec_key, old_flags);
-		free(ep);
+	/* PKCS#11 12.11: don't include parameters in the SEC1 private key. */
+	EC_KEY_set_enc_flags(eckey, flags | EC_PKEY_NO_PARAMETERS);
+
+	if ((key_len = i2d_ECPrivateKey(eckey, &key)) <= 0) {
 		ECerror(ERR_R_EC_LIB);
-		return 0;
+		key_len = 0;
+		goto err;
 	}
-	/* restore old encoding flags */
-	EC_KEY_set_enc_flags(ec_key, old_flags);
+	if ((aobj = OBJ_nid2obj(NID_X9_62_id_ecPublicKey)) == NULL)
+		goto err;
+	if (!PKCS8_pkey_set0(p8, aobj, 0, ptype, pval, key, key_len))
+		goto err;
+	pval = NULL;
+	key = NULL;
+	key_len = 0;
 
-	if (!PKCS8_pkey_set0(p8, OBJ_nid2obj(NID_X9_62_id_ecPublicKey), 0,
-		ptype, pval, ep, eplen))
-		return 0;
+	ret = 1;
 
-	return 1;
+ err:
+	eckey_param_free(ptype, pval);
+	freezero(key, key_len);
+
+	EC_KEY_set_enc_flags(eckey, flags);
+
+	return ret;
 }
 
 static int