From: Tobias Brunner Date: Thu, 20 May 2021 16:11:58 +0000 (+0200) Subject: child-create: Abort initiating a duplicate CHILD_SA X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=refs%2Fheads%2Fchild-sa-dedup;p=thirdparty%2Fstrongswan.git 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 0fada13c91..7384608134 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 20a47f6bff..da0447aaf1 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, 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);