child-rekey: Don't install outbound SA in case of lost collisions

This splits the SA installation also on the initiator, so we can avoid
installing the outbound SA if we lost a rekey collision, which might
have caused traffic loss depending on the timing of the DELETEs that are
sent in both directions.

diff --git a/src/libcharon/sa/ikev2/tasks/child_create.c b/src/libcharon/sa/ikev2/tasks/child_create.c
index 9d7d08c..9fe6929 100644 (file)
--- a/src/libcharon/sa/ikev2/tasks/child_create.c
+++ b/src/libcharon/sa/ikev2/tasks/child_create.c
@@ -478,6 +478,7 @@ static status_t select_and_install(private_child_create_t *this,
                                                                   bool no_dh, bool ike_auth)
 {
        status_t status, status_i, status_o;
+       child_sa_outbound_state_t out_state;
        chunk_t nonce_i, nonce_r;
        chunk_t encr_i = chunk_empty, encr_r = chunk_empty;
        chunk_t integ_i = chunk_empty, integ_r = chunk_empty;
@@ -678,29 +679,42 @@ static status_t select_and_install(private_child_create_t *this,
                        status_i = this->child_sa->install(this->child_sa, encr_r, integ_r,
                                                        this->my_spi, this->my_cpi, this->initiator,
                                                        TRUE, this->tfcv3);
-                       status_o = this->child_sa->install(this->child_sa, encr_i, integ_i,
-                                                       this->other_spi, this->other_cpi, this->initiator,
-                                                       FALSE, this->tfcv3);
                }
-               else if (!this->rekey)
+               else
                {
                        status_i = this->child_sa->install(this->child_sa, encr_i, integ_i,
                                                        this->my_spi, this->my_cpi, this->initiator,
                                                        TRUE, this->tfcv3);
-                       status_o = this->child_sa->install(this->child_sa, encr_r, integ_r,
+               }
+               if (this->rekey)
+               {       /* during rekeyings we install the outbound SA and/or policies
+                        * separately: as responder when we receive the delete for the old
+                        * SA, as initiator pretty much immediately in the ike-rekey task,
+                        * unless there was a rekey collision that we lost */
+                       if (this->initiator)
+                       {
+                               status_o = this->child_sa->register_outbound(this->child_sa,
+                                                       encr_i, integ_i, this->other_spi, this->other_cpi,
+                                                       this->tfcv3);
+                       }
+                       else
+                       {
+                               status_o = this->child_sa->register_outbound(this->child_sa,
+                                                       encr_r, integ_r, this->other_spi, this->other_cpi,
+                                                       this->tfcv3);
+                       }
+               }
+               else if (this->initiator)
+               {
+                       status_o = this->child_sa->install(this->child_sa, encr_i, integ_i,
                                                        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);
-                       status_o = this->child_sa->register_outbound(this->child_sa, encr_r,
-                                                       integ_r, this->other_spi, this->other_cpi,
-                                                       this->tfcv3);
+               {
+                       status_o = this->child_sa->install(this->child_sa, encr_r, integ_r,
+                                                       this->other_spi, this->other_cpi, this->initiator,
+                                                       FALSE, this->tfcv3);
                }
        }
 
@@ -749,10 +763,11 @@ static status_t select_and_install(private_child_create_t *this,
                                this->child_sa->create_ts_enumerator(this->child_sa, TRUE));
        other_ts = linked_list_create_from_enumerator(
                                this->child_sa->create_ts_enumerator(this->child_sa, FALSE));
+       out_state = this->child_sa->get_outbound_state(this->child_sa);
 
        DBG0(DBG_IKE, "%sCHILD_SA %s{%d} established "
                 "with SPIs %.8x_i %.8x_o and TS %#R === %#R",
-                this->rekey && !this->initiator ? "inbound " : "",
+                (out_state == CHILD_OUTBOUND_INSTALLED) ? "" : "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)),

diff --git a/src/libcharon/sa/ikev2/tasks/child_delete.c b/src/libcharon/sa/ikev2/tasks/child_delete.c
index 6267963..2217295 100644 (file)
--- a/src/libcharon/sa/ikev2/tasks/child_delete.c
+++ b/src/libcharon/sa/ikev2/tasks/child_delete.c
@@ -196,7 +196,6 @@ static void install_outbound(private_child_delete_t *this,
                /* 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));

diff --git a/src/libcharon/sa/ikev2/tasks/child_rekey.c b/src/libcharon/sa/ikev2/tasks/child_rekey.c
index 761c860..619ec41 100644 (file)
--- a/src/libcharon/sa/ikev2/tasks/child_rekey.c
+++ b/src/libcharon/sa/ikev2/tasks/child_rekey.c
@@ -283,7 +283,8 @@ METHOD(task_t, build_r, status_t,
 /**
  * Handle a rekey collision
  */
-static child_sa_t *handle_collision(private_child_rekey_t *this)
+static child_sa_t *handle_collision(private_child_rekey_t *this,
+                                                                       child_sa_t **to_install)
 {
        child_sa_t *to_delete;
 
@@ -303,6 +304,7 @@ static child_sa_t *handle_collision(private_child_rekey_t *this)
                        child_sa_t *child_sa;
 
                        DBG1(DBG_IKE, "CHILD_SA rekey collision won, deleting old child");
+                       *to_install = this->child_create->get_child(this->child_create);
                        to_delete = this->child_sa;
                        /* don't touch child other created, it has already been deleted */
                        if (!this->other_child_destroyed)
@@ -353,7 +355,7 @@ METHOD(task_t, process_i, status_t,
 {
        protocol_id_t protocol;
        uint32_t spi;
-       child_sa_t *to_delete;
+       child_sa_t *to_delete, *to_install = NULL;
 
        if (message->get_notify(message, NO_ADDITIONAL_SAS))
        {
@@ -415,19 +417,48 @@ METHOD(task_t, process_i, status_t,
        /* check for rekey collisions */
        if (this->collision)
        {
-               to_delete = handle_collision(this);
+               to_delete = handle_collision(this, &to_install);
        }
        else
        {
+               to_install = this->child_create->get_child(this->child_create);
                to_delete = this->child_sa;
        }
-
+       if (to_install)
+       {
+               if (to_install->install_outbound(to_install) != SUCCESS)
+               {
+                       DBG1(DBG_IKE, "unable to install outbound IPsec SA (SAD) in kernel");
+                       charon->bus->alert(charon->bus, ALERT_INSTALL_CHILD_SA_FAILED,
+                                                          to_install);
+                       /* FIXME: delete the child_sa? fail the task? */
+               }
+               else
+               {
+                       linked_list_t *my_ts, *other_ts;
+
+                       my_ts = linked_list_create_from_enumerator(
+                                               to_install->create_ts_enumerator(to_install, TRUE));
+                       other_ts = linked_list_create_from_enumerator(
+                                               to_install->create_ts_enumerator(to_install, FALSE));
+
+                       DBG0(DBG_IKE, "outbound CHILD_SA %s{%d} established "
+                                "with SPIs %.8x_i %.8x_o and TS %#R === %#R",
+                                to_install->get_name(to_install),
+                                to_install->get_unique_id(to_install),
+                                ntohl(to_install->get_spi(to_install, TRUE)),
+                                ntohl(to_install->get_spi(to_install, FALSE)),
+                                my_ts, other_ts);
+
+                       my_ts->destroy(my_ts);
+                       other_ts->destroy(other_ts);
+               }
+       }
        if (to_delete != this->child_create->get_child(this->child_create))
        {       /* invoke rekey hook if rekeying successful */
                charon->bus->child_rekey(charon->bus, this->child_sa,
                                                        this->child_create->get_child(this->child_create));
        }
-
        if (to_delete == NULL)
        {
                return SUCCESS;

diff --git a/src/libcharon/tests/suites/test_child_rekey.c b/src/libcharon/tests/suites/test_child_rekey.c
index 76b23f5..ac16972 100644 (file)
--- a/src/libcharon/tests/suites/test_child_rekey.c
+++ b/src/libcharon/tests/suites/test_child_rekey.c
@@ -483,6 +483,9 @@ START_TEST(test_collision)
                                                          CHILD_OUTBOUND_REGISTERED);
                assert_child_sa_state(a, data[_i].spi_a, CHILD_INSTALLED,
                                                          CHILD_OUTBOUND_INSTALLED);
+               assert_child_sa_state(a, data[_i].spi_del_a, CHILD_DELETING,
+                                                         CHILD_OUTBOUND_INSTALLED);
+               assert_ipsec_sas_installed(a, 1, 2, 3, 5, 6);
        }
        else
        {
@@ -493,10 +496,10 @@ START_TEST(test_collision)
                                                          CHILD_OUTBOUND_INSTALLED);
                assert_child_sa_state(a, data[_i].spi_a, CHILD_INSTALLED,
                                                          CHILD_OUTBOUND_REGISTERED);
+               assert_child_sa_state(a, data[_i].spi_del_a, CHILD_DELETING,
+                                                         CHILD_OUTBOUND_REGISTERED);
+               assert_ipsec_sas_installed(a, 1, 2, 3, 6);
        }
-       assert_child_sa_state(a, data[_i].spi_del_a, CHILD_DELETING,
-                                                 CHILD_OUTBOUND_INSTALLED);
-       assert_ipsec_sas_installed(a, 1, 2, 3, 5, 6);
        /* CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } --> */
        if (data[_i].spi_del_b == 2)
        {
@@ -507,6 +510,9 @@ START_TEST(test_collision)
                                                          CHILD_OUTBOUND_REGISTERED);
                assert_child_sa_state(b, data[_i].spi_b, CHILD_INSTALLED,
                                                          CHILD_OUTBOUND_INSTALLED);
+               assert_child_sa_state(b, data[_i].spi_del_b, CHILD_DELETING,
+                                                         CHILD_OUTBOUND_INSTALLED);
+               assert_ipsec_sas_installed(b, 1, 2, 4, 5, 6);
        }
        else
        {
@@ -517,10 +523,10 @@ START_TEST(test_collision)
                                                          CHILD_OUTBOUND_INSTALLED);
                assert_child_sa_state(b, data[_i].spi_b, CHILD_INSTALLED,
                                                          CHILD_OUTBOUND_REGISTERED);
+               assert_child_sa_state(b, data[_i].spi_del_b, CHILD_DELETING,
+                                                         CHILD_OUTBOUND_REGISTERED);
+               assert_ipsec_sas_installed(b, 1, 2, 4, 5);
        }
-       assert_child_sa_state(b, data[_i].spi_del_b, CHILD_DELETING,
-                                                 CHILD_OUTBOUND_INSTALLED);
-       assert_ipsec_sas_installed(b, 1, 2, 4, 5, 6);
 
        /* we don't expect this hook to get called anymore */
        assert_hook_not_called(child_rekey);
@@ -528,27 +534,41 @@ START_TEST(test_collision)
        assert_jobs_scheduled(1);
        exchange_test_helper->process_message(exchange_test_helper, b, NULL);
        assert_child_sa_state(b, data[_i].spi_del_b, CHILD_DELETING,
-                                                 CHILD_OUTBOUND_INSTALLED);
+                                                 data[_i].spi_del_b == 2 ? CHILD_OUTBOUND_INSTALLED
+                                                                                                 : CHILD_OUTBOUND_REGISTERED);
        assert_child_sa_state(b, data[_i].spi_del_a, CHILD_DELETING,
                                                  CHILD_OUTBOUND_NONE);
        assert_child_sa_state(b, data[_i].spi_b, CHILD_INSTALLED,
                                                  CHILD_OUTBOUND_INSTALLED);
        assert_child_sa_count(b, 3);
-       assert_ipsec_sas_installed(b, 2, 4, 5, 6,
-                                                          data[_i].spi_del_b == 2 ? 1 : 3);
+       if (data[_i].spi_del_b == 2)
+       {
+               assert_ipsec_sas_installed(b, 1, 2, 4, 5, 6);
+       }
+       else
+       {
+               assert_ipsec_sas_installed(b, 2, 3, 4, 5);
+       }
        assert_scheduler();
        /* <-- INFORMATIONAL { D } */
        assert_jobs_scheduled(1);
        exchange_test_helper->process_message(exchange_test_helper, a, NULL);
        assert_child_sa_state(a, data[_i].spi_del_a, CHILD_DELETING,
-                                                 CHILD_OUTBOUND_INSTALLED);
+                                                 data[_i].spi_del_a == 1 ? CHILD_OUTBOUND_INSTALLED
+                                                                                                 : CHILD_OUTBOUND_REGISTERED);
        assert_child_sa_state(a, data[_i].spi_del_b, CHILD_DELETING,
                                                  CHILD_OUTBOUND_NONE);
        assert_child_sa_state(a, data[_i].spi_a, CHILD_INSTALLED,
                                                  CHILD_OUTBOUND_INSTALLED);
        assert_child_sa_count(a, 3);
-       assert_ipsec_sas_installed(a, 1, 3, 5, 6,
-                                                          data[_i].spi_del_a == 1 ? 2 : 4);
+       if (data[_i].spi_del_a == 1)
+       {
+               assert_ipsec_sas_installed(a, 1, 2, 3, 5, 6);
+       }
+       else
+       {
+               assert_ipsec_sas_installed(a, 1, 3, 4, 6);
+       }
        assert_scheduler();
        /* <-- INFORMATIONAL { D } */
        assert_jobs_scheduled(1);
@@ -682,6 +702,9 @@ START_TEST(test_collision_delayed_response)
                                                          CHILD_OUTBOUND_REGISTERED);
                assert_child_sa_state(b, data[_i].spi_b, CHILD_INSTALLED,
                                                          CHILD_OUTBOUND_INSTALLED);
+               assert_child_sa_state(b, data[_i].spi_del_b, CHILD_DELETING,
+                                                         CHILD_OUTBOUND_INSTALLED);
+               assert_ipsec_sas_installed(b, 1, 2, 4, 5, 6);
        }
        else
        {
@@ -692,10 +715,10 @@ START_TEST(test_collision_delayed_response)
                                                          CHILD_OUTBOUND_INSTALLED);
                assert_child_sa_state(b, data[_i].spi_b, CHILD_INSTALLED,
                                                          CHILD_OUTBOUND_REGISTERED);
+               assert_child_sa_state(b, data[_i].spi_del_b, CHILD_DELETING,
+                                                         CHILD_OUTBOUND_REGISTERED);
+               assert_ipsec_sas_installed(b, 1, 2, 4, 5);
        }
