From: Tobias Brunner <tobias@strongswan.org>
Date: Fri, 12 May 2023 16:36:30 +0000 (+0200)
Subject: credential-manager: Improve selection of local certificate and trust chain
X-Git-Tag: 5.9.11rc1~6
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=efdcbd13cb65528a91867118521595ac22623654;p=thirdparty%2Fstrongswan.git

credential-manager: Improve selection of local certificate and trust chain

The previous code was problematic if a certificate request for a known
but unrelated CA was received and the local trust chain was incomplete.
Due to the received anchor, the incomplete trust chain was dismissed and
any intermediate CA certificates were, therefore, not sent to the peer.

This new approach doesn't dismiss an incomplete trust chain, but prefers
one that can be resolved to a received anchor.  If no such chain is found,
the first one is used.
---

diff --git a/src/libstrongswan/credentials/credential_manager.c b/src/libstrongswan/credentials/credential_manager.c
index cf26889160..6f030ef2a5 100644
--- a/src/libstrongswan/credentials/credential_manager.c
+++ b/src/libstrongswan/credentials/credential_manager.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015-2022 Tobias Brunner
+ * Copyright (C) 2015-2023 Tobias Brunner
  * Copyright (C) 2007 Martin Willi
  *
  * Copyright (C) secunet Security Networks AG
