From: Claudio Jeker Subject: Re: rpki-client: rework CRL parsing, first pass To: Theo Buehler Cc: tech@openbsd.org Date: Wed, 29 May 2024 14:12:40 +0200 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