From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: rpki-client: convert cms.c to opaque ASN1_STRING
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Wed, 3 Dec 2025 11:13:07 +0100

On Wed, Dec 03, 2025 at 11:04:28AM +0100, Theo Buehler wrote:
> On Wed, Dec 03, 2025 at 10:58:00AM +0100, Claudio Jeker wrote:
> > On Tue, Dec 02, 2025 at 04:58:20PM +0100, Theo Buehler wrote:
> > > Here's the conversion of CMS to opaque ASN1_STRING.
> > > 
> > > I decided to add two sanity checks on *rsz. I don't think these can
> > > actually be hit: the required OID should ensure non-zero length and
> > > since we don't allow files larger than MAX_FILE_SIZE, the upper bound
> > > should also be impossible even if indefinite length encoding is used.
> > > 
> > > Anyway, it seems like the cautious thing to do: it avoids a malloc(0)
> > > and spending time on parsing a preposterously large eContent or, worse,
> > > hitting a malloc failure due to malicious remote data.
> > 
> > OK claudio@ but I dislike how *rsz is set as last inside a "complex" if
> > statement.  In this case it is fine but conditional assignments like this
> > make me a bit uneasy. I shot myself too often with constructs like this.
> 
> I agree. I didn't like it either. But I also didn't like a repeated
> error message, which is the only other option I see.  Perhaps this is
> the lesser evil...
> 
> I expect this to be improved in a future refactoring anyway. There
> should be no need for such a copy in the first place.

OK claudio@

> Index: cms.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/cms.c,v
> diff -u -p -r1.58 cms.c
> --- cms.c	19 Nov 2025 23:21:56 -0000	1.58
> +++ cms.c	3 Dec 2025 10:00:47 -0000
> @@ -43,15 +43,24 @@ cms_extract_econtent(const char *fn, CMS
>  		return 0;
>  	}
>  
> +	if ((*rsz = ASN1_STRING_length(*os)) == 0) {
> +		warnx("%s: RFC 6488 section 2.1.4: "
> +		    "eContent: zero-length content", fn);
> +		return 0;
> +	}
> +	if (*rsz > MAX_FILE_SIZE) {
> +		warnx("%s: overlong eContent of length %zu", fn, *rsz);
> +		return 0;
> +	}
> +
>  	/*
>  	 * The eContent in os is owned by the cms object and it has to outlive
>  	 * it for further processing by the signedObject handlers. Since there
>  	 * is no convenient API for this purpose, duplicate it by hand.
>  	 */
> -	if ((*res = malloc((*os)->length)) == NULL)
> +	if ((*res = malloc(*rsz)) == NULL)
>  		err(1, NULL);
> -	memcpy(*res, (*os)->data, (*os)->length);
> -	*rsz = (*os)->length;
> +	memcpy(*res, ASN1_STRING_get0_data(*os), *rsz);
>  
>  	return 1;
>  }
> 

-- 
:wq Claudio