From 5c9cc2d0035cb59dfcacb47001ca9b8392b3732b Mon Sep 17 00:00:00 2001
From: tb <>
Date: Thu, 31 Jul 2025 02:02:35 +0000
Subject: Plug leaks due to misuse of PKCS7_add_signed_attribute()

set0/add0 functions that can fail are the worst. Without fail this trips
up both users and authors (by and large these are two identical groups
consisting of a single person), resulting in leaks and double frees.

In today's episode of spelunking in the gruesome gore provided by the
PKCS#7 and Time-Stamp protocol "implementations", we fix a couple of
leaks in PKCS7_add_attrib_smimecap() and ESS_add_signing_cert().

We do so by recalling that there is i2d_X509_ALGORS(), so we might
as well put it to use instead of inlining it poorly (aka, without
error checking). Normalize said error checking and ensure ownership
is handled correctly in the usual single-exit idiom.

ESS_add_signing_cert() can also make use of proper i2d handling, so
it's simpler and correct and in the end looks pretty much the same
as PKCS7_add_attrib_smimecap().

ok kenjiro
---
 src/lib/libcrypto/pkcs7/pk7_attr.c | 38 ++++++++++++++++++++++++++++--------
 src/lib/libcrypto/ts/ts_rsp_sign.c | 40 +++++++++++++++++++++-----------------
 2 files changed, 52 insertions(+), 26 deletions(-)

(limited to 'src')

diff --git a/src/lib/libcrypto/pkcs7/pk7_attr.c b/src/lib/libcrypto/pkcs7/pk7_attr.c
index bf775735d9..9a822eaecd 100644
--- a/src/lib/libcrypto/pkcs7/pk7_attr.c
+++ b/src/lib/libcrypto/pkcs7/pk7_attr.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: pk7_attr.c,v 1.18 2025/07/28 04:29:00 tb Exp $ */
+/* $OpenBSD: pk7_attr.c,v 1.19 2025/07/31 02:02:35 tb Exp $ */
 /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL
  * project 2001.
  */
@@ -69,15 +69,37 @@
 int
 PKCS7_add_attrib_smimecap(PKCS7_SIGNER_INFO *si, STACK_OF(X509_ALGOR) *cap)
 {
-	ASN1_STRING *seq;
-	if (!(seq = ASN1_STRING_new())) {
+	ASN1_STRING *seq = NULL;
+	unsigned char *data = NULL;
+	int len = 0;
+	int ret = 0;
+
+	if ((len = i2d_X509_ALGORS(cap, &data)) <= 0) {
+		len = 0;
+		goto err;
+	}
+
+	if ((seq = ASN1_STRING_new()) == NULL) {
 		PKCS7error(ERR_R_MALLOC_FAILURE);
-		return 0;
+		goto err;
 	}
-	seq->length = ASN1_item_i2d((ASN1_VALUE *)cap, &seq->data,
-	    &X509_ALGORS_it);
-	return PKCS7_add_signed_attribute(si, NID_SMIMECapabilities,
-	    V_ASN1_SEQUENCE, seq);
+
+	ASN1_STRING_set0(seq, data, len);
+	data = NULL;
+	len = 0;
+
+	if (!PKCS7_add_signed_attribute(si, NID_SMIMECapabilities,
+	    V_ASN1_SEQUENCE, seq))
+		goto err;
+	seq = NULL;
+
+	ret = 1;
+
+ err:
+	ASN1_STRING_free(seq);
+	freezero(data, len);
+
+	return ret;
 }
 LCRYPTO_ALIAS(PKCS7_add_attrib_smimecap);
 
diff --git a/src/lib/libcrypto/ts/ts_rsp_sign.c b/src/lib/libcrypto/ts/ts_rsp_sign.c
index d35f6a7c94..b8cc7e2baf 100644
--- a/src/lib/libcrypto/ts/ts_rsp_sign.c
+++ b/src/lib/libcrypto/ts/ts_rsp_sign.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ts_rsp_sign.c,v 1.36 2025/05/10 05:54:39 tb Exp $ */
+/* $OpenBSD: ts_rsp_sign.c,v 1.37 2025/07/31 02:02:35 tb Exp $ */
 /* Written by Zoltan Glozik (zglozik@stones.com) for the OpenSSL
  * project 2002.
  */
@@ -955,28 +955,32 @@ static int
 ESS_add_signing_cert(PKCS7_SIGNER_INFO *si, ESS_SIGNING_CERT *sc)
 {
 	ASN1_STRING *seq = NULL;
-	unsigned char *p, *pp = NULL;
-	int len;
+	unsigned char *data = NULL;
+	int len = 0;
+	int ret = 0;
 
-	len = i2d_ESS_SIGNING_CERT(sc, NULL);
-	if (!(pp = malloc(len))) {
-		TSerror(ERR_R_MALLOC_FAILURE);
+	if ((len = i2d_ESS_SIGNING_CERT(sc, &data)) <= 0) {
+		len = 0;
 		goto err;
 	}
-	p = pp;
-	i2d_ESS_SIGNING_CERT(sc, &p);
-	if (!(seq = ASN1_STRING_new()) || !ASN1_STRING_set(seq, pp, len)) {
-		TSerror(ERR_R_MALLOC_FAILURE);
+
+	if ((seq = ASN1_STRING_new()) == NULL)
 		goto err;
-	}
-	free(pp);
-	pp = NULL;
-	return PKCS7_add_signed_attribute(si,
-	    NID_id_smime_aa_signingCertificate, V_ASN1_SEQUENCE, seq);
 
-err:
+	ASN1_STRING_set0(seq, data, len);
+	data = NULL;
+	len = 0;
+
+	if (!PKCS7_add_signed_attribute(si, NID_id_smime_aa_signingCertificate,
+	    V_ASN1_SEQUENCE, seq))
+		goto err;
+	seq = NULL;
+
+	ret = 1;
+
+ err:
 	ASN1_STRING_free(seq);
-	free(pp);
+	freezero(data, len);
 
-	return 0;
+	return ret;
 }
-- 
cgit v1.2.3-55-g6feb