Fix leak on invalid input to a2i_GENERAL_NAME.
Also add some tests for this syntax. The error-handling here is slightly
subtle. Although we do call GENERAL_NAME_free on the temporary
GENERAL_NAME on error, GENERAL_NAME's value is freed based on the
type field. That means if you add an object to the value but don't set
the type, it won't be freed.
Only the OTHERNAME codepath was affected by this, and a malloc
failure-only case in the is_string path. I've gone ahead and reworked
all the paths so setting the type happens at the same time as setting
the value, so this invariant is more locally obvious.
This only impacts the unsafe, stringly-typed extensions-building APIs
that no one should be using anyway.
Bug: oss-fuzz:55569
Change-Id: I6390e4ac1142264cdc86f95fd850f1b8f81e3fc9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56725
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 63ce092..0928d46 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -5720,6 +5720,35 @@
// value is not allowed.
{"issuingDistributionPoint", "fullname", nullptr, {}},
+ // subjectAltName has a series of string-based inputs for each name type.
+ {"subjectAltName",
+ "email:foo@example.com, URI:https://example.com, DNS:example.com, "
+ "RID:1.2.3.4, IP:127.0.0.1, IP:::1, dirName:section, "
+ "otherName:1.2.3.4;BOOLEAN:TRUE",
+ "[section]\nCN=Test\n",
+ {0x30, 0x78, 0x06, 0x03, 0x55, 0x1d, 0x11, 0x04, 0x71, 0x30, 0x6f, 0x81,
+ 0x0f, 0x66, 0x6f, 0x6f, 0x40, 0x65, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65,
+ 0x2e, 0x63, 0x6f, 0x6d, 0x86, 0x13, 0x68, 0x74, 0x74, 0x70, 0x73, 0x3a,
+ 0x2f, 0x2f, 0x65, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65, 0x2e, 0x63, 0x6f,
+ 0x6d, 0x82, 0x0b, 0x65, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65, 0x2e, 0x63,
+ 0x6f, 0x6d, 0x88, 0x03, 0x2a, 0x03, 0x04, 0x87, 0x04, 0x7f, 0x00, 0x00,
+ 0x01, 0x87, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0xa4, 0x11, 0x30, 0x0f, 0x31,
+ 0x0d, 0x30, 0x0b, 0x06, 0x03, 0x55, 0x04, 0x03, 0x0c, 0x04, 0x54, 0x65,
+ 0x73, 0x74, 0xa0, 0x0a, 0x06, 0x03, 0x2a, 0x03, 0x04, 0xa0, 0x03, 0x01,
+ 0x01, 0xff}},
+
+ // Syntax errors in each case, where they exist. (The string types just
+ // copy the string in as-is.)
+ {"subjectAltName", "RID:not_an_oid", nullptr, {}},
+ {"subjectAltName", "IP:not_an_ip", nullptr, {}},
+ {"subjectAltName", "dirName:no_conf_db", nullptr, {}},
+ {"subjectAltName", "dirName:missing_section", "[section]\nCN=Test\n", {}},
+ {"subjectAltName", "otherName:missing_semicolon", nullptr, {}},
+ {"subjectAltName", "otherName:1.2.3.4", nullptr, {}},
+ {"subjectAltName", "otherName:invalid_oid;BOOLEAN:TRUE", nullptr, {}},
+ {"subjectAltName", "otherName:1.2.3.4;invalid_value", nullptr, {}},
+
// The "DER:" prefix just specifies an arbitrary byte string. Colons
// separators are ignored.
{kTestOID, "DER:0001020304", nullptr, {0x30, 0x15, 0x06, 0x0c, 0x2a, 0x86,
diff --git a/crypto/x509v3/v3_alt.c b/crypto/x509v3/v3_alt.c
index 660ced5..f10c1ce 100644
--- a/crypto/x509v3/v3_alt.c
+++ b/crypto/x509v3/v3_alt.c
@@ -459,14 +459,12 @@
const X509V3_EXT_METHOD *method,
const X509V3_CTX *ctx, int gen_type,
const char *value, int is_nc) {
- char is_string = 0;
- GENERAL_NAME *gen = NULL;
-
if (!value) {
OPENSSL_PUT_ERROR(X509V3, X509V3_R_MISSING_VALUE);
return NULL;
}
+ GENERAL_NAME *gen = NULL;
if (out) {
gen = out;
} else {
@@ -480,9 +478,17 @@
switch (gen_type) {
case GEN_URI:
case GEN_EMAIL:
- case GEN_DNS:
- is_string = 1;
+ case GEN_DNS: {
+ ASN1_IA5STRING *str = ASN1_IA5STRING_new();
+ if (str == NULL || !ASN1_STRING_set(str, value, strlen(value))) {
+ ASN1_STRING_free(str);
+ OPENSSL_PUT_ERROR(X509V3, ERR_R_MALLOC_FAILURE);
+ goto err;
+ }
+ gen->type = gen_type;
+ gen->d.ia5 = str;
break;
+ }
case GEN_RID: {
ASN1_OBJECT *obj;
@@ -491,10 +497,13 @@
ERR_add_error_data(2, "value=", value);
goto err;
}
+ gen->type = GEN_RID;
gen->d.rid = obj;
- } break;
+ break;
+ }
case GEN_IPADD:
+ gen->type = GEN_IPADD;
if (is_nc) {
gen->d.ip = a2i_IPADDRESS_NC(value);
} else {
@@ -525,16 +534,6 @@
goto err;
}
- if (is_string) {
- if (!(gen->d.ia5 = ASN1_IA5STRING_new()) ||
- !ASN1_STRING_set(gen->d.ia5, (unsigned char *)value, strlen(value))) {
- OPENSSL_PUT_ERROR(X509V3, ERR_R_MALLOC_FAILURE);
- goto err;
- }
- }
-
- gen->type = gen_type;
-
return gen;
err:
@@ -581,33 +580,40 @@
static int do_othername(GENERAL_NAME *gen, const char *value,
const X509V3_CTX *ctx) {
- char *objtmp = NULL;
- const char *p;
- int objlen;
- if (!(p = strchr(value, ';'))) {
+ const char *semicolon = strchr(value, ';');
+ if (semicolon == NULL) {
return 0;
}
- if (!(gen->d.otherName = OTHERNAME_new())) {
+
+ OTHERNAME *name = OTHERNAME_new();
+ if (name == NULL) {
return 0;
}
- // Free this up because we will overwrite it. no need to free type_id
- // because it is static
- ASN1_TYPE_free(gen->d.otherName->value);
- if (!(gen->d.otherName->value = ASN1_generate_v3(p + 1, ctx))) {
- return 0;
- }
- objlen = p - value;
- objtmp = OPENSSL_malloc(objlen + 1);
+
+ char *objtmp = OPENSSL_strndup(value, semicolon - value);
if (objtmp == NULL) {
- return 0;
+ goto err;
}
- OPENSSL_strlcpy(objtmp, value, objlen + 1);
- gen->d.otherName->type_id = OBJ_txt2obj(objtmp, 0);
+ ASN1_OBJECT_free(name->type_id);
+ name->type_id = OBJ_txt2obj(objtmp, /*dont_search_names=*/0);
OPENSSL_free(objtmp);
- if (!gen->d.otherName->type_id) {
- return 0;
+ if (name->type_id == NULL) {
+ goto err;
}
+
+ ASN1_TYPE_free(name->value);
+ name->value = ASN1_generate_v3(semicolon + 1, ctx);
+ if (name->value == NULL) {
+ goto err;
+ }
+
+ gen->type = GEN_OTHERNAME;
+ gen->d.otherName = name;
return 1;
+
+err:
+ OTHERNAME_free(name);
+ return 0;
}
static int do_dirname(GENERAL_NAME *gen, const char *value,
@@ -627,6 +633,7 @@
if (!X509V3_NAME_from_section(nm, sk, MBSTRING_ASC)) {
goto err;
}
+ gen->type = GEN_DIRNAME;
gen->d.dirn = nm;
ret = 1;
