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 399aa1f81c9ddbb8c75e06a077bae2641b9ce577..dd693c7004215213fc2a59c9e3da979ae163b0a5 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 4f6733a92083789c3164f7115a07b7ad62a1c13d..07ce07ae900ea0b32799058993bc733f3324993c 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, NULL);
+       assert_child_sa_count(a, 1);
+       assert_sa_idle(a);
+
+       call_ikesa(b, initiate, child_cfg, 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);