Fix a bug in PEM_X509_INFO_read_bio(3) that is very likely to cause

use-after-free and double-free issues in calling programs.
The bug was introduced in SSLeay-0.6.0 released on June 21, 1996
and has been present since OpenBSD 2.4.
I found the bug while documenting the function.

The bug could bite in two ways that looked quite different from the
perspective of the calling code:
 * If a stack was passed in that already contained some X509_INFO
   objects and an error occurred, all the objects passed in would be
   freed, but without removing the freed pointers from the stack,
   so the calling code would probable continue to access the freed
   pointers and eventually free them a second time.
 * If the input BIO contained at least two valid PEM objects followed by
   at least one PEM object causing an error, at least one freed pointer
   would be put onto the stack, even though the function would return NULL
   rather than the stack.  But the calling code would still have a pointer
   to the stack, so it would be likely to access the new bogus pointers
   sooner or later.

Fix all this by remembering the size of the input stack on entry
and cutting it back to exactly that size when exiting due to an
error, but no further.

While here, do some related cleanup:
 * Garbage collect the automatic variables "error" and "i"
   which were only used at one single place each.
 * Use NULL rather than 0 for pointers.

I like bugfixes that make the code four lines shorter, reduce the
number of variables by one, reduce the number of brace-blocks by
one, reduce the number if if-statements by one, and reduce the
number of else-clauses by one.

Tweaks and OK tb@.

4 files changed, 211 insertions, 22 deletions

