From 904d9c60a494ce97b94afaf3fd42c67d7804546d Mon Sep 17 00:00:00 2001 From: tb Date: Thu, 12 Sep 2024 10:33:25 +0000 Subject: [PATCH] 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 --- usr.sbin/rpki-client/crl.c | 39 +++++++++++-------- usr.sbin/rpki-client/extern.h | 7 +++- usr.sbin/rpki-client/mft.c | 5 ++- usr.sbin/rpki-client/print.c | 35 ++++++----------- usr.sbin/rpki-client/x509.c | 73 ++++++++++++++++++++++++----------- 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 * Copyright (c) 2019 Kristaps Dzonsons @@ -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 * @@ -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 * Copyright (c) 2019 Kristaps Dzonsons @@ -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 * Copyright (c) 2019 Kristaps Dzonsons @@ -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 * Copyright (c) 2021 Claudio Jeker @@ -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. */ -- 2.20.1