From cdfc2595bc330ba284216b9fde5b8ed06e17604d Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 25 Aug 2021 11:11:01 -0400 Subject: [PATCH] Fix some error-handling in i2v functions. See upstream commits: 32f3b98d1302d4c0950dc1bf94b50269b6edbd95 432f8688bb72e21939845ac7a69359ca718c6676 7bb50cbc4af78a0c8d36fdf2c141ad1330125e2f 8c74c9d1ade0fbdab5b815ddb747351b8b839641 Change-Id: Iff614260c1b1582856edb4ae7a226f2e07537698 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49045 Commit-Queue: David Benjamin Reviewed-by: Adam Langley --- crypto/x509v3/v3_akey.c | 29 ++++++++++++++++++++++++----- crypto/x509v3/v3_alt.c | 19 ++++++++++++++----- crypto/x509v3/v3_utl.c | 16 +++++++++------- include/openssl/x509v3.h | 40 +++++++++++++++++++++++++++++++++++++++- 4 files changed, 86 insertions(+), 18 deletions(-) diff --git a/crypto/x509v3/v3_akey.c b/crypto/x509v3/v3_akey.c index 10376737a..0aba20ed8 100644 --- a/crypto/x509v3/v3_akey.c +++ b/crypto/x509v3/v3_akey.c @@ -93,20 +93,39 @@ static STACK_OF(CONF_VALUE) *i2v_AUTHORITY_KEYID(X509V3_EXT_METHOD *method, STACK_OF(CONF_VALUE) *extlist) { - char *tmp; + char *tmp = NULL; + int extlist_was_null = extlist == NULL; if (akeyid->keyid) { tmp = x509v3_bytes_to_hex(akeyid->keyid->data, akeyid->keyid->length); - X509V3_add_value("keyid", tmp, &extlist); + int ok = tmp != NULL && X509V3_add_value("keyid", tmp, &extlist); OPENSSL_free(tmp); + if (!ok) { + goto err; + } + } + if (akeyid->issuer) { + STACK_OF(CONF_VALUE) *tmpextlist = + i2v_GENERAL_NAMES(NULL, akeyid->issuer, extlist); + if (tmpextlist == NULL) { + goto err; + } + extlist = tmpextlist; } - if (akeyid->issuer) - extlist = i2v_GENERAL_NAMES(NULL, akeyid->issuer, extlist); if (akeyid->serial) { tmp = x509v3_bytes_to_hex(akeyid->serial->data, akeyid->serial->length); - X509V3_add_value("serial", tmp, &extlist); + int ok = tmp != NULL && X509V3_add_value("serial", tmp, &extlist); OPENSSL_free(tmp); + if (!ok) { + goto err; + } } return extlist; + +err: + if (extlist_was_null) { + sk_CONF_VALUE_pop_free(extlist, X509V3_conf_free); + } + return NULL; } /* diff --git a/crypto/x509v3/v3_alt.c b/crypto/x509v3/v3_alt.c index 9ea3ed018..0c5581631 100644 --- a/crypto/x509v3/v3_alt.c +++ b/crypto/x509v3/v3_alt.c @@ -104,11 +104,17 @@ STACK_OF(CONF_VALUE) *i2v_GENERAL_NAMES(X509V3_EXT_METHOD *method, GENERAL_NAMES *gens, STACK_OF(CONF_VALUE) *ret) { - size_t i; - GENERAL_NAME *gen; - for (i = 0; i < sk_GENERAL_NAME_num(gens); i++) { - gen = sk_GENERAL_NAME_value(gens, i); - ret = i2v_GENERAL_NAME(method, gen, ret); + int ret_was_null = ret == NULL; + for (size_t i = 0; i < sk_GENERAL_NAME_num(gens); i++) { + GENERAL_NAME *gen = sk_GENERAL_NAME_value(gens, i); + STACK_OF(CONF_VALUE) *tmp = i2v_GENERAL_NAME(method, gen, ret); + if (tmp == NULL) { + if (ret_was_null) { + sk_CONF_VALUE_pop_free(ret, X509V3_conf_free); + } + return NULL; + } + ret = tmp; } if (!ret) return sk_CONF_VALUE_new_null(); @@ -119,6 +125,9 @@ STACK_OF(CONF_VALUE) *i2v_GENERAL_NAME(X509V3_EXT_METHOD *method, GENERAL_NAME *gen, STACK_OF(CONF_VALUE) *ret) { + /* Note the error-handling for this function relies on there being at most + * one |X509V3_add_value| call. If there were two and the second failed, we + * would need to sometimes free the first call's result. */ unsigned char *p; char oline[256], htmp[5]; int i; diff --git a/crypto/x509v3/v3_utl.c b/crypto/x509v3/v3_utl.c index d40401361..5d91aede7 100644 --- a/crypto/x509v3/v3_utl.c +++ b/crypto/x509v3/v3_utl.c @@ -94,6 +94,7 @@ static int x509V3_add_len_value(const char *name, const char *value, { CONF_VALUE *vtmp = NULL; char *tname = NULL, *tvalue = NULL; + int extlist_was_null = *extlist == NULL; if (name && !(tname = OPENSSL_strdup(name))) goto malloc_err; if (!omit_value) { @@ -120,12 +121,13 @@ static int x509V3_add_len_value(const char *name, const char *value, malloc_err: OPENSSL_PUT_ERROR(X509V3, ERR_R_MALLOC_FAILURE); err: - if (vtmp) - OPENSSL_free(vtmp); - if (tname) - OPENSSL_free(tname); - if (tvalue) - OPENSSL_free(tvalue); + if (extlist_was_null) { + sk_CONF_VALUE_free(*extlist); + *extlist = NULL; + } + OPENSSL_free(vtmp); + OPENSSL_free(tname); + OPENSSL_free(tvalue); return 0; } @@ -293,7 +295,7 @@ ASN1_INTEGER *s2i_ASN1_INTEGER(X509V3_EXT_METHOD *method, const char *value) return aint; } -int X509V3_add_value_int(const char *name, ASN1_INTEGER *aint, +int X509V3_add_value_int(const char *name, const ASN1_INTEGER *aint, STACK_OF(CONF_VALUE) **extlist) { char *strtmp; diff --git a/include/openssl/x509v3.h b/include/openssl/x509v3.h index 5dcb89477..8e4a511c0 100644 --- a/include/openssl/x509v3.h +++ b/include/openssl/x509v3.h @@ -483,12 +483,30 @@ OPENSSL_EXPORT STACK_OF(CONF_VALUE) *i2v_ASN1_BIT_STRING( X509V3_EXT_METHOD *method, ASN1_BIT_STRING *bits, STACK_OF(CONF_VALUE) *extlist); +// i2v_GENERAL_NAME serializes |gen| as a |CONF_VALUE|. If |ret| is non-NULL, it +// appends the value to |ret| and returns |ret| on success or NULL on error. If +// it returns NULL, the caller is still responsible for freeing |ret|. If |ret| +// is NULL, it returns a newly-allocated |STACK_OF(CONF_VALUE)| containing the +// result. |method| is ignored. +// +// Do not use this function. This is an internal implementation detail of the +// human-readable print functions. If extracting a SAN list from a certificate, +// look at |gen| directly. OPENSSL_EXPORT STACK_OF(CONF_VALUE) *i2v_GENERAL_NAME( X509V3_EXT_METHOD *method, GENERAL_NAME *gen, STACK_OF(CONF_VALUE) *ret); OPENSSL_EXPORT int GENERAL_NAME_print(BIO *out, GENERAL_NAME *gen); DECLARE_ASN1_FUNCTIONS(GENERAL_NAMES) +// i2v_GENERAL_NAMES serializes |gen| as a list of |CONF_VALUE|s. If |ret| is +// non-NULL, it appends the values to |ret| and returns |ret| on success or NULL +// on error. If it returns NULL, the caller is still responsible for freeing +// |ret|. If |ret| is NULL, it returns a newly-allocated |STACK_OF(CONF_VALUE)| +// containing the results. |method| is ignored. +// +// Do not use this function. This is an internal implementation detail of the +// human-readable print functions. If extracting a SAN list from a certificate, +// look at |gen| directly. OPENSSL_EXPORT STACK_OF(CONF_VALUE) *i2v_GENERAL_NAMES( X509V3_EXT_METHOD *method, GENERAL_NAMES *gen, STACK_OF(CONF_VALUE) *extlist); @@ -602,15 +620,35 @@ OPENSSL_EXPORT void X509V3_section_free(X509V3_CTX *ctx, OPENSSL_EXPORT void X509V3_set_ctx(X509V3_CTX *ctx, X509 *issuer, X509 *subject, X509_REQ *req, X509_CRL *crl, int flags); +// X509V3_add_value appends a |CONF_VALUE| containing |name| and |value| to +// |*extlist|. It returns one on success and zero on error. If |*extlist| is +// NULL, it sets |*extlist| to a newly-allocated |STACK_OF(CONF_VALUE)| +// containing the result. Either |name| or |value| may be NULL to omit the +// field. +// +// On failure, if |*extlist| was NULL, |*extlist| will remain NULL when the +// function returns. OPENSSL_EXPORT int X509V3_add_value(const char *name, const char *value, STACK_OF(CONF_VALUE) **extlist); + +// X509V3_add_value_uchar behaves like |X509V3_add_value| but takes an +// |unsigned char| pointer. OPENSSL_EXPORT int X509V3_add_value_uchar(const char *name, const unsigned char *value, STACK_OF(CONF_VALUE) **extlist); + +// X509V3_add_value_bool behaves like |X509V3_add_value| but stores the value +// "TRUE" if |asn1_bool| is non-zero and "FALSE" otherwise. OPENSSL_EXPORT int X509V3_add_value_bool(const char *name, int asn1_bool, STACK_OF(CONF_VALUE) **extlist); -OPENSSL_EXPORT int X509V3_add_value_int(const char *name, ASN1_INTEGER *aint, + +// X509V3_add_value_bool behaves like |X509V3_add_value| but stores a string +// representation of |aint|. Note this string representation may be decimal or +// hexadecimal, depending on the size of |aint|. +OPENSSL_EXPORT int X509V3_add_value_int(const char *name, + const ASN1_INTEGER *aint, STACK_OF(CONF_VALUE) **extlist); + OPENSSL_EXPORT char *i2s_ASN1_INTEGER(X509V3_EXT_METHOD *meth, const ASN1_INTEGER *aint); OPENSSL_EXPORT ASN1_INTEGER *s2i_ASN1_INTEGER(X509V3_EXT_METHOD *meth,