From: Job Snijders <job@openbsd.org>
Subject: Re: rpki-client: fix some incorrect checks
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Mon, 7 Oct 2024 09:45:50 +0000

OK job@

On Mon, Oct 07, 2024 at 11:29:41AM +0200, Theo Buehler wrote:
> The validity interval is defined to be
> 
>    the date on which the certificate validity period begins (notBefore)
>    and the date on which the certificate validity period ends
>    (notAfter).
> 
> https://www.rfc-editor.org/rfc/rfc5280#section-4.1.2.5
> 
> This should therefore include the end points.
> 
> Instead of trying to be smart, let's check the BN itself. This way we
> can be sure the top bit isn't set and we will only accept numbers in
> the interval [0..2^159 - 1].
> 
> Index: cert.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> diff -u -p -r1.150 cert.c
> --- cert.c	8 Jul 2024 15:31:58 -0000	1.150
> +++ cert.c	7 Oct 2024 09:17:53 -0000
> @@ -1085,11 +1085,11 @@ ta_parse(const char *fn, struct cert *p,
>  		    "pubkey does not match TAL pubkey", fn);
>  		goto badcert;
>  	}
> -	if (p->notbefore >= now) {
> +	if (p->notbefore > now) {
>  		warnx("%s: certificate not yet valid", fn);
>  		goto badcert;
>  	}
> -	if (p->notafter <= now) {
> +	if (p->notafter < now) {
>  		warnx("%s: certificate has expired", fn);
>  		goto badcert;
>  	}
> Index: x509.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v
> diff -u -p -r1.101 x509.c
> --- x509.c	12 Sep 2024 10:33:25 -0000	1.101
> +++ x509.c	7 Oct 2024 09:18:53 -0000
> @@ -1023,11 +1023,6 @@ x509_seqnum_to_bn(const char *fn, const 
>  {
>  	BIGNUM *bn = NULL;
>  
> -	if (ASN1_STRING_length(i) > 20) {
> -		warnx("%s: %s should fit in 20 octets", fn, descr);
> -		goto out;
> -	}
> -
>  	if ((bn = ASN1_INTEGER_to_BN(i, NULL)) == NULL) {
>  		warnx("%s: %s: ASN1_INTEGER_to_BN error", fn, descr);
>  		goto out;
> @@ -1035,6 +1030,12 @@ x509_seqnum_to_bn(const char *fn, const 
>  
>  	if (BN_is_negative(bn)) {
>  		warnx("%s: %s should be non-negative", fn, descr);
> +		goto out;
> +	}
> +
> +	/* Reject values larger than or equal to 2^159. */
> +	if (BN_num_bytes(bn) > 20 || BN_is_bit_set(bn, 159)) {
> +		warnx("%s: %s should fit in 20 octets", fn, descr);
>  		goto out;
>  	}
>  
>