Reintroduce check that CRL Number is in range
authortb <tb@openbsd.org>
Thu, 12 Sep 2024 10:33:25 +0000 (10:33 +0000)
committertb <tb@openbsd.org>
Thu, 12 Sep 2024 10:33:25 +0000 (10:33 +0000)
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

usr.sbin/rpki-client/crl.c
usr.sbin/rpki-client/extern.h
usr.sbin/rpki-client/mft.c
usr.sbin/rpki-client/print.c
usr.sbin/rpki-client/x509.c

index fddd27a..8611167 100644 (file)
@@ -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>
 #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;
index 8d5d9c2..b6612c1 100644 (file)
@@ -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 *);
index 238b686..e981e04 100644 (file)
@@ -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;
 
index 6738cbc..d69dad2 100644 (file)
@@ -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) {
index d63cb56..48d1a70 100644 (file)
@@ -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.
  */