-       assert_child_sa_state(b, data[_i].spi_del_b, CHILD_DELETING,
-                                                 CHILD_OUTBOUND_INSTALLED);
-       assert_ipsec_sas_installed(b, 1, 2, 4, 5, 6);
 
        /* <-- INFORMATIONAL { D } */
        assert_hook_not_called(child_rekey);
@@ -748,21 +771,23 @@ START_TEST(test_collision_delayed_response)
                assert_hook_rekey(child_rekey, 1, data[_i].spi_a);
                exchange_test_helper->process_message(exchange_test_helper, a, msg);
                assert_hook();
+               assert_child_sa_state(a, data[_i].spi_del_a, CHILD_DELETING,
+                                                         CHILD_OUTBOUND_INSTALLED);
+               assert_ipsec_sas_installed(a, 1, 2, 3, 5, 6);
        }
        else
        {
                assert_hook_not_called(child_rekey);
                exchange_test_helper->process_message(exchange_test_helper, a, msg);
                assert_hook();
+               assert_child_sa_state(a, data[_i].spi_del_a, CHILD_DELETING,
+                                                         CHILD_OUTBOUND_REGISTERED);
+               assert_ipsec_sas_installed(a, 1, 3, 4, 6);
        }
-       assert_child_sa_state(a, data[_i].spi_del_a, CHILD_DELETING,
-                                                 CHILD_OUTBOUND_INSTALLED);
        assert_child_sa_state(a, data[_i].spi_del_b, CHILD_DELETING,
                                                  CHILD_OUTBOUND_NONE);
        assert_child_sa_state(a, data[_i].spi_a, CHILD_INSTALLED,
                                                  CHILD_OUTBOUND_INSTALLED);
