Merge two bugfixes in ASN1_STRING_TABLE_add(3) and ASN1_STRING_TABLE_get(3)

from the OpenSSL 1.1.1 branch, which is still under a free license,
mostly this commit:

commit d35c0ff30b31be9fd5dcf3d552a16feb8de464bc
Author: Dr. Stephen Henson <steve@openssl.org>
Date: Fri Oct 19 15:06:31 2012 +0000
fix ASN1_STRING_TABLE_add so it can override existing string table values

This fixes a segfault in ASN1_STRING_TABLE_add(3), which tried to change a
static const entry when called with an nid already in the default table,
and it switches the precedence of the two tables in ASN1_STRING_TABLE_get(3).

In addition, it changes behaviour in the following minor ways:
* Ignore negative minsize and maxsize arguments, not just -1.
* Ignore a zero mask and zero flags.
It's unclear whether these additional changes make the API absolutely
better, but we want compatibility with OpenSSL in these functions.

Tweaks & OK tb@.

3 files changed, 189 insertions, 36 deletions

diff --git a/lib/libcrypto/asn1/a_strnid.c b/lib/libcrypto/asn1/a_strnid.c
index 0cc8dc84285..08043f723b6 100644
--- a/lib/libcrypto/asn1/a_strnid.c
+++ b/lib/libcrypto/asn1/a_strnid.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: a_strnid.c,v 1.22 2021/12/11 22:34:36 schwarze Exp $ */
+/* $OpenBSD: a_strnid.c,v 1.23 2021/12/11 22:58:48 schwarze Exp $ */
 /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL
  * project 1999.
  */
@@ -64,6 +64,8 @@
 #include <openssl/objects.h>
 
 static STACK_OF(ASN1_STRING_TABLE) *stable = NULL;
+
+static ASN1_STRING_TABLE *stable_get(int nid);
 static void st_free(ASN1_STRING_TABLE *tbl);
 static int sk_table_cmp(const ASN1_STRING_TABLE * const *a,
     const ASN1_STRING_TABLE * const *b);
