From: Theo Buehler Subject: rpki-client: improve purpose handling To: tech@openbsd.org Date: Sat, 8 Jun 2024 05:35:41 +0200 We should not just distinguish between CA and BGPsec router certs. It's always been confusing and I think the below is clearer, slightly stricter and more correct. This introduces two more cert purposes to cover TA certs and EE certs. With a little bit more effort in x509_get_purpose() we can distinguish between all four kinds of certificates we expect to see in the RPKI. Most of the diff is straightforward. I factored the X509_check_purpose() magic into x509_cache_extensions() since that's clearer in the callers. I also sprinkled a few calls to this at some places where I found myself checking over and over again. The expensive part of extension caching is done only once since libcrypto sets a flag on the cert. Check in cert_parse_ee_cert() that the purpose is EE, in ta_parse() that it is a TA and reject EE certs in cert_parse_pre() (well, technically BGPsec Router certs are also EE certs... They're special and few and far between). The caller should tell us if we should expect a TA or not. It would be great if it could indicate CA or BGPsec router, so we could just pass in the expected purpose, but I don't think it can. In x509_get_purpose() there are a few changes. The extension caching is for my future self. X509_check_ca() is a strange API. In the RPKI it should only return 0 or 1, but is_ca == 4 is a conceivable misissuance. The rest is summarized in the doc comment. I left two XXX for follow-ups. Index: cert.c =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v diff -u -p -r1.141 cert.c --- cert.c 7 Jun 2024 08:36:54 -0000 1.141 +++ cert.c 8 Jun 2024 02:44:55 -0000 @@ -744,6 +744,15 @@ cert_parse_ee_cert(const char *fn, int t if (!cert_check_subject_and_issuer(fn, x)) goto out; + if (!x509_cache_extensions(x, fn)) + goto out; + + if ((cert->purpose = x509_get_purpose(x, fn)) != CERT_PURPOSE_EE) { + warnx("%s: expected EE cert, got %s", fn, + purpose2str(cert->purpose)); + goto out; + } + if (X509_get_key_usage(x) != KU_DIGITAL_SIGNATURE) { warnx("%s: RFC 6487 section 4.8.4: KU must be digitalSignature", fn); @@ -827,11 +836,8 @@ cert_parse_pre(const char *fn, const uns goto out; } - /* Cache X509v3 extensions, see X509_check_ca(3). */ - if (X509_check_purpose(x, -1, -1) <= 0) { - warnx("%s: could not cache X509v3 extensions", fn); + if (!x509_cache_extensions(x, fn)) goto out; - } if (X509_get_version(x) != 2) { warnx("%s: RFC 6487 4.1: X.509 version must be v3", fn); @@ -957,11 +963,12 @@ cert_parse_pre(const char *fn, const uns goto out; if (!x509_get_notafter(x, fn, &cert->notafter)) goto out; - cert->purpose = x509_get_purpose(x, fn); /* Validation on required fields. */ - + cert->purpose = x509_get_purpose(x, fn); switch (cert->purpose) { + case CERT_PURPOSE_TA: + /* XXX - caller should indicate if it expects TA or CA cert */ case CERT_PURPOSE_CA: if ((pkey = X509_get0_pubkey(x)) == NULL) { warnx("%s: X509_get0_pubkey failed", fn); @@ -1015,6 +1022,9 @@ cert_parse_pre(const char *fn, const uns goto out; } break; + case CERT_PURPOSE_EE: + warn("%s: unexpected EE cert", fn); + goto out; default: warnx("%s: x509_get_purpose failed in %s", fn, __func__); goto out; @@ -1117,12 +1127,9 @@ ta_parse(const char *fn, struct cert *p, "trust anchor may not specify CRL resource", fn); goto badcert; } - /* - * XXX - this check for BGPsec router certs doesn't make all that much - * sense. Consider introducing a TA purpose for self-issued CA certs. - */ - if (p->purpose == CERT_PURPOSE_BGPSEC_ROUTER) { - warnx("%s: BGPsec cert cannot be a trust anchor", fn); + if (p->purpose != CERT_PURPOSE_TA) { + warnx("%s: expected trust anchor purpose, got %s", fn, + purpose2str(p->purpose)); goto badcert; } /* Index: cms.c =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/cms.c,v diff -u -p -r1.45 cms.c --- cms.c 24 May 2024 12:57:20 -0000 1.45 +++ cms.c 7 Jun 2024 19:38:55 -0000 @@ -324,11 +324,8 @@ cms_parse_validate_internal(X509 **xp, c goto out; } - /* Cache X509v3 extensions, see X509_check_ca(3). */ - if (X509_check_purpose(*xp, -1, -1) <= 0) { - warnx("%s: could not cache X509v3 extensions", fn); + if (!x509_cache_extensions(*xp, fn)) goto out; - } if (!x509_get_notafter(*xp, fn, ¬after)) goto out; Index: extern.h =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v diff -u -p -r1.221 extern.h --- extern.h 4 Jun 2024 04:17:18 -0000 1.221 +++ extern.h 8 Jun 2024 03:16:34 -0000 @@ -107,8 +107,10 @@ struct cert_ip { enum cert_purpose { CERT_PURPOSE_INVALID, + CERT_PURPOSE_TA, CERT_PURPOSE_CA, - CERT_PURPOSE_BGPSEC_ROUTER + CERT_PURPOSE_EE, + CERT_PURPOSE_BGPSEC_ROUTER, }; /* @@ -901,6 +903,7 @@ struct ibuf *io_buf_recvfd(int, struct i /* X509 helpers. */ void x509_init_oid(void); +int x509_cache_extensions(X509 *, const char *); int x509_get_aia(X509 *, const char *, char **); int x509_get_aki(X509 *, const char *, char **); int x509_get_sia(X509 *, const char *, char **); @@ -922,6 +925,7 @@ time_t x509_find_expires(time_t, struc /* printers */ char *nid2str(int); +const char *purpose2str(enum cert_purpose); char *time2str(time_t); void x509_print(const X509 *); void tal_print(const struct tal *); Index: filemode.c =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/filemode.c,v diff -u -p -r1.43 filemode.c --- filemode.c 6 Jun 2024 07:19:10 -0000 1.43 +++ filemode.c 7 Jun 2024 21:04:24 -0000 @@ -157,7 +157,8 @@ parse_load_cert(char *uri) if (cert == NULL) goto done; if (cert->purpose != CERT_PURPOSE_CA) { - warnx("AIA reference to bgpsec cert %s", uri); + warnx("AIA reference to %s in %s", + purpose2str(cert->purpose), uri); goto done; } /* try to load the CRL of this cert */ Index: main.c =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v diff -u -p -r1.259 main.c --- main.c 7 Jun 2024 08:22:53 -0000 1.259 +++ main.c 7 Jun 2024 20:46:09 -0000 @@ -618,6 +618,7 @@ entity_process(struct ibuf *b, struct st } cert = cert_read(b); switch (cert->purpose) { + case CERT_PURPOSE_TA: case CERT_PURPOSE_CA: queue_add_from_cert(cert); break; @@ -626,7 +627,7 @@ entity_process(struct ibuf *b, struct st repo_stat_inc(rp, talid, type, STYPE_BGPSEC); break; default: - errx(1, "unexpected cert purpose received"); + errx(1, "unexpected %s", purpose2str(cert->purpose)); break; } cert_free(cert); Index: print.c =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/print.c,v diff -u -p -r1.54 print.c --- print.c 6 Jun 2024 05:57:36 -0000 1.54 +++ print.c 7 Jun 2024 19:40:13 -0000 @@ -65,6 +65,25 @@ nid2str(int nid) return buf; } +const char * +purpose2str(enum cert_purpose purpose) +{ + switch (purpose) { + case CERT_PURPOSE_INVALID: + return "invalid cert"; + case CERT_PURPOSE_TA: + return "TA cert"; + case CERT_PURPOSE_CA: + return "CA cert"; + case CERT_PURPOSE_EE: + return "EE cert"; + case CERT_PURPOSE_BGPSEC_ROUTER: + return "BGPsec Router cert"; + default: + return "unknown cert"; + } +} + char * time2str(time_t t) { Index: x509.c =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v diff -u -p -r1.94 x509.c --- x509.c 7 Jun 2024 08:36:54 -0000 1.94 +++ x509.c 8 Jun 2024 03:22:40 -0000 @@ -133,6 +133,26 @@ x509_init_oid(void) } /* + * A number of critical OpenSSL API functions can't properly indicate failure + * and are unreliable if the extensions aren't already cached. An old trick is + * to cache the extensions using an error-checked call to X509_check_purpose() + * with a purpose of -1. This way functions such as X509_check_ca(), X509_cmp(), + * X509_get_key_usage(), X509_get_extended_key_usage() won't lie. + * + * Should be called right after deserialization and is essentially free to call + * multiple times. + */ +int +x509_cache_extensions(X509 *x509, const char *fn) +{ + if (X509_check_purpose(x509, -1, 0) <= 0) { + warnx("%s: could not cache X509v3 extensions", fn); + return 0; + } + return 1; +} + +/* * Parse X509v3 authority key identifier (AKI), RFC 6487 sec. 4.8.3. * Returns the AKI or NULL if it could not be parsed. * The AKI is formatted as a hex string. @@ -246,18 +266,34 @@ x509_get_ski(X509 *x, const char *fn, ch } /* - * Check the certificate's purpose: CA or BGPsec Router. - * Return a member of enum cert_purpose. + * Check the cert's purpose: the cA bit in basic constraints distinguishes + * between TA/CA and EE/BGPsec router. TAs are self-signed, CAs not self-issued, + * EEs have no extended key usage, BGPsec router have id-kp-bgpsec-router OID. */ enum cert_purpose x509_get_purpose(X509 *x, const char *fn) { BASIC_CONSTRAINTS *bc = NULL; EXTENDED_KEY_USAGE *eku = NULL; - int crit; + int crit, ext_flags, is_ca; enum cert_purpose purpose = CERT_PURPOSE_INVALID; - if (X509_check_ca(x) == 1) { + if (!x509_cache_extensions(x, fn)) + goto out; + + ext_flags = X509_get_extension_flags(x); + + /* This weird API can return 0, 1, 2, 4, 5 but can't error... */ + if ((is_ca = X509_check_ca(x)) > 1) { + if (is_ca == 4) + warnx("%s: RFC 6487: sections 4.8.1 and 4.8.4: " + "no basic constraints, but keyCertSign set", fn); + else + warnx("%s: unexpected legacy certificate", fn); + goto out; + } + + if (is_ca) { bc = X509_get_ext_d2i(x, NID_basic_constraints, &crit, NULL); if (bc == NULL) { if (crit != -1) @@ -278,28 +314,50 @@ x509_get_purpose(X509 *x, const char *fn "Constraint must be absent", fn); goto out; } - purpose = CERT_PURPOSE_CA; - /* XXX - we may want to check EXFLAG_SI and add a TA purpose. */ + /* + * EXFLAG_SI means that issuer and subject are identical. + * EXFLAG_SS is SI plus the AKI is absent or matches the SKI. + * Thus, exactly the trust anchors should have EXFLAG_SS set + * and we should never see EXFLAG_SI without EXFLAG_SS. + */ + if ((ext_flags & EXFLAG_SS) != 0) + purpose = CERT_PURPOSE_TA; + else if ((ext_flags & EXFLAG_SI) == 0) + purpose = CERT_PURPOSE_CA; + else + warnx("%s: RFC 6487, sections 4.8.3: " + "self-issued cert with AKI-SKI mismatch", fn); goto out; } - if (X509_get_extension_flags(x) & EXFLAG_BCONS) { + if ((ext_flags & EXFLAG_BCONS) != 0) { warnx("%s: Basic Constraints ext in non-CA cert", fn); goto out; } + /* + * EKU is only defined for BGPsec Router certs and must be absent from + * EE certs. + */ eku = X509_get_ext_d2i(x, NID_ext_key_usage, &crit, NULL); if (eku == NULL) { if (crit != -1) warnx("%s: error parsing EKU", fn); else - warnx("%s: EKU: extension missing", fn); + purpose = CERT_PURPOSE_EE; /* EKU absent */ goto out; } if (crit != 0) { warnx("%s: EKU: extension must not be marked critical", fn); goto out; } + + /* + * XXX - this isn't quite correct: other EKU OIDs are allowed per + * RFC 8209, section 3.1.3.2, e.g., anyEKU could potentially help + * avoid tripping up validators that don't know about the BGPsec + * router purpose. Drop check or downgrade from error to warning? + */ if (sk_ASN1_OBJECT_num(eku) != 1) { warnx("%s: EKU: expected 1 purpose, have %d", fn, sk_ASN1_OBJECT_num(eku));