Avoid potential double frees in i2v_AUTHORITY_KEYID(), i2v_GENERAL_NAME()

and i2v_GENERAL_NAMES() by taking ownership of the extlist only if we
were passed NULL.  Otherwise it remains the caller's responsibility to
free it.  To do so, we allocate the extlist explicitly instead of using
X509V3_add_value()'s implicit allocation feature.  Preserve behavior in
i2v_AUTHORITY_KEYID() by adding an explicit check that something was
pushed onto the stack.

The other i2v_* functions will receive a similar treatment in upcoming
commits.

ok jsing

2 files changed, 22 insertions, 6 deletions

diff --git a/lib/libcrypto/x509v3/v3_akey.c b/lib/libcrypto/x509v3/v3_akey.c
index 65184b19b60..e49f45fe0a6 100644
--- a/lib/libcrypto/x509v3/v3_akey.c
+++ b/lib/libcrypto/x509v3/v3_akey.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: v3_akey.c,v 1.21 2019/04/21 16:50:34 tb Exp $ */
+/* $OpenBSD: v3_akey.c,v 1.22 2019/04/22 17:10:01 tb Exp $ */
 /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL
  * project 1999.
  */
@@ -91,8 +91,14 @@ static STACK_OF(CONF_VALUE) *
 i2v_AUTHORITY_KEYID(X509V3_EXT_METHOD *method, AUTHORITY_KEYID *akeyid,
     STACK_OF(CONF_VALUE) *extlist)
 {
+	STACK_OF(CONF_VALUE) *free_extlist = NULL;
 	char *tmpstr = NULL;
 
+	if (extlist == NULL) {
+		if ((free_extlist = extlist = sk_CONF_VALUE_new_null()) == NULL)
+			return NULL;
+	}
+
 	if (akeyid->keyid != NULL) {
 		if ((tmpstr = hex_to_string(akeyid->keyid->data,
 		    akeyid->keyid->length)) == NULL)
@@ -119,11 +125,14 @@ i2v_AUTHORITY_KEYID(X509V3_EXT_METHOD *method, AUTHORITY_KEYID *akeyid,
 		tmpstr = NULL;
 	}
 
+	if (sk_CONF_VALUE_num(extlist) <= 0)
+		goto err;
+
 	return extlist;
 
  err:
 	free(tmpstr);
-	sk_CONF_VALUE_pop_free(extlist, X509V3_conf_free);
+	sk_CONF_VALUE_pop_free(free_extlist, X509V3_conf_free);
 
 	return NULL;
 }
diff --git a/lib/libcrypto/x509v3/v3_alt.c b/lib/libcrypto/x509v3/v3_alt.c
index 2dc07b4025f..0f0177ff8ba 100644
--- a/lib/libcrypto/x509v3/v3_alt.c
+++ b/lib/libcrypto/x509v3/v3_alt.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: v3_alt.c,v 1.29 2019/04/21 16:50:34 tb Exp $ */
+/* $OpenBSD: v3_alt.c,v 1.30 2019/04/22 17:10:01 tb Exp $ */
 /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL
  * project.
  */
@@ -127,11 +127,12 @@ STACK_OF(CONF_VALUE) *
 i2v_GENERAL_NAMES(X509V3_EXT_METHOD *method, GENERAL_NAMES *gens,
     STACK_OF(CONF_VALUE) *ret)
 {
+	STACK_OF(CONF_VALUE) *free_ret = NULL;
 	GENERAL_NAME *gen;
 	int i;
 
 	if (ret == NULL) {
-		if ((ret = sk_CONF_VALUE_new_null()) == NULL)
+		if ((free_ret = ret = sk_CONF_VALUE_new_null()) == NULL)
 			return NULL;
 	}
 
@@ -145,7 +146,7 @@ i2v_GENERAL_NAMES(X509V3_EXT_METHOD *method, GENERAL_NAMES *gens,
 	return ret;
 
  err:
-	sk_CONF_VALUE_pop_free(ret, X509V3_conf_free);
+	sk_CONF_VALUE_pop_free(free_ret, X509V3_conf_free);
 
 	return NULL;
 }
@@ -154,10 +155,16 @@ STACK_OF(CONF_VALUE) *
 i2v_GENERAL_NAME(X509V3_EXT_METHOD *method, GENERAL_NAME *gen,
     STACK_OF(CONF_VALUE) *ret)
 {
+	STACK_OF(CONF_VALUE) *free_ret = NULL;
 	unsigned char *p;
 	char oline[256], htmp[5];
 	int i;
 
+	if (ret == NULL) {
+		if ((free_ret = ret = sk_CONF_VALUE_new_null()) == NULL)
+			return NULL;
+	}
+
 	switch (gen->type) {
 	case GEN_OTHERNAME:
 		if (!X509V3_add_value("othername", "<unsupported>", &ret))
@@ -231,7 +238,7 @@ i2v_GENERAL_NAME(X509V3_EXT_METHOD *method, GENERAL_NAME *gen,
 	return ret;
 
  err:
-	sk_CONF_VALUE_pop_free(ret, X509V3_conf_free);
+	sk_CONF_VALUE_pop_free(free_ret, X509V3_conf_free);
 
 	return NULL;
 }