Use scopers in certificate policy conf logic policy_section did not free qual if pushing to the stack failed on malloc failure. Change-Id: I97aa4dc14432d1f35945b893aba9548bee23d7a2 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/96609 Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: David Benjamin <davidben@google.com> Auto-Submit: David Benjamin <davidben@google.com> Presubmit-BoringSSL-Verified: boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com>
diff --git a/crypto/x509/v3_cpols.cc b/crypto/x509/v3_cpols.cc index f69c82d..840262e 100644 --- a/crypto/x509/v3_cpols.cc +++ b/crypto/x509/v3_cpols.cc
@@ -15,6 +15,8 @@ #include <stdio.h> #include <string.h> +#include <utility> + #include <openssl/asn1.h> #include <openssl/asn1t.h> #include <openssl/conf.h> @@ -38,12 +40,11 @@ static void print_qualifiers(BIO *out, const STACK_OF(POLICYQUALINFO) *quals, int indent); static void print_notice(BIO *out, const USERNOTICE *notice, int indent); -static POLICYINFO *policy_section(const X509V3_CTX *ctx, - const STACK_OF(CONF_VALUE) *polstrs, - int ia5org); -static POLICYQUALINFO *notice_section(const X509V3_CTX *ctx, - const STACK_OF(CONF_VALUE) *unot, - int ia5org); +static UniquePtr<POLICYINFO> policy_section(const X509V3_CTX *ctx, + const STACK_OF(CONF_VALUE) *polstrs, + int ia5org); +static UniquePtr<POLICYQUALINFO> notice_section( + const X509V3_CTX *ctx, const STACK_OF(CONF_VALUE) *unot, int ia5org); static int nref_nos(STACK_OF(ASN1_INTEGER) *nnums, const STACK_OF(CONF_VALUE) *nos); @@ -110,186 +111,169 @@ static void *r2i_certpol(const X509V3_EXT_METHOD *method, const X509V3_CTX *ctx, const char *value) { - STACK_OF(POLICYINFO) *pols = sk_POLICYINFO_new_null(); + UniquePtr<STACK_OF(POLICYINFO)> pols(sk_POLICYINFO_new_null()); if (pols == nullptr) { return nullptr; } - STACK_OF(CONF_VALUE) *vals = X509V3_parse_list(value); - - { - if (vals == nullptr) { - OPENSSL_PUT_ERROR(X509V3, ERR_R_X509V3_LIB); - goto err; - } - int ia5org = 0; - for (size_t i = 0; i < sk_CONF_VALUE_num(vals); i++) { - const CONF_VALUE *cnf = sk_CONF_VALUE_value(vals, i); - if (cnf->value || !cnf->name) { - OPENSSL_PUT_ERROR(X509V3, X509V3_R_INVALID_POLICY_IDENTIFIER); - X509V3_conf_err(cnf); - goto err; - } - POLICYINFO *pol; - const char *pstr = cnf->name; - if (!strcmp(pstr, "ia5org")) { - ia5org = 1; - continue; - } else if (*pstr == '@') { - const STACK_OF(CONF_VALUE) *polsect = X509V3_get_section(ctx, pstr + 1); - if (!polsect) { - OPENSSL_PUT_ERROR(X509V3, X509V3_R_INVALID_SECTION); - - X509V3_conf_err(cnf); - goto err; - } - pol = policy_section(ctx, polsect, ia5org); - if (!pol) { - goto err; - } - } else { - ASN1_OBJECT *pobj = OBJ_txt2obj(cnf->name, 0); - if (pobj == nullptr) { - OPENSSL_PUT_ERROR(X509V3, X509V3_R_INVALID_OBJECT_IDENTIFIER); - X509V3_conf_err(cnf); - goto err; - } - pol = POLICYINFO_new(); - if (pol == nullptr) { - ASN1_OBJECT_free(pobj); - goto err; - } - pol->policyid = pobj; - } - if (!sk_POLICYINFO_push(pols, pol)) { - POLICYINFO_free(pol); - goto err; - } - } - sk_CONF_VALUE_pop_free(vals, X509V3_conf_free); - return pols; + UniquePtr<STACK_OF(CONF_VALUE)> vals(X509V3_parse_list(value)); + if (vals == nullptr) { + OPENSSL_PUT_ERROR(X509V3, ERR_R_X509V3_LIB); + return nullptr; } + int ia5org = 0; + for (const CONF_VALUE *cnf : vals.get()) { + if (cnf->value || !cnf->name) { + OPENSSL_PUT_ERROR(X509V3, X509V3_R_INVALID_POLICY_IDENTIFIER); + X509V3_conf_err(cnf); + return nullptr; + } + UniquePtr<POLICYINFO> pol; + const char *pstr = cnf->name; + if (!strcmp(pstr, "ia5org")) { + ia5org = 1; + continue; + } else if (*pstr == '@') { + const STACK_OF(CONF_VALUE) *polsect = X509V3_get_section(ctx, pstr + 1); + if (!polsect) { + OPENSSL_PUT_ERROR(X509V3, X509V3_R_INVALID_SECTION); -err: - sk_CONF_VALUE_pop_free(vals, X509V3_conf_free); - sk_POLICYINFO_pop_free(pols, POLICYINFO_free); - return nullptr; + X509V3_conf_err(cnf); + return nullptr; + } + pol = policy_section(ctx, polsect, ia5org); + if (!pol) { + return nullptr; + } + } else { + UniquePtr<ASN1_OBJECT> pobj(OBJ_txt2obj(cnf->name, 0)); + if (pobj == nullptr) { + OPENSSL_PUT_ERROR(X509V3, X509V3_R_INVALID_OBJECT_IDENTIFIER); + X509V3_conf_err(cnf); + return nullptr; + } + pol.reset(POLICYINFO_new()); + if (pol == nullptr) { + return nullptr; + } + pol->policyid = pobj.release(); + } + if (!PushToStack(pols.get(), std::move(pol))) { + return nullptr; + } + } + return pols.release(); } -static POLICYINFO *policy_section(const X509V3_CTX *ctx, - const STACK_OF(CONF_VALUE) *polstrs, - int ia5org) { - POLICYINFO *pol; - POLICYQUALINFO *qual; - if (!(pol = POLICYINFO_new())) { - goto err; +static UniquePtr<POLICYINFO> policy_section(const X509V3_CTX *ctx, + const STACK_OF(CONF_VALUE) *polstrs, + int ia5org) { + UniquePtr<POLICYINFO> pol(POLICYINFO_new()); + if (pol == nullptr) { + return nullptr; } - for (size_t i = 0; i < sk_CONF_VALUE_num(polstrs); i++) { - const CONF_VALUE *cnf = sk_CONF_VALUE_value(polstrs, i); + for (const CONF_VALUE *cnf : polstrs) { if (!strcmp(cnf->name, "policyIdentifier")) { ASN1_OBJECT *pobj; if (!(pobj = OBJ_txt2obj(cnf->value, 0))) { OPENSSL_PUT_ERROR(X509V3, X509V3_R_INVALID_OBJECT_IDENTIFIER); X509V3_conf_err(cnf); - goto err; + return nullptr; } + ASN1_OBJECT_free(pol->policyid); pol->policyid = pobj; } else if (x509v3_conf_name_matches(cnf->name, "CPS")) { - if (!pol->qualifiers) { + if (pol->qualifiers == nullptr) { pol->qualifiers = sk_POLICYQUALINFO_new_null(); } - if (!(qual = POLICYQUALINFO_new())) { - goto err; - } - if (!sk_POLICYQUALINFO_push(pol->qualifiers, qual)) { - goto err; + UniquePtr<POLICYQUALINFO> qual(POLICYQUALINFO_new()); + if (qual == nullptr || pol->qualifiers == nullptr) { + return nullptr; } qual->pqualid = OBJ_nid2obj(NID_id_qt_cps); if (qual->pqualid == nullptr) { OPENSSL_PUT_ERROR(X509V3, ERR_R_INTERNAL_ERROR); - goto err; + return nullptr; } qual->d.cpsuri = ASN1_IA5STRING_new(); if (qual->d.cpsuri == nullptr) { - goto err; + return nullptr; } if (!ASN1_STRING_set(qual->d.cpsuri, cnf->value, strlen(cnf->value))) { - goto err; + return nullptr; + } + if (!PushToStack(pol->qualifiers, std::move(qual))) { + return nullptr; } } else if (x509v3_conf_name_matches(cnf->name, "userNotice")) { if (*cnf->value != '@') { OPENSSL_PUT_ERROR(X509V3, X509V3_R_EXPECTED_A_SECTION_NAME); X509V3_conf_err(cnf); - goto err; + return nullptr; } const STACK_OF(CONF_VALUE) *unot = X509V3_get_section(ctx, cnf->value + 1); if (!unot) { OPENSSL_PUT_ERROR(X509V3, X509V3_R_INVALID_SECTION); X509V3_conf_err(cnf); - goto err; + return nullptr; } - qual = notice_section(ctx, unot, ia5org); - if (!qual) { - goto err; + UniquePtr<POLICYQUALINFO> qual = notice_section(ctx, unot, ia5org); + if (qual == nullptr) { + return nullptr; } - if (!pol->qualifiers) { + if (pol->qualifiers == nullptr) { pol->qualifiers = sk_POLICYQUALINFO_new_null(); } - if (!sk_POLICYQUALINFO_push(pol->qualifiers, qual)) { - goto err; + if (pol->qualifiers == nullptr || + !PushToStack(pol->qualifiers, std::move(qual))) { + return nullptr; } } else { OPENSSL_PUT_ERROR(X509V3, X509V3_R_INVALID_OPTION); X509V3_conf_err(cnf); - goto err; + return nullptr; } } if (!pol->policyid) { OPENSSL_PUT_ERROR(X509V3, X509V3_R_NO_POLICY_IDENTIFIER); - goto err; + return nullptr; } return pol; - -err: - POLICYINFO_free(pol); - return nullptr; } -static POLICYQUALINFO *notice_section(const X509V3_CTX *ctx, - const STACK_OF(CONF_VALUE) *unot, - int ia5org) { - USERNOTICE *notice; - POLICYQUALINFO *qual; - if (!(qual = POLICYQUALINFO_new())) { - goto err; +static UniquePtr<POLICYQUALINFO> notice_section( + const X509V3_CTX *ctx, const STACK_OF(CONF_VALUE) *unot, int ia5org) { + UniquePtr<POLICYQUALINFO> qual(POLICYQUALINFO_new()); + if (qual == nullptr) { + return nullptr; } qual->pqualid = OBJ_nid2obj(NID_id_qt_unotice); if (qual->pqualid == nullptr) { OPENSSL_PUT_ERROR(X509V3, ERR_R_INTERNAL_ERROR); - goto err; + return nullptr; } - if (!(notice = USERNOTICE_new())) { - goto err; + USERNOTICE *notice = USERNOTICE_new(); + if (notice == nullptr) { + return nullptr; } qual->d.usernotice = notice; - for (size_t i = 0; i < sk_CONF_VALUE_num(unot); i++) { - const CONF_VALUE *cnf = sk_CONF_VALUE_value(unot, i); + for (const CONF_VALUE *cnf : unot) { if (!strcmp(cnf->name, "explicitText")) { notice->exptext = ASN1_VISIBLESTRING_new(); if (notice->exptext == nullptr) { - goto err; + return nullptr; } if (!ASN1_STRING_set(notice->exptext, cnf->value, strlen(cnf->value))) { - goto err; + return nullptr; } } else if (!strcmp(cnf->name, "organization")) { NOTICEREF *nref; if (!notice->noticeref) { if (!(nref = NOTICEREF_new())) { - goto err; + return nullptr; } notice->noticeref = nref; } else { @@ -302,49 +286,41 @@ } if (!ASN1_STRING_set(nref->organization, cnf->value, strlen(cnf->value))) { - goto err; + return nullptr; } } else if (!strcmp(cnf->name, "noticeNumbers")) { NOTICEREF *nref; - STACK_OF(CONF_VALUE) *nos; if (!notice->noticeref) { if (!(nref = NOTICEREF_new())) { - goto err; + return nullptr; } notice->noticeref = nref; } else { nref = notice->noticeref; } - nos = X509V3_parse_list(cnf->value); - if (!nos || !sk_CONF_VALUE_num(nos)) { + UniquePtr<STACK_OF(CONF_VALUE)> nos(X509V3_parse_list(cnf->value)); + if (!nos || !sk_CONF_VALUE_num(nos.get())) { OPENSSL_PUT_ERROR(X509V3, X509V3_R_INVALID_NUMBERS); X509V3_conf_err(cnf); - sk_CONF_VALUE_pop_free(nos, X509V3_conf_free); - goto err; + return nullptr; } - int ret = nref_nos(nref->noticenos, nos); - sk_CONF_VALUE_pop_free(nos, X509V3_conf_free); - if (!ret) { - goto err; + if (!nref_nos(nref->noticenos, nos.get())) { + return nullptr; } } else { OPENSSL_PUT_ERROR(X509V3, X509V3_R_INVALID_OPTION); X509V3_conf_err(cnf); - goto err; + return nullptr; } } if (notice->noticeref && (!notice->noticeref->noticenos || !notice->noticeref->organization)) { OPENSSL_PUT_ERROR(X509V3, X509V3_R_NEED_ORGANIZATION_AND_NUMBERS); - goto err; + return nullptr; } return qual; - -err: - POLICYQUALINFO_free(qual); - return nullptr; } static int nref_nos(STACK_OF(ASN1_INTEGER) *nnums,
diff --git a/include/openssl/x509.h b/include/openssl/x509.h index da74824..d507d1e 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h
@@ -5306,6 +5306,7 @@ BORINGSSL_MAKE_DELETER(POLICY_CONSTRAINTS, POLICY_CONSTRAINTS_free) BORINGSSL_MAKE_DELETER(POLICY_MAPPING, POLICY_MAPPING_free) BORINGSSL_MAKE_DELETER(POLICYINFO, POLICYINFO_free) +BORINGSSL_MAKE_DELETER(POLICYQUALINFO, POLICYQUALINFO_free) BORINGSSL_MAKE_DELETER(RSA_PSS_PARAMS, RSA_PSS_PARAMS_free) BORINGSSL_MAKE_DELETER(X509, X509_free) BORINGSSL_MAKE_UP_REF(X509, X509_up_ref)