From: Theo Buehler Subject: Re: rpki-client: add self-issuance check for EE certs To: tech@openbsd.org Date: Thu, 19 Jun 2025 10:20:48 +0200 On Thu, Jun 19, 2025 at 07:54:55AM +0200, Claudio Jeker wrote: > On Thu, Jun 19, 2025 at 07:49:24AM +0200, Theo Buehler wrote: > > Next simple step of reworking the extension handling and in particular > > making checks for EE certs stricter. > > > > Tangentially, we never agreed on a better name for x509_get_purpose(). > > Since it does a decent amount of checking, x509_check_purpose() would > > perhaps be better. This clashes with the related X509_check_purpose() > > from libcrypto, which I'm sure will confuse me down the road. So I think > > I want to move that function to cert.c, make it static and call it > > cert_check_purpose(). > > OK claudio@, also for the plan to move the function. That would be this diff for moving, renaming and making it static. I switched the argument order since that matches what most other functions do. Nothing else changed. I think putting the cert first for various x509_* functions is an artefact from originally having some X509 **x509 arguments first. That's cleanup for another day. The extern stuff also isn't ideal but that's for another turd polishing session on another day, too. Index: cert.c =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v diff -u -p -r1.162 cert.c --- cert.c 19 Jun 2025 06:47:57 -0000 1.162 +++ cert.c 19 Jun 2025 08:08:37 -0000 @@ -30,6 +30,7 @@ #include "extern.h" +extern ASN1_OBJECT *bgpsec_oid; /* id-kp-bgpsec-router Key Purpose */ extern ASN1_OBJECT *certpol_oid; /* id-cp-ipAddr-asNumber cert policy */ extern ASN1_OBJECT *carepo_oid; /* 1.3.6.1.5.5.7.48.5 (caRepository) */ extern ASN1_OBJECT *manifest_oid; /* 1.3.6.1.5.5.7.48.10 (rpkiManifest) */ @@ -737,6 +738,151 @@ cert_check_subject_and_issuer(const char } /* + * Check the cert's purpose: the cA bit in basic constraints distinguishes + * between TA/CA and EE/BGPsec router and the key usage bits must match. + * TAs are self-signed, CAs not self-issued, EEs have no extended key usage, + * BGPsec router have id-kp-bgpsec-router OID. + */ +static enum cert_purpose +cert_check_purpose(const char *fn, X509 *x) +{ + BASIC_CONSTRAINTS *bc = NULL; + EXTENDED_KEY_USAGE *eku = NULL; + const X509_EXTENSION *ku; + int crit, ext_flags, i, is_ca, ku_idx; + enum cert_purpose purpose = CERT_PURPOSE_INVALID; + + if (!x509_cache_extensions(x, fn)) + goto out; + + ext_flags = X509_get_extension_flags(x); + + /* Key usage must be present and critical. KU bits are checked below. */ + if ((ku_idx = X509_get_ext_by_NID(x, NID_key_usage, -1)) < 0) { + warnx("%s: RFC 6487, section 4.8.4: missing KeyUsage", fn); + goto out; + } + if ((ku = X509_get_ext(x, ku_idx)) == NULL) { + warnx("%s: RFC 6487, section 4.8.4: missing KeyUsage", fn); + goto out; + } + if (!X509_EXTENSION_get_critical(ku)) { + warnx("%s: RFC 6487, section 4.8.4: KeyUsage not critical", fn); + goto out; + } + + /* 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) + warnx("%s: RFC 6487 section 4.8.1: " + "error parsing basic constraints", fn); + else + warnx("%s: RFC 6487 section 4.8.1: " + "missing basic constraints", fn); + goto out; + } + if (crit != 1) { + warnx("%s: RFC 6487 section 4.8.1: Basic Constraints " + "must be marked critical", fn); + goto out; + } + if (bc->pathlen != NULL) { + warnx("%s: RFC 6487 section 4.8.1: Path Length " + "Constraint must be absent", fn); + goto out; + } + + if (X509_get_key_usage(x) != (KU_KEY_CERT_SIGN | KU_CRL_SIGN)) { + warnx("%s: RFC 6487 section 4.8.4: key usage violation", + fn); + goto out; + } + + if (X509_get_extended_key_usage(x) != UINT32_MAX) { + warnx("%s: RFC 6487 section 4.8.5: EKU not allowed", + fn); + goto out; + } + + /* + * 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, section 4.8.3: " + "self-issued cert with AKI-SKI mismatch", fn); + goto out; + } + + if ((ext_flags & EXFLAG_BCONS) != 0) { + warnx("%s: Basic Constraints ext in non-CA cert", fn); + goto out; + } + + if ((ext_flags & (EXFLAG_SI | EXFLAG_SS)) != 0) { + warnx("%s: EE cert must not be self-issued or self-signed", fn); + goto out; + } + + if (X509_get_key_usage(x) != KU_DIGITAL_SIGNATURE) { + warnx("%s: RFC 6487 section 4.8.4: KU must be digitalSignature", + 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 + purpose = CERT_PURPOSE_EE; /* EKU absent */ + goto out; + } + if (crit != 0) { + warnx("%s: EKU: extension must not be marked critical", fn); + goto out; + } + + /* + * Per RFC 8209, section 3.1.3.2 the id-kp-bgpsec-router OID must be + * present and others are allowed, which we don't need to recognize. + * This matches RFC 5280, section 4.2.1.12. + */ + for (i = 0; i < sk_ASN1_OBJECT_num(eku); i++) { + if (OBJ_cmp(bgpsec_oid, sk_ASN1_OBJECT_value(eku, i)) == 0) { + purpose = CERT_PURPOSE_BGPSEC_ROUTER; + break; + } + } + + out: + BASIC_CONSTRAINTS_free(bc); + EXTENDED_KEY_USAGE_free(eku); + return purpose; +} + +/* * Lightweight version of cert_parse_pre() for EE certs. * Parses the two RFC 3779 extensions, and performs some sanity checks. * Returns cert on success and NULL on failure. @@ -766,7 +912,7 @@ cert_parse_ee_cert(const char *fn, int t * Check issuance, basic constraints and (extended) key usage bits are * appropriate for an EE cert. Covers RFC 6487, 4.8.1, 4.8.4, 4.8.5. */ - if ((cert->purpose = x509_get_purpose(x, fn)) != CERT_PURPOSE_EE) { + if ((cert->purpose = cert_check_purpose(fn, x)) != CERT_PURPOSE_EE) { warnx("%s: expected EE cert, got %s", fn, purpose2str(cert->purpose)); goto out; @@ -968,7 +1114,7 @@ cert_parse_pre(const char *fn, const uns goto out; /* Validation on required fields. */ - cert->purpose = x509_get_purpose(x, fn); + cert->purpose = cert_check_purpose(fn, x); switch (cert->purpose) { case CERT_PURPOSE_TA: /* XXX - caller should indicate if it expects TA or CA cert */ Index: extern.h =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v diff -u -p -r1.240 extern.h --- extern.h 4 Jun 2025 09:18:28 -0000 1.240 +++ extern.h 19 Jun 2025 08:08:37 -0000 @@ -952,7 +952,6 @@ int x509_get_notafter(X509 *, const ch int x509_get_crl(X509 *, const char *, 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 *); int x509_get_time(const ASN1_TIME *, time_t *); char *x509_convert_seqnum(const char *, const char *, const ASN1_INTEGER *); Index: x509.c =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v diff -u -p -r1.106 x509.c --- x509.c 19 Jun 2025 06:46:56 -0000 1.106 +++ x509.c 19 Jun 2025 08:08:37 -0000 @@ -266,151 +266,6 @@ x509_get_ski(X509 *x, const char *fn, ch } /* - * Check the cert's purpose: the cA bit in basic constraints distinguishes - * between TA/CA and EE/BGPsec router and the key usage bits must match. - * 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; - const X509_EXTENSION *ku; - int crit, ext_flags, i, is_ca, ku_idx; - enum cert_purpose purpose = CERT_PURPOSE_INVALID; - - if (!x509_cache_extensions(x, fn)) - goto out; - - ext_flags = X509_get_extension_flags(x); - - /* Key usage must be present and critical. KU bits are checked below. */ - if ((ku_idx = X509_get_ext_by_NID(x, NID_key_usage, -1)) < 0) { - warnx("%s: RFC 6487, section 4.8.4: missing KeyUsage", fn); - goto out; - } - if ((ku = X509_get_ext(x, ku_idx)) == NULL) { - warnx("%s: RFC 6487, section 4.8.4: missing KeyUsage", fn); - goto out; - } - if (!X509_EXTENSION_get_critical(ku)) { - warnx("%s: RFC 6487, section 4.8.4: KeyUsage not critical", fn); - goto out; - } - - /* 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) - warnx("%s: RFC 6487 section 4.8.1: " - "error parsing basic constraints", fn); - else - warnx("%s: RFC 6487 section 4.8.1: " - "missing basic constraints", fn); - goto out; - } - if (crit != 1) { - warnx("%s: RFC 6487 section 4.8.1: Basic Constraints " - "must be marked critical", fn); - goto out; - } - if (bc->pathlen != NULL) { - warnx("%s: RFC 6487 section 4.8.1: Path Length " - "Constraint must be absent", fn); - goto out; - } - - if (X509_get_key_usage(x) != (KU_KEY_CERT_SIGN | KU_CRL_SIGN)) { - warnx("%s: RFC 6487 section 4.8.4: key usage violation", - fn); - goto out; - } - - if (X509_get_extended_key_usage(x) != UINT32_MAX) { - warnx("%s: RFC 6487 section 4.8.5: EKU not allowed", - fn); - goto out; - } - - /* - * 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, section 4.8.3: " - "self-issued cert with AKI-SKI mismatch", fn); - goto out; - } - - if ((ext_flags & EXFLAG_BCONS) != 0) { - warnx("%s: Basic Constraints ext in non-CA cert", fn); - goto out; - } - - if ((ext_flags & (EXFLAG_SI | EXFLAG_SS)) != 0) { - warnx("%s: EE cert must not be self-issued or self-signed", fn); - goto out; - } - - if (X509_get_key_usage(x) != KU_DIGITAL_SIGNATURE) { - warnx("%s: RFC 6487 section 4.8.4: KU must be digitalSignature", - 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 - purpose = CERT_PURPOSE_EE; /* EKU absent */ - goto out; - } - if (crit != 0) { - warnx("%s: EKU: extension must not be marked critical", fn); - goto out; - } - - /* - * Per RFC 8209, section 3.1.3.2 the id-kp-bgpsec-router OID must be - * present and others are allowed, which we don't need to recognize. - * This matches RFC 5280, section 4.2.1.12. - */ - for (i = 0; i < sk_ASN1_OBJECT_num(eku); i++) { - if (OBJ_cmp(bgpsec_oid, sk_ASN1_OBJECT_value(eku, i)) == 0) { - purpose = CERT_PURPOSE_BGPSEC_ROUTER; - break; - } - } - - out: - BASIC_CONSTRAINTS_free(bc); - EXTENDED_KEY_USAGE_free(eku); - return purpose; -} - -/* * Extract Subject Public Key Info (SPKI) from BGPsec X.509 Certificate. * Returns NULL on failure, on success return the SPKI as base64 encoded pubkey */