child-delete: Don't send delete for expired CHILD_SAs that were already rekeyed

The peer might not have seen the CREATE_CHILD_SA response yet, receiving a
DELETE for the SA could then trigger it to abort the rekeying, causing
the deletion of the newly established SA (it can't know whether the
DELETE was sent due to an expire or because the user manually deleted
it).  We just treat this SA as if we received a DELETE for it.  This is
not an ideal situation anyway, as it causes some traffic to get dropped,
so it should usually be avoided by setting appropriate soft and hard limits.

References #2815.

Author: tobiasbrunner

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

@@ -174,6 +174,11 @@ static void install_outbound(private_child_delete_t *this,
 	linked_list_t *my_ts, *other_ts;
 	status_t status;
 
+	if (!spi)
+	{
+		return;
+	}
+
 	child_sa = this->ike_sa->get_child_sa(this->ike_sa, protocol,
 										  spi, FALSE);
 	if (!child_sa)
@@ -312,7 +317,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, rekey_spi;
+	uint32_t spi, reqid;
 	action_t action;
 	status_t status = SUCCESS;
 	time_t now, expire;
@@ -335,11 +340,7 @@ static status_t destroy_and_reestablish(private_child_delete_t *this)
 		}
 		else
 		{
-			rekey_spi = child_sa->get_rekey_spi(child_sa);
-			if (rekey_spi)
-			{
-				install_outbound(this, protocol, rekey_spi);
-			}
+			install_outbound(this, protocol, child_sa->get_rekey_spi(child_sa));
 			/* for rekeyed CHILD_SAs we uninstall the outbound SA but don't
 			 * immediately destroy it, by default, so we can process delayed
 			 * packets */
@@ -459,6 +460,17 @@ METHOD(task_t, build_i, status_t,
 		this->spi = child_sa->get_spi(child_sa, TRUE);
 	}
 
+	if (this->expired && child_sa->get_state(child_sa) == CHILD_REKEYED)
+	{	/* the peer was expected to delete this SA, but if we send a DELETE
+		 * we might cause a collision there if the CREATE_CHILD_SA response
+		 * is delayed (the peer wouldn't know if we deleted this SA due to an
+		 * expire or because of a forced delete by the user and might then
+		 * ignore the CREATE_CHILD_SA response once it arrives) */
+		child_sa->set_state(child_sa, CHILD_DELETED);
+		install_outbound(this, this->protocol,
+						 child_sa->get_rekey_spi(child_sa));
+	}
+
 	if (child_sa->get_state(child_sa) == CHILD_DELETED)
 	{	/* DELETEs for this CHILD_SA were already exchanged, but it was not yet
 		 * destroyed to allow delayed packets to get processed */

src/libcharon/tests/suites/test_child_rekey.c

@@ -370,8 +370,8 @@ END_TEST
 
 /**
  * Check that the responder handles hard expires properly while waiting for the
- * delete after a rekeying (e.g. if the initiator of the rekeying fails to
- * delete the CHILD_SA for some reason).
+ * delete after a rekeying (e.g. if the rekey settings are tight or the
+ * CREATE_CHILD_SA response is delayed).
  */
 START_TEST(test_regular_responder_handle_hard_expire)
 {
@@ -405,28 +405,22 @@ START_TEST(test_regular_responder_handle_hard_expire)
 
 	/* we don't expect this to get called anymore */
 	assert_hook_not_called(child_rekey);
-	/* this is similar to a regular delete collision */
-	assert_single_payload(OUT, PLV2_DELETE);
+	/* this is similar to a regular delete collision, but we don't actually
+	 * want to send a delete back as that might conflict with a delayed
+	 * CREATE_CHILD_SA response */
 	call_ikesa(b, delete_child_sa, PROTO_ESP, 2, TRUE);
-	assert_child_sa_state(b, 2, CHILD_DELETING, CHILD_OUTBOUND_INSTALLED);
-	assert_child_sa_state(b, 4, CHILD_INSTALLED, CHILD_OUTBOUND_REGISTERED);
-	/* since the SAs expired they would not actually be installed in the kernel
-	 * anymore and since we have not yet installed a new outbound SA this
-	 * will result in dropped packets and possibly acquires */
-	assert_ipsec_sas_installed(b, 1, 2, 4);
+	assert_child_sa_count(b, 1);
+	assert_child_sa_state(b, 4, CHILD_INSTALLED, CHILD_OUTBOUND_INSTALLED);
+	/* the expire causes the outbound SA to get installed */
+	assert_ipsec_sas_installed(b, 3, 4);
 
 	/* INFORMATIONAL { D } --> */
+	assert_no_jobs_scheduled();
 	assert_single_payload(IN, PLV2_DELETE);
 	exchange_test_helper->process_message(exchange_test_helper, b, NULL);
-	assert_child_sa_state(b, 2, CHILD_DELETING, CHILD_OUTBOUND_INSTALLED);
-	assert_child_sa_state(b, 4, CHILD_INSTALLED, CHILD_OUTBOUND_REGISTERED);
-	assert_ipsec_sas_installed(b, 1, 2, 4);
-	/* <-- INFORMATIONAL { D } */
-	assert_single_payload(IN, PLV2_DELETE);
-	exchange_test_helper->process_message(exchange_test_helper, a, NULL);
-	assert_child_sa_state(a, 1, CHILD_DELETING, CHILD_OUTBOUND_INSTALLED);
-	assert_child_sa_state(a, 3, CHILD_INSTALLED, CHILD_OUTBOUND_INSTALLED);
-	assert_ipsec_sas_installed(a, 1, 2, 3, 4);
+	assert_child_sa_state(b, 4, CHILD_INSTALLED, CHILD_OUTBOUND_INSTALLED);
+	assert_ipsec_sas_installed(b, 3, 4);
+	assert_scheduler();
 	/* <-- INFORMATIONAL { } */
 	assert_jobs_scheduled(1);
 	assert_message_empty(IN);
@@ -436,23 +430,11 @@ START_TEST(test_regular_responder_handle_hard_expire)
 	assert_child_sa_count(a, 2);
 	assert_ipsec_sas_installed(a, 1, 3, 4);
 	assert_scheduler();
-	/* INFORMATIONAL { } --> */
-	assert_jobs_scheduled(1);
-	assert_message_empty(IN);
-	exchange_test_helper->process_message(exchange_test_helper, b, NULL);
-	assert_child_sa_state(b, 2, CHILD_DELETED, CHILD_OUTBOUND_NONE);
-	assert_child_sa_state(b, 4, CHILD_INSTALLED, CHILD_OUTBOUND_INSTALLED);
-	assert_child_sa_count(b, 2);
-	assert_ipsec_sas_installed(b, 2, 3, 4);
-	assert_scheduler();
 
-	/* simulate the execution of the scheduled jobs */
+	/* simulate the execution of the scheduled job */
 	destroy_rekeyed(a, 1);
 	assert_child_sa_count(a, 1);
 	assert_ipsec_sas_installed(a, 3, 4);
-	destroy_rekeyed(b, 2);
-	assert_child_sa_count(b, 1);
-	assert_ipsec_sas_installed(b, 3, 4);
 
 	/* child_rekey/child_updown */
 	assert_hook();