From 5d593caa4397fa2eafb4771d98476f5a53cecd1a Mon Sep 17 00:00:00 2001
From: tb <>
Date: Sun, 27 Jul 2025 07:06:41 +0000
Subject: Fix incorrect ownership handling in add_attribute()

This little gem has a number of issues.

On failure, the caller can't know whether ownership of value was taken
or not, so to avoid a double free, the only option is to leak value on
failure. As X509_ATTRIBUTE_create() takes ownership on success, this
call must be the last one that can fail. This way ownership is only
taken on success.

Next, if X509_ATTRIBUTE_create() fails in the case that the input stack
already contains an attribute of type nid, that attr is freed and the
caller freeing the stack with pop_free() will double free.

So, rework this in a few ways. Make this transactional, so we don't fail
with a modified *in_sk, so work with a local sk as usual. Then walk the
stack and see if we have an attribute with the appropriate nid already.
If not, make sure there's room to place the new attribute. Create the
new attribute, free the old attribute if necessary and replace it with
the new one. Finally assign the local sk to *in_sk and return success.
On error unwind all we did.

The behavior now matches OpenSSL 3's new behavior, except that we don't
leave an empty stack around on error.

ok kenjiro
---
 src/lib/libcrypto/pkcs7/pk7_doit.c | 76 +++++++++++++++++++++-----------------
 1 file changed, 42 insertions(+), 34 deletions(-)

(limited to 'src')

diff --git a/src/lib/libcrypto/pkcs7/pk7_doit.c b/src/lib/libcrypto/pkcs7/pk7_doit.c
index 3a2076a25f..e39d960780 100644
--- a/src/lib/libcrypto/pkcs7/pk7_doit.c
+++ b/src/lib/libcrypto/pkcs7/pk7_doit.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: pk7_doit.c,v 1.60 2025/05/10 05:54:38 tb Exp $ */
+/* $OpenBSD: pk7_doit.c,v 1.61 2025/07/27 07:06:41 tb Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -1208,43 +1208,51 @@ PKCS7_set_attributes(PKCS7_SIGNER_INFO *p7si, STACK_OF(X509_ATTRIBUTE) *sk)
 LCRYPTO_ALIAS(PKCS7_set_attributes);
 
 static int
-add_attribute(STACK_OF(X509_ATTRIBUTE) **sk, int nid, int atrtype, void *value)
+add_attribute(STACK_OF(X509_ATTRIBUTE) **in_sk, int nid, int atrtype, void *value)
 {
-	X509_ATTRIBUTE *attr = NULL;
+	STACK_OF(X509_ATTRIBUTE) *sk;
+	X509_ATTRIBUTE *old_attr = NULL, *new_attr = NULL;
+	int need_pop = 0;
+	int i;
 
-	if (*sk == NULL) {
-		*sk = sk_X509_ATTRIBUTE_new_null();
-		if (*sk == NULL)
-			return 0;
-new_attrib:
-		if (!(attr = X509_ATTRIBUTE_create(nid, atrtype, value)))
-			return 0;
-		if (!sk_X509_ATTRIBUTE_push(*sk, attr)) {
-			X509_ATTRIBUTE_free(attr);
-			return 0;
-		}
-	} else {
-		int i;
-
-		for (i = 0; i < sk_X509_ATTRIBUTE_num(*sk); i++) {
-			attr = sk_X509_ATTRIBUTE_value(*sk, i);
-			if (OBJ_obj2nid(attr->object) == nid) {
-				X509_ATTRIBUTE_free(attr);
-				attr = X509_ATTRIBUTE_create(nid, atrtype,
-				    value);
-				if (attr == NULL)
-					return 0;
-				if (!sk_X509_ATTRIBUTE_set(*sk, i, attr)) {
-					X509_ATTRIBUTE_free(attr);
-					return 0;
-				}
-				goto end;
-			}
-		}
-		goto new_attrib;
+	if ((sk = *in_sk) == NULL)
+		sk = sk_X509_ATTRIBUTE_new_null();
+	if (sk == NULL)
+		goto err;
+
+	/* Replace an already existing attribute with the given nid. */
+	for (i = 0; i < sk_X509_ATTRIBUTE_num(sk); i++) {
+		old_attr = sk_X509_ATTRIBUTE_value(sk, i);
+		if(OBJ_obj2nid(old_attr->object) == nid)
+			break;
+	}
+
+	/* If there is none, make room for the new one, so _set() succeeds. */
+	if (i == sk_X509_ATTRIBUTE_num(sk)) {
+		old_attr = NULL;
+		if (sk_X509_ATTRIBUTE_push(sk, NULL) <= 0)
+			goto err;
+		need_pop = 1;
 	}
-end:
+
+	/* On success, new_attr owns value. */
+	if ((new_attr = X509_ATTRIBUTE_create(nid, atrtype, value)) == NULL)
+		goto err;
+
+	X509_ATTRIBUTE_free(old_attr);
+	(void)sk_X509_ATTRIBUTE_set(sk, i, new_attr);
+
+	*in_sk = sk;
+
 	return 1;
+
+ err:
+	if (need_pop)
+		(void)sk_X509_ATTRIBUTE_pop(sk);
+	if (*in_sk != sk)
+		sk_X509_ATTRIBUTE_pop_free(sk, X509_ATTRIBUTE_free);
+
+	return 0;
 }
 
 int
-- 
cgit v1.2.3-55-g6feb