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

diff --git a/usr.sbin/rpki-client/cert.c b/usr.sbin/rpki-client/cert.c
index ba29b7e..6c8f7a2 100644 (file)
--- 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 <tb@openbsd.org>
  * Copyright (c) 2021 Job Snijders <job@openbsd.org>
@@ -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 c264b73..0b29ea4 100644 (file)
--- 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 <tb@openbsd.org>
  * Copyright (c) 2021 Claudio Jeker <claudio@openbsd.org>
@@ -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.