From: Theo Buehler Subject: Re: rpki-client: rework CRL parsing, first pass To: tech@openbsd.org Date: Wed, 29 May 2024 13:49:28 +0200 On Wed, May 22, 2024 at 05:40:01AM +0200, Theo Buehler wrote: > Recent discussions revealed that the CRL Number has little to no meaning > in the RPKI [1]. Consequently it seems preferable to ignore its value, > only enforcing its presence and well-formedness as an X.509 extension. > rpki-client never really did anything with this number anyway. > > The diff below moves CRL AKI parsing and a weaker form of the CRL Number > parsing from x509.c to crl.c and moves the actual CRL Number parsing > to the printing in filemode where it still warns about malformed numbers. > I reversed the order of the checks so the cheapest comes first and the > most expensive last. Also avoid double warnings. > > In addition, do some checking of the list of revoked certificates. At > the moment bugs in rpki-rs and Krill (an old one and a new one) prevent > stricter conformance checks. The current checks are cheap since the RPKI > was careful to keep CRLs relatively small. > > There's still more that we should be checking about CRLs, but that's > for a later diff. > > [1]: https://mailarchive.ietf.org/arch/msg/sidrops/D-TY5ODjudygdjovjnrbe_WV78M/ Essentially the same diff with minor cosmetic changes. I'd really like to commit this so I can send out follow-ups. Index: crl.c =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/crl.c,v diff -u -p -r1.34 crl.c --- crl.c 21 Apr 2024 19:27:44 -0000 1.34 +++ crl.c 28 May 2024 04:54:20 -0000 @@ -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 * 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 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); } Index: extern.h =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v diff -u -p -r1.218 extern.h --- extern.h 20 May 2024 15:51:43 -0000 1.218 +++ extern.h 22 May 2024 02:56:06 -0000 @@ -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 *, 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 *); Index: print.c =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/print.c,v diff -u -p -r1.52 print.c --- print.c 26 Feb 2024 10:02:37 -0000 1.52 +++ print.c 22 May 2024 03:02:27 -0000 @@ -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); Index: x509.c =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v diff -u -p -r1.87 x509.c --- x509.c 21 Apr 2024 09:03:22 -0000 1.87 +++ x509.c 28 May 2024 06:56:44 -0000 @@ -787,92 +787,6 @@ x509_get_crl(X509 *x, const char *fn, ch } /* - * 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 } /* - * 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 *