ikev2: Delay installation of outbound SAs during rekeying on the responder

The responder has all the information needed to install both SAs before
the initiator does.  So if the responder immediately installs the outbound
SA it might send packets using the new SA which the initiator is not yet
able to process.  This can be avoided by delaying the installation of the
outbound SA until the replaced SA is deleted.

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

@@ -682,7 +682,7 @@ static status_t select_and_install(private_child_create_t *this,
 							this->other_spi, this->other_cpi, this->initiator,
 							FALSE, this->tfcv3);
 		}
-		else
+		else if (!this->rekey)
 		{
 			status_i = this->child_sa->install(this->child_sa, encr_i, integ_i,
 							this->my_spi, this->my_cpi, this->initiator,
@@ -691,6 +691,17 @@ static status_t select_and_install(private_child_create_t *this,
 							this->other_spi, this->other_cpi, this->initiator,
 							FALSE, this->tfcv3);
 		}
+		else
+		{	/* as responder during a rekeying we only install the inbound
+			 * SA now, the outbound SA and policies are installed when we
+			 * receive the delete for the old SA */
+			status_i = this->child_sa->install(this->child_sa, encr_i, integ_i,
+							this->my_spi, this->my_cpi, this->initiator,
+							TRUE, this->tfcv3);
+			this->child_sa->register_outbound(this->child_sa, encr_r, integ_r,
+							this->other_spi, this->other_cpi, this->tfcv3);
+			status_o = SUCCESS;
+		}
 	}
 
 	if (status_i != SUCCESS || status_o != SUCCESS)
@@ -734,8 +745,14 @@ static status_t select_and_install(private_child_create_t *this,
 	charon->bus->child_keys(charon->bus, this->child_sa, this->initiator,
 							this->dh, nonce_i, nonce_r);
 
-	/* add to IKE_SA, and remove from task */
+	if (this->rekey && !this->initiator)
-	this->child_sa->set_state(this->child_sa, CHILD_INSTALLED);
+	{
+		this->child_sa->set_state(this->child_sa, CHILD_INSTALLED_INBOUND);
+	}
+	else
+	{
+		this->child_sa->set_state(this->child_sa, CHILD_INSTALLED);
+	}
 	this->ike_sa->add_child_sa(this->ike_sa, this->child_sa);
 	this->established = TRUE;
 
@@ -746,16 +763,17 @@ static status_t select_and_install(private_child_create_t *this,
 	other_ts = linked_list_create_from_enumerator(
 				this->child_sa->create_ts_enumerator(this->child_sa, FALSE));
 
-	DBG0(DBG_IKE, "CHILD_SA %s{%d} established "
+	DBG0(DBG_IKE, "%sCHILD_SA %s{%d} established "
 		 "with SPIs %.8x_i %.8x_o and TS %#R === %#R",
+		 this->rekey && !this->initiator ? "inbound " : "",
 		 this->child_sa->get_name(this->child_sa),
 		 this->child_sa->get_unique_id(this->child_sa),
 		 ntohl(this->child_sa->get_spi(this->child_sa, TRUE)),
-		 ntohl(this->child_sa->get_spi(this->child_sa, FALSE)), my_ts, other_ts);
+		 ntohl(this->child_sa->get_spi(this->child_sa, FALSE)),
+		 my_ts, other_ts);
 
 	my_ts->destroy(my_ts);
 	other_ts->destroy(other_ts);
-
 	return SUCCESS;
 }
 
@@ -1688,7 +1706,6 @@ METHOD(task_t, destroy, void,
 	{
 		this->proposals->destroy_offset(this->proposals, offsetof(proposal_t, destroy));
 	}
-
 	DESTROY_IF(this->config);
 	DESTROY_IF(this->nonceg);
 	free(this);

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

@@ -146,6 +146,57 @@ static bool is_redundant(private_child_delete_t *this, child_sa_t *child)
 	return FALSE;
 }
 
+/**
+ * Install the outbound CHILD_SA with the given SPI
+ */
+static void install_outbound(private_child_delete_t *this,
+							 protocol_id_t protocol, uint32_t spi)
+{
+	child_sa_t *child_sa;
+	linked_list_t *my_ts, *other_ts;
+	status_t status;
+
+	child_sa = this->ike_sa->get_child_sa(this->ike_sa, protocol,
+										  spi, FALSE);
+	if (!child_sa)
+	{
+		DBG1(DBG_IKE, "CHILD_SA not found after rekeying");
+		return;
+	}
+	if (this->initiator && is_redundant(this, child_sa))
+	{	/* if we won the rekey collision we don't want to install the
+		 * redundant SA created by the peer */
+		return;
+	}
+
+	status = child_sa->install_outbound(child_sa);
+	if (status != SUCCESS)
+	{
+		DBG1(DBG_IKE, "unable to install outbound IPsec SA (SAD) in kernel");
+		charon->bus->alert(charon->bus, ALERT_INSTALL_CHILD_SA_FAILED,
+						   child_sa);
+		/* FIXME: delete the new child_sa? */
+		return;
+	}
+	child_sa->set_state(child_sa, CHILD_INSTALLED);
+
+	my_ts = linked_list_create_from_enumerator(
+							child_sa->create_ts_enumerator(child_sa, TRUE));
+	other_ts = linked_list_create_from_enumerator(
+							child_sa->create_ts_enumerator(child_sa, FALSE));
+
+	DBG0(DBG_IKE, "outbound CHILD_SA %s{%d} established "
+		 "with SPIs %.8x_i %.8x_o and TS %#R === %#R",
+		 child_sa->get_name(child_sa),
+		 child_sa->get_unique_id(child_sa),
+		 ntohl(child_sa->get_spi(child_sa, TRUE)),
+		 ntohl(child_sa->get_spi(child_sa, FALSE)),
+		 my_ts, other_ts);
+
+	my_ts->destroy(my_ts);
+	other_ts->destroy(other_ts);
+}
+
 /**
  * read in payloads and find the children to delete
  */
@@ -197,6 +248,7 @@ static void process_payloads(private_child_delete_t *this, message_t *message)
 						/* fall through */
 					case CHILD_REKEYING:
 						/* we reply as usual, rekeying will fail */
+					case CHILD_INSTALLED_INBOUND:
 					case CHILD_INSTALLED:
 						if (!this->initiator)
 						{
@@ -234,7 +286,7 @@ static status_t destroy_and_reestablish(private_child_delete_t *this)
 	child_sa_t *child_sa;
 	child_cfg_t *child_cfg;
 	protocol_id_t protocol;
-	uint32_t spi, reqid;
+	uint32_t spi, reqid, rekey_spi;
 	action_t action;
 	status_t status = SUCCESS;
 
@@ -242,13 +294,21 @@ static status_t destroy_and_reestablish(private_child_delete_t *this)
 	while (enumerator->enumerate(enumerator, (void**)&child_sa))
 	{
 		/* signal child down event if we weren't rekeying */
+		protocol = child_sa->get_protocol(child_sa);
 		if (!this->rekeyed)
 		{
 			charon->bus->child_updown(charon->bus, child_sa, FALSE);
 		}
+		else
+		{
+			rekey_spi = child_sa->get_rekey_spi(child_sa);
+			if (rekey_spi)
+			{
+				install_outbound(this, protocol, rekey_spi);
+			}
+		}
 		spi = child_sa->get_spi(child_sa, TRUE);
 		reqid = child_sa->get_reqid(child_sa);
-		protocol = child_sa->get_protocol(child_sa);
 		child_cfg = child_sa->get_config(child_sa);
 		child_cfg->get_ref(child_cfg);
 		action = child_sa->get_close_action(child_sa);

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

@@ -227,6 +227,7 @@ METHOD(task_t, build_r, status_t,
 	child_cfg_t *config;
 	uint32_t reqid;
 	child_sa_state_t state;
+	child_sa_t *child_sa;
 
 	if (!this->child_sa)
 	{
@@ -260,7 +261,10 @@ METHOD(task_t, build_r, status_t,
 		return SUCCESS;
 	}
 
+	child_sa = this->child_create->get_child(this->child_create);
 	this->child_sa->set_state(this->child_sa, CHILD_REKEYED);
+	this->child_sa->set_rekey_spi(this->child_sa,
+								  child_sa->get_spi(child_sa, FALSE));
 
 	/* invoke rekey hook */
 	charon->bus->child_rekey(charon->bus, this->child_sa,
@@ -472,7 +476,8 @@ METHOD(child_rekey_t, collide, void,
 		/* ignore passive tasks that did not successfully create a CHILD_SA */
 		other_child = rekey->child_create->get_child(rekey->child_create);
 		if (!other_child ||
-			 other_child->get_state(other_child) != CHILD_INSTALLED)
+			(other_child->get_state(other_child) != CHILD_INSTALLED &&
+			 other_child->get_state(other_child) != CHILD_INSTALLED_INBOUND))
 		{
 			other->destroy(other);
 			return;

src/libcharon/tests/suites/test_child_rekey.c

@@ -62,7 +62,7 @@ START_TEST(test_regular)
 	assert_notify(IN, REKEY_SA);
 	exchange_test_helper->process_message(exchange_test_helper, b, NULL);
 	assert_child_sa_state(b, spi_b, CHILD_REKEYED);
-	assert_child_sa_state(b, 4, CHILD_INSTALLED);
+	assert_child_sa_state(b, 4, CHILD_INSTALLED_INBOUND);
 	assert_hook();
 
 	/* <-- CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } */
@@ -150,7 +150,7 @@ START_TEST(test_regular_ke_invalid)
 	assert_notify(IN, REKEY_SA);
 	exchange_test_helper->process_message(exchange_test_helper, b, NULL);
 	assert_child_sa_state(b, spi_b, CHILD_REKEYED);
-	assert_child_sa_state(b, 6, CHILD_INSTALLED);
+	assert_child_sa_state(b, 6, CHILD_INSTALLED_INBOUND);
 	assert_hook();
 
 	/* <-- CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } */
@@ -204,7 +204,7 @@ START_TEST(test_regular_responder_ignore_soft_expire)
 	assert_notify(IN, REKEY_SA);
 	exchange_test_helper->process_message(exchange_test_helper, b, NULL);
 	assert_child_sa_state(b, 2, CHILD_REKEYED);
-	assert_child_sa_state(b, 4, CHILD_INSTALLED);
+	assert_child_sa_state(b, 4, CHILD_INSTALLED_INBOUND);
 	assert_hook();
 
 	/* <-- CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } */
@@ -263,7 +263,7 @@ START_TEST(test_regular_responder_handle_hard_expire)
 	assert_notify(IN, REKEY_SA);
 	exchange_test_helper->process_message(exchange_test_helper, b, NULL);
 	assert_child_sa_state(b, 2, CHILD_REKEYED);
-	assert_child_sa_state(b, 4, CHILD_INSTALLED);
+	assert_child_sa_state(b, 4, CHILD_INSTALLED_INBOUND);
 	assert_hook();
 
 	/* <-- CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } */
@@ -284,7 +284,7 @@ START_TEST(test_regular_responder_handle_hard_expire)
 	/* INFORMATIONAL { D } --> */
 	assert_single_payload(IN, PLV2_DELETE);
 	exchange_test_helper->process_message(exchange_test_helper, b, NULL);
-	assert_child_sa_state(b, 4, CHILD_INSTALLED);
+	assert_child_sa_state(b, 4, CHILD_INSTALLED_INBOUND);
 	assert_child_sa_state(a, 2, CHILD_DELETING);
 	/* <-- INFORMATIONAL { D } */
 	assert_single_payload(IN, PLV2_DELETE);
@@ -361,14 +361,14 @@ START_TEST(test_collision)
 	assert_hook_rekey(child_rekey, 2, 5);
 	exchange_test_helper->process_message(exchange_test_helper, b, NULL);
 	assert_child_sa_state(b, 2, CHILD_REKEYED);
-	assert_child_sa_state(b, 5, CHILD_INSTALLED);
+	assert_child_sa_state(b, 5, CHILD_INSTALLED_INBOUND);
 	assert_hook();
 	/* <-- CREATE_CHILD_SA { N(REKEY_SA), SA, Ni, [KEi,] TSi, TSr } */
 	exchange_test_helper->nonce_first_byte = data[_i].nonces[3];
 	assert_hook_rekey(child_rekey, 1, 6);
 	exchange_test_helper->process_message(exchange_test_helper, a, NULL);
 	assert_child_sa_state(a, 1, CHILD_REKEYED);
-	assert_child_sa_state(a, 6, CHILD_INSTALLED);
+	assert_child_sa_state(a, 6, CHILD_INSTALLED_INBOUND);
 	assert_hook();
 
 	/* <-- CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } */
@@ -387,7 +387,9 @@ START_TEST(test_collision)
 	}
 	assert_child_sa_state(a, data[_i].spi_del_a, CHILD_DELETING);
 	assert_child_sa_state(a, data[_i].spi_del_b, CHILD_REKEYED);
-	assert_child_sa_state(a, data[_i].spi_a, CHILD_INSTALLED);
+	assert_child_sa_state(a, data[_i].spi_a,
+						  data[_i].spi_del_a == 1 ? CHILD_INSTALLED
+												  : CHILD_INSTALLED_INBOUND);
 	/* CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } --> */
 	if (data[_i].spi_del_b == 2)
 	{
@@ -403,7 +405,9 @@ START_TEST(test_collision)
 	}
 	assert_child_sa_state(b, data[_i].spi_del_b, CHILD_DELETING);
 	assert_child_sa_state(b, data[_i].spi_del_a, CHILD_REKEYED);
-	assert_child_sa_state(b, data[_i].spi_b, CHILD_INSTALLED);
+	assert_child_sa_state(b, data[_i].spi_b,
+						  data[_i].spi_del_b == 2 ? CHILD_INSTALLED
+												  : CHILD_INSTALLED_INBOUND);
 
 	/* we don't expect this hook to get called anymore */
 	assert_hook_not_called(child_rekey);
@@ -494,14 +498,14 @@ START_TEST(test_collision_delayed_response)
 	assert_hook_rekey(child_rekey, 2, 5);
 	exchange_test_helper->process_message(exchange_test_helper, b, NULL);
 	assert_child_sa_state(b, 2, CHILD_REKEYED);
-	assert_child_sa_state(b, 5, CHILD_INSTALLED);
+	assert_child_sa_state(b, 5, CHILD_INSTALLED_INBOUND);
 	assert_hook();
 	/* <-- CREATE_CHILD_SA { N(REKEY_SA), SA, Ni, [KEi,] TSi, TSr } */
 	exchange_test_helper->nonce_first_byte = data[_i].nonces[3];
 	assert_hook_rekey(child_rekey, 1, 6);
 	exchange_test_helper->process_message(exchange_test_helper, a, NULL);
 	assert_child_sa_state(a, 1, CHILD_REKEYED);
-	assert_child_sa_state(a, 6, CHILD_INSTALLED);
+	assert_child_sa_state(a, 6, CHILD_INSTALLED_INBOUND);
 	assert_hook();
 
 	/* delay the CREATE_CHILD_SA response from b to a */
@@ -522,7 +526,9 @@ START_TEST(test_collision_delayed_response)
 	}
 	assert_child_sa_state(b, data[_i].spi_del_b, CHILD_DELETING);
 	assert_child_sa_state(b, data[_i].spi_del_a, CHILD_REKEYED);
-	assert_child_sa_state(b, data[_i].spi_b, CHILD_INSTALLED);
+	assert_child_sa_state(b, data[_i].spi_b,
+						  data[_i].spi_del_b == 2 ? CHILD_INSTALLED
+												  : CHILD_INSTALLED_INBOUND);
 
 	/* <-- INFORMATIONAL { D } */
 	assert_hook_not_called(child_rekey);
@@ -540,7 +546,9 @@ START_TEST(test_collision_delayed_response)
 	/* INFORMATIONAL { D } --> */
 	exchange_test_helper->process_message(exchange_test_helper, b, NULL);
 	assert_child_sa_state(b, data[_i].spi_del_a, CHILD_REKEYED);
-	assert_child_sa_state(b, data[_i].spi_b, CHILD_INSTALLED);
+	assert_child_sa_state(b, data[_i].spi_b,
+						  data[_i].spi_del_b == 2 ? CHILD_INSTALLED
+												  : CHILD_INSTALLED_INBOUND);
 	assert_child_sa_count(b, 2);
 	assert_hook();
 
@@ -635,7 +643,7 @@ START_TEST(test_collision_delayed_request)
 	assert_hook_rekey(child_rekey, 1, 5);
 	exchange_test_helper->process_message(exchange_test_helper, a, NULL);
 	assert_child_sa_state(a, 1, CHILD_REKEYED);
-	assert_child_sa_state(a, 5, CHILD_INSTALLED);
+	assert_child_sa_state(a, 5, CHILD_INSTALLED_INBOUND);
 	assert_hook();
 	/* CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } --> */
 	assert_hook_rekey(child_rekey, 2, 4);
@@ -736,7 +744,7 @@ START_TEST(test_collision_delayed_request_more)
 	assert_hook_rekey(child_rekey, 1, 5);
 	exchange_test_helper->process_message(exchange_test_helper, a, NULL);
 	assert_child_sa_state(a, 1, CHILD_REKEYED);
-	assert_child_sa_state(a, 5, CHILD_INSTALLED);
+	assert_child_sa_state(a, 5, CHILD_INSTALLED_INBOUND);
 	assert_hook();
 	/* CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } --> */
 	assert_hook_rekey(child_rekey, 2, 4);
@@ -874,14 +882,14 @@ START_TEST(test_collision_ke_invalid)
 	assert_hook_rekey(child_rekey, 2, 9);
 	exchange_test_helper->process_message(exchange_test_helper, b, NULL);
 	assert_child_sa_state(b, 2, CHILD_REKEYED);
-	assert_child_sa_state(b, 9, CHILD_INSTALLED);
+	assert_child_sa_state(b, 9, CHILD_INSTALLED_INBOUND);
 	assert_hook();
 	/* <-- CREATE_CHILD_SA { N(REKEY_SA), SA, Ni, [KEi,] TSi, TSr } */
 	exchange_test_helper->nonce_first_byte = data[_i].nonces[3];
 	assert_hook_rekey(child_rekey, 1, 10);
 	exchange_test_helper->process_message(exchange_test_helper, a, NULL);
 	assert_child_sa_state(a, 1, CHILD_REKEYED);
-	assert_child_sa_state(a,10, CHILD_INSTALLED);
+	assert_child_sa_state(a,10, CHILD_INSTALLED_INBOUND);
 	assert_hook();
 
 	/* <-- CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } */
@@ -898,7 +906,9 @@ START_TEST(test_collision_ke_invalid)
 	}
 	assert_child_sa_state(a, data[_i].spi_del_a, CHILD_DELETING);
 	assert_child_sa_state(a, data[_i].spi_del_b, CHILD_REKEYED);
-	assert_child_sa_state(a, data[_i].spi_a, CHILD_INSTALLED);
+	assert_child_sa_state(a, data[_i].spi_a,
+						  data[_i].spi_del_a == 1 ? CHILD_INSTALLED
+												  : CHILD_INSTALLED_INBOUND);
 	/* CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } --> */
 	if (data[_i].spi_del_b == 2)
 	{
@@ -912,7 +922,9 @@ START_TEST(test_collision_ke_invalid)
 	}
 	assert_child_sa_state(b, data[_i].spi_del_b, CHILD_DELETING);
 	assert_child_sa_state(b, data[_i].spi_del_a, CHILD_REKEYED);
-	assert_child_sa_state(b, data[_i].spi_b, CHILD_INSTALLED);
+	assert_child_sa_state(b, data[_i].spi_b,
+						  data[_i].spi_del_b == 2 ? CHILD_INSTALLED
+												  : CHILD_INSTALLED_INBOUND);
 
 	/* we don't expect this hook to get called anymore */
 	assert_hook_not_called(child_rekey);
@@ -1039,7 +1051,7 @@ START_TEST(test_collision_ke_invalid_delayed_retry)
 	assert_hook_rekey(child_rekey, 1, 9);
 	exchange_test_helper->process_message(exchange_test_helper, a, NULL);
 	assert_child_sa_state(a, 1, CHILD_REKEYED);
-	assert_child_sa_state(a, 9, CHILD_INSTALLED);
+	assert_child_sa_state(a, 9, CHILD_INSTALLED_INBOUND);
 	assert_hook();
 	/* CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } --> */
 	assert_hook_rekey(child_rekey, 2, 8);