From: Claudio Jeker Subject: Re: rpki-client: add self-issuance check for EE certs To: Theo Buehler Cc: tech@openbsd.org Date: Thu, 19 Jun 2025 10:57:47 +0200 On Thu, Jun 19, 2025 at 10:20:48AM +0200, Theo Buehler wrote: > 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. OK claudio@ > 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 > */ > -- :wq Claudio