Download raw body.
rpki-client: rework CRL parsing, first pass
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 *
rpki-client: rework CRL parsing, first pass