From: Theo Buehler <tb@theobuehler.org>
Subject: rpki-client: factor attribute checking out of cms validation
To: tech@openbsd.org
Date: Mon, 17 Nov 2025 07:05:49 +0100

cms_parse_validate_internal() has grown into an extremely complicated
beast that I find hard to reason about. Upcoming refactoring will turn
the logic upside down and inside out, so I'd like to make side effects
or absence thereof more obvious.

Here's a first simple step that moves the attribute checking and sign
time extraction into a helper.

Index: cms.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/cms.c,v
diff -u -p -r1.55 cms.c
--- cms.c	14 Nov 2025 22:16:15 -0000	1.55
+++ cms.c	17 Nov 2025 05:38:41 -0000
@@ -85,6 +85,87 @@ cms_get_signtime(const char *fn, X509_AT
 }
 
 static int
+cms_SignerInfo_check_attributes(const char *fn, const CMS_SignerInfo *si,
+    time_t *signtime)
+{
+	char buf[128];
+	const ASN1_OBJECT *obj;
+	int i, nattrs;
+	int has_ct = 0, has_md = 0, has_st = 0;
+
+	*signtime = 0;
+
+	nattrs = CMS_signed_get_attr_count(si);
+	if (nattrs <= 0) {
+		warnx("%s: RFC 6488: error extracting signedAttrs", fn);
+		return 0;
+	}
+	for (i = 0; i < nattrs; i++) {
+		X509_ATTRIBUTE *attr;
+
+		attr = CMS_signed_get_attr(si, i);
+		if (attr == NULL || X509_ATTRIBUTE_count(attr) != 1) {
+			warnx("%s: RFC 6488: bad signed attribute encoding",
+			    fn);
+			return 0;
+		}
+
+		obj = X509_ATTRIBUTE_get0_object(attr);
+		if (obj == NULL) {
+			warnx("%s: RFC 6488: bad signed attribute", fn);
+			return 0;
+		}
+		if (OBJ_cmp(obj, cnt_type_oid) == 0) {
+			if (has_ct++ != 0) {
+				warnx("%s: RFC 6488: duplicate "
+				    "signed attribute", fn);
+				return 0;
+			}
+		} else if (OBJ_cmp(obj, msg_dgst_oid) == 0) {
+			if (has_md++ != 0) {
+				warnx("%s: RFC 6488: duplicate "
+				    "signed attribute", fn);
+				return 0;
+			}
+		} else if (OBJ_cmp(obj, sign_time_oid) == 0) {
+			if (has_st++ != 0) {
+				warnx("%s: RFC 6488: duplicate "
+				    "signed attribute", fn);
+				return 0;
+			}
+			if (!cms_get_signtime(fn, attr, signtime))
+				return 0;
+		} else {
+			OBJ_obj2txt(buf, sizeof(buf), obj, 1);
+			warnx("%s: RFC 6488: "
+			    "CMS has unexpected signed attribute %s",
+			    fn, buf);
+			return 0;
+		}
+	}
+
+	if (!has_ct || !has_md) {
+		/* RFC 9589, section 4 */
+		warnx("%s: RFC 6488: CMS missing required "
+		    "signed attribute", fn);
+		return 0;
+	}
+
+	if (!has_st) {
+		/* RFC 9589, section 4 */
+		warnx("%s: missing CMS signing-time attribute", fn);
+		return 0;
+	}
+
+	if (CMS_unsigned_get_attr_count(si) != -1) {
+		warnx("%s: RFC 6488: CMS has unsignedAttrs", fn);
+		return 0;
+	}
+
+	return 1;
+}
+
+static int
 cms_parse_validate_internal(struct cert **out_cert, const char *fn, int talid,
     const unsigned char *der, size_t len, const ASN1_OBJECT *oid, BIO *bio,
     unsigned char **res, size_t *rsz, time_t *signtime)
@@ -101,8 +182,7 @@ cms_parse_validate_internal(struct cert 
 	STACK_OF(CMS_SignerInfo)	*sinfos;
 	CMS_SignerInfo			*si;
 	X509_ALGOR			*pdig, *psig;
-	int				 i, nattrs, nid;
-	int				 has_ct = 0, has_md = 0, has_st = 0;
+	int				 nid;
 	int				 rc = 0;
 
 	assert(*out_cert == NULL);
@@ -171,72 +251,8 @@ cms_parse_validate_internal(struct cert 
 		goto out;
 	}
 
-	nattrs = CMS_signed_get_attr_count(si);
-	if (nattrs <= 0) {
-		warnx("%s: RFC 6488: error extracting signedAttrs", fn);
+	if (!cms_SignerInfo_check_attributes(fn, si, signtime))
 		goto out;
-	}
-	for (i = 0; i < nattrs; i++) {
-		X509_ATTRIBUTE *attr;
-
-		attr = CMS_signed_get_attr(si, i);
-		if (attr == NULL || X509_ATTRIBUTE_count(attr) != 1) {
-			warnx("%s: RFC 6488: bad signed attribute encoding",
-			    fn);
-			goto out;
-		}
-
-		obj = X509_ATTRIBUTE_get0_object(attr);
-		if (obj == NULL) {
-			warnx("%s: RFC 6488: bad signed attribute", fn);
-			goto out;
-		}
-		if (OBJ_cmp(obj, cnt_type_oid) == 0) {
-			if (has_ct++ != 0) {
-				warnx("%s: RFC 6488: duplicate "
-				    "signed attribute", fn);
-				goto out;
-			}
-		} else if (OBJ_cmp(obj, msg_dgst_oid) == 0) {
-			if (has_md++ != 0) {
-				warnx("%s: RFC 6488: duplicate "
-				    "signed attribute", fn);
-				goto out;
-			}
-		} else if (OBJ_cmp(obj, sign_time_oid) == 0) {
-			if (has_st++ != 0) {
-				warnx("%s: RFC 6488: duplicate "
-				    "signed attribute", fn);
-				goto out;
-			}
-			if (!cms_get_signtime(fn, attr, signtime))
-				goto out;
-		} else {
-			OBJ_obj2txt(buf, sizeof(buf), obj, 1);
-			warnx("%s: RFC 6488: "
-			    "CMS has unexpected signed attribute %s",
-			    fn, buf);
-			goto out;
-		}
-	}
-
-	if (!has_ct || !has_md) {
-		/* RFC 9589, section 4 */
-		warnx("%s: RFC 6488: CMS missing required "
-		    "signed attribute", fn);
-		goto out;
-	}
-
-	if (!has_st) {
-		/* RFC 9589, section 4 */
-		warnx("%s: missing CMS signing-time attribute", fn);
-		goto out;
-	}
-
-	if (CMS_unsigned_get_attr_count(si) != -1) {
-		warnx("%s: RFC 6488: CMS has unsignedAttrs", fn);
-		goto out;
-	}
 
 	/* Check digest and signature algorithms (RFC 7935) */
 	CMS_SignerInfo_get0_algs(si, NULL, NULL, &pdig, &psig);