From ecba84a06b848dc0ee622016f8ff7d85cc878560 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Tue, 6 Nov 2018 12:13:35 +0100 Subject: [PATCH] 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. --- src/libcharon/sa/ikev2/tasks/child_delete.c | 24 +++++++--- src/libcharon/tests/suites/test_child_rekey.c | 46 ++++++------------- 2 files changed, 32 insertions(+), 38 deletions(-) diff --git a/src/libcharon/sa/ikev2/tasks/child_delete.c b/src/libcharon/sa/ikev2/tasks/child_delete.c index 6c8b29018..0e3711898 100644 --- a/src/libcharon/sa/ikev2/tasks/child_delete.c +++ b/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 */ diff --git a/src/libcharon/tests/suites/test_child_rekey.c b/src/libcharon/tests/suites/test_child_rekey.c index 51d577cd8..b9f6ea0bc 100644 --- a/src/libcharon/tests/suites/test_child_rekey.c +++ b/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();