It looks like we're circling back to enforcing the CRL number, including
its value, conforms to RFC 5280.

This diff refactors x509_convert_seqnum() so we don't add a third copy
of the same checks. The diff modifies a few warning messages and adds a
missing non-criticality check to crl_parse_number().

The new x509_seqnum_to_bn() helper could be avoided at the cost of doing
an extra BN_bn2hex() for each CRL. I guess that would be acceptable - we
did it before. If that's preferred, I'd move crl_parse_number() to crl.c
and free the hex string in crl_parse().

Index: crl.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/crl.c,v
diff -u -p -r1.42 crl.c
--- crl.c	17 Jun 2024 18:52:50 -0000	1.42
+++ crl.c	11 Sep 2024 06:58:43 -0000
@@ -26,30 +26,37 @@
 #include "extern.h"
 
 /*
- * Check that the CRL number extension is present and that it is non-critical.
+ * Check CRL Number is present, non-critical and in [0, 2^159-1].
  * Otherwise ignore it per draft-spaghetti-sidrops-rpki-crl-numbers.
  */
 static int
-crl_has_crl_number(const char *fn, const X509_CRL *x509_crl)
+crl_validate_crl_number(const char *fn, const X509_CRL *x509_crl)
 {
-	const X509_EXTENSION	*ext;
-	int			 idx;
+	ASN1_INTEGER		*aint = NULL;
+	int			 crit;
+	int			 ok = 0;
 
-	if ((idx = X509_CRL_get_ext_by_NID(x509_crl, NID_crl_number, -1)) < 0) {
-		warnx("%s: RFC 6487, section 5: missing CRL number", fn);
-		return 0;
-	}
-	if ((ext = X509_CRL_get_ext(x509_crl, idx)) == NULL) {
-		warnx("%s: RFC 6487, section 5: failed to get CRL number", fn);
-		return 0;
+	aint = X509_CRL_get_ext_d2i(x509_crl, NID_crl_number, &crit, NULL);
+	if (aint == NULL) {
+		if (crit != -1)
+			warnx("%s: RFC 6487, section 5: "
+			    "failed to parse CRL number", fn);
+		else
+			warnx("%s: RFC 6487, section 5: missing CRL number",
+			    fn);
+		goto out;
 	}
-	if (X509_EXTENSION_get_critical(ext) != 0) {
+	if (crit != 0) {
 		warnx("%s: RFC 6487, section 5: CRL number not non-critical",
 		    fn);
-		return 0;
+		goto out;
 	}
 
-	return 1;
+	ok = x509_valid_seqnum(fn, "CRL number", aint);
+
+ out:
+	ASN1_INTEGER_free(aint);
+	return ok;
 }
 
 /*
@@ -222,7 +229,7 @@ crl_parse(const char *fn, const unsigned
 		    "%d != 2", fn, count);
 		goto out;
 	}
-	if (!crl_has_crl_number(fn, crl->x509_crl))
+	if (!crl_validate_crl_number(fn, crl->x509_crl))
 		goto out;
 	if ((crl->aki = crl_get_aki(fn, crl->x509_crl)) == NULL)
 		goto out;
Index: extern.h
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
diff -u -p -r1.227 extern.h
--- extern.h	29 Aug 2024 09:53:04 -0000	1.227
+++ extern.h	11 Sep 2024 06:47:01 -0000
@@ -916,7 +916,10 @@ char		*x509_get_pubkey(X509 *, const cha
 char		*x509_pubkey_get_ski(X509_PUBKEY *, const char *);
 enum cert_purpose	 x509_get_purpose(X509 *, const char *);
 int		 x509_get_time(const ASN1_TIME *, time_t *);
-char		*x509_convert_seqnum(const char *, const ASN1_INTEGER *);
+char		*x509_convert_seqnum(const char *, const char *,
+		    const ASN1_INTEGER *);
+int		 x509_valid_seqnum(const char *, const char *,
+		    const ASN1_INTEGER *);
 int		 x509_location(const char *, const char *, GENERAL_NAME *,
 		    char **);
 int		 x509_inherits(X509 *);
Index: mft.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/mft.c,v
diff -u -p -r1.118 mft.c
--- mft.c	8 Sep 2024 07:23:36 -0000	1.118
+++ mft.c	11 Sep 2024 06:52:39 -0000
@@ -333,7 +333,8 @@ mft_parse_econtent(const char *fn, struc
 	if (!valid_econtent_version(fn, mft_asn1->version, 0))
 		goto out;
 
-	mft->seqnum = x509_convert_seqnum(fn, mft_asn1->manifestNumber);
+	mft->seqnum = x509_convert_seqnum(fn, "manifest number",
+	    mft_asn1->manifestNumber);
 	if (mft->seqnum == NULL)
 		goto out;
 
Index: print.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/print.c,v
diff -u -p -r1.55 print.c
--- print.c	8 Jun 2024 13:30:35 -0000	1.55
+++ print.c	11 Sep 2024 06:59:28 -0000
@@ -159,7 +159,8 @@ x509_print(const X509 *x)
 		goto out;
 	}
 
-	if ((serial = x509_convert_seqnum(__func__, xserial)) == NULL)
+	if ((serial = x509_convert_seqnum(__func__, "serial number",
+	    xserial)) == NULL)
 		goto out;
 
 	if (outformats & FORMAT_JSON) {
@@ -342,45 +343,33 @@ cert_print(const struct cert *p)
 		json_do_end();
 }
 
-/*
- * XXX - dedup with x509_convert_seqnum()?
- */
 static char *
 crl_parse_number(const X509_CRL *x509_crl)
 {
 	ASN1_INTEGER	*aint = NULL;
 	int		 crit;
-	BIGNUM		*seqnum = NULL;
 	char		*s = NULL;
 
 	aint = X509_CRL_get_ext_d2i(x509_crl, NID_crl_number, &crit, NULL);
 	if (aint == NULL) {
 		if (crit != -1)
-			warnx("failed to parse CRL Number");
+			warnx("%s: RFC 6487, section 5: "
+			    "failed to parse CRL number", __func__);
 		else
-			warnx("CRL Number missing");
+			warnx("%s: RFC 6487, section 5: missing CRL number",
+			    __func__);
 		goto out;
 	}
-
-	if (ASN1_STRING_length(aint) > 20)
-		warnx("CRL Number should fit in 20 octets");
-
-	seqnum = ASN1_INTEGER_to_BN(aint, NULL);
-	if (seqnum == NULL) {
-		warnx("CRL Number: ASN1_INTEGER_to_BN error");
+	if (crit != 0) {
+		warnx("%s: RFC 6487, section 5: CRL number not non-critical",
+		    __func__);
 		goto out;
 	}
 
-	if (BN_is_negative(seqnum))
-		warnx("CRL Number should be positive");
-
-	s = BN_bn2hex(seqnum);
-	if (s == NULL)
-		warnx("CRL Number: BN_bn2hex error");
+	s = x509_convert_seqnum(__func__, "CRL Number", aint);
 
  out:
 	ASN1_INTEGER_free(aint);
-	BN_free(seqnum);
 	return s;
 }
 
@@ -435,7 +424,7 @@ crl_print(const struct crl *p)
 	revlist = X509_CRL_get_REVOKED(p->x509_crl);
 	for (i = 0; i < sk_X509_REVOKED_num(revlist); i++) {
 		rev = sk_X509_REVOKED_value(revlist, i);
-		serial = x509_convert_seqnum(__func__,
+		serial = x509_convert_seqnum(__func__, "serial number",
 		    X509_REVOKED_get0_serialNumber(rev));
 		x509_get_time(X509_REVOKED_get0_revocationDate(rev), &t);
 		if (serial != NULL) {
Index: x509.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v
diff -u -p -r1.100 x509.c
--- x509.c	8 Jul 2024 16:11:47 -0000	1.100
+++ x509.c	11 Sep 2024 07:12:20 -0000
@@ -1015,44 +1015,72 @@ x509_valid_name(const char *fn, const ch
 }
 
 /*
- * Convert an ASN1_INTEGER into a hexstring, enforcing that it is non-negative
- * and representable by at most 20 octets (RFC 5280, section 4.1.2.2).
- * Returned string needs to be freed by the caller.
+ * Check ASN1_INTEGER is non-negative and fits in 20 octets.
+ * Returns allocated BIGNUM if true, NULL otherwise.
  */
-char *
-x509_convert_seqnum(const char *fn, const ASN1_INTEGER *i)
+static BIGNUM *
+x509_seqnum_to_bn(const char *fn, const char *descr, const ASN1_INTEGER *i)
 {
-	BIGNUM	*seqnum = NULL;
-	char	*s = NULL;
-
-	if (i == NULL)
-		goto out;
+	BIGNUM *bn = NULL;
 
 	if (ASN1_STRING_length(i) > 20) {
-		warnx("%s: %s: want 20 octets or fewer, have more.",
-		    __func__, fn);
+		warnx("%s: %s should fit in 20 octets", fn, descr);
 		goto out;
 	}
 
-	seqnum = ASN1_INTEGER_to_BN(i, NULL);
-	if (seqnum == NULL) {
-		warnx("%s: ASN1_INTEGER_to_BN error", fn);
+	if ((bn = ASN1_INTEGER_to_BN(i, NULL)) == NULL) {
+		warnx("%s: %s: ASN1_INTEGER_to_BN error", fn, descr);
 		goto out;
 	}
 
-	if (BN_is_negative(seqnum)) {
-		warnx("%s: %s: want positive integer, have negative.",
-		    __func__, fn);
+	if (BN_is_negative(bn)) {
+		warnx("%s: %s should be non-negative", fn, descr);
 		goto out;
 	}
 
-	s = BN_bn2hex(seqnum);
-	if (s == NULL)
-		warnx("%s: BN_bn2hex error", fn);
+	return bn;
 
  out:
-	BN_free(seqnum);
+	BN_free(bn);
+	return NULL;
+}
+
+/*
+ * Convert an ASN1_INTEGER into a hexstring, enforcing that it is non-negative
+ * and representable by at most 20 octets (RFC 5280, section 4.1.2.2).
+ * Returned string needs to be freed by the caller.
+ */
+char *
+x509_convert_seqnum(const char *fn, const char *descr, const ASN1_INTEGER *i)
+{
+	BIGNUM	*bn = NULL;
+	char	*s = NULL;
+
+	if (i == NULL)
+		goto out;
+
+	if ((bn = x509_seqnum_to_bn(fn, descr, i)) == NULL)
+		goto out;
+
+	if ((s = BN_bn2hex(bn)) == NULL)
+		warnx("%s: %s: BN_bn2hex error", fn, descr);
+
+ out:
+	BN_free(bn);
 	return s;
+}
+
+int
+x509_valid_seqnum(const char *fn, const char *descr, const ASN1_INTEGER *i)
+{
+	BIGNUM *bn;
+
+	if ((bn = x509_seqnum_to_bn(fn, descr, i)) == NULL)
+		return 0;
+
+	BN_free(bn);
+
+	return 1;
 }
 
 /*