From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: rpki-client: improve SIA handling
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Mon, 3 Jun 2024 11:09:57 +0200

On Sun, Jun 02, 2024 at 04:56:51PM +0200, Theo Buehler wrote:
> The way I read 4.8.8.1 of RFC 6487, the current handling of the SIA
> extension is not entirely correct for CA certs. We reject non-rsync URI
> for the caRepository and rpkiManifest access methods while the spec only
> requires that an rsync URI be present for both of them, not that all
> URIs use the rsync scheme. So accept all valid URIs and retain the first
> rsync one we encounter. If there's no rsync URI we will reject the cert.
> In contrast, RFC 8182 3.2 only allows https URI for rpkiNotify.
> 
> Also make it clearer that sbgp_sia() is for CA certs and x509_get_sia()
> for EE certs.
> 
> Index: cert.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> diff -u -p -r1.132 cert.c
> --- cert.c	31 May 2024 02:45:15 -0000	1.132
> +++ cert.c	2 Jun 2024 14:34:56 -0000
> @@ -495,7 +495,8 @@ sbgp_ipaddrblk(const char *fn, struct ce
>  }
>  
>  /*
> - * Parse "Subject Information Access" extension, RFC 6487 4.8.8.
> + * Parse "Subject Information Access" extension for a CA cert,
> + * RFC 6487, section 4.8.8.1 and RFC 8182, section 3.2.
>   * Returns zero on failure, non-zero on success.
>   */
>  static int
> @@ -525,13 +526,23 @@ sbgp_sia(const char *fn, struct cert *ce
>  		oid = ad->method;
>  
>  		if (OBJ_cmp(oid, carepo_oid) == 0) {
> -			if (!x509_location(fn, "SIA: caRepository",
> -			    RSYNC_PROTO, ad->location, &cert->repo))
> +			if (!x509_location(fn, "SIA: caRepository", NULL,
> +			    ad->location, &cert->repo))
>  				goto out;
> +			if (strncasecmp(cert->repo, RSYNC_PROTO,
> +			    RSYNC_PROTO_LEN) == 0)
> +				continue;
> +			free(cert->repo);
> +			cert->repo = NULL;

This seems very fragile on how multiple locations are handled.
Especially the warnx() in x509_location() depends on the order of uri.

So if it is https:// foobar:// and rsync:// there will be no warning but
if it is rsync:// then https:// and foobar:// then there will be two
warnings.

Maybe we need a local uri var that is reset on every loop and do the
multiple URI check here.

>  		} else if (OBJ_cmp(oid, manifest_oid) == 0) {
> -			if (!x509_location(fn, "SIA: rpkiManifest",
> -			    RSYNC_PROTO, ad->location, &cert->mft))
> +			if (!x509_location(fn, "SIA: rpkiManifest", NULL,
> +			    ad->location, &cert->mft))
>  				goto out;
> +			if (strncasecmp(cert->mft, RSYNC_PROTO,
> +			    RSYNC_PROTO_LEN) == 0)
> +				continue;
> +			free(cert->mft);
> +			cert->mft = NULL;

Obiously this has the same issue.

>  		} else if (OBJ_cmp(oid, notify_oid) == 0) {
>  			if (!x509_location(fn, "SIA: rpkiNotify",
>  			    HTTPS_PROTO, ad->location, &cert->notify))
> Index: x509.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v
> diff -u -p -r1.90 x509.c
> --- x509.c	31 May 2024 11:27:34 -0000	1.90
> +++ x509.c	2 Jun 2024 14:13:14 -0000
> @@ -467,8 +467,8 @@ out:
>  }
>  
>  /*
> - * Parse the Subject Information Access (SIA) extension
> - * See RFC 6487, section 4.8.8 for details.
> + * Parse the Subject Information Access (SIA) extension for an EE cert.
> + * See RFC 6487, section 4.8.8.2 for details.
>   * Returns NULL on failure, on success returns the SIA signedObject URI
>   * (which has to be freed after use).
>   */
> 

-- 
:wq Claudio