From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: rpki-client: fix some incorrect checks
To: Job Snijders <job@openbsd.org>
Cc: Theo Buehler <tb@theobuehler.org>, tech@openbsd.org
Date: Mon, 7 Oct 2024 13:46:13 +0200

On Mon, Oct 07, 2024 at 09:45:50AM +0000, Job Snijders wrote:
> OK job@

Also OK claudio@
 
> 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;
> >  	}
> >  
> > 
> 

-- 
:wq Claudio