From: Theo Buehler <tb@theobuehler.org>
Subject: rpki-client: move key usage to x509_get_purpose()
To: tech@openbsd.org
Date: Mon, 10 Jun 2024 05:33:19 +0200

x509_get_purpose() handles basic constraints and extended key usage.
Key usage also belongs there. By moving it, we now also check it for
BGPsec Router certs.

Add missing check that the extension is critical. Criticality is only a
SHOULD in RFC 5280, so libcrypto doesn't enforce it and of course makes
it annoying to check. It's a MUST in 6487, 4.8.4.

EKU for EE certs is handled in x509_get_purpose(), so zap that check in
cert_parse_ee_cert() and move the one for CA/TA in cert_parse_pre()
without the incorrect 'may be allowed' comment since that applies only
to EE certs.

Index: cert.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
diff -u -p -r1.144 cert.c
--- cert.c	8 Jun 2024 13:33:49 -0000	1.144
+++ cert.c	10 Jun 2024 02:46:53 -0000
@@ -753,18 +753,6 @@ cert_parse_ee_cert(const char *fn, int t
 		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))
@@ -976,19 +964,6 @@ cert_parse_pre(const char *fn, const uns
 		}
 		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);
Index: x509.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v
diff -u -p -r1.97 x509.c
--- x509.c	8 Jun 2024 13:32:30 -0000	1.97
+++ x509.c	10 Jun 2024 03:12:36 -0000
@@ -267,15 +267,17 @@ 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. 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.
@@ -332,6 +361,12 @@ x509_get_purpose(X509 *x, const char *fn
 
 	if ((ext_flags & EXFLAG_BCONS) != 0) {
 		warnx("%s: Basic Constraints ext in non-CA cert", 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;
 	}