Change responder to prefer DH group from KE payload.

Without this change the responder would always prefer the first DH
group configured in its policy. This would lead to invalid KE
messages that cause an additional exchange which old
implementations do not support correctly. Now we ignore the order
of DH groups in the policy and prefer the group from the policy
that matches the KE payload.

from markus@
ok patrick@

iked/iked.h

@@ -1,4 +1,4 @@
-/*	$OpenBSD: iked.h,v 1.193 2021/09/01 15:30:06 tobhe Exp $	*/
+/*	$OpenBSD: iked.h,v 1.194 2021/10/12 10:01:59 tobhe Exp $	*/
 
 /*
  * Copyright (c) 2019-2021 Tobias Heider <[email protected]>
@@ -920,7 +920,7 @@ struct iked_sa *
 	 sa_dstid_insert(struct iked *, struct iked_sa *);
 void	 sa_dstid_remove(struct iked *, struct iked_sa *);
 int	 proposals_negotiate(struct iked_proposals *, struct iked_proposals *,
-	    struct iked_proposals *, int);
+	    struct iked_proposals *, int, int);
 RB_PROTOTYPE(iked_sas, iked_sa, sa_entry, sa_cmp);
 RB_PROTOTYPE(iked_dstid_sas, iked_sa, sa_dstid_entry, sa_dstid_cmp);
 RB_PROTOTYPE(iked_addrpool, iked_sa, sa_addrpool_entry, sa_addrpool_cmp);

iked/ikev2.c

@@ -1,4 +1,4 @@
-/*	$OpenBSD: ikev2.c,v 1.328 2021/10/12 09:27:21 tobhe Exp $	*/
+/*	$OpenBSD: ikev2.c,v 1.329 2021/10/12 10:01:59 tobhe Exp $	*/
 
 /*
  * Copyright (c) 2019 Tobias Heider <[email protected]>
@@ -974,7 +974,7 @@ ikev2_ike_auth_recv(struct iked *env, struct iked_sa *sa,
 	if (!TAILQ_EMPTY(&msg->msg_proposals)) {
 		if (proposals_negotiate(&sa->sa_proposals,
 		    &sa->sa_policy->pol_proposals, &msg->msg_proposals,
-		    0) != 0) {
+		    0, -1) != 0) {
 			log_info("%s: no proposal chosen", __func__);
 			msg->msg_error = IKEV2_N_NO_PROPOSAL_CHOSEN;
 			return (-1);
@@ -4227,7 +4227,7 @@ ikev2_init_create_child_sa(struct iked *env, struct iked_message *msg)
 	}
 
 	if (proposals_negotiate(&sa->sa_proposals, &sa->sa_proposals,
-	    &msg->msg_proposals, 1) != 0) {
+	    &msg->msg_proposals, 1, -1) != 0) {
 		log_info("%s: no proposal chosen", SPI_SA(sa, __func__));
 		return (-1);
 	}
@@ -4730,7 +4730,7 @@ ikev2_resp_create_child_sa(struct iked *env, struct iked_message *msg)
 
 		if (proposals_negotiate(&proposals,
 		    &sa->sa_policy->pol_proposals, &msg->msg_proposals,
-		    1) != 0) {
+		    1, msg->msg_dhgroup) != 0) {
 			log_info("%s: no proposal chosen", __func__);
 			msg->msg_error = IKEV2_N_NO_PROPOSAL_CHOSEN;
 			goto fail;
@@ -5245,7 +5245,7 @@ ikev2_sa_negotiate_common(struct iked *env, struct iked_sa *sa, struct iked_mess
 
 	/* XXX we need a better way to get this */
 	if (proposals_negotiate(&sa->sa_proposals,
-	    &msg->msg_policy->pol_proposals, &msg->msg_proposals, 0) != 0) {
+	    &msg->msg_policy->pol_proposals, &msg->msg_proposals, 0, -1) != 0) {
 		log_info("%s: proposals_negotiate", __func__);
 		return (-1);
 	}

iked/policy.c

@@ -1,4 +1,4 @@
-/*	$OpenBSD: policy.c,v 1.83 2021/09/01 15:30:06 tobhe Exp $	*/
+/*	$OpenBSD: policy.c,v 1.84 2021/10/12 10:01:59 tobhe Exp $	*/
 
 /*
  * Copyright (c) 2020-2021 Tobias Heider <[email protected]>
@@ -53,7 +53,7 @@ static __inline int
 
 static int	policy_test_flows(struct iked_policy *, struct iked_policy *);
 static int	proposals_match(struct iked_proposal *, struct iked_proposal *,
-		    struct iked_transform **, int);
+		    struct iked_transform **, int, int);
 
 void
 policy_init(struct iked *env)
@@ -276,7 +276,7 @@ policy_test(struct iked *env, struct iked_policy *key)
 			/* Make sure the proposals are compatible */
 			if (TAILQ_FIRST(&key->pol_proposals) &&
 			    proposals_negotiate(NULL, &p->pol_proposals,
-			    &key->pol_proposals, 0) == -1) {
+			    &key->pol_proposals, 0, -1) == -1) {
 				p = TAILQ_NEXT(p, pol_entry);
 				continue;
 			}
@@ -973,7 +973,7 @@ user_cmp(struct iked_user *a, struct iked_user *b)
  */
 int
 proposals_negotiate(struct iked_proposals *result, struct iked_proposals *local,
-    struct iked_proposals *peer, int rekey)
+    struct iked_proposals *peer, int rekey, int groupid)
 {
 	struct iked_proposal	*ppeer = NULL, *plocal, *prop, vpeer, vlocal;
 	struct iked_transform	 chosen[IKEV2_XFORMTYPE_MAX];
@@ -998,7 +998,7 @@ proposals_negotiate(struct iked_proposals *result, struct iked_proposals *local,
 				continue;
 			bzero(match, sizeof(match));
 			score = proposals_match(plocal, ppeer, match,
-			    rekey);
+			    rekey, groupid);
 			log_debug("%s: score %d", __func__, score);
 			if (score && (!chosen_score || score < chosen_score)) {
 				chosen_score = score;
@@ -1053,10 +1053,11 @@ proposals_negotiate(struct iked_proposals *result, struct iked_proposals *local,
 
 static int
 proposals_match(struct iked_proposal *local, struct iked_proposal *peer,
-    struct iked_transform **xforms, int rekey)
+    struct iked_transform **xforms, int rekey, int dhgroup)
 {
 	struct iked_transform	*tpeer, *tlocal;
 	unsigned int		 i, j, type, score, requiredh = 0, nodh = 0, noauth = 0;
+	unsigned int		 dhforced = 0;
 	uint8_t			 protoid = peer->prop_protoid;
 	uint8_t			 peerxfs[IKEV2_XFORMTYPE_MAX];
 
@@ -1114,6 +1115,18 @@ proposals_match(struct iked_proposal *local, struct iked_proposal *peer,
 				continue;
 			type = tpeer->xform_type;
 
+			if (rekey && nodh == 0 && dhgroup >= 0 &&
+			    protoid == IKEV2_SAPROTO_ESP &&
+			    type == IKEV2_XFORMTYPE_DH) {
+				if (dhforced)
+					continue;
+				/* reset xform, so this xform w/matching group is enforced */
+				if (tlocal->xform_id == dhgroup) {
+					xforms[type] = NULL;
+					dhforced = 1;
+				}
+			}
+
 			if (xforms[type] == NULL || tlocal->xform_score <
 			    xforms[type]->xform_score) {
 				xforms[type] = tlocal;