Rewrite PEM_X509_INFO_read_bio.

This fixes:

- Undefined function pointer casts.
- Missing X509_INFO_new malloc failure checks.
- Pointless (int) cast on strlen.
- Missing ERR_GET_LIB in PEM_R_NO_START_LINE check.
- Broken error-handling if passing in an existing stack and we hit a
  syntax error.

Bug: chromium:785442
Change-Id: I8be3523b0f13bdb3745938af9740d491486f8bf1
Reviewed-on: https://boringssl-review.googlesource.com/32109
Reviewed-by: Adam Langley <agl@google.com>

crypto/pem/pem_info.c

@@ -86,35 +86,84 @@ STACK_OF(X509_INFO) *PEM_X509_INFO_read(FILE *fp, STACK_OF(X509_INFO) *sk,
 }
 #endif
 
+enum parse_result_t {
+    parse_ok,
+    parse_error,
+    parse_new_entry,
+};
+
+static enum parse_result_t parse_x509(X509_INFO *info, const uint8_t *data,
+                                      size_t len, int key_type)
+{
+    if (info->x509 != NULL) {
+        return parse_new_entry;
+    }
+    info->x509 = d2i_X509(NULL, &data, len);
+    return info->x509 != NULL ? parse_ok : parse_error;
+}
+
+static enum parse_result_t parse_x509_aux(X509_INFO *info, const uint8_t *data,
+                                          size_t len, int key_type)
+{
+    if (info->x509 != NULL) {
+        return parse_new_entry;
+    }
+    info->x509 = d2i_X509_AUX(NULL, &data, len);
+    return info->x509 != NULL ? parse_ok : parse_error;
+}
+
+static enum parse_result_t parse_crl(X509_INFO *info, const uint8_t *data,
+                                     size_t len, int key_type)
+{
+    if (info->crl != NULL) {
+        return parse_new_entry;
+    }
+    info->crl = d2i_X509_CRL(NULL, &data, len);
+    return info->crl != NULL ? parse_ok : parse_error;
+}
+
+static enum parse_result_t parse_key(X509_INFO *info, const uint8_t *data,
+                                     size_t len, int key_type)
+{
+    if (info->x_pkey != NULL) {
+        return parse_new_entry;
+    }
+    info->x_pkey = X509_PKEY_new();
+    if (info->x_pkey == NULL) {
+        return parse_error;
+    }
+    info->x_pkey->dec_pkey = d2i_PrivateKey(key_type, NULL, &data, len);
+    return info->x_pkey->dec_pkey != NULL ? parse_ok : parse_error;
+}
+
 STACK_OF(X509_INFO) *PEM_X509_INFO_read_bio(BIO *bp, STACK_OF(X509_INFO) *sk,
                                             pem_password_cb *cb, void *u)
 {
-    X509_INFO *xi = NULL;
+    X509_INFO *info = NULL;
     char *name = NULL, *header = NULL;
-    void *pp;
     unsigned char *data = NULL;
-    const unsigned char *p;
     long len;
     int ok = 0;
     STACK_OF(X509_INFO) *ret = NULL;
-    unsigned int i, raw, ptype;
-    d2i_of_void *d2i = 0;
 
     if (sk == NULL) {
-        if ((ret = sk_X509_INFO_new_null()) == NULL) {
+        ret = sk_X509_INFO_new_null();
+        if (ret == NULL) {
             OPENSSL_PUT_ERROR(PEM, ERR_R_MALLOC_FAILURE);
-            goto err;
+            return NULL;
         }
-    } else
+    } else {
         ret = sk;
+    }
+    size_t orig_num = sk_X509_INFO_num(ret);
 
-    if ((xi = X509_INFO_new()) == NULL)
+    info = X509_INFO_new();
+    if (info == NULL) {
         goto err;
+    }
+
     for (;;) {
-        raw = 0;
+        if (!PEM_read_bio(bp, &name, &header, &data, &len)) {
-        ptype = 0;
-        i = PEM_read_bio(bp, &name, &header, &data, &len);
-        if (i == 0) {
             uint32_t error = ERR_peek_last_error();
             if (ERR_GET_LIB(error) == ERR_LIB_PEM &&
                 ERR_GET_REASON(error) == PEM_R_NO_START_LINE) {
@@ -123,171 +172,106 @@ STACK_OF(X509_INFO) *PEM_X509_INFO_read_bio(BIO *bp, STACK_OF(X509_INFO) *sk,
             }
             goto err;
         }
- start:
+
-        if ((strcmp(name, PEM_STRING_X509) == 0) ||
+        enum parse_result_t (*parse_function)(X509_INFO *, const uint8_t *,
-            (strcmp(name, PEM_STRING_X509_OLD) == 0)) {
+                                              size_t, int) = NULL;
-            d2i = (D2I_OF(void)) d2i_X509;
+        int key_type = EVP_PKEY_NONE;
-            if (xi->x509 != NULL) {
+        if (strcmp(name, PEM_STRING_X509) == 0 ||
-                if (!sk_X509_INFO_push(ret, xi))
+            strcmp(name, PEM_STRING_X509_OLD) == 0) {
-                    goto err;
+            parse_function = parse_x509;
-                if ((xi = X509_INFO_new()) == NULL)
+        } else if (strcmp(name, PEM_STRING_X509_TRUSTED) == 0) {
-                    goto err;
+            parse_function = parse_x509_aux;
-                goto start;
-            }
-            pp = &(xi->x509);
-        } else if ((strcmp(name, PEM_STRING_X509_TRUSTED) == 0)) {
-            d2i = (D2I_OF(void)) d2i_X509_AUX;
-            if (xi->x509 != NULL) {
-                if (!sk_X509_INFO_push(ret, xi))
-                    goto err;
-                if ((xi = X509_INFO_new()) == NULL)
-                    goto err;
-                goto start;
-            }
-            pp = &(xi->x509);
         } else if (strcmp(name, PEM_STRING_X509_CRL) == 0) {
-            d2i = (D2I_OF(void)) d2i_X509_CRL;
+            parse_function = parse_crl;
-            if (xi->crl != NULL) {
-                if (!sk_X509_INFO_push(ret, xi))
-                    goto err;
-                if ((xi = X509_INFO_new()) == NULL)
-                    goto err;
-                goto start;
-            }
-            pp = &(xi->crl);
         } else if (strcmp(name, PEM_STRING_RSA) == 0) {
-            d2i = (D2I_OF(void)) d2i_RSAPrivateKey;
+            parse_function = parse_key;
-            if (xi->x_pkey != NULL) {
+            key_type = EVP_PKEY_RSA;
-                if (!sk_X509_INFO_push(ret, xi))
+        } else if (strcmp(name, PEM_STRING_DSA) == 0) {
-                    goto err;
+            parse_function = parse_key;
-                if ((xi = X509_INFO_new()) == NULL)
+            key_type = EVP_PKEY_DSA;
-                    goto err;
+        } else if (strcmp(name, PEM_STRING_ECPRIVATEKEY) == 0) {
-                goto start;
+            parse_function = parse_key;
-            }
+            key_type = EVP_PKEY_EC;
-
-            xi->enc_data = NULL;
-            xi->enc_len = 0;
-
-            xi->x_pkey = X509_PKEY_new();
-            ptype = EVP_PKEY_RSA;
-            pp = &xi->x_pkey->dec_pkey;
-            if ((int)strlen(header) > 10) /* assume encrypted */
-                raw = 1;
-        } else
-#ifndef OPENSSL_NO_DSA
-        if (strcmp(name, PEM_STRING_DSA) == 0) {
-            d2i = (D2I_OF(void)) d2i_DSAPrivateKey;
-            if (xi->x_pkey != NULL) {
-                if (!sk_X509_INFO_push(ret, xi))
-                    goto err;
-                if ((xi = X509_INFO_new()) == NULL)
-                    goto err;
-                goto start;
-            }
-
-            xi->enc_data = NULL;
-            xi->enc_len = 0;
-
-            xi->x_pkey = X509_PKEY_new();
-            ptype = EVP_PKEY_DSA;
-            pp = &xi->x_pkey->dec_pkey;
-            if ((int)strlen(header) > 10) /* assume encrypted */
-                raw = 1;
-        } else
-#endif
-        if (strcmp(name, PEM_STRING_ECPRIVATEKEY) == 0) {
-            d2i = (D2I_OF(void)) d2i_ECPrivateKey;
-            if (xi->x_pkey != NULL) {
-                if (!sk_X509_INFO_push(ret, xi))
-                    goto err;
-                if ((xi = X509_INFO_new()) == NULL)
-                    goto err;
-                goto start;
-            }
-
-            xi->enc_data = NULL;
-            xi->enc_len = 0;
-
-            xi->x_pkey = X509_PKEY_new();
-            ptype = EVP_PKEY_EC;
-            pp = &xi->x_pkey->dec_pkey;
-            if ((int)strlen(header) > 10) /* assume encrypted */
-                raw = 1;
-        } else {
-            d2i = NULL;
-            pp = NULL;
         }
 
-        if (d2i != NULL) {
+        /* If a private key has a header, assume it is encrypted. */
-            if (!raw) {
+        if (key_type != EVP_PKEY_NONE && strlen(header) > 10) {
-                EVP_CIPHER_INFO cipher;
+            if (info->x_pkey != NULL) {
-
+                if (!sk_X509_INFO_push(ret, info)) {
-                if (!PEM_get_EVP_CIPHER_INFO(header, &cipher))
-                    goto err;
-                if (!PEM_do_header(&cipher, data, &len, cb, u))
-                    goto err;
-                p = data;
-                if (ptype) {
-                    if (!d2i_PrivateKey(ptype, pp, &p, len)) {
-                        OPENSSL_PUT_ERROR(PEM, ERR_R_ASN1_LIB);
-                        goto err;
-                    }
-                } else if (d2i(pp, &p, len) == NULL) {
-                    OPENSSL_PUT_ERROR(PEM, ERR_R_ASN1_LIB);
                     goto err;
                 }
-            } else {            /* encrypted RSA data */
+                info = X509_INFO_new();
-                if (!PEM_get_EVP_CIPHER_INFO(header, &xi->enc_cipher))
+                if (info == NULL) {
                     goto err;
-                xi->enc_data = (char *)data;
+                }
-                xi->enc_len = (int)len;
+            }
-                data = NULL;
+            /* Historically, raw entries pushed an empty key. */
+            info->x_pkey = X509_PKEY_new();
+            if (info->x_pkey == NULL ||
+                !PEM_get_EVP_CIPHER_INFO(header, &info->enc_cipher)) {
+                goto err;
+            }
+            info->enc_data = (char *)data;
+            info->enc_len = (int)len;
+            data = NULL;
+        } else if (parse_function != NULL) {
+            EVP_CIPHER_INFO cipher;
+            if (!PEM_get_EVP_CIPHER_INFO(header, &cipher) ||
+                !PEM_do_header(&cipher, data, &len, cb, u)) {
+                goto err;
+            }
+            enum parse_result_t result =
+                parse_function(info, data, len, key_type);
+            if (result == parse_new_entry) {
+                if (!sk_X509_INFO_push(ret, info)) {
+                    goto err;
+                }
+                info = X509_INFO_new();
+                if (info == NULL) {
+                    goto err;
+                }
+                result = parse_function(info, data, len, key_type);
+            }
+            if (result != parse_ok) {
+                OPENSSL_PUT_ERROR(PEM, ERR_R_ASN1_LIB);
+                goto err;
             }
-        } else {
-            /* unknown */
         }
-        if (name != NULL)
+        OPENSSL_free(name);
-            OPENSSL_free(name);
+        OPENSSL_free(header);
-        if (header != NULL)
+        OPENSSL_free(data);
-            OPENSSL_free(header);
-        if (data != NULL)
-            OPENSSL_free(data);
         name = NULL;
         header = NULL;
         data = NULL;
     }
 
-    /*
+    /* Push the last entry on the stack if not empty. */
-     * if the last one hasn't been pushed yet and there is anything in it
+    if (info->x509 != NULL || info->crl != NULL ||
-     * then add it to the stack ...
+        info->x_pkey != NULL || info->enc_data != NULL) {
-     */
+        if (!sk_X509_INFO_push(ret, info)) {
-    if ((xi->x509 != NULL) || (xi->crl != NULL) ||
-        (xi->x_pkey != NULL) || (xi->enc_data != NULL)) {
-        if (!sk_X509_INFO_push(ret, xi))
             goto err;
-        xi = NULL;
-    }
-    ok = 1;
- err:
-    if (xi != NULL)
-        X509_INFO_free(xi);
-    if (!ok) {
-        for (i = 0; i < sk_X509_INFO_num(ret); i++) {
-            xi = sk_X509_INFO_value(ret, i);
-            X509_INFO_free(xi);
         }
-        if (ret != sk)
+        info = NULL;
+    }
+
+    ok = 1;
+
+ err:
+    X509_INFO_free(info);
+    if (!ok) {
+        while (sk_X509_INFO_num(ret) > orig_num) {
+            X509_INFO_free(sk_X509_INFO_pop(ret));
+        }
+        if (ret != sk) {
             sk_X509_INFO_free(ret);
+        }
         ret = NULL;
     }
 
-    if (name != NULL)
+    OPENSSL_free(name);
-        OPENSSL_free(name);
+    OPENSSL_free(header);
-    if (header != NULL)
+    OPENSSL_free(data);
-        OPENSSL_free(header);
+    return ret;
-    if (data != NULL)
-        OPENSSL_free(data);
-    return (ret);
 }
 
 /* A TJH addition */

crypto/x509/x509_test.cc

@@ -1568,6 +1568,11 @@ TEST(X509Test, PEMX509Info) {
       "AAAA\n"
       "-----END UNKNOWN-----\n";
 
+  std::string invalid =
+      "-----BEGIN CERTIFICATE-----\n"
+      "AAAA\n"
+      "-----END CERTIFICATE-----\n";
+
   // Each X509_INFO contains at most one certificate, CRL, etc. The format
   // creates a new X509_INFO when a repeated type is seen.
   std::string pem =
@@ -1650,4 +1655,19 @@ TEST(X509Test, PEMX509Info) {
         &kExpected[i],
         sk_X509_INFO_value(infos.get(), i + OPENSSL_ARRAY_SIZE(kExpected)));
   }
+
+  // Gracefully handle errors in both the append and fresh cases.
+  std::string bad_pem = cert + cert + invalid;
+
+  bio.reset(BIO_new_mem_buf(bad_pem.data(), bad_pem.size()));
+  ASSERT_TRUE(bio);
+  bssl::UniquePtr<STACK_OF(X509_INFO)> infos2(
+      PEM_X509_INFO_read_bio(bio.get(), nullptr, nullptr, nullptr));
+  EXPECT_FALSE(infos2);
+
+  bio.reset(BIO_new_mem_buf(bad_pem.data(), bad_pem.size()));
+  ASSERT_TRUE(bio);
+  EXPECT_FALSE(
+      PEM_X509_INFO_read_bio(bio.get(), infos.get(), nullptr, nullptr));
+  EXPECT_EQ(2 * OPENSSL_ARRAY_SIZE(kExpected), sk_X509_INFO_num(infos.get()));
 }

include/openssl/asn1.h

@@ -298,10 +298,6 @@ typedef struct ASN1_VALUE_st ASN1_VALUE;
 	OPENSSL_EXPORT int fname##_print_ctx(BIO *out, stname *x, int indent, \
 					 const ASN1_PCTX *pctx);
 
-#define D2I_OF(type) type *(*)(type **,const unsigned char **,long)
-#define I2D_OF(type) int (*)(type *,unsigned char **)
-#define I2D_OF_const(type) int (*)(const type *,unsigned char **)
-
 typedef void *d2i_of_void(void **, const unsigned char **, long);
 typedef int i2d_of_void(const void *, unsigned char **);