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@.

1 files changed, 17 insertions, 21 deletions

diff --git a/src/lib/libcrypto/pem/pem_info.c b/src/lib/libcrypto/pem/pem_info.c
index f02aaa8bb4..9561b5f4df 100644
--- a/src/lib/libcrypto/pem/pem_info.c
+++ b/src/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;
 }