From: tb <tb@openbsd.org>
Date: Mon, 10 Jun 2024 10:50:13 +0000 (+0000)
Subject: rpki-client: fix and move more KU/EKU to x509_get_purpose()
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=36d6639c6eec0fb07a314b47dc572bf5a3144bbc;p=openbsd

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 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 <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 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 <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.