Download raw body.
rpki-client: rework CRL parsing, first pass
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/
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 22 May 2024 03:38:39 -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 *crl)
+{
+ const X509_EXTENSION *ext;
+ int idx;
+
+ if ((idx = X509_CRL_get_ext_by_NID(crl, NID_crl_number, -1)) < 0) {
+ warnx("%s: RFC 6487, section 5: missing CRL number", fn);
+ return 0;
+ }
+ if ((ext = X509_CRL_get_ext(crl, idx)) == 0) {
+ 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.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;
+}
+
+/*
+ * 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 22 May 2024 03:01:37 -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 fewer than 20 octets (RFC 5280, section 4.1.2.2).
* Returned string needs to be freed by the caller.
*/
char *
rpki-client: rework CRL parsing, first pass