child-create: Abort initiating a duplicate CHILD_SA

This could happen if an acquire is triggered while we respond to a
CREATE_CHILD_SA request from the peer, or if an acquire is triggered
while an IKE_SA (with its existing CHILD_SAs) is reestablished (also
with break-before-make reauthentication).  Also catches multiple
manual initiations.

Note that this ignores the traffic selectors from acquires (narrowing to
them seems rare in practice anyway).

Duplicates can still get created if e.g. both peers initiate them
concurrently.

diff --git a/src/libcharon/sa/ikev2/tasks/child_create.c b/src/libcharon/sa/ikev2/tasks/child_create.c
index 0fada13c91edee1c460652f223ad740bc90c1bfe..73846081341de336c66149f420179104ed881d31 100644 (file)
--- a/src/libcharon/sa/ikev2/tasks/child_create.c
+++ b/src/libcharon/sa/ikev2/tasks/child_create.c
@@ -1079,6 +1079,67 @@ static status_t defer_child_sa(private_child_create_t *this)
        return NOT_SUPPORTED;
 }
 
+/**
+ * Compare two CHILD_SA objects for equality
+ */
+static bool child_sa_equals(child_sa_t *a, child_sa_t *b)
+{
+       child_cfg_t *cfg = a->get_config(a);
+       return cfg->equals(cfg, b->get_config(b)) &&
+               /* reqids are allocated based on the final TS, so we can only compare
+                * them if they are static (i.e. both have them) */
+               (!a->get_reqid(a) || !b->get_reqid(b) ||
+                 a->get_reqid(a) == b->get_reqid(b)) &&
+               a->get_mark(a, TRUE).value == b->get_mark(b, TRUE).value &&
+               a->get_mark(a, FALSE).value == b->get_mark(b, FALSE).value &&
+               a->get_if_id(a, TRUE) == b->get_if_id(b, TRUE) &&
+               a->get_if_id(a, FALSE) == b->get_if_id(b, FALSE);
+}
+
+/**
+ * Check if there is a duplicate CHILD_SA already established and we can abort
+ * initiating this one
+ */
+static bool check_for_duplicate(private_child_create_t *this)
+{
+       enumerator_t *enumerator;
+       child_sa_t *child_sa, *found = NULL;
+
+       enumerator = this->ike_sa->create_child_sa_enumerator(this->ike_sa);
+       while (enumerator->enumerate(enumerator, (void**)&child_sa))
+       {
+               if (child_sa->get_state(child_sa) == CHILD_INSTALLED &&
+                       child_sa_equals(child_sa, this->child_sa))
+               {
+                       found = child_sa;
+                       break;
+               }
+       }
+       enumerator->destroy(enumerator);
+
+       if (found)
+       {
+               linked_list_t *my_ts, *other_ts;
+
+               my_ts = linked_list_create_from_enumerator(
+                                       found->create_ts_enumerator(found, TRUE));
+               other_ts = linked_list_create_from_enumerator(
+                                       found->create_ts_enumerator(found, FALSE));
+
+               DBG1(DBG_IKE, "not establishing CHILD_SA %s{%d} due to existing "
+                        "duplicate {%d} with SPIs %.8x_i %.8x_o and TS %#R === %#R",
+                        this->child_sa->get_name(this->child_sa),
+                        this->child_sa->get_unique_id(this->child_sa),
+                        found->get_unique_id(found),
+                        ntohl(found->get_spi(found, TRUE)),
+                        ntohl(found->get_spi(found, FALSE)), my_ts, other_ts);
+
+               my_ts->destroy(my_ts);
+               other_ts->destroy(other_ts);
+       }
+       return found;
+}
+
 METHOD(task_t, build_i, status_t,
        private_child_create_t *this, message_t *message)
 {
@@ -1180,6 +1241,16 @@ METHOD(task_t, build_i, status_t,
                                                                         this->ike_sa->get_other_host(this->ike_sa),
                                                                         this->config, &this->child);
 
+       /* check this after creating the object so that its destruction is detected
+        * by controller and trap manager */
+       if (!this->rekey &&
+               message->get_exchange_type(message) == CREATE_CHILD_SA &&
+               check_for_duplicate(this))
+       {
+               message->set_exchange_type(message, EXCHANGE_TYPE_UNDEFINED);
+               return SUCCESS;
+       }
+
        if (this->child.reqid)
        {
                DBG0(DBG_IKE, "establishing CHILD_SA %s{%d} reqid %d",

diff --git a/src/libcharon/tests/suites/test_child_create.c b/src/libcharon/tests/suites/test_child_create.c
index 20a47f6bffece60a542054290fc5d69ef4134529..da0447aaf1f1cfd1466e67a9a28efa4be779c96c 100644 (file)
--- a/src/libcharon/tests/suites/test_child_create.c
+++ b/src/libcharon/tests/suites/test_child_create.c
@@ -21,6 +21,47 @@
 #include <tests/utils/job_asserts.h>
 #include <tests/utils/sa_asserts.h>
 
+/**
+ * The peers try to create a new CHILD_SA that looks exactly the same
+ * as the existing one, so it won't get initiated.
+ */
+START_TEST(test_duplicate)
+{
+       child_cfg_t *child_cfg;
+       child_cfg_create_t child = {
+               .mode = MODE_TUNNEL,
+       };
+       ike_sa_t *a, *b;
+
+       exchange_test_helper->establish_sa(exchange_test_helper,
+                                                                          &a, &b, NULL);
+
+       assert_no_jobs_scheduled();
+       assert_hook_not_called(child_updown);
+       assert_hook_not_called(message);
+       child_cfg = child_cfg_create("child", &child);
+       child_cfg->add_proposal(child_cfg, proposal_create_default(PROTO_ESP));
+       child_cfg->add_traffic_selector(child_cfg, TRUE,
+                                                               traffic_selector_create_dynamic(0, 0, 65535));
+       child_cfg->add_traffic_selector(child_cfg, FALSE,
+                                                               traffic_selector_create_dynamic(0, 0, 65535));
+       child_cfg->get_ref(child_cfg);
+       call_ikesa(a, initiate, child_cfg, 0, NULL, NULL);
+       assert_child_sa_count(a, 1);
+       assert_sa_idle(a);
+
+       call_ikesa(b, initiate, child_cfg, 0, NULL, NULL);
+       assert_child_sa_count(b, 1);
+       assert_sa_idle(b);
+       assert_hook();
+       assert_hook();
+       assert_scheduler();
+
+       call_ikesa(a, destroy);
+       call_ikesa(b, destroy);
+}
+END_TEST
+
 /**
  * One of the peers tries to create a new CHILD_SA while the other concurrently
  * started to rekey the IKE_SA. TEMPORARY_FAILURE should be returned on both
@@ -31,6 +72,8 @@ START_TEST(test_collision_ike_rekey)
        child_cfg_t *child_cfg;
        child_cfg_create_t child = {
                .mode = MODE_TUNNEL,
+               /* make sure this is not a duplicate of the initial CHILD_SA */
+               .mark_out = { .value = 42, .mask = 0xffffffff },
        };
        ike_sa_t *a, *b;
 
@@ -98,6 +141,10 @@ Suite *child_create_suite_create()
 
        s = suite_create("child create");
 
+       tc = tcase_create("initiate duplicate");
+       tcase_add_test(tc, test_duplicate);
+       suite_add_tcase(s, tc);
+
        tc = tcase_create("collisions ike rekey");
        tcase_add_test(tc, test_collision_ike_rekey);
        suite_add_tcase(s, tc);