From d0cf9aeca512581235a63d9ed8e8a3c69039b9df Mon Sep 17 00:00:00 2001 From: schwarze <> Date: Thu, 4 Jun 2020 21:21:03 +0000 Subject: 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@ --- src/lib/libcrypto/asn1/x_attrib.c | 7 +- src/lib/libcrypto/man/PKCS7_add_attribute.3 | 16 +--- src/regress/lib/libcrypto/x509/Makefile | 13 +-- src/regress/lib/libcrypto/x509/x509attribute.c | 107 +++++++++++++++++++++++++ 4 files changed, 124 insertions(+), 19 deletions(-) create mode 100644 src/regress/lib/libcrypto/x509/x509attribute.c 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 .\" @@ -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 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 + * + * 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 +#include +#include + +#include +#include + +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; +} -- cgit v1.2.3-55-g6feb