@@ -1114,61 +1114,55 @@ static bool auth_contains_cacert(auth_cfg_t *auth, certificate_t *cert)
 }
 
 /**
- * build a trustchain from subject up to a trust anchor in trusted
+ * Build a trust chain for subject, optionally only up to one of the CA
+ * certificates in auth. Returns whether one of the anchors was found.
  */
 static auth_cfg_t *build_trustchain(private_credential_manager_t *this,
-									 certificate_t *subject, auth_cfg_t *auth)
+									certificate_t *subject, auth_cfg_t *auth,
+									bool *found_anchor)
 {
 	certificate_t *issuer, *current;
 	auth_cfg_t *trustchain;
 	int pathlen = 0;
-	bool has_anchor;
+
+	*found_anchor = FALSE;
 
 	trustchain = auth_cfg_create();
-	has_anchor = auth->get(auth, AUTH_RULE_CA_CERT) != NULL;
+	/* immediately return for self-signed certificates */
+	if (issued_by(this, subject, subject, NULL))
+	{
+		return trustchain;
+	}
 	current = subject->get_ref(subject);
-	while (TRUE)
+	for (pathlen = 0; pathlen <= MAX_TRUST_PATH_LEN; pathlen++)
 	{
-		if (auth_contains_cacert(auth, current))
-		{
-			trustchain->add(trustchain, AUTH_RULE_CA_CERT, current);
-			return trustchain;
-		}
-		if (subject == current)
-		{
-			trustchain->add(trustchain, AUTH_RULE_SUBJECT_CERT, current);
-		}
-		else
-		{
-			if (!has_anchor && issued_by(this, current, current, NULL))
-			{	/* If no trust anchor specified, accept any CA */
-				trustchain->add(trustchain, AUTH_RULE_CA_CERT, current);
-				return trustchain;
-			}
-			trustchain->add(trustchain, AUTH_RULE_IM_CERT, current);
-		}
-		if (pathlen++ > MAX_TRUST_PATH_LEN)
-		{
-			break;
-		}
 		issuer = get_issuer_cert(this, current, FALSE, NULL);
 		if (!issuer)
-		{
-			if (!has_anchor)
-			{	/* If no trust anchor specified, accept incomplete chains */
-				return trustchain;
-			}
+		{	/* return the incomplete trust chain */
 			break;
 		}
-		if (has_anchor && issuer->equals(issuer, current))
-		{
-			issuer->destroy(issuer);
+		if (auth_contains_cacert(auth, issuer))
+		{	/* stop if we find one of the anchors */
+			trustchain->add(trustchain, AUTH_RULE_CA_CERT, issuer);
+			*found_anchor = TRUE;
 			break;
 		}
-		current = issuer;
+		if (issued_by(this, issuer, issuer, NULL))
+		{	/* trust chain is complete */
+			trustchain->add(trustchain, AUTH_RULE_CA_CERT, issuer);
+			break;
+		}
+		trustchain->add(trustchain, AUTH_RULE_IM_CERT, issuer);
+		current->destroy(current);
+		current = issuer->get_ref(issuer);
 	}
-	trustchain->destroy(trustchain);
-	return NULL;
+	current->destroy(current);
+	if (pathlen > MAX_TRUST_PATH_LEN)
+	{
+		trustchain->destroy(trustchain);
+		return NULL;
+	}
+	return trustchain;
 }
 
 /**
@@ -1225,9 +1219,10 @@ METHOD(credential_manager_t, get_private, private_key_t*,
 {
 	enumerator_t *enumerator;
 	certificate_t *cert;
-	private_key_t *private = NULL;
-	auth_cfg_t *trustchain;
+	private_key_t *private = NULL, *first_private = NULL;
+	auth_cfg_t *trustchain, *first_trustchain = NULL;
 	auth_rule_t rule;
+	bool has_anchor, found_anchor;
 
 	/* check if this is a lookup by key ID, and do it if so */
 	if (id && id->get_type(id) == ID_KEY_ID)
@@ -1241,7 +1236,10 @@ METHOD(credential_manager_t, get_private, private_key_t*,
 
 	if (auth)
 	{
-		/* try to find a trustchain with one of the configured subject certs */
+		has_anchor = auth->get(auth, AUTH_RULE_CA_CERT) != NULL;
+
+		/* try to find a trust chain with one of the configured subject certs,
+		 * prefer one with any given anchor */
 		enumerator = auth->create_enumerator(auth);
 		while (enumerator->enumerate(enumerator, &rule, &cert))
 		{
@@ -1250,13 +1248,24 @@ METHOD(credential_manager_t, get_private, private_key_t*,
 				private = get_private_by_cert(this, cert, type);
 				if (private)
 				{
-					trustchain = build_trustchain(this, cert, auth);
+					trustchain = build_trustchain(this, cert, auth, &found_anchor);
 					if (trustchain)
 					{
-						auth->merge(auth, trustchain, FALSE);
-						prefer_cert(auth, cert->get_ref(cert));
+						if (!has_anchor || found_anchor)
+						{
+							auth->merge(auth, trustchain, FALSE);
+							prefer_cert(auth, cert->get_ref(cert));
+							trustchain->destroy(trustchain);
+							break;
+						}
+						else if (!first_private)
+						{
+							first_private = private;
+							first_trustchain = trustchain;
+							private = NULL;
+							continue;
+						}
 						trustchain->destroy(trustchain);
-						break;
 					}
 					private->destroy(private);
 					private = NULL;
@@ -1264,62 +1273,66 @@ METHOD(credential_manager_t, get_private, private_key_t*,
 			}
 		}
 		enumerator->destroy(enumerator);
-		if (private)
-		{
-			return private;
-		}
 
-		/* if none yielded a trustchain, enforce the first configured cert */
-		cert = auth->get(auth, AUTH_RULE_SUBJECT_CERT);
-		if (cert)
+		/* if no certificates are configured, try to find one based on the
+		 * identity, preferably with any of the given anchors */
+		if (!private && !first_private)
 		{
-			private = get_private_by_cert(this, cert, type);
-			if (private)
+			enumerator = create_cert_enumerator(this, CERT_ANY, type, id, FALSE);
+			while (enumerator->enumerate(enumerator, &cert))
 			{
-				trustchain = build_trustchain(this, cert, auth);
-				if (trustchain)
+				private = get_private_by_cert(this, cert, type);
+				if (private)
 				{
-					auth->merge(auth, trustchain, FALSE);
-					trustchain->destroy(trustchain);
+					trustchain = build_trustchain(this, cert, auth, &found_anchor);
+					if (trustchain)
+					{
+						if (!has_anchor || found_anchor)
+						{
+							auth->merge(auth, trustchain, FALSE);
+							prefer_cert(auth, cert->get_ref(cert));
+							trustchain->destroy(trustchain);
+							break;
+						}
+						else if (!first_private)
+						{
+							/* add this certificate, if we end up choosing a
+							 * different one, it gets replaced above */
+							auth->add(auth, AUTH_RULE_SUBJECT_CERT,
+									  cert->get_ref(cert));
+							first_private = private;
+							first_trustchain = trustchain;
+							private = NULL;
+							continue;
+						}
+						trustchain->destroy(trustchain);
+					}
+					private->destroy(private);
+					private = NULL;
 				}
-				return private;
 			}
+			enumerator->destroy(enumerator);
 		}
 
-		/* try to build a trust chain for each certificate found */
-		enumerator = create_cert_enumerator(this, CERT_ANY, type, id, FALSE);
-		while (enumerator->enumerate(enumerator, &cert))
+		/* fall back to the first configured or found private key */
+		if (!private && first_private)
 		{
-			private = get_private_by_cert(this, cert, type);
-			if (private)
-			{
-				trustchain = build_trustchain(this, cert, auth);
-				if (trustchain)
-				{
-					auth->merge(auth, trustchain, FALSE);
-					trustchain->destroy(trustchain);
-					break;
-				}
-				private->destroy(private);
-				private = NULL;
-			}
+			auth->merge(auth, first_trustchain, FALSE);
+			private = first_private->get_ref(first_private);
 		}
-		enumerator->destroy(enumerator);
+		DESTROY_IF(first_private);
+		DESTROY_IF(first_trustchain);
 	}
-
-	/* if no valid trustchain was found, fall back to the first usable cert */
-	if (!private)
+	else
 	{
+		/* if we have no config, use the first usable cert with the given
+		 * identity */
 		enumerator = create_cert_enumerator(this, CERT_ANY, type, id, FALSE);
 		while (enumerator->enumerate(enumerator, &cert))
 		{
 			private = get_private_by_cert(this, cert, type);
 			if (private)
 			{
-				if (auth)
-				{
-					auth->add(auth, AUTH_RULE_SUBJECT_CERT, cert->get_ref(cert));
-				}
 				break;
 			}
 		}