Fix leak in set_dist_point_name error handling. The temporary X509_NAME wasn't destroyed if the section didn't exist. Also document the weird 0 vs -1 convention (see callers), and revise the NULL check added in https://boringssl-review.googlesource.com/c/boringssl/+/56705. It doesn't make a difference, but we should only apply the NULL check after we've looked at the name, and return -1 because, after the name is checked, it's a known syntax error. Also fix a couple of comments that were wrong. It's that the RDNSequence we take from X509_NAME must have one RDN, not that there's one RDNSequence. (This is a consequence of X509_NAME's somewhat odd in-memory representation.) Bug: oss-fuzz:55700 Change-Id: I5745752bfa82802d361803868f962b2b0fa4bd32 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56929 Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: Bob Beck <bbe@google.com> Reviewed-by: Bob Beck <bbe@google.com>
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 4760044..e152f3d 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc
@@ -5733,6 +5733,35 @@ // value is not allowed. {"issuingDistributionPoint", "fullname", nullptr, {}}, + {"issuingDistributionPoint", + "relativename:name", + "[name]\nCN=Hello\n", + {0x30, 0x1b, 0x06, 0x03, 0x55, 0x1d, 0x1c, 0x04, 0x14, 0x30, + 0x12, 0xa0, 0x10, 0xa1, 0x0e, 0x30, 0x0c, 0x06, 0x03, 0x55, + 0x04, 0x03, 0x0c, 0x05, 0x48, 0x65, 0x6c, 0x6c, 0x6f}}, + + // relativename referencing a section which doesn't exist. + {"issuingDistributionPoint", + "relativename:wrong_section_name", + "[name]\nCN=Hello\n", + {}}, + + // relativename must be a single RDN. By default, the section-based name + // syntax puts each attribute into its own RDN. + {"issuingDistributionPoint", + "relativename:name", + "[name]\nCN=Hello\nC=US\n", + {}}, + + // A single RDN with multiple attributes is allowed. + {"issuingDistributionPoint", + "relativename:name", + "[name]\nCN=Hello\n+C=US\n", + {0x30, 0x26, 0x06, 0x03, 0x55, 0x1d, 0x1c, 0x04, 0x1f, 0x30, + 0x1d, 0xa0, 0x1b, 0xa1, 0x19, 0x30, 0x09, 0x06, 0x03, 0x55, + 0x04, 0x06, 0x13, 0x02, 0x55, 0x53, 0x30, 0x0c, 0x06, 0x03, + 0x55, 0x04, 0x03, 0x0c, 0x05, 0x48, 0x65, 0x6c, 0x6c, 0x6f}}, + // Duplicate reason keys are an error. Reaching this case is interesting. // The value can a string like "key:value,key:value", or it can be // "@section" and reference a config section. If using a string, duplicate
diff --git a/crypto/x509v3/v3_crld.c b/crypto/x509v3/v3_crld.c index 0206c7a..a3cae42 100644 --- a/crypto/x509v3/v3_crld.c +++ b/crypto/x509v3/v3_crld.c
@@ -127,27 +127,30 @@ return gens; } +// set_dist_point_name decodes a DistributionPointName from |cnf| and writes the +// result in |*pdp|. It returns 1 on success, -1 on error, and 0 if |cnf| used +// an unrecognized input type. The zero return can be used by callers to support +// additional syntax. static int set_dist_point_name(DIST_POINT_NAME **pdp, const X509V3_CTX *ctx, const CONF_VALUE *cnf) { - // If |cnf| comes from |X509V3_parse_list|, which is possible for a v2i - // function, |cnf->value| may be NULL. - if (cnf->value == NULL) { - OPENSSL_PUT_ERROR(X509V3, X509V3_R_MISSING_VALUE); - return 0; - } - STACK_OF(GENERAL_NAME) *fnm = NULL; STACK_OF(X509_NAME_ENTRY) *rnm = NULL; if (!strncmp(cnf->name, "fullname", 9)) { + // If |cnf| comes from |X509V3_parse_list|, which is possible for a v2i + // function, |cnf->value| may be NULL. + if (cnf->value == NULL) { + OPENSSL_PUT_ERROR(X509V3, X509V3_R_MISSING_VALUE); + return -1; + } fnm = gnames_from_sectname(ctx, cnf->value); if (!fnm) { goto err; } } else if (!strcmp(cnf->name, "relativename")) { - int ret; - X509_NAME *nm; - nm = X509_NAME_new(); - if (!nm) { + // If |cnf| comes from |X509V3_parse_list|, which is possible for a v2i + // function, |cnf->value| may be NULL. + if (cnf->value == NULL) { + OPENSSL_PUT_ERROR(X509V3, X509V3_R_MISSING_VALUE); return -1; } const STACK_OF(CONF_VALUE) *dnsect = X509V3_get_section(ctx, cnf->value); @@ -155,14 +158,18 @@ OPENSSL_PUT_ERROR(X509V3, X509V3_R_SECTION_NOT_FOUND); return -1; } - ret = X509V3_NAME_from_section(nm, dnsect, MBSTRING_ASC); + X509_NAME *nm = X509_NAME_new(); + if (!nm) { + return -1; + } + int ret = X509V3_NAME_from_section(nm, dnsect, MBSTRING_ASC); rnm = nm->entries; nm->entries = NULL; X509_NAME_free(nm); if (!ret || sk_X509_NAME_ENTRY_num(rnm) <= 0) { goto err; } - // Since its a name fragment can't have more than one RDNSequence + // There can only be one RDN in nameRelativeToCRLIssuer. if (sk_X509_NAME_ENTRY_value(rnm, sk_X509_NAME_ENTRY_num(rnm) - 1)->set) { OPENSSL_PUT_ERROR(X509V3, X509V3_R_INVALID_MULTIPLE_RDNS); goto err;
diff --git a/include/openssl/x509.h b/include/openssl/x509.h index e3ba8e0..f38574f 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h
@@ -938,7 +938,7 @@ // success or zero on error. The entry's attribute type is |obj|. The entry's // attribute value is determined by |type|, |bytes|, and |len|, as in // |X509_NAME_ENTRY_set_data|. The entry's position is determined by |loc| and -// |set| as in |X509_NAME_entry|. +// |set| as in |X509_NAME_add_entry|. OPENSSL_EXPORT int X509_NAME_add_entry_by_OBJ(X509_NAME *name, const ASN1_OBJECT *obj, int type, const uint8_t *bytes, int len,