-       assert_ipsec_sas_installed(a, 1, 3, 5, 6,
-                                                          data[_i].spi_del_a == 1 ? 2 : 4);
        assert_child_sa_count(a, 3);
 
        /* we don't expect this hook to get called anymore */
@@ -1173,6 +1198,8 @@ START_TEST(test_collision_ke_invalid)
                                                          CHILD_OUTBOUND_REGISTERED);
                assert_child_sa_state(a, data[_i].spi_a, CHILD_INSTALLED,
                                                          CHILD_OUTBOUND_INSTALLED);
+               assert_child_sa_state(a, data[_i].spi_del_a, CHILD_DELETING,
+                                                         CHILD_OUTBOUND_INSTALLED);
        }
        else
        {
@@ -1181,9 +1208,9 @@ START_TEST(test_collision_ke_invalid)
                                                          CHILD_OUTBOUND_INSTALLED);
                assert_child_sa_state(a, data[_i].spi_a, CHILD_INSTALLED,
                                                          CHILD_OUTBOUND_REGISTERED);
+               assert_child_sa_state(a, data[_i].spi_del_a, CHILD_DELETING,
+                                                         CHILD_OUTBOUND_REGISTERED);
        }
-       assert_child_sa_state(a, data[_i].spi_del_a, CHILD_DELETING,
-                                                 CHILD_OUTBOUND_INSTALLED);
        /* CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } --> */
        if (data[_i].spi_del_b == 2)
        {
@@ -1194,6 +1221,8 @@ START_TEST(test_collision_ke_invalid)
                                                          CHILD_OUTBOUND_REGISTERED);
                assert_child_sa_state(b, data[_i].spi_b, CHILD_INSTALLED,
                                                          CHILD_OUTBOUND_INSTALLED);