@@ -235,20 +237,59 @@ ASN1_STRING_TABLE *
 ASN1_STRING_TABLE_get(int nid)
 {
 	int idx;
-	ASN1_STRING_TABLE *ttmp;
 	ASN1_STRING_TABLE fnd;
 
 	fnd.nid = nid;
-	ttmp = OBJ_bsearch_table(&fnd, tbl_standard,
+	if (stable != NULL) {
+		idx = sk_ASN1_STRING_TABLE_find(stable, &fnd);
+		if (idx >= 0)
+			return sk_ASN1_STRING_TABLE_value(stable, idx);
+	}
+	return OBJ_bsearch_table(&fnd, tbl_standard,
 	    sizeof(tbl_standard)/sizeof(ASN1_STRING_TABLE));
-	if (ttmp)
-		return ttmp;
-	if (!stable)
+}
+
+/*
+ * Return a string table pointer which can be modified: either directly
+ * from table or a copy of an internal value added to the table.
+ */
+
+static ASN1_STRING_TABLE *
+stable_get(int nid)
+{
+	ASN1_STRING_TABLE *tmp, *rv;
+
+	/* Always need a string table so allocate one if NULL */
+	if (stable == NULL) {
+		stable = sk_ASN1_STRING_TABLE_new(sk_table_cmp);
+		if (stable == NULL)
+			return NULL;
+	}
+	tmp = ASN1_STRING_TABLE_get(nid);
+	if (tmp != NULL && (tmp->flags & STABLE_FLAGS_MALLOC) != 0)
+		return tmp;
+
+	if ((rv = calloc(1, sizeof(*rv))) == NULL) {
+		ASN1error(ERR_R_MALLOC_FAILURE);
 		return NULL;
-	idx = sk_ASN1_STRING_TABLE_find(stable, &fnd);
-	if (idx < 0)
+	}
+	if (!sk_ASN1_STRING_TABLE_push(stable, rv)) {
+		free(rv);
 		return NULL;
-	return sk_ASN1_STRING_TABLE_value(stable, idx);
+	}
+	if (tmp != NULL) {
+		rv->nid = tmp->nid;
+		rv->minsize = tmp->minsize;
+		rv->maxsize = tmp->maxsize;
+		rv->mask = tmp->mask;
+		rv->flags = tmp->flags | STABLE_FLAGS_MALLOC;
+	} else {
+		rv->nid = nid;
+		rv->minsize = -1;
+		rv->maxsize = -1;
+		rv->flags = STABLE_FLAGS_MALLOC;
+	}
+	return rv;
 }
 
 int
@@ -256,37 +297,20 @@ ASN1_STRING_TABLE_add(int nid, long minsize, long maxsize, unsigned long mask,
     unsigned long flags)
 {
 	ASN1_STRING_TABLE *tmp;
-	char new_nid = 0;
 
-	flags &= ~STABLE_FLAGS_MALLOC;
-	if (!stable)
-		stable = sk_ASN1_STRING_TABLE_new(sk_table_cmp);
-	if (!stable) {
+	if ((tmp = stable_get(nid)) == NULL) {
 		ASN1error(ERR_R_MALLOC_FAILURE);
 		return 0;
 	}
-	if (!(tmp = ASN1_STRING_TABLE_get(nid))) {
-		tmp = malloc(sizeof(ASN1_STRING_TABLE));
-		if (!tmp) {
-			ASN1error(ERR_R_MALLOC_FAILURE);
-			return 0;
-		}
-		tmp->flags = flags | STABLE_FLAGS_MALLOC;
-		tmp->nid = nid;
-		new_nid = 1;
-	} else tmp->flags = (tmp->flags & STABLE_FLAGS_MALLOC) | flags;
-		if (minsize != -1)
+	if (minsize >= 0)
 		tmp->minsize = minsize;
-	if (maxsize != -1)
+	if (maxsize >= 0)
 		tmp->maxsize = maxsize;
-	tmp->mask = mask;
-	if (new_nid) {
-		if (sk_ASN1_STRING_TABLE_push(stable, tmp) == 0) {
-			free(tmp);
-			ASN1error(ERR_R_MALLOC_FAILURE);
-			return 0;
-		}
-	}
+	if (mask != 0)
+		tmp->mask = mask;
+	if (flags != 0)
+		tmp->flags = flags | STABLE_FLAGS_MALLOC;
+
 	return 1;
 }
 
diff --git a/regress/lib/libcrypto/asn1/Makefile b/regress/lib/libcrypto/asn1/Makefile
index 1c4a42b80b9..90eca92f8ec 100644
--- a/regress/lib/libcrypto/asn1/Makefile
+++ b/regress/lib/libcrypto/asn1/Makefile
@@ -1,4 +1,4 @@
-#	$OpenBSD: Makefile,v 1.9 2021/12/09 16:30:57 jsing Exp $
+#	$OpenBSD: Makefile,v 1.10 2021/12/11 22:58:48 schwarze Exp $
 
 TESTS = \
 	asn1basic \
@@ -7,7 +7,8 @@ TESTS = \
 	asn1string_copy \
 	asn1time \
 	asn1x509 \
-	rfc5280time
+	rfc5280time \
+	string_table
 
 PROGS = ${TESTS}
 
diff --git a/regress/lib/libcrypto/asn1/string_table.c b/regress/lib/libcrypto/asn1/string_table.c
new file mode 100644
index 00000000000..e80cf0f2069
--- /dev/null
+++ b/regress/lib/libcrypto/asn1/string_table.c
@@ -0,0 +1,128 @@
+/* $OpenBSD: string_table.c,v 1.1 2021/12/11 22:58:48 schwarze Exp $ */
+/*
+ * Copyright (c) 2021 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 <err.h>
+#include <stdarg.h>
+#include <openssl/asn1.h>
+#include <openssl/objects.h>
+
+static int errcount;
+
+static void
+report(const char *fmt, ...)
+{
+	va_list ap;
+
+	va_start(ap, fmt);
+	vwarnx(fmt, ap);
+	va_end(ap);
+
+	errcount++;
+}
+
+static void
+stable_check(const char *testname, ASN1_STRING_TABLE *have,
+    ASN1_STRING_TABLE *want, unsigned long want_flags)
+{
+	if (have == NULL) {
+		report("%s returned NULL", testname);
+		return;
+	}
+	if (have->nid != want->nid)
+		report("%s nid %d, expected %d", testname,
+		    have->nid, want->nid);
+	if (have->minsize != want->minsize)
+		report("%s minsize %ld, expected %ld", testname,
+		    have->minsize, want->minsize);
+	if (have->maxsize != want->maxsize)
+		report("%s maxsize %ld, expected %ld", testname,
+		    have->maxsize, want->maxsize);
+	if (have->mask != want->mask)
+		report("%s mask %lu, expected %lu", testname,
+		    have->mask, want->mask);
+	if (have->flags != want_flags)
+		report("%s flags %lu, expected %lu", testname,
+		    have->flags, want_flags);
+}
+
+int
+main(void)
+{
+	ASN1_STRING_TABLE	 orig, mine, *have;
+	int			 irc;
+
+	orig.nid = NID_name;
+	orig.minsize = 1;
+	orig.maxsize = ub_name;
+	orig.mask = DIRSTRING_TYPE;
+	orig.flags = 0;
+
+	mine.nid = NID_name;
+	mine.minsize = 4;
+	mine.maxsize = 64;
+	mine.mask = B_ASN1_PRINTABLESTRING;
+	mine.flags = STABLE_NO_MASK;
+
+	/* Original entry. */
+
+	have = ASN1_STRING_TABLE_get(orig.nid);
+	stable_check("orig", have, &orig, 0);
+
+	/* Copy, but don't really change. */
+
+	irc = ASN1_STRING_TABLE_add(orig.nid, -1, -1, 0, 0);
+	if (irc != 1)
+		report("set noop returned %d, expected 1", irc);
+	have = ASN1_STRING_TABLE_get(orig.nid);
+	stable_check("noop", have, &orig, STABLE_FLAGS_MALLOC);
+
+	/* Change entry. */
+
+	irc = ASN1_STRING_TABLE_add(mine.nid, mine.minsize, mine.maxsize,
+	    mine.mask, mine.flags);
+	if (irc != 1)
+		report("set returned %d, expected 1", irc);
+	have = ASN1_STRING_TABLE_get(mine.nid);
+	stable_check("set", have, &mine, STABLE_FLAGS_MALLOC | STABLE_NO_MASK);
+
+	/* New entry. */
+
+	mine.nid = NID_title;
+	irc = ASN1_STRING_TABLE_add(mine.nid, mine.minsize, mine.maxsize,
+	    mine.mask, mine.flags);
+	if (irc != 1)
+		report("new returned %d, expected 1", irc);
+	have = ASN1_STRING_TABLE_get(mine.nid);
+	stable_check("new", have, &mine, STABLE_FLAGS_MALLOC | STABLE_NO_MASK);
+
+	/* Back to the initial state. */
+
+	ASN1_STRING_TABLE_cleanup();
+	have = ASN1_STRING_TABLE_get(orig.nid);
+	stable_check("back", have, &orig, 0);
+	if (ASN1_STRING_TABLE_get(mine.nid) != NULL)
+		report("deleted entry is not NULL");
+
+	switch (errcount) {
+	case 0:
+		return 0;
+	case 1:
+		errx(1, "one error");
+	default:
+		errx(1, "%d errors", errcount);
+	}
+}