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@

2 files changed, 115 insertions, 5 deletions

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;
+}