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 <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/x509v3/v3_akey.c b/crypto/x509v3/v3_akey.c
index 1037673..0aba20e 100644
--- a/crypto/x509v3/v3_akey.c
+++ b/crypto/x509v3/v3_akey.c
@@ -93,20 +93,39 @@
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)
- extlist = i2v_GENERAL_NAMES(NULL, akeyid->issuer, extlist);
+ if (akeyid->issuer) {
+ STACK_OF(CONF_VALUE) *tmpextlist =
+ i2v_GENERAL_NAMES(NULL, akeyid->issuer, extlist);
+ if (tmpextlist == NULL) {
+ goto err;
+ }
+ extlist = tmpextlist;
+ }
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 9ea3ed0..0c55816 100644
--- a/crypto/x509v3/v3_alt.c
+++ b/crypto/x509v3/v3_alt.c
@@ -104,11 +104,17 @@
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 @@
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 d404013..5d91aed 100644
--- a/crypto/x509v3/v3_utl.c
+++ b/crypto/x509v3/v3_utl.c
@@ -94,6 +94,7 @@
{
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 @@
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 @@
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 5dcb894..8e4a511 100644
--- a/include/openssl/x509v3.h
+++ b/include/openssl/x509v3.h
@@ -483,12 +483,30 @@
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_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,
