Add optional 'group none' transform for child SAs and fix handling of

'group none'.   We currently send no transform of type DH by default,
which should be equivalent to explicitly sending a single DH transform
of type 'none'.  However, the proposal matching logic had a bug where
these two would not match, effectively breaking the ability to negotiate
optional PFS.  This commit fixes the bug but continues to send
no DH proposal by default to remain backwards compatible with older
versions.

ok patrick@

3 files changed, 39 insertions, 8 deletions

diff --git a/sbin/iked/ikev2.c b/sbin/iked/ikev2.c
index 2ea7e7b23ac..2f37cd8edf3 100644
--- a/sbin/iked/ikev2.c
+++ b/sbin/iked/ikev2.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ikev2.c,v 1.303 2021/02/04 20:38:26 tobhe Exp $	*/
+/*	$OpenBSD: ikev2.c,v 1.304 2021/02/09 21:35:48 tobhe Exp $	*/
 
 /*
  * Copyright (c) 2019 Tobias Heider <tobias.heider@stusta.de>
@@ -2435,12 +2435,15 @@ ikev2_add_proposals(struct iked *env, struct iked_sa *sa, struct ibuf *buf,
 	uint64_t			 spi64;
 	uint32_t			 spi32, spi = 0;
 	unsigned int			 i, xfi, nxforms;
+	int				 prop_skipdh;
 
 	TAILQ_FOREACH(prop, proposals, prop_entry) {
 		if ((protoid && prop->prop_protoid != protoid) ||
 		    (!protoid && prop->prop_protoid == IKEV2_SAPROTO_IKE))
 			continue;
 
+		prop_skipdh = skipdh;
+
 		if (protoid != IKEV2_SAPROTO_IKE && initiator) {
 			if (spi == 0) {
 				bzero(&csa, sizeof(csa));
@@ -2472,11 +2475,27 @@ ikev2_add_proposals(struct iked *env, struct iked_sa *sa, struct ibuf *buf,
 		}
 
 		/*
+		 * A single DH transform of type NONE is equivalent with
+		 * not sending a DH transform at all.
+		 * Prefer the latter for downwards compatibility.
+		 */
+		if (protoid != IKEV2_SAPROTO_IKE) {
+			for (i = 0; i < prop->prop_nxforms; i++) {
+				xform = prop->prop_xforms + i;
+				if (xform->xform_type == IKEV2_XFORMTYPE_DH &&
+				    xform->xform_id != IKEV2_XFORMDH_NONE)
+					break;
+			}
+			if (i == prop->prop_nxforms)
+				prop_skipdh = 1;
+		}
+
+		/*
 		 * RFC 7296: 1.2. The Initial Exchanges
 		 * IKE_AUTH messages do not contain KE/N payloads, thus
 		 * SA payloads cannot contain groups.
 		 */
-		if (skipdh) {
+		if (prop_skipdh) {
 			nxforms = 0;
 			for (i = 0; i < prop->prop_nxforms; i++) {
 				xform = prop->prop_xforms + i;
@@ -2514,7 +2533,7 @@ ikev2_add_proposals(struct iked *env, struct iked_sa *sa, struct ibuf *buf,
 		for (i = 0, xfi = 0; i < prop->prop_nxforms; i++) {
 			xform = prop->prop_xforms + i;
 
-			if (skipdh && xform->xform_type == IKEV2_XFORMTYPE_DH)
+			if (prop_skipdh && xform->xform_type == IKEV2_XFORMTYPE_DH)
 				continue;
 
 			if ((xflen = ikev2_add_transform(buf,
@@ -3826,6 +3845,7 @@ ikev2_send_create_child_sa(struct iked *env, struct iked_sa *sa,
 {
 	struct iked_policy		*pol = sa->sa_policy;
 	struct iked_childsa		*csa = NULL, *csb = NULL;
+	struct iked_transform		*xform;
 	struct ikev2_notify		*n;
 	struct ikev2_payload		*pld = NULL;
 	struct ikev2_keyexchange	*ke;
@@ -3918,8 +3938,8 @@ ikev2_send_create_child_sa(struct iked *env, struct iked_sa *sa,
 		goto done;
 	len = ibuf_size(nonce);
 
-	if (config_findtransform(&pol->pol_proposals, IKEV2_XFORMTYPE_DH,
-	    protoid)) {
+	if ((xform = config_findtransform(&pol->pol_proposals, IKEV2_XFORMTYPE_DH,
+	    protoid)) && group_get(xform->xform_id) != IKEV2_XFORMDH_NONE) {
 		log_debug("%s: enable PFS", __func__);
 		ikev2_sa_cleanup_dh(sa);
 		if (proposed_group) {
diff --git a/sbin/iked/parse.y b/sbin/iked/parse.y
index ff6fa16f094..e9a57c97667 100644
--- a/sbin/iked/parse.y
+++ b/sbin/iked/parse.y
@@ -1,4 +1,4 @@
-/*	$OpenBSD: parse.y,v 1.126 2021/02/07 00:51:53 tobhe Exp $	*/
+/*	$OpenBSD: parse.y,v 1.127 2021/02/09 21:35:48 tobhe Exp $	*/
 
 /*
  * Copyright (c) 2019 Tobias Heider <tobias.heider@stusta.de>
@@ -192,6 +192,7 @@ struct iked_transform ikev2_default_esp_transforms[] = {
 	{ IKEV2_XFORMTYPE_INTEGR, IKEV2_XFORMAUTH_HMAC_SHA2_384_192 },
 	{ IKEV2_XFORMTYPE_INTEGR, IKEV2_XFORMAUTH_HMAC_SHA2_512_256 },
 	{ IKEV2_XFORMTYPE_INTEGR, IKEV2_XFORMAUTH_HMAC_SHA1_96 },
+	{ IKEV2_XFORMTYPE_DH,	IKEV2_XFORMDH_NONE },
 	{ IKEV2_XFORMTYPE_ESN,	IKEV2_XFORMESN_ESN },
 	{ IKEV2_XFORMTYPE_ESN,	IKEV2_XFORMESN_NONE },
 	{ 0 }
@@ -202,6 +203,7 @@ size_t ikev2_default_nesp_transforms = ((sizeof(ikev2_default_esp_transforms) /
 struct iked_transform ikev2_default_esp_transforms_noauth[] = {
 	{ IKEV2_XFORMTYPE_ENCR,	IKEV2_XFORMENCR_AES_GCM_16, 128 },
 	{ IKEV2_XFORMTYPE_ENCR,	IKEV2_XFORMENCR_AES_GCM_16, 256 },
+	{ IKEV2_XFORMTYPE_DH,	IKEV2_XFORMDH_NONE },
 	{ IKEV2_XFORMTYPE_ESN,	IKEV2_XFORMESN_ESN },
 	{ IKEV2_XFORMTYPE_ESN,	IKEV2_XFORMESN_NONE },
 	{ 0 }
@@ -267,6 +269,7 @@ const struct ipsec_xf ipsecencxfs[] = {
 };
 
 const struct ipsec_xf groupxfs[] = {
+	{ "none",		IKEV2_XFORMDH_NONE },
 	{ "modp768",		IKEV2_XFORMDH_MODP_768 },
 	{ "grp1",		IKEV2_XFORMDH_MODP_768 },
 	{ "modp1024",		IKEV2_XFORMDH_MODP_1024 },
@@ -2840,6 +2843,13 @@ create_ike(char *name, int af, uint8_t ipproto,
 				else
 					auth++;
 			}
+			for (j = 0; j < ike_sa->xfs[i]->ngroupxf; j++) {
+				if (ike_sa->xfs[i]->groupxf[j]->id
+				    == IKEV2_XFORMDH_NONE) {
+					yyerror("IKE group can not be \"none\".");
+					goto done;
+				}
+			}
 			if (ike_sa->xfs[i]->nauthxf)
 				auth++;
 
diff --git a/sbin/iked/policy.c b/sbin/iked/policy.c
index df7f2676dd1..d312980c392 100644
--- a/sbin/iked/policy.c
+++ b/sbin/iked/policy.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: policy.c,v 1.75 2021/02/01 16:37:48 tobhe Exp $	*/
+/*	$OpenBSD: policy.c,v 1.76 2021/02/09 21:35:48 tobhe Exp $	*/
 
 /*
  * Copyright (c) 2010-2013 Reyk Floeter <reyk@openbsd.org>
@@ -1009,7 +1009,8 @@ proposals_match(struct iked_proposal *local, struct iked_proposal *peer,
 			 */
 			if (rekey && requiredh == 0 &&
 			    protoid == IKEV2_SAPROTO_ESP &&
-			    tlocal->xform_type == IKEV2_XFORMTYPE_DH)
+			    tlocal->xform_type == IKEV2_XFORMTYPE_DH &&
+			    tlocal->xform_id != IKEV2_XFORMDH_NONE)
 				requiredh = 1;
 
 			/* Compare peer and local proposals */