From 36d6639c6eec0fb07a314b47dc572bf5a3144bbc Mon Sep 17 00:00:00 2001 From: tb Date: Mon, 10 Jun 2024 10:50:13 +0000 Subject: [PATCH] rpki-client: fix and move more KU/EKU to x509_get_purpose() Now all key usage and extended key usage handling is at the same place. This fixes a bug for BGPsec Router certs where key usage was ignored. Another omission that is fixed here is that criticality of the key usage extension was not checked. Drop a comment about possible use of EKU that was in the TA/CA code path but would only apply to EE certs. ok claudio --- usr.sbin/rpki-client/cert.c | 27 +---------------------- usr.sbin/rpki-client/x509.c | 43 +++++++++++++++++++++++++++++++++---- 2 files changed, 40 insertions(+), 30 deletions(-) diff --git a/usr.sbin/rpki-client/cert.c b/usr.sbin/rpki-client/cert.c index ba29b7e48ea..6c8f7a2493b 100644 --- a/usr.sbin/rpki-client/cert.c +++ b/usr.sbin/rpki-client/cert.c @@ -1,4 +1,4 @@ -/* $OpenBSD: cert.c,v 1.144 2024/06/08 13:33:49 tb Exp $ */ +/* $OpenBSD: cert.c,v 1.145 2024/06/10 10:50:13 tb Exp $ */ /* * Copyright (c) 2022 Theo Buehler * Copyright (c) 2021 Job Snijders @@ -753,18 +753,6 @@ cert_parse_ee_cert(const char *fn, int talid, X509 *x) 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 may be allowed for some purposes in the future. */ - if (X509_get_extended_key_usage(x) != UINT32_MAX) { - warnx("%s: RFC 6487 section 4.8.5: EKU not allowed", fn); - goto out; - } - index = X509_get_ext_by_NID(x, NID_sbgp_ipAddrBlock, -1); if ((ext = X509_get_ext(x, index)) != NULL) { if (!sbgp_ipaddrblk(fn, cert, ext)) @@ -977,19 +965,6 @@ cert_parse_pre(const char *fn, const unsigned char *der, size_t len) if (!valid_ca_pkey(fn, pkey)) 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; - } - - /* EKU may be allowed for some purposes in the future. */ - if (X509_get_extended_key_usage(x) != UINT32_MAX) { - warnx("%s: RFC 6487 section 4.8.5: EKU not allowed", - fn); - goto out; - } - if (cert->mft == NULL) { warnx("%s: RFC 6487 section 4.8.8: missing SIA", fn); goto out; diff --git a/usr.sbin/rpki-client/x509.c b/usr.sbin/rpki-client/x509.c index c264b7327d3..0b29ea4f8bb 100644 --- a/usr.sbin/rpki-client/x509.c +++ b/usr.sbin/rpki-client/x509.c @@ -1,4 +1,4 @@ -/* $OpenBSD: x509.c,v 1.97 2024/06/08 13:32:30 tb Exp $ */ +/* $OpenBSD: x509.c,v 1.98 2024/06/10 10:50:13 tb Exp $ */ /* * Copyright (c) 2022 Theo Buehler * Copyright (c) 2021 Claudio Jeker @@ -267,15 +267,17 @@ x509_get_ski(X509 *x, const char *fn, char **ski) /* * 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. + * 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; - int crit, ext_flags, is_ca; + const X509_EXTENSION *ku; + int crit, ext_flags, is_ca, ku_idx; enum cert_purpose purpose = CERT_PURPOSE_INVALID; if (!x509_cache_extensions(x, fn)) @@ -283,6 +285,20 @@ x509_get_purpose(X509 *x, const char *fn) 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) @@ -314,6 +330,19 @@ x509_get_purpose(X509 *x, const char *fn) "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. @@ -335,6 +364,12 @@ x509_get_purpose(X509 *x, const char *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. -- 2.20.1