From: tb Date: Wed, 29 May 2024 13:26:24 +0000 (+0000) Subject: rpki-client: rework CRL handling X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=09383acc57b0f7e232321f510ba55cbc7949f9ba;p=openbsd rpki-client: rework CRL handling There is no benefit in parsing the CRLNumber in the RPKI. It is redundant with other mechanisms, notably the requirements on manifests. rpki-client never did anything with the CRL number anyway so stop parsing it in the main process. Move CRL AKI and CRL number handling from x509.c to crl.c, slightly improve error checking for X509_CRL_get_ext_d2i() and only check well-formedness of the CRL number: check it's there and non-critical. Avoid double warnings. Add some checks for the well-formedness of the list of revoked certs. Due to bugs in rpki-rs and Krill we can't reject empty lists (because ~15% of CRL's have this). And some people still use CRLs revoking certs at the time they expire. This latter point might change mid-2025. Add a hook for printing CRL numbers in file mode and warn about ill-formed numbers (negative and overlong ones). ok claudio job --- diff --git a/usr.sbin/rpki-client/crl.c b/usr.sbin/rpki-client/crl.c index 1f65073763c..45eaa623cdd 100644 --- a/usr.sbin/rpki-client/crl.c +++ b/usr.sbin/rpki-client/crl.c @@ -1,4 +1,4 @@ -/* $OpenBSD: crl.c,v 1.34 2024/04/21 19:27:44 claudio Exp $ */ +/* $OpenBSD: crl.c,v 1.35 2024/05/29 13:26:24 tb Exp $ */ /* * Copyright (c) 2019 Kristaps Dzonsons * @@ -24,6 +24,142 @@ #include "extern.h" +/* + * Check that the CRL number extension is present and that it is non-critical. + * Otherwise ignore it per draft-spaghetti-sidrops-rpki-crl-numbers. + */ +static int +crl_has_crl_number(const char *fn, const X509_CRL *x509_crl) +{ + const X509_EXTENSION *ext; + int idx; + + 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; + } + if (X509_EXTENSION_get_critical(ext) != 0) { + warnx("%s: RFC 6487, section 5: CRL number not non-critical", + fn); + return 0; + } + + return 1; +} + +/* + * Parse X509v3 authority key identifier (AKI) from the CRL. + * Returns the AKI or NULL if it could not be parsed. + * The AKI is formatted as a hex string. + */ +static char * +crl_get_aki(const char *fn, X509_CRL *x509_crl) +{ + AUTHORITY_KEYID *akid = NULL; + ASN1_OCTET_STRING *os; + const unsigned char *d; + int dsz, crit; + char *res = NULL; + + if ((akid = X509_CRL_get_ext_d2i(x509_crl, NID_authority_key_identifier, + &crit, NULL)) == NULL) { + if (crit != -1) + warnx("%s: RFC 6487 section 4.8.3: AKI: " + "failed to parse CRL extension", fn); + else + warnx("%s: RFC 6487 section 4.8.3: AKI: " + "CRL extension missing", fn); + goto out; + } + if (crit != 0) { + warnx("%s: RFC 6487 section 4.8.3: " + "AKI: extension not non-critical", fn); + goto out; + } + if (akid->issuer != NULL || akid->serial != NULL) { + warnx("%s: RFC 6487 section 4.8.3: AKI: " + "authorityCertIssuer or authorityCertSerialNumber present", + fn); + goto out; + } + + os = akid->keyid; + if (os == NULL) { + warnx("%s: RFC 6487 section 4.8.3: AKI: " + "Key Identifier missing", fn); + goto out; + } + + d = os->data; + dsz = os->length; + + if (dsz != SHA_DIGEST_LENGTH) { + warnx("%s: RFC 6487 section 4.8.3: AKI: " + "want %d bytes SHA1 hash, have %d bytes", + fn, SHA_DIGEST_LENGTH, dsz); + goto out; + } + + res = hex_encode(d, dsz); + out: + AUTHORITY_KEYID_free(akid); + return res; +} + +/* + * Check that the list of revoked certificates contains only the specified + * two fields, Serial Number and Revocation Date, and that no extensions are + * present. + */ +static int +crl_check_revoked(const char *fn, X509_CRL *x509_crl) +{ + STACK_OF(X509_REVOKED) *list; + X509_REVOKED *revoked; + int count, i; + + /* If there are no revoked certificates, there's nothing to check. */ + if ((list = X509_CRL_get_REVOKED(x509_crl)) == NULL) + return 1; + + if ((count = sk_X509_REVOKED_num(list)) <= 0) { + /* + * XXX - as of May 2024, ~15% of RPKI CRLs fail this check due + * to a bug in rpki-rs/Krill. So silently accept this for now. + * https://github.com/NLnetLabs/krill/issues/1197 + */ + if (verbose > 0) + warnx("%s: RFC 5280, section 5.1.2.6: revoked " + "certificate list without entries disallowed", fn); + return 1; + } + + for (i = 0; i < count; i++) { + revoked = sk_X509_REVOKED_value(list, i); + + /* + * serialNumber and revocationDate are mandatory in the ASN.1 + * template, so no need to check their presence. + * + * XXX - due to an old bug in Krill, we can't enforce that + * revocationDate is in the past until at least mid-2025: + * https://github.com/NLnetLabs/krill/issues/788. + */ + + if (X509_REVOKED_get0_extensions(revoked) != NULL) { + warnx("%s: RFC 6487, section 5: CRL entry extensions " + "disallowed", fn); + return 0; + } + } + + return 1; +} + struct crl * crl_parse(const char *fn, const unsigned char *der, size_t len) { @@ -76,19 +212,15 @@ crl_parse(const char *fn, const unsigned char *der, size_t len) * RFC 6487, section 5: AKI and crlNumber MUST be present, no other * CRL extensions are allowed. */ - if ((crl->aki = x509_crl_get_aki(crl->x509_crl, fn)) == NULL) { - warnx("%s: x509_crl_get_aki failed", fn); - goto out; - } - if ((crl->number = x509_crl_get_number(crl->x509_crl, fn)) == NULL) { - warnx("%s: x509_crl_get_number failed", fn); - goto out; - } if ((count = X509_CRL_get_ext_count(crl->x509_crl)) != 2) { warnx("%s: RFC 6487 section 5: unexpected number of extensions " "%d != 2", fn, count); goto out; } + if (!crl_has_crl_number(fn, crl->x509_crl)) + goto out; + if ((crl->aki = crl_get_aki(fn, crl->x509_crl)) == NULL) + goto out; at = X509_CRL_get0_lastUpdate(crl->x509_crl); if (at == NULL) { @@ -110,6 +242,9 @@ crl_parse(const char *fn, const unsigned char *der, size_t len) goto out; } + if (!crl_check_revoked(fn, crl->x509_crl)) + goto out; + rc = 1; out: if (rc == 0) { @@ -178,7 +313,6 @@ crl_free(struct crl *crl) return; free(crl->aki); free(crl->mftpath); - free(crl->number); X509_CRL_free(crl->x509_crl); free(crl); } diff --git a/usr.sbin/rpki-client/extern.h b/usr.sbin/rpki-client/extern.h index 6bdaab97638..6a871baad8a 100644 --- a/usr.sbin/rpki-client/extern.h +++ b/usr.sbin/rpki-client/extern.h @@ -1,4 +1,4 @@ -/* $OpenBSD: extern.h,v 1.218 2024/05/20 15:51:43 claudio Exp $ */ +/* $OpenBSD: extern.h,v 1.219 2024/05/29 13:26:24 tb Exp $ */ /* * Copyright (c) 2019 Kristaps Dzonsons * @@ -480,7 +480,6 @@ struct crl { RB_ENTRY(crl) entry; char *aki; char *mftpath; - char *number; X509_CRL *x509_crl; time_t thisupdate; /* do not use before */ time_t nextupdate; /* do not use after */ @@ -909,8 +908,6 @@ int x509_get_ski(X509 *, const char *, char **); int x509_get_notbefore(X509 *, const char *, time_t *); int x509_get_notafter(X509 *, const char *, time_t *); int x509_get_crl(X509 *, const char *, char **); -char *x509_crl_get_aki(X509_CRL *, const char *); -char *x509_crl_get_number(X509_CRL *, const char *); 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 *); diff --git a/usr.sbin/rpki-client/print.c b/usr.sbin/rpki-client/print.c index 9ef44b73620..a88fbfba7fa 100644 --- a/usr.sbin/rpki-client/print.c +++ b/usr.sbin/rpki-client/print.c @@ -1,4 +1,4 @@ -/* $OpenBSD: print.c,v 1.52 2024/02/26 10:02:37 job Exp $ */ +/* $OpenBSD: print.c,v 1.53 2024/05/29 13:26:24 tb Exp $ */ /* * Copyright (c) 2021 Claudio Jeker * Copyright (c) 2019 Kristaps Dzonsons @@ -324,6 +324,48 @@ 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"); + else + warnx("CRL Number missing"); + 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"); + 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"); + + out: + ASN1_INTEGER_free(aint); + BN_free(seqnum); + return s; +} + void crl_print(const struct crl *p) { @@ -342,13 +384,20 @@ crl_print(const struct crl *p) xissuer = X509_CRL_get_issuer(p->x509_crl); issuer = X509_NAME_oneline(xissuer, NULL, 0); - if (issuer != NULL && p->number != NULL) { - if (outformats & FORMAT_JSON) { - json_do_string("crl_issuer", issuer); - json_do_string("crl_serial", p->number); - } else { - printf("CRL issuer: %s\n", issuer); - printf("CRL serial number: %s\n", p->number); + if (issuer != NULL) { + char *number; + + if ((number = crl_parse_number(p->x509_crl)) != NULL) { + if (outformats & FORMAT_JSON) { + json_do_string("crl_issuer", issuer); + json_do_string("crl_serial", number); + } else { + printf("CRL issuer: %s\n", + issuer); + printf("CRL serial number: %s\n", + number); + } + free(number); } } free(issuer); diff --git a/usr.sbin/rpki-client/x509.c b/usr.sbin/rpki-client/x509.c index 8ce43b3dfdb..d3d3a6c6ae5 100644 --- a/usr.sbin/rpki-client/x509.c +++ b/usr.sbin/rpki-client/x509.c @@ -1,4 +1,4 @@ -/* $OpenBSD: x509.c,v 1.87 2024/04/21 09:03:22 job Exp $ */ +/* $OpenBSD: x509.c,v 1.88 2024/05/29 13:26:24 tb Exp $ */ /* * Copyright (c) 2022 Theo Buehler * Copyright (c) 2021 Claudio Jeker @@ -786,92 +786,6 @@ x509_get_crl(X509 *x, const char *fn, char **crl) return rsync_found; } -/* - * Parse X509v3 authority key identifier (AKI) from the CRL. - * This is matched against the string from x509_get_ski() above. - * Returns the AKI or NULL if it could not be parsed. - * The AKI is formatted as a hex string. - */ -char * -x509_crl_get_aki(X509_CRL *crl, const char *fn) -{ - const unsigned char *d; - AUTHORITY_KEYID *akid; - ASN1_OCTET_STRING *os; - int dsz, crit; - char *res = NULL; - - akid = X509_CRL_get_ext_d2i(crl, NID_authority_key_identifier, &crit, - NULL); - if (akid == NULL) { - warnx("%s: RFC 6487 section 4.8.3: AKI: extension missing", fn); - return NULL; - } - if (crit != 0) { - warnx("%s: RFC 6487 section 4.8.3: " - "AKI: extension not non-critical", fn); - goto out; - } - if (akid->issuer != NULL || akid->serial != NULL) { - warnx("%s: RFC 6487 section 4.8.3: AKI: " - "authorityCertIssuer or authorityCertSerialNumber present", - fn); - goto out; - } - - os = akid->keyid; - if (os == NULL) { - warnx("%s: RFC 6487 section 4.8.3: AKI: " - "Key Identifier missing", fn); - goto out; - } - - d = os->data; - dsz = os->length; - - if (dsz != SHA_DIGEST_LENGTH) { - warnx("%s: RFC 6487 section 4.8.2: AKI: " - "want %d bytes SHA1 hash, have %d bytes", - fn, SHA_DIGEST_LENGTH, dsz); - goto out; - } - - res = hex_encode(d, dsz); -out: - AUTHORITY_KEYID_free(akid); - return res; -} - -/* - * Retrieve CRL Number extension. Returns a printable hexadecimal representation - * of the number which has to be freed after use. - */ -char * -x509_crl_get_number(X509_CRL *crl, const char *fn) -{ - ASN1_INTEGER *aint; - int crit; - char *res = NULL; - - aint = X509_CRL_get_ext_d2i(crl, NID_crl_number, &crit, NULL); - if (aint == NULL) { - warnx("%s: RFC 6487 section 5: CRL Number missing", fn); - return NULL; - } - if (crit != 0) { - warnx("%s: RFC 5280, section 5.2.3: " - "CRL Number not non-critical", fn); - goto out; - } - - /* This checks that the number is non-negative and <= 20 bytes. */ - res = x509_convert_seqnum(fn, aint); - - out: - ASN1_INTEGER_free(aint); - return res; -} - /* * Convert passed ASN1_TIME to time_t *t. * Returns 1 on success and 0 on failure. @@ -1008,7 +922,8 @@ x509_valid_subject(const char *fn, const X509 *x) } /* - * Convert an ASN1_INTEGER into a hexstring. + * 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 *