When X509_ATTRIBUTE_create() receives an invalid NID (e.g., -1), return

failure rather than silently constructing a broken X509_ATTRIBUTE object
that might cause NULL pointer accesses later on.  This matters because
X509_ATTRIBUTE_create() is used by documented API functions like
PKCS7_add_attribute(3) and the NID comes straight from the user.

This fixes a bug found while working on documentation.

OK tb@  and  "thanks" bluhm@

4 files changed, 124 insertions, 19 deletions

diff --git a/lib/libcrypto/asn1/x_attrib.c b/lib/libcrypto/asn1/x_attrib.c
index bb74a1b6c71..04816eab770 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.13 2015/02/14 14:56:45 jsing Exp $ */
+/* $OpenBSD: x_attrib.c,v 1.14 2020/06/04 21:21:03 schwarze Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -174,10 +174,13 @@ X509_ATTRIBUTE_create(int nid, int atrtype, void *value)
 {
 	X509_ATTRIBUTE *ret = NULL;
 	ASN1_TYPE *val = NULL;
+	ASN1_OBJECT *oid;
 
+	if ((oid = OBJ_nid2obj(nid)) == NULL)
+		return (NULL);
 	if ((ret = X509_ATTRIBUTE_new()) == NULL)
 		return (NULL);
-	ret->object = OBJ_nid2obj(nid);
+	ret->object = oid;
 	ret->single = 0;
 	if ((ret->value.set = sk_ASN1_TYPE_new_null()) == NULL)
 		goto err;
diff --git a/lib/libcrypto/man/PKCS7_add_attribute.3 b/lib/libcrypto/man/PKCS7_add_attribute.3
index 09c36a4d5d3..081703f0f32 100644
--- a/lib/libcrypto/man/PKCS7_add_attribute.3
+++ b/lib/libcrypto/man/PKCS7_add_attribute.3
@@ -1,4 +1,4 @@
-.\" $OpenBSD: PKCS7_add_attribute.3,v 1.1 2020/06/04 10:24:27 schwarze Exp $
+.\" $OpenBSD: PKCS7_add_attribute.3,v 1.2 2020/06/04 21:21:03 schwarze Exp $
 .\"
 .\" Copyright (c) 2020 Ingo Schwarze <schwarze@openbsd.org>
 .\"
@@ -123,7 +123,9 @@ exist.
 and
 .Fn PKCS7_add_signed_attribute
 return 1 on success or 0 on failure.
-The most common reason for failure is lack of memory.
+The most common reasons for failure are an invalid
+.Fa nid
+argument or lack of memory.
 .Pp
 .Fn PKCS7_get_attribute
 and
@@ -153,16 +155,6 @@ These functions first appeared in OpenSSL 0.9.1
 and have been available since
 .Ox 2.6 .
 .Sh BUGS
-Adding an attribute with an invalid
-.Fa nid
-ought to fail, but it actually succeeds
-setting the type of the new attribute to
-.Dv NULL .
-Subsequent attempts to retrieve attributes
-may cause the program to crash due to
-.Dv NULL
-pointer access.
-.Pp
 A function to remove individual attributes from these lists
 does not appear to exist.
 A program desiring to do that might have to manually iterate the fields
diff --git a/regress/lib/libcrypto/x509/Makefile b/regress/lib/libcrypto/x509/Makefile
index 106a9f2bf2b..19edf9398b8 100644
--- a/regress/lib/libcrypto/x509/Makefile
+++ b/regress/lib/libcrypto/x509/Makefile
@@ -1,16 +1,19 @@
-#	$OpenBSD: Makefile,v 1.1 2018/04/07 13:54:46 schwarze Exp $
+#	$OpenBSD: Makefile,v 1.2 2020/06/04 21:21:03 schwarze Exp $
 
-PROG=	x509name
+PROGS =	x509attribute x509name
 LDADD=	-lcrypto
 DPADD=	${LIBCRYPTO}
 WARNINGS=	Yes
 CFLAGS+=	-Wall -Werror
 
-REGRESS_TARGETS=regress-x509name
+REGRESS_TARGETS=regress-x509attribute regress-x509name
 CLEANFILES+=	x509name.result
 
-regress-x509name: ${PROG}
-	./${PROG} > x509name.result
+regress-x509attribute: x509attribute
+	./x509attribute
+
+regress-x509name: x509name
+	./x509name > x509name.result
 	diff -u ${.CURDIR}/x509name.expected x509name.result
 
 .include <bsd.regress.mk>
diff --git a/regress/lib/libcrypto/x509/x509attribute.c b/regress/lib/libcrypto/x509/x509attribute.c
new file mode 100644
index 00000000000..3dd6d2912c8
--- /dev/null
+++ b/regress/lib/libcrypto/x509/x509attribute.c
@@ -0,0 +1,107 @@
+/* $OpenBSD: x509attribute.c,v 1.1 2020/06/04 21:21:03 schwarze Exp $ */
+/*
+ * Copyright (c) 2020 Ingo Schwarze <schwarze@openbsd.org>
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include <errno.h>
+#include <stdio.h>
+#include <string.h>
+
+#include <openssl/err.h>
+#include <openssl/x509.h>
+
+void	 fail_head(const char *);
+void	 fail_tail(void);
+void	 fail_str(const char *, const char *);
+void	 fail_int(const char *, int);
+
+static const char	*testname;
+static int		 errcount;
+
+void
+fail_head(const char *stepname)
+{
+	fprintf(stderr, "failure#%d testname=%s stepname=%s ",
+	    ++errcount, testname, stepname);
+}
+
+void
+fail_tail(void)
+{
+	unsigned long	 errnum;
+
+	if ((errnum = ERR_get_error()))
+		fprintf(stderr, "OpenSSL says: %s\n",
+		    ERR_error_string(errnum, NULL));
+	if (errno)
+		fprintf(stderr, "libc says: %s\n", strerror(errno));
+}
+
+void
+fail_str(const char *stepname, const char *result)
+{
+	fail_head(stepname);
+	fprintf(stderr, "wrong result=%s\n", result);
+	fail_tail();
+}
+
+void
+fail_int(const char *stepname, int result)
+{
+	fail_head(stepname);
+	fprintf(stderr, "wrong result=%d\n", result);
+	fail_tail();
+}
+
+int
+main(void)
+{
+	X509_ATTRIBUTE	*attrib;
+	ASN1_TYPE	*any;
+	ASN1_OBJECT	*coid;
+	int		 num;
+
+	testname = "preparation";
+	if ((coid = OBJ_nid2obj(NID_pkcs7_data)) == NULL) {
+		fail_str("OBJ_nid2obj", "NULL");
+		return 1;
+	}
+
+	testname = "valid_args";
+	if ((attrib = X509_ATTRIBUTE_create(NID_pkcs9_contentType,
+	    V_ASN1_OBJECT, coid)) == NULL)
+		fail_str("X509_ATTRIBUTE_create", "NULL");
+	else if (attrib->object == NULL)
+		fail_str("attrib->object", "NULL");
+	else if (attrib->single)
+		fail_int("attrib->single", attrib->single);
+	else if ((num = sk_ASN1_TYPE_num(attrib->value.set)) != 1)
+		fail_int("num", num);
+	else if ((any = sk_ASN1_TYPE_value(attrib->value.set, 0)) == NULL)
+		fail_str("any", "NULL");
+	else if (any->type != V_ASN1_OBJECT)
+		fail_int("any->type", any->type);
+	else if (any->value.object != coid)
+		fail_str("value", "wrong pointer");
+	X509_ATTRIBUTE_free(attrib);
+
+	testname = "bad_nid";
+	if ((attrib = X509_ATTRIBUTE_create(-1,
+	    V_ASN1_OBJECT, coid)) != NULL)
+		fail_str("X509_ATTRIBUTE_create", "not NULL");
+	X509_ATTRIBUTE_free(attrib);
+
+	return errcount != 0;
+}