From: Theo Buehler <tb@theobuehler.org>
Subject: Re: rpki-client: convert cms.c to opaque ASN1_STRING
To: tech@openbsd.org
Date: Wed, 3 Dec 2025 11:04:28 +0100

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.

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;
 }