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>
diff --git a/crypto/pem/pem_info.c b/crypto/pem/pem_info.c
index 7ed8aee..3627a45 100644
--- a/crypto/pem/pem_info.c
+++ b/crypto/pem/pem_info.c
@@ -86,35 +86,84 @@
}
#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;
- ptype = 0;
- i = PEM_read_bio(bp, &name, &header, &data, &len);
- if (i == 0) {
+ if (!PEM_read_bio(bp, &name, &header, &data, &len)) {
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 @@
}
goto err;
}
- start:
- if ((strcmp(name, PEM_STRING_X509) == 0) ||
- (strcmp(name, PEM_STRING_X509_OLD) == 0)) {
- d2i = (D2I_OF(void)) d2i_X509;
- 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_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);
+
+ enum parse_result_t (*parse_function)(X509_INFO *, const uint8_t *,
+ size_t, int) = NULL;
+ int key_type = EVP_PKEY_NONE;
+ if (strcmp(name, PEM_STRING_X509) == 0 ||
+ strcmp(name, PEM_STRING_X509_OLD) == 0) {
+ parse_function = parse_x509;
+ } else if (strcmp(name, PEM_STRING_X509_TRUSTED) == 0) {
+ parse_function = parse_x509_aux;
} else if (strcmp(name, PEM_STRING_X509_CRL) == 0) {
- d2i = (D2I_OF(void)) d2i_X509_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);
+ parse_function = parse_crl;
} else if (strcmp(name, PEM_STRING_RSA) == 0) {
- d2i = (D2I_OF(void)) d2i_RSAPrivateKey;
- 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_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;
+ parse_function = parse_key;
+ key_type = EVP_PKEY_RSA;
+ } else if (strcmp(name, PEM_STRING_DSA) == 0) {
+ parse_function = parse_key;
+ key_type = EVP_PKEY_DSA;
+ } else if (strcmp(name, PEM_STRING_ECPRIVATEKEY) == 0) {
+ parse_function = parse_key;
+ key_type = EVP_PKEY_EC;
}
- if (d2i != NULL) {
- if (!raw) {
- EVP_CIPHER_INFO cipher;
-
- 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);
+ /* If a private key has a header, assume it is encrypted. */
+ if (key_type != EVP_PKEY_NONE && strlen(header) > 10) {
+ if (info->x_pkey != NULL) {
+ if (!sk_X509_INFO_push(ret, info)) {
goto err;
}
- } else { /* encrypted RSA data */
- if (!PEM_get_EVP_CIPHER_INFO(header, &xi->enc_cipher))
+ info = X509_INFO_new();
+ if (info == NULL) {
goto err;
- xi->enc_data = (char *)data;
- xi->enc_len = (int)len;
- data = NULL;
+ }
}
- } else {
- /* unknown */
+ /* 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;
+ }
}
- if (name != NULL)
- OPENSSL_free(name);
- if (header != NULL)
- OPENSSL_free(header);
- if (data != NULL)
- OPENSSL_free(data);
+ OPENSSL_free(name);
+ OPENSSL_free(header);
+ OPENSSL_free(data);
name = NULL;
header = NULL;
data = NULL;
}
- /*
- * if the last one hasn't been pushed yet and there is anything in it
- * then add it to the stack ...
- */
- if ((xi->x509 != NULL) || (xi->crl != NULL) ||
- (xi->x_pkey != NULL) || (xi->enc_data != NULL)) {
- if (!sk_X509_INFO_push(ret, xi))
+ /* Push the last entry on the stack if not empty. */
+ if (info->x509 != NULL || info->crl != NULL ||
+ info->x_pkey != NULL || info->enc_data != NULL) {
+ if (!sk_X509_INFO_push(ret, info)) {
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);
- if (header != NULL)
- OPENSSL_free(header);
- if (data != NULL)
- OPENSSL_free(data);
- return (ret);
+ OPENSSL_free(name);
+ OPENSSL_free(header);
+ OPENSSL_free(data);
+ return ret;
}
/* A TJH addition */
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index bca52bb..bf0b29a 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -1568,6 +1568,11 @@
"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 @@
&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()));
}
diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h
index 7be3d63..8b61eaa 100644
--- a/include/openssl/asn1.h
+++ b/include/openssl/asn1.h
@@ -298,10 +298,6 @@
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 **);
