From: jsing <jsing@openbsd.org>
Date: Mon, 9 May 2022 19:19:33 +0000 (+0000)
Subject: Simplify X509_ATTRIBUTE ASN.1 encoding.
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=ab72b8b8c221a7e276a078466106e0297ab6d900;p=openbsd

Simplify X509_ATTRIBUTE ASN.1 encoding.

For some unknown historical reason, X509_ATTRIBUTE allows for a single
ASN.1 value or an ASN.1 SET OF, rather than requiring an ASN.1 SET OF.
Simplify encoding and remove support for single values - this is similar
to OpenSSL e20b57270dec.

This removes the last use of COMBINE in the ASN.1 decoder.

ok tb@
---

diff --git a/lib/libcrypto/asn1/t_req.c b/lib/libcrypto/asn1/t_req.c
index cc9da46439a..4b27a4ddbe1 100644
--- a/lib/libcrypto/asn1/t_req.c
+++ b/lib/libcrypto/asn1/t_req.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: t_req.c,v 1.21 2021/12/25 13:17:48 jsing Exp $ */
+/* $OpenBSD: t_req.c,v 1.22 2022/05/09 19:19:33 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -176,7 +176,6 @@ X509_REQ_print_ex(BIO *bp, X509_REQ *x, unsigned long nmflags,
 				ASN1_TYPE *at;
 				X509_ATTRIBUTE *a;
 				ASN1_BIT_STRING *bs = NULL;
-				ASN1_TYPE *t;
 				int j, type = 0, count = 1, ii = 0;
 
 				a = sk_X509_ATTRIBUTE_value(sk, i);
@@ -186,20 +185,12 @@ X509_REQ_print_ex(BIO *bp, X509_REQ *x, unsigned long nmflags,
 				if (BIO_printf(bp, "%12s", "") <= 0)
 					goto err;
 				if ((j = i2a_ASN1_OBJECT(bp, a->object)) > 0) {
-					if (a->single) {
-						t = a->value.single;
-						type = t->type;
-						bs = t->value.bit_string;
-					} else {
-						ii = 0;
-						count = sk_ASN1_TYPE_num(
-						    a->value.set);
+					ii = 0;
+					count = sk_ASN1_TYPE_num(a->set);
  get_next:
-						at = sk_ASN1_TYPE_value(
-						    a->value.set, ii);
-						type = at->type;
-						bs = at->value.asn1_string;
-					}
+					at = sk_ASN1_TYPE_value(a->set, ii);
+					type = at->type;
+					bs = at->value.asn1_string;
 				}
 				for (j = 25 - j; j > 0; j--)
 					if (BIO_write(bp, " ", 1) != 1)
diff --git a/lib/libcrypto/asn1/x_attrib.c b/lib/libcrypto/asn1/x_attrib.c
index 47b5afd95d2..e8822a33a5a 100644
--- a/lib/libcrypto/asn1/x_attrib.c
+++ b/lib/libcrypto/asn1/x_attrib.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: x_attrib.c,v 1.16 2021/12/25 13:17:48 jsing Exp $ */
+/* $OpenBSD: x_attrib.c,v 1.17 2022/05/09 19:19:33 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -64,48 +64,14 @@
 
 #include "x509_lcl.h"
 
-/* X509_ATTRIBUTE: this has the following form:
- *
- * typedef struct x509_attributes_st
- *	{
- *	ASN1_OBJECT *object;
- *	int single;
- *	union	{
- *		char		*ptr;
- * 		STACK_OF(ASN1_TYPE) *set;
- * 		ASN1_TYPE	*single;
- *		} value;
- *	} X509_ATTRIBUTE;
- *
- * this needs some extra thought because the CHOICE type is
- * merged with the main structure and because the value can
- * be anything at all we *must* try the SET OF first because
- * the ASN1_ANY type will swallow anything including the whole
- * SET OF structure.
+/*
+ * XXX - remove X509_ATTRIBUTE_SET_it with next major bump.
  */
-
-static const ASN1_TEMPLATE X509_ATTRIBUTE_SET_ch_tt[] = {
-	{
-		.flags = ASN1_TFLG_SET_OF,
-		.tag = 0,
-		.offset = offsetof(X509_ATTRIBUTE, value.set),
-		.field_name = "value.set",
-		.item = &ASN1_ANY_it,
-	},
-	{
-		.flags = 0,
-		.tag = 0,
-		.offset = offsetof(X509_ATTRIBUTE, value.single),
-		.field_name = "value.single",
-		.item = &ASN1_ANY_it,
-	},
-};
-
 const ASN1_ITEM X509_ATTRIBUTE_SET_it = {
 	.itype = ASN1_ITYPE_CHOICE,
-	.utype = offsetof(X509_ATTRIBUTE, single),
-	.templates = X509_ATTRIBUTE_SET_ch_tt,
-	.tcount = sizeof(X509_ATTRIBUTE_SET_ch_tt) / sizeof(ASN1_TEMPLATE),
+	.utype = 0,
+	.templates = NULL,
+	.tcount = 0,
 	.funcs = NULL,
 	.size = sizeof(X509_ATTRIBUTE),
 	.sname = "X509_ATTRIBUTE",
@@ -119,13 +85,12 @@ static const ASN1_TEMPLATE X509_ATTRIBUTE_seq_tt[] = {
 		.field_name = "object",
 		.item = &ASN1_OBJECT_it,
 	},
-	/* CHOICE type merged with parent */
 	{
-		.flags = 0 | ASN1_TFLG_COMBINE,
+		.flags = ASN1_TFLG_SET_OF,
 		.tag = 0,
-		.offset = 0,
-		.field_name = NULL,
-		.item = &X509_ATTRIBUTE_SET_it,
+		.offset = offsetof(X509_ATTRIBUTE, set),
+		.field_name = "set",
+		.item = &ASN1_ANY_it,
 	},
 };
 
@@ -183,12 +148,9 @@ X509_ATTRIBUTE_create(int nid, int atrtype, void *value)
 	if ((ret = X509_ATTRIBUTE_new()) == NULL)
 		return (NULL);
 	ret->object = oid;
-	ret->single = 0;
-	if ((ret->value.set = sk_ASN1_TYPE_new_null()) == NULL)
-		goto err;
 	if ((val = ASN1_TYPE_new()) == NULL)
 		goto err;
-	if (!sk_ASN1_TYPE_push(ret->value.set, val))
+	if (!sk_ASN1_TYPE_push(ret->set, val))
 		goto err;
 
 	ASN1_TYPE_set(val, atrtype, value);
diff --git a/lib/libcrypto/pkcs12/p12_attr.c b/lib/libcrypto/pkcs12/p12_attr.c
index dc38b7c897c..a35a148b11e 100644
--- a/lib/libcrypto/pkcs12/p12_attr.c
+++ b/lib/libcrypto/pkcs12/p12_attr.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: p12_attr.c,v 1.14 2021/11/01 20:53:08 tb Exp $ */
+/* $OpenBSD: p12_attr.c,v 1.15 2022/05/09 19:19:33 jsing Exp $ */
 /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL
  * project 1999.
  */
@@ -129,12 +129,8 @@ PKCS12_get_attr_gen(const STACK_OF(X509_ATTRIBUTE) *attrs, int attr_nid)
 		return NULL;
 	for (i = 0; i < sk_X509_ATTRIBUTE_num(attrs); i++) {
 		attrib = sk_X509_ATTRIBUTE_value(attrs, i);
-		if (OBJ_obj2nid(attrib->object) == attr_nid) {
-			if (sk_ASN1_TYPE_num(attrib->value.set))
-				return sk_ASN1_TYPE_value(attrib->value.set, 0);
-			else
-				return NULL;
-		}
+		if (OBJ_obj2nid(attrib->object) == attr_nid)
+			return sk_ASN1_TYPE_value(attrib->set, 0);
 	}
 	return NULL;
 }
