From a5430e1601fa42147ffb725c2dea3fc5e2b982f1 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Thu, 20 May 2021 18:11:58 +0200 Subject: [PATCH] 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. --- src/libcharon/sa/ikev2/tasks/child_create.c | 71 +++++++++++++++++++ .../tests/suites/test_child_create.c | 47 ++++++++++++ 2 files changed, 118 insertions(+) diff --git a/src/libcharon/sa/ikev2/tasks/child_create.c b/src/libcharon/sa/ikev2/tasks/child_create.c index 399aa1f81c..dd693c7004 100644 --- 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 4f6733a920..07ce07ae90 100644 --- a/src/libcharon/tests/suites/test_child_create.c +++ b/src/libcharon/tests/suites/test_child_create.c @@ -21,6 +21,47 @@ #include #include +/** + * 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); -- 2.47.2