From 792c1dc43eb888470b2ecb162be2acc5e0e1d61b Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 23 Sep 2018 00:39:53 -0500 Subject: [PATCH] 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 --- crypto/pem/pem_info.c | 300 ++++++++++++++++++--------------------- crypto/x509/x509_test.cc | 20 +++ include/openssl/asn1.h | 4 - 3 files changed, 162 insertions(+), 162 deletions(-) diff --git a/crypto/pem/pem_info.c b/crypto/pem/pem_info.c index 7ed8aeed..3627a450 100644 --- a/crypto/pem/pem_info.c +++ b/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; - 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 @@ 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) || - (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; + } + } + /* 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); - 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 bca52bba..bf0b29a2 100644 --- a/crypto/x509/x509_test.cc +++ b/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 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 7be3d63f..8b61eaa3 100644 --- a/include/openssl/asn1.h +++ b/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 **);