diff --git a/lib/libcrypto/pkcs7/pk7_doit.c b/lib/libcrypto/pkcs7/pk7_doit.c
index c9d64bcf342..b3140696104 100644
--- a/lib/libcrypto/pkcs7/pk7_doit.c
+++ b/lib/libcrypto/pkcs7/pk7_doit.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: pk7_doit.c,v 1.46 2021/12/12 21:30:14 tb Exp $ */
+/* $OpenBSD: pk7_doit.c,v 1.47 2022/05/09 19:19:33 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -1129,12 +1129,8 @@ get_attribute(STACK_OF(X509_ATTRIBUTE) *sk, int nid)
 		return (NULL);
 	for (i = 0; i < sk_X509_ATTRIBUTE_num(sk); i++) {
 		xa = sk_X509_ATTRIBUTE_value(sk, i);
-		if (OBJ_cmp(xa->object, o) == 0) {
-			if (!xa->single && sk_ASN1_TYPE_num(xa->value.set))
-				return (sk_ASN1_TYPE_value(xa->value.set, 0));
-			else
-				return (NULL);
-		}
+		if (OBJ_cmp(xa->object, o) == 0)
+			return (sk_ASN1_TYPE_value(xa->set, 0));
 	}
 	return (NULL);
 }
diff --git a/lib/libcrypto/x509/x509_att.c b/lib/libcrypto/x509/x509_att.c
index 38aa0631432..8d369df9006 100644
--- a/lib/libcrypto/x509/x509_att.c
+++ b/lib/libcrypto/x509/x509_att.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: x509_att.c,v 1.18 2021/11/01 20:53:08 tb Exp $ */
+/* $OpenBSD: x509_att.c,v 1.19 2022/05/09 19:19:33 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -324,10 +324,8 @@ X509_ATTRIBUTE_set1_data(X509_ATTRIBUTE *attr, int attrtype, const void *data,
 			goto err;
 		atype = attrtype;
 	}
-	if (!(attr->value.set = sk_ASN1_TYPE_new_null()))
-		goto err;
-	attr->single = 0;
-	/* This is a bit naughty because the attribute should really have
+	/*
+	 * This is a bit naughty because the attribute should really have
 	 * at least one value but some types use and zero length SET and
 	 * require this.
 	 */
@@ -343,7 +341,7 @@ X509_ATTRIBUTE_set1_data(X509_ATTRIBUTE *attr, int attrtype, const void *data,
 			goto err;
 	} else
 		ASN1_TYPE_set(ttmp, atype, stmp);
-	if (!sk_ASN1_TYPE_push(attr->value.set, ttmp))
+	if (!sk_ASN1_TYPE_push(attr->set, ttmp))
 		goto err;
 	return 1;
 
@@ -357,11 +355,10 @@ err:
 int
 X509_ATTRIBUTE_count(const X509_ATTRIBUTE *attr)
 {
-	if (!attr->single)
-		return sk_ASN1_TYPE_num(attr->value.set);
-	if (attr->value.single)
-		return 1;
-	return 0;
+	if (attr == NULL)
+		return 0;
+
+	return sk_ASN1_TYPE_num(attr->set);
 }
 
 ASN1_OBJECT *
@@ -392,10 +389,6 @@ X509_ATTRIBUTE_get0_type(X509_ATTRIBUTE *attr, int idx)
 {
 	if (attr == NULL)
 		return (NULL);
-	if (idx >= X509_ATTRIBUTE_count(attr))
-		return NULL;
-	if (!attr->single)
-		return sk_ASN1_TYPE_value(attr->value.set, idx);
-	else
-		return attr->value.single;
+
+	return sk_ASN1_TYPE_value(attr->set, idx);
 }
diff --git a/lib/libcrypto/x509/x509_lcl.h b/lib/libcrypto/x509/x509_lcl.h
index e7eb733f7d9..5beef8a94dc 100644
--- a/lib/libcrypto/x509/x509_lcl.h
+++ b/lib/libcrypto/x509/x509_lcl.h
@@ -109,12 +109,7 @@ struct X509_extension_st {
 
 struct x509_attributes_st {
 	ASN1_OBJECT *object;
-	int single; /* 0 for a set, 1 for a single item (which is wrong) */
-	union {
-		char		*ptr;
-/* 0 */		STACK_OF(ASN1_TYPE) *set;
-/* 1 */		ASN1_TYPE	*single;
-	} value;
+	STACK_OF(ASN1_TYPE) *set;
 } /* X509_ATTRIBUTE */;
 
 struct X509_req_info_st {