Merge branch 'incorrect-inval-ke'

This improves the behavior during CREATE_CHILD_SA exchanges if the peer
sends an INVALID_KE_PAYLOAD with a DH group we didn't request or continues
to return the same notify even if we use the requested group.

Fixes #2536.

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;
-	{
+}
-		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 &&
+ * Update the proposals with the allocated SPIs as initiator and check the DH
-					!proposal->promote_dh_group(proposal, this->dh_group))
+ * group and promote it if necessary
-				{	/* proposals that don't contain the selected group are
+ */
-					 * moved to the back */
+static bool update_and_check_proposals(private_child_create_t *this)
-					this->proposals->remove_at(this->proposals, enumerator);
+{
-					other_dh_groups->insert_last(other_dh_groups, proposal);
+	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))
+	{
+		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))
+			{
+				this->proposals->remove_at(this->proposals, enumerator);
+				other_dh_groups->insert_last(other_dh_groups, proposal);
 			}
-			enumerator->destroy(enumerator);
+			else
-			enumerator = other_dh_groups->create_enumerator(other_dh_groups);
+			{
-			while (enumerator->enumerate(enumerator, (void**)&proposal))
+				found = TRUE;
-			{	/* 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);
 		}
-		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))
+	if (!this->initiator)
-	{	/* responder has no SPI allocated yet */
+	{
-		DBG1(DBG_IKE, "allocating SPI failed");
+		if (!allocate_spi(this))
-		return FAILED;
+		{
+			/* 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);
 
@@ -981,7 +994,12 @@ static void process_payloads(private_child_create_t *this, message_t *message)
 					this->dh = this->keymat->keymat.create_dh(
 										&this->keymat->keymat, this->dh_group);
 				}
-				if (this->dh)
+				else if (this->dh)
+				{
+					this->dh_failed = this->dh->get_dh_group(this->dh) !=
+									ke_payload->get_dh_group_number(ke_payload);
+				}
+				if (this->dh && !this->dh_failed)
 				{
 					this->dh_failed = !this->dh->set_other_public_value(this->dh,
 								ke_payload->get_key_exchange_data(ke_payload));
@@ -1111,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,
@@ -1544,6 +1570,15 @@ METHOD(task_t, process_i, status_t,
 						memcpy(&group, data.ptr, data.len);
 						group = ntohs(group);
 					}
+					if (this->retry)
+					{
+						DBG1(DBG_IKE, "already retried with DH group %N, ignore"
+							 "requested %N", diffie_hellman_group_names,
+							 this->dh_group, diffie_hellman_group_names, group);
+						handle_child_sa_failure(this, message);
+						/* an error in CHILD_SA creation is not critical */
+						return SUCCESS;
+					}
 					DBG1(DBG_IKE, "peer didn't accept DH group %N, "
 						 "it requested %N", diffie_hellman_group_names,
 						 this->dh_group, diffie_hellman_group_names, group);

src/libcharon/sa/ikev2/tasks/child_rekey.c

@@ -213,7 +213,8 @@ METHOD(task_t, build_i, status_t,
 									   message) != NEED_MORE)
 	{
 		schedule_delayed_rekey(this);
-		return FAILED;
+		message->set_exchange_type(message, EXCHANGE_TYPE_UNDEFINED);
+		return SUCCESS;
 	}
 	if (message->get_exchange_type(message) == CREATE_CHILD_SA)
 	{