Download raw body.
rpki-client: rework CRL parsing, first pass
On Wed, May 29, 2024 at 01:49:28PM +0200, Theo Buehler wrote:
> 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.
I looked over it and did run the previous version. I see nothing that
stands out but I'm also not an X509 connoisseur
OK claudio@
> 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 *
>
--
:wq Claudio
rpki-client: rework CRL parsing, first pass