From: Claudio Jeker Subject: Re: rpki-client: more x509_location() fixes To: Theo Buehler Cc: tech@openbsd.org Date: Tue, 4 Jun 2024 14:20:48 +0200 On Tue, Jun 04, 2024 at 01:02:47PM +0200, Theo Buehler wrote: > There are various things that are still wrong with the way x509_location() > is used. I think we should get rid of the warnx in x509_location() if > *out != NULL since it results in warnings only once we found an acceptable > URI. This is also an issue in the SIA for EE certs handling and would be > for CRLs if we validated all access location URIs (which we should). > > For sbgp_sia(), my reading of RFC 8182, section 3.2 is that there is only > one rpkiNotify URI, although it's not made explicit. Also, I think per > usual, what's not explicitly mentioned in 6487 or subsequent specs is > not allowed, so reject a cert containing such access description OIDs. > None of the other children of id-ad make sense here anyway: > https://oid-rep.orange-labs.fr/get/1.3.6.1.5.5.7.48 > > For AIA, SIA, and CRL, do not blindly set the out parameter to NULL. > assert that it is NULL, since this smells like a leak. Use a local > variable and assign the first acceptable URI to the out parameter, check > all URIs for validity and thus make the functions match sbgp_sia() more > closely. AIA only has one accessMethod, so it is slightly different. > > With all this mess out of the way, we can drop the (*out != NULL) warnx() > and instead assert that *out == NULL in x509_location(). OK claudio@ > Index: cert.c > =================================================================== > RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v > diff -u -p -r1.134 cert.c > --- cert.c 4 Jun 2024 04:17:18 -0000 1.134 > +++ cert.c 4 Jun 2024 09:26:24 -0000 > @@ -506,7 +506,7 @@ sbgp_sia(const char *fn, struct cert *ce > ACCESS_DESCRIPTION *ad; > ASN1_OBJECT *oid; > const char *mftfilename; > - char *carepo = NULL, *rpkimft = NULL; > + char *carepo = NULL, *rpkimft = NULL, *notify = NULL; > int i, rc = 0; > > assert(cert->repo == NULL && cert->mft == NULL && cert->notify == NULL); > @@ -560,14 +560,30 @@ sbgp_sia(const char *fn, struct cert *ce > rpkimft = NULL; > } else if (OBJ_cmp(oid, notify_oid) == 0) { > if (!x509_location(fn, "SIA: rpkiNotify", > - ad->location, &cert->notify)) > + ad->location, ¬ify)) > goto out; > - if (strncasecmp(cert->notify, HTTPS_PROTO, > + if (strncasecmp(notify, HTTPS_PROTO, > HTTPS_PROTO_LEN) != 0) { > warnx("%s: non-https uri in rpkiNotify: %s", > fn, cert->notify); > + free(notify); > goto out; > } > + if (cert->notify != NULL) { > + warnx("%s: unexpected rpkiNotify accessMethod", > + fn); > + free(notify); > + goto out; > + } > + cert->notify = notify; > + notify = NULL; > + } else { > + char buf[128]; > + > + OBJ_obj2txt(buf, sizeof(buf), oid, 0); > + warnx("%s: RFC 6487 section 4.8.8.1: unexpected" > + " accessMethod: %s", fn, buf); > + goto out; > } > } > > Index: x509.c > =================================================================== > RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v > diff -u -p -r1.92 x509.c > --- x509.c 4 Jun 2024 04:17:18 -0000 1.92 > +++ x509.c 4 Jun 2024 10:52:28 -0000 > @@ -17,6 +17,7 @@ > * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. > */ > > +#include > #include > #include > #include > @@ -413,13 +414,14 @@ x509_pubkey_get_ski(X509_PUBKEY *pubkey, > * (which has to be freed after use). > */ > int > -x509_get_aia(X509 *x, const char *fn, char **aia) > +x509_get_aia(X509 *x, const char *fn, char **out_aia) > { > ACCESS_DESCRIPTION *ad; > AUTHORITY_INFO_ACCESS *info; > int crit, rc = 0; > > - *aia = NULL; > + assert(*out_aia == NULL); > + > info = X509_get_ext_d2i(x, NID_info_access, &crit, NULL); > if (info == NULL) { > if (crit != -1) { > @@ -456,7 +458,7 @@ x509_get_aia(X509 *x, const char *fn, ch > goto out; > } > > - if (!x509_location(fn, "AIA: caIssuers", ad->location, aia)) > + if (!x509_location(fn, "AIA: caIssuers", ad->location, out_aia)) > goto out; > > rc = 1; > @@ -473,14 +475,14 @@ out: > * (which has to be freed after use). > */ > int > -x509_get_sia(X509 *x, const char *fn, char **sia) > +x509_get_sia(X509 *x, const char *fn, char **out_sia) > { > ACCESS_DESCRIPTION *ad; > AUTHORITY_INFO_ACCESS *info; > ASN1_OBJECT *oid; > - int i, crit, rsync_found = 0; > + int i, crit, rc = 0; > > - *sia = NULL; > + assert(*out_sia == NULL); > > info = X509_get_ext_d2i(x, NID_sinfo_access, &crit, NULL); > if (info == NULL) { > @@ -498,6 +500,8 @@ x509_get_sia(X509 *x, const char *fn, ch > } > > for (i = 0; i < sk_ACCESS_DESCRIPTION_num(info); i++) { > + char *sia; > + > ad = sk_ACCESS_DESCRIPTION_value(info, i); > oid = ad->method; > > @@ -522,51 +526,51 @@ x509_get_sia(X509 *x, const char *fn, ch > goto out; > } > > - if (!x509_location(fn, "SIA: signedObject", ad->location, sia)) > + sia = NULL; > + if (!x509_location(fn, "SIA: signedObject", ad->location, &sia)) > goto out; > > - if (rsync_found) > - continue; > - > - if (strncasecmp(*sia, RSYNC_PROTO, RSYNC_PROTO_LEN) == 0) { > - const char *p = *sia + RSYNC_PROTO_LEN; > + if (*out_sia == NULL && strncasecmp(sia, RSYNC_PROTO, > + RSYNC_PROTO_LEN) == 0) { > + const char *p = sia + RSYNC_PROTO_LEN; > size_t fnlen, plen; > > - rsync_found = 1; > - > - if (filemode) > + if (filemode) { > + *out_sia = sia; > continue; > + } > > fnlen = strlen(fn); > plen = strlen(p); > > if (fnlen < plen || strcmp(p, fn + fnlen - plen) != 0) { > warnx("%s: mismatch between pathname and SIA " > - "(%s)", fn, *sia); > + "(%s)", fn, sia); > + free(sia); > goto out; > } > > + *out_sia = sia; > continue; > } > - > - free(*sia); > - *sia = NULL; > + if (verbose) > + warnx("%s: RFC 6487 section 4.8.8: SIA: " > + "ignoring location %s", fn, sia); > + free(sia); > } > > - if (!rsync_found) { > + if (*out_sia == NULL) { > warnx("%s: RFC 6487 section 4.8.8.2: " > "SIA without rsync accessLocation", fn); > goto out; > } > > - AUTHORITY_INFO_ACCESS_free(info); > - return 1; > + rc = 1; > > out: > - free(*sia); > - *sia = NULL; > + > AUTHORITY_INFO_ACCESS_free(info); > - return 0; > + return rc; > } > > /* > @@ -698,15 +702,16 @@ x509_any_inherits(X509 *x) > * after use. > */ > int > -x509_get_crl(X509 *x, const char *fn, char **crl) > +x509_get_crl(X509 *x, const char *fn, char **out_crl) > { > CRL_DIST_POINTS *crldp; > DIST_POINT *dp; > GENERAL_NAMES *names; > GENERAL_NAME *name; > - int i, crit, rsync_found = 0; > + int i, crit, rc = 0; > + > + assert(*out_crl == NULL); > > - *crl = NULL; > crldp = X509_get_ext_d2i(x, NID_crl_distribution_points, &crit, NULL); > if (crldp == NULL) { > if (crit != -1) { > @@ -760,26 +765,35 @@ x509_get_crl(X509 *x, const char *fn, ch > > names = dp->distpoint->name.fullname; > for (i = 0; i < sk_GENERAL_NAME_num(names); i++) { > + char *crl = NULL; > + > name = sk_GENERAL_NAME_value(names, i); > > - if (!x509_location(fn, "CRL distribution point", name, crl)) > + if (!x509_location(fn, "CRL distribution point", name, &crl)) > goto out; > > - if (strncasecmp(*crl, RSYNC_PROTO, RSYNC_PROTO_LEN) == 0) { > - rsync_found = 1; > - goto out; > + if (*out_crl == NULL && strncasecmp(crl, RSYNC_PROTO, > + RSYNC_PROTO_LEN) == 0) { > + *out_crl = crl; > + continue; > } > + if (verbose) > + warnx("%s: ignoring CRL distribution point %s", > + fn, crl); > + free(crl); > + } > > - free(*crl); > - *crl = NULL; > + if (*out_crl == NULL) { > + warnx("%s: RFC 6487 section 4.8.6: no rsync URI " > + "in CRL distributionPoint", fn); > + goto out; > } > > - warnx("%s: RFC 6487 section 4.8.6: no rsync URI " > - "in CRL distributionPoint", fn); > + rc = 1; > > out: > CRL_DIST_POINTS_free(crldp); > - return rsync_found; > + return rc; > } > > /* > @@ -813,6 +827,8 @@ x509_location(const char *fn, const char > { > ASN1_IA5STRING *uri; > > + assert(*out == NULL); > + > if (location->type != GEN_URI) { > warnx("%s: RFC 6487 section 4.8: %s not URI", fn, descr); > return 0; > @@ -823,12 +839,6 @@ x509_location(const char *fn, const char > if (!valid_uri(uri->data, uri->length, NULL)) { > warnx("%s: RFC 6487 section 4.8: %s bad location", fn, descr); > return 0; > - } > - > - if (*out != NULL) { > - warnx("%s: RFC 6487 section 4.8: multiple %s specified, " > - "using the first one", fn, descr); > - return 1; > } > > if ((*out = strndup(uri->data, uri->length)) == NULL) > -- :wq Claudio