From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: rpki-client: fix use of X509_get_ext_count()
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Sat, 3 Feb 2024 17:34:47 +0100

On Sat, Feb 03, 2024 at 02:38:58PM +0100, Theo Buehler wrote:
> X509_get_ext_count(x) can't return < 0 since X509v3_get_ext_count()
> returns 0 on an empty stack of extensions ensuring sk_*_num() returns
> a non-negative value. If X509_get_ext_count() could actually return < 0,
> someone could feed rpki-client a bad cert and make it error out. That's
> bad, So don't do that.
> 
> A cert without any extensions will be rejected for many reasons. At the
> very least it won't pass the 'missing SKI' check at the end.
> 
> Index: cert.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> diff -u -p -r1.123 cert.c
> --- cert.c	1 Feb 2024 15:11:38 -0000	1.123
> +++ cert.c	3 Feb 2024 13:21:27 -0000
> @@ -737,8 +737,7 @@ struct cert *
>  cert_parse_pre(const char *fn, const unsigned char *der, size_t len)
>  {
>  	const unsigned char	*oder;
> -	int			 extsz;
> -	size_t			 i;
> +	int			 i;
>  	X509			*x = NULL;
>  	X509_EXTENSION		*ext = NULL;
>  	const X509_ALGOR	*palg;
> @@ -810,10 +809,7 @@ cert_parse_pre(const char *fn, const uns
>  
>  	/* Look for X509v3 extensions. */
>  
> -	if ((extsz = X509_get_ext_count(x)) < 0)
> -		errx(1, "X509_get_ext_count");
> -
> -	for (i = 0; i < (size_t)extsz; i++) {
> +	for (i = 0; i < X509_get_ext_count(x); i++) {
>  		ext = X509_get_ext(x, i);
>  		assert(ext != NULL);
>  		obj = X509_EXTENSION_get_object(ext);
> @@ -942,7 +938,7 @@ cert_parse_pre(const char *fn, const uns
>  			    p.fn);
>  			goto out;
>  		}
> -		for (i = 0; i < p.res->asz; i++) {
> +		for (i = 0; (size_t)i < p.res->asz; i++) {
>  			if (p.res->as[i].type == CERT_AS_INHERIT) {
>  				warnx("%s: inherit elements not allowed in EE"
>  				    " cert", p.fn);
> 

I wonder if it would make sense to split 'i' into two variables. One as
int and another size_t one. Then the typecast in the second for loop is
not needed.

Is the compiler smart enough to not constantly evaluate
X509_get_ext_count() in the for loop? It should I guess.

Diff is OK claudio@
-- 
:wq Claudio