From: Tobias Brunner <tobias@strongswan.org>
Date: Fri, 9 Feb 2018 14:16:24 +0000 (+0100)
Subject: child-create: Make sure we actually propose the requested DH group
X-Git-Tag: 5.6.3dr1~44^2~2
X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=7754c714c122c851c3986b86d2076d520318f91c;p=thirdparty%2Fstrongswan.git

child-create: Make sure we actually propose the requested DH group

If we receive an INVALID_KE_PAYLOAD notify we should not just retry
with the requested DH group without checking first if we actually propose
the group (or any at all).
---

diff --git a/src/libcharon/sa/ikev2/tasks/child_create.c b/src/libcharon/sa/ikev2/tasks/child_create.c
index 952f9cd779..f39c623493 100644
--- a/src/libcharon/sa/ikev2/tasks/child_create.c
+++ b/src/libcharon/sa/ikev2/tasks/child_create.c
@@ -277,13 +277,11 @@ static bool ts_list_is_host(linked_list_t *list, host_t *host)
 }
 
 /**
- * Allocate SPIs and update proposals, we also promote the selected DH group
+ * Allocate local SPI
  */
 static bool allocate_spi(private_child_create_t *this)
 {
-	enumerator_t *enumerator;
 	proposal_t *proposal;
-	linked_list_t *other_dh_groups;
 
 	if (this->initiator)
 	{
@@ -301,41 +299,51 @@ static bool allocate_spi(private_child_create_t *this)
 		this->proto = this->proposal->get_protocol(this->proposal);
 	}
 	this->my_spi = this->child_sa->alloc_spi(this->child_sa, this->proto);
-	if (this->my_spi)
+	return this->my_spi != 0;
+}
+
+/**
+ * Update the proposals with the allocated SPIs as initiator and check the DH
+ * group and promote it if necessary
+ */
+static bool update_and_check_proposals(private_child_create_t *this)
+{
+	enumerator_t *enumerator;
+	proposal_t *proposal;
+	linked_list_t *other_dh_groups;
+	bool found = FALSE;
+
+	other_dh_groups = linked_list_create();
+	enumerator = this->proposals->create_enumerator(this->proposals);
+	while (enumerator->enumerate(enumerator, &proposal))
 	{
-		if (this->initiator)
-		{
-			other_dh_groups = linked_list_create();
-			enumerator = this->proposals->create_enumerator(this->proposals);
-			while (enumerator->enumerate(enumerator, &proposal))
+		proposal->set_spi(proposal, this->my_spi);
+
+		/* move the selected DH group to the front, if any */
+		if (this->dh_group != MODP_NONE)
+		{	/* proposals that don't contain the selected group are
+			 * moved to the back */
+			if (!proposal->promote_dh_group(proposal, this->dh_group))
 			{
-				proposal->set_spi(proposal, this->my_spi);
-
-				/* move the selected DH group to the front, if any */
-				if (this->dh_group != MODP_NONE &&
-					!proposal->promote_dh_group(proposal, this->dh_group))
-				{	/* proposals that don't contain the selected group are
-					 * moved to the back */
-					this->proposals->remove_at(this->proposals, enumerator);
-					other_dh_groups->insert_last(other_dh_groups, proposal);
-				}
+				this->proposals->remove_at(this->proposals, enumerator);
+				other_dh_groups->insert_last(other_dh_groups, proposal);
 			}
-			enumerator->destroy(enumerator);
-			enumerator = other_dh_groups->create_enumerator(other_dh_groups);
-			while (enumerator->enumerate(enumerator, (void**)&proposal))
-			{	/* no need to remove from the list as we destroy it anyway*/
-				this->proposals->insert_last(this->proposals, proposal);
+			else
+			{
+				found = TRUE;
 			}
-			enumerator->destroy(enumerator);
-			other_dh_groups->destroy(other_dh_groups);
 		}
-		else
-		{
-			this->proposal->set_spi(this->proposal, this->my_spi);
-		}
-		return TRUE;
 	}
-	return FALSE;
+	enumerator->destroy(enumerator);
+	enumerator = other_dh_groups->create_enumerator(other_dh_groups);
+	while (enumerator->enumerate(enumerator, (void**)&proposal))
+	{	/* no need to remove from the list as we destroy it anyway*/
+		this->proposals->insert_last(this->proposals, proposal);
+	}
+	enumerator->destroy(enumerator);
+	other_dh_groups->destroy(other_dh_groups);
+
+	return this->dh_group == MODP_NONE || found;
 }
 
 /**
@@ -532,10 +540,15 @@ static status_t select_and_install(private_child_create_t *this,
 	}
 	this->other_spi = this->proposal->get_spi(this->proposal);
 
-	if (!this->initiator && !allocate_spi(this))
-	{	/* responder has no SPI allocated yet */
-		DBG1(DBG_IKE, "allocating SPI failed");
-		return FAILED;
+	if (!this->initiator)
+	{
+		if (!allocate_spi(this))
+		{
+			/* responder has no SPI allocated yet */
+			DBG1(DBG_IKE, "allocating SPI failed");
+			return FAILED;
+		}
+		this->proposal->set_spi(this->proposal, this->my_spi);
 	}
 	this->child_sa->set_proposal(this->child_sa, this->proposal);
 
@@ -1116,6 +1129,14 @@ METHOD(task_t, build_i, status_t,
 		return FAILED;
 	}
 
+	if (!update_and_check_proposals(this))
+	{
+		DBG1(DBG_IKE, "requested DH group %N not contained in any of our "
+			 "proposals",
+			 diffie_hellman_group_names, this->dh_group);
+		return FAILED;
+	}
+
 	if (this->dh_group != MODP_NONE)
 	{
 		this->dh = this->keymat->keymat.create_dh(&this->keymat->keymat,