Reintroduce check that CRL Number is in range

The CRL number draft clarified what ignoring means and it includes checking
that the CRL number is well-formed again. So do this but continue to ignore
the value for any other purpose. This refactors x509_convert_seqnum() into
a couple of helpers. There's some duplication between crl_check_crl_number()
and crl_parse_crl_number() which could be removed if anyone cares.

tweaks/ok job

5 files changed, 93 insertions, 66 deletions

diff --git a/usr.sbin/rpki-client/crl.c b/usr.sbin/rpki-client/crl.c
index fddd27aee5a..8611167961c 100644
--- a/usr.sbin/rpki-client/crl.c
+++ b/usr.sbin/rpki-client/crl.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: crl.c,v 1.42 2024/06/17 18:52:50 tb Exp $ */
+/*	$OpenBSD: crl.c,v 1.43 2024/09/12 10:33:25 tb Exp $ */
 /*
  * Copyright (c) 2024 Theo Buehler <tb@openbsd.org>
  * Copyright (c) 2019 Kristaps Dzonsons <kristaps@bsd.lv>
@@ -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_check_crl_number(const char *fn, const X509_CRL *x509_crl)
 {
-	const X509_EXTENSION	*ext;
-	int			 idx;
+	ASN1_INTEGER		*aint = NULL;
+	int			 crit;
+	int			 ret = 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;
+	ret = x509_valid_seqnum(fn, "CRL number", aint);
+
+ out:
+	ASN1_INTEGER_free(aint);
+	return ret;
 }
 
 /*
@@ -222,7 +229,7 @@ crl_parse(const char *fn, const unsigned char *der, size_t len)
 		    "%d != 2", fn, count);
 		goto out;
 	}
-	if (!crl_has_crl_number(fn, crl->x509_crl))
+	if (!crl_check_crl_number(fn, crl->x509_crl))
 		goto out;
 	if ((crl->aki = crl_get_aki(fn, crl->x509_crl)) == NULL)
 		goto out;
diff --git a/usr.sbin/rpki-client/extern.h b/usr.sbin/rpki-client/extern.h
index 8d5d9c2983a..b6612c170da 100644
--- a/usr.sbin/rpki-client/extern.h
+++ b/usr.sbin/rpki-client/extern.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: extern.h,v 1.227 2024/08/29 09:53:04 job Exp $ */
+/*	$OpenBSD: extern.h,v 1.228 2024/09/12 10:33:25 tb Exp $ */
 /*
  * Copyright (c) 2019 Kristaps Dzonsons <kristaps@bsd.lv>
  *
@@ -916,7 +916,10 @@ char		*x509_get_pubkey(X509 *, const char *);
 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 *);
diff --git a/usr.sbin/rpki-client/mft.c b/usr.sbin/rpki-client/mft.c
index 238b6865916..e981e04b7be 100644
--- a/usr.sbin/rpki-client/mft.c
+++ b/usr.sbin/rpki-client/mft.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: mft.c,v 1.118 2024/09/08 07:23:36 tb Exp $ */
+/*	$OpenBSD: mft.c,v 1.119 2024/09/12 10:33:25 tb Exp $ */
 /*
  * Copyright (c) 2022 Theo Buehler <tb@openbsd.org>
  * Copyright (c) 2019 Kristaps Dzonsons <kristaps@bsd.lv>
@@ -333,7 +333,8 @@ mft_parse_econtent(const char *fn, struct mft *mft, const unsigned char *d,
 	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;
 
diff --git a/usr.sbin/rpki-client/print.c b/usr.sbin/rpki-client/print.c
index 6738cbc7a43..d69dad299a3 100644
--- a/usr.sbin/rpki-client/print.c
+++ b/usr.sbin/rpki-client/print.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: print.c,v 1.55 2024/06/08 13:30:35 tb Exp $ */
+/*	$OpenBSD: print.c,v 1.56 2024/09/12 10:33:25 tb Exp $ */
 /*
  * Copyright (c) 2021 Claudio Jeker <claudio@openbsd.org>
  * Copyright (c) 2019 Kristaps Dzonsons <kristaps@bsd.lv>
@@ -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) {
diff --git a/usr.sbin/rpki-client/x509.c b/usr.sbin/rpki-client/x509.c
index d63cb56b8ec..48d1a7075e8 100644
--- a/usr.sbin/rpki-client/x509.c
+++ b/usr.sbin/rpki-client/x509.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: x509.c,v 1.100 2024/07/08 16:11:47 tb Exp $ */
+/*	$OpenBSD: x509.c,v 1.101 2024/09/12 10:33:25 tb Exp $ */
 /*
  * Copyright (c) 2022 Theo Buehler <tb@openbsd.org>
  * Copyright (c) 2021 Claudio Jeker <claudio@openbsd.org>
@@ -1015,46 +1015,73 @@ x509_valid_name(const char *fn, const char *descr, const X509_NAME *xn)
 }
 
 /*
- * 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(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(seqnum);
+	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;
+}
+
 /*
  * Find the closest expiry moment by walking the chain of authorities.
  */