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, &notify))
>  				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 <assert.h>
>  #include <err.h>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -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