diff --git a/lib/libcrypto/pem/pem_info.c b/lib/libcrypto/pem/pem_info.c
index f02aaa8bb4b..9561b5f4df1 100644
--- a/lib/libcrypto/pem/pem_info.c
+++ b/lib/libcrypto/pem/pem_info.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: pem_info.c,v 1.22 2017/01/29 17:49:23 beck Exp $ */
+/* $OpenBSD: pem_info.c,v 1.23 2020/07/23 17:15:35 schwarze Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -101,29 +101,28 @@ PEM_X509_INFO_read_bio(BIO *bp, STACK_OF(X509_INFO) *sk, pem_password_cb *cb,
 	void *pp;
 	unsigned char *data = NULL;
 	const unsigned char *p;
-	long len, error = 0;
+	long len;
 	int ok = 0;
-	STACK_OF(X509_INFO) *ret = NULL;
-	unsigned int i, raw, ptype;
-	d2i_of_void *d2i = 0;
+	int num_in, ptype, raw;
+	STACK_OF(X509_INFO) *ret = sk;
+	d2i_of_void *d2i = NULL;
 
-	if (sk == NULL) {
+	if (ret == NULL) {
 		if ((ret = sk_X509_INFO_new_null()) == NULL) {
 			PEMerror(ERR_R_MALLOC_FAILURE);
-			return 0;
+			return NULL;
 		}
-	} else
-		ret = sk;
+	}
+	num_in = sk_X509_INFO_num(ret);
 
 	if ((xi = X509_INFO_new()) == NULL)
 		goto err;
 	for (;;) {
 		raw = 0;
 		ptype = 0;
-		i = PEM_read_bio(bp, &name, &header, &data, &len);
-		if (i == 0) {
-			error = ERR_GET_REASON(ERR_peek_last_error());
-			if (error == PEM_R_NO_START_LINE) {
+		if (!PEM_read_bio(bp, &name, &header, &data, &len)) {
+			if (ERR_GET_REASON(ERR_peek_last_error()) ==
+			    PEM_R_NO_START_LINE) {
 				ERR_clear_error();
 				break;
 			}
@@ -286,22 +285,19 @@ start:
 	ok = 1;
 
 err:
-	if (xi != NULL)
-		X509_INFO_free(xi);
 	if (!ok) {
-		for (i = 0; ((int)i) < sk_X509_INFO_num(ret); i++) {
-			xi = sk_X509_INFO_value(ret, i);
-			X509_INFO_free(xi);
-		}
+		while (sk_X509_INFO_num(ret) > num_in)
+			X509_INFO_free(sk_X509_INFO_pop(ret));
 		if (ret != sk)
 			sk_X509_INFO_free(ret);
 		ret = NULL;
 	}
-
+	X509_INFO_free(xi);
 	free(name);
 	free(header);
 	free(data);
-	return (ret);
+
+	return ret;
 }
 
 
diff --git a/regress/lib/libcrypto/Makefile b/regress/lib/libcrypto/Makefile
index a5b4cc38555..8d6d40d62e4 100644
--- a/regress/lib/libcrypto/Makefile
+++ b/regress/lib/libcrypto/Makefile
@@ -1,4 +1,4 @@
-#	$OpenBSD: Makefile,v 1.38 2020/07/14 18:33:34 jsing Exp $
+#	$OpenBSD: Makefile,v 1.39 2020/07/23 17:15:35 schwarze Exp $
 
 SUBDIR += aead
 SUBDIR += aeswrap
@@ -33,6 +33,7 @@ SUBDIR += init
 SUBDIR += md4
 SUBDIR += md5
 SUBDIR += pbkdf2
+SUBDIR += pem
 SUBDIR += pkcs7
 SUBDIR += poly1305
 SUBDIR += rand
diff --git a/regress/lib/libcrypto/pem/Makefile b/regress/lib/libcrypto/pem/Makefile
new file mode 100644
index 00000000000..34561e217f5
--- /dev/null
+++ b/regress/lib/libcrypto/pem/Makefile
@@ -0,0 +1,8 @@
+#	$OpenBSD: Makefile,v 1.1 2020/07/23 17:15:35 schwarze Exp $
+
+PROG =		x509_info
+LDADD =		-lcrypto
+WARNINGS =	Yes
+CFLAGS +=	-Werror
+
+.include <bsd.regress.mk>
diff --git a/regress/lib/libcrypto/pem/x509_info.c b/regress/lib/libcrypto/pem/x509_info.c
new file mode 100644
index 00000000000..e6b5388a710
--- /dev/null
+++ b/regress/lib/libcrypto/pem/x509_info.c
@@ -0,0 +1,184 @@
+/*	$OpenBSD: x509_info.c,v 1.1 2020/07/23 17:15:35 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 <err.h>
+#include <string.h>
+
+#include <openssl/bio.h>
+#include <openssl/err.h>
+#include <openssl/pem.h>
+#include <openssl/x509.h>
+
+static const char *const bogus_pem = "\
+-----BEGIN BOGUS----- \n\
+-----END BOGUS----- \n\
+";
+
+static const char *const cert_pem = "\
+-----BEGIN CERTIFICATE----- \n\
+MIIDpTCCAo2gAwIBAgIJAPYm3GvOr5eTMA0GCSqGSIb3DQEBBQUAMHAxCzAJBgNV \n\
+BAYTAlVLMRYwFAYDVQQKDA1PcGVuU1NMIEdyb3VwMSIwIAYDVQQLDBlGT1IgVEVT \n\
+VElORyBQVVJQT1NFUyBPTkxZMSUwIwYDVQQDDBxPcGVuU1NMIFRlc3QgSW50ZXJt \n\
+ZWRpYXRlIENBMB4XDTE0MDUyNDE0NDUxMVoXDTI0MDQwMTE0NDUxMVowZDELMAkG \n\
+A1UEBhMCVUsxFjAUBgNVBAoMDU9wZW5TU0wgR3JvdXAxIjAgBgNVBAsMGUZPUiBU \n\
+RVNUSU5HIFBVUlBPU0VTIE9OTFkxGTAXBgNVBAMMEFRlc3QgQ2xpZW50IENlcnQw \n\
+ggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQC0ranbHRLcLVqN+0BzcZpY \n\
++yOLqxzDWT1LD9eW1stC4NzXX9/DCtSIVyN7YIHdGLrIPr64IDdXXaMRzgZ2rOKs \n\
+lmHCAiFpO/ja99gGCJRxH0xwQatqAULfJVHeUhs7OEGOZc2nWifjqKvGfNTilP7D \n\
+nwi69ipQFq9oS19FmhwVHk2wg7KZGHI1qDyG04UrfCZMRitvS9+UVhPpIPjuiBi2 \n\
+x3/FZIpL5gXJvvFK6xHY63oq2asyzBATntBgnP4qJFWWcvRx24wF1PnZabxuVoL2 \n\
+bPnQ/KvONDrw3IdqkKhYNTul7jEcu3OlcZIMw+7DiaKJLAzKb/bBF5gm/pwW6As9 \n\
+AgMBAAGjTjBMMAwGA1UdEwEB/wQCMAAwDgYDVR0PAQH/BAQDAgXgMCwGCWCGSAGG \n\
++EIBDQQfFh1PcGVuU1NMIEdlbmVyYXRlZCBDZXJ0aWZpY2F0ZTANBgkqhkiG9w0B \n\
+AQUFAAOCAQEAJzA4KTjkjXGSC4He63yX9Br0DneGBzjAwc1H6f72uqnCs8m7jgkE \n\
+PQJFdTzQUKh97QPUuayZ2gl8XHagg+iWGy60Kw37gQ0+lumCN2sllvifhHU9R03H \n\
+bWtS4kue+yQjMbrzf3zWygMDgwvFOUAIgBpH9qGc+CdNu97INTYd0Mvz51vLlxRn \n\
+sC5aBYCWaZFnw3lWYxf9eVFRy9U+DkYFqX0LpmbDtcKP7AZGE6ZwSzaim+Cnoz1u \n\
+Cgn+QmpFXgJKMFIZ82iSZISn+JkCCGxctZX1lMvai4Wi8Y0HxW9FTFZ6KBNwwE4B \n\
+zjbN/ehBkgLlW/DWfi44DvwUHmuU6QP3cw== \n\
+-----END CERTIFICATE----- \n\
+";
+
+int
+main(void)
+{
+	BIO			*bp;
+	STACK_OF(X509_INFO)	*skin, *skout;
+	X509_INFO		*info0, *info1;
+	const char		*errdata;
+	unsigned long		 errcode;
+	int			 errcount, errflags, num;
+
+	errcount = 0;
+	if ((skin = sk_X509_INFO_new_null()) == NULL)
+		err(1, "sk_X509_INFO_new_null");
+
+	/* Test with empty input. */
+
+	if ((bp = BIO_new_mem_buf("", 0)) == NULL)
+		err(1, "BIO_new_mem_buf(empty)");
+	if ((skout = PEM_X509_INFO_read_bio(bp, skin, NULL, NULL)) == NULL)
+		err(1, "empty input: %s",
+		    ERR_error_string(ERR_get_error(), NULL));
+	if (skout != skin)
+		errx(1, "empty input did not return the same stack");
+	skout = NULL;
+	if ((num = sk_X509_INFO_num(skin)) != 0)
+		errx(1, "empty input created %d X509_INFO objects", num);
+	BIO_free(bp);
+
+	/* Test with bogus input. */
+
+	if ((bp = BIO_new_mem_buf(bogus_pem, strlen(bogus_pem))) == NULL)
+		err(1, "BIO_new_mem_buf(bogus_pem)");
+	if ((skout = PEM_X509_INFO_read_bio(bp, skin, NULL, NULL)) != NULL)
+		errx(1, "success with bogus input on first try");
+	if ((num = sk_X509_INFO_num(skin)) != 0)
+		errx(1, "bogus input created %d X509_INFO objects", num);
+	if (BIO_reset(bp) != 1)
+		errx(1, "BIO_reset");
+
+	/* Populate stack and test again with bogus input. */
+
+	if ((info0 = X509_INFO_new()) == NULL)
+		err(1, "X509_INFO_new");
+	info0->references = 2;  /* X509_INFO_up_ref(3) doesn't exist. */
+	if (sk_X509_INFO_push(skin, info0) != 1)
+		err(1, "sk_X509_INFO_push");
+	if ((skout = PEM_X509_INFO_read_bio(bp, skin, NULL, NULL)) != NULL)
+		errx(1, "success with bogus input on second try");
+	if ((num = sk_X509_INFO_num(skin)) != 1)
+		errx(1, "bogus input changed stack size from 1 to %d", num);
+	if (sk_X509_INFO_value(skin, 0) != info0)
+		errx(1, "bogus input changed stack content");
+	if (info0->references != 2) {
+		warnx("bogus input changed ref count from 2 to %d",
+		    info0->references);
+		info0->references = 2;
+		errcount++;
+	}
+	BIO_free(bp);
+
+	/* Use a real certificate object. */
+
+	if ((bp = BIO_new_mem_buf(cert_pem, strlen(cert_pem))) == NULL)
+		err(1, "BIO_new_mem_buf(cert_pem)");
+	if ((skout = PEM_X509_INFO_read_bio(bp, skin, NULL, NULL)) == NULL) {
+		errdata = NULL;
+		errflags = 0;
+		while ((errcode = ERR_get_error_line_data(NULL, NULL,
+		    &errdata, &errflags)) != 0)
+			if (errdata != NULL && (errflags & ERR_TXT_STRING))
+				warnx("%s --- %s",
+				    ERR_error_string(errcode, NULL),
+				    errdata);
+			else
+				warnx("%s", ERR_error_string(errcode, NULL));
+		err(1, "real input: parsing failed");
+	}
+	if (skout != skin)
+		errx(1, "real input did not return the same stack");
+	skout = NULL;
+	if ((num = sk_X509_INFO_num(skin)) != 2)
+		errx(1, "real input changed stack size from 1 to %d", num);
+	if (sk_X509_INFO_value(skin, 0) != info0)
+		errx(1, "real input changed stack content");
+	if (info0->references != 2)
+		errx(1, "real input changed ref count from 2 to %d",
+		    info0->references);
+        info1 = sk_X509_INFO_pop(skin);
+	if (info1->x509 == NULL)
+		errx(1, "real input did not create a certificate");
+	X509_INFO_free(info1);
+	info1 = NULL;
+	BIO_free(bp);
+
+	/* Two real certificates followed by bogus input. */
+
+	if ((bp = BIO_new(BIO_s_mem())) == NULL)
+		err(1, "BIO_new");
+	if (BIO_puts(bp, cert_pem) != strlen(cert_pem))
+		err(1, "BIO_puts(cert_pem) first copy");
+	if (BIO_puts(bp, cert_pem) != strlen(cert_pem))
+		err(1, "BIO_puts(cert_pem) second copy");
+	if (BIO_puts(bp, bogus_pem) != strlen(bogus_pem))
+		err(1, "BIO_puts(bogus_pem)");
+	if ((skout = PEM_X509_INFO_read_bio(bp, skin, NULL, NULL)) != NULL)
+		errx(1, "success with real + bogus input");
+	if ((num = sk_X509_INFO_num(skin)) != 1) {
+		warnx("real + bogus input changed stack size from 1 to %d",
+		    num);
+		while (sk_X509_INFO_num(skin) > 1)
+			sk_X509_INFO_pop(skin);
+		errcount++;
+	}
+	if (sk_X509_INFO_value(skin, 0) != info0)
+		errx(1, "real + bogus input changed stack content");
+	if (info0->references != 2) {
+		warnx("real + bogus input changed ref count from 2 to %d",
+		    info0->references);
+		errcount++;
+	}
+	BIO_free(bp);
+	info0->references = 1;
+	X509_INFO_free(info0);
+	sk_X509_INFO_free(skin);
+
+	if (errcount > 0)
+		errx(1, "%d errors detected", errcount);
+	return 0;
+}