+               assert_child_sa_state(b, data[_i].spi_del_b, CHILD_DELETING,
+                                                         CHILD_OUTBOUND_INSTALLED);
        }
        else
        {
@@ -1202,9 +1231,10 @@ START_TEST(test_collision_ke_invalid)
                                                          CHILD_OUTBOUND_INSTALLED);
                assert_child_sa_state(b, data[_i].spi_b, CHILD_INSTALLED,
                                                          CHILD_OUTBOUND_REGISTERED);
+               assert_child_sa_state(b, data[_i].spi_del_b, CHILD_DELETING,
+                                                         CHILD_OUTBOUND_REGISTERED);
        }
-       assert_child_sa_state(b, data[_i].spi_del_b, CHILD_DELETING,
-                                                 CHILD_OUTBOUND_INSTALLED);
+
 
        /* we don't expect this hook to get called anymore */
        assert_hook_not_called(child_rekey);
@@ -1212,7 +1242,8 @@ START_TEST(test_collision_ke_invalid)
        assert_jobs_scheduled(1);
        exchange_test_helper->process_message(exchange_test_helper, b, NULL);
        assert_child_sa_state(b, data[_i].spi_del_b, CHILD_DELETING,
-                                                 CHILD_OUTBOUND_INSTALLED);
+                                                 data[_i].spi_del_b == 2 ? CHILD_OUTBOUND_INSTALLED
+                                                                                                 : CHILD_OUTBOUND_REGISTERED);
        assert_child_sa_state(b, data[_i].spi_del_a, CHILD_DELETING,
                                                  CHILD_OUTBOUND_NONE);
        assert_child_sa_state(b, data[_i].spi_b, CHILD_INSTALLED,
@@ -1223,7 +1254,8 @@ START_TEST(test_collision_ke_invalid)
        assert_jobs_scheduled(1);
        exchange_test_helper->process_message(exchange_test_helper, a, NULL);
        assert_child_sa_state(a, data[_i].spi_del_a, CHILD_DELETING,
-                                                 CHILD_OUTBOUND_INSTALLED);
+                                                 data[_i].spi_del_a == 1 ? CHILD_OUTBOUND_INSTALLED
+                                                                                                 : CHILD_OUTBOUND_REGISTERED);
        assert_child_sa_state(a, data[_i].spi_del_b, CHILD_DELETING,
                                                  CHILD_OUTBOUND_NONE);
        assert_child_sa_state(a, data[_i].spi_a, CHILD_INSTALLED,