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 10:58:00 +0100

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.
 
> 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	2 Dec 2025 15:32:14 -0000
> @@ -37,21 +37,26 @@ cms_extract_econtent(const char *fn, CMS
>  	if (res == NULL || rsz == NULL)
>  		return 1;
>  
> -	if ((os = CMS_get0_content(cms)) == NULL || *os == NULL) {
> +	if ((os = CMS_get0_content(cms)) == NULL || *os == NULL ||
> +	    (*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