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/src/lib/libcrypto/asn1/x_attrib.c b/src/lib/libcrypto/asn1/x_attrib.c
index bb74a1b6c7..04816eab77 100644
--- a/src/lib/libcrypto/asn1/x_attrib.c
+++ b/src/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/src/lib/libcrypto/man/PKCS7_add_attribute.3 b/src/lib/libcrypto/man/PKCS7_add_attribute.3
index 09c36a4d5d..081703f0f3 100644
--- a/src/lib/libcrypto/man/PKCS7_add_attribute.3
+++ b/src/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/src/regress/lib/libcrypto/x509/Makefile b/src/regress/lib/libcrypto/x509/Makefile
index 106a9f2bf2..19edf9398b 100644
--- a/src/regress/lib/libcrypto/x509/Makefile
+++ b/src/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/src/regress/lib/libcrypto/x509/x509attribute.c b/src/regress/lib/libcrypto/x509/x509attribute.c
new file mode 100644
index 0000000000..3dd6d2912c
--- /dev/null
+++ b/src/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;
+}