From dc3710e987b49c96ee2e81c1979fb2a6133bd30d Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Wed, 1 Mar 2017 18:02:38 +0100 Subject: [PATCH] ikev2: Delay installation of outbound SAs during rekeying on the responder The responder has all the information needed to install both SAs before the initiator does. So if the responder immediately installs the outbound SA it might send packets using the new SA which the initiator is not yet able to process. This can be avoided by delaying the installation of the outbound SA until the replaced SA is deleted. --- src/libcharon/sa/ikev2/tasks/child_create.c | 31 +++++++-- src/libcharon/sa/ikev2/tasks/child_delete.c | 64 ++++++++++++++++++- src/libcharon/sa/ikev2/tasks/child_rekey.c | 7 +- src/libcharon/tests/suites/test_child_rekey.c | 52 +++++++++------ 4 files changed, 124 insertions(+), 30 deletions(-) diff --git a/src/libcharon/sa/ikev2/tasks/child_create.c b/src/libcharon/sa/ikev2/tasks/child_create.c index d4d05c9c1..db57ee815 100644 --- a/src/libcharon/sa/ikev2/tasks/child_create.c +++ b/src/libcharon/sa/ikev2/tasks/child_create.c @@ -682,7 +682,7 @@ static status_t select_and_install(private_child_create_t *this, this->other_spi, this->other_cpi, this->initiator, FALSE, this->tfcv3); } - else + else if (!this->rekey) { status_i = this->child_sa->install(this->child_sa, encr_i, integ_i, this->my_spi, this->my_cpi, this->initiator, @@ -691,6 +691,17 @@ static status_t select_and_install(private_child_create_t *this, 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); + this->child_sa->register_outbound(this->child_sa, encr_r, integ_r, + this->other_spi, this->other_cpi, this->tfcv3); + status_o = SUCCESS; + } } if (status_i != SUCCESS || status_o != SUCCESS) @@ -734,8 +745,14 @@ static status_t select_and_install(private_child_create_t *this, charon->bus->child_keys(charon->bus, this->child_sa, this->initiator, this->dh, nonce_i, nonce_r); - /* add to IKE_SA, and remove from task */ - this->child_sa->set_state(this->child_sa, CHILD_INSTALLED); + if (this->rekey && !this->initiator) + { + this->child_sa->set_state(this->child_sa, CHILD_INSTALLED_INBOUND); + } + else + { + this->child_sa->set_state(this->child_sa, CHILD_INSTALLED); + } this->ike_sa->add_child_sa(this->ike_sa, this->child_sa); this->established = TRUE; @@ -746,16 +763,17 @@ static status_t select_and_install(private_child_create_t *this, other_ts = linked_list_create_from_enumerator( this->child_sa->create_ts_enumerator(this->child_sa, FALSE)); - DBG0(DBG_IKE, "CHILD_SA %s{%d} established " + DBG0(DBG_IKE, "%sCHILD_SA %s{%d} established " "with SPIs %.8x_i %.8x_o and TS %#R === %#R", + this->rekey && !this->initiator ? "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)), - ntohl(this->child_sa->get_spi(this->child_sa, FALSE)), my_ts, other_ts); + ntohl(this->child_sa->get_spi(this->child_sa, FALSE)), + my_ts, other_ts); my_ts->destroy(my_ts); other_ts->destroy(other_ts); - return SUCCESS; } @@ -1688,7 +1706,6 @@ METHOD(task_t, destroy, void, { this->proposals->destroy_offset(this->proposals, offsetof(proposal_t, destroy)); } - DESTROY_IF(this->config); DESTROY_IF(this->nonceg); free(this); diff --git a/src/libcharon/sa/ikev2/tasks/child_delete.c b/src/libcharon/sa/ikev2/tasks/child_delete.c index 6fa8836ac..d69f44b0d 100644 --- a/src/libcharon/sa/ikev2/tasks/child_delete.c +++ b/src/libcharon/sa/ikev2/tasks/child_delete.c @@ -146,6 +146,57 @@ static bool is_redundant(private_child_delete_t *this, child_sa_t *child) return FALSE; } +/** + * Install the outbound CHILD_SA with the given SPI + */ +static void install_outbound(private_child_delete_t *this, + protocol_id_t protocol, uint32_t spi) +{ + child_sa_t *child_sa; + linked_list_t *my_ts, *other_ts; + status_t status; + + child_sa = this->ike_sa->get_child_sa(this->ike_sa, protocol, + spi, FALSE); + if (!child_sa) + { + DBG1(DBG_IKE, "CHILD_SA not found after rekeying"); + return; + } + if (this->initiator && is_redundant(this, child_sa)) + { /* if we won the rekey collision we don't want to install the + * redundant SA created by the peer */ + return; + } + + status = child_sa->install_outbound(child_sa); + if (status != SUCCESS) + { + DBG1(DBG_IKE, "unable to install outbound IPsec SA (SAD) in kernel"); + charon->bus->alert(charon->bus, ALERT_INSTALL_CHILD_SA_FAILED, + child_sa); + /* 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)); + other_ts = linked_list_create_from_enumerator( + child_sa->create_ts_enumerator(child_sa, FALSE)); + + DBG0(DBG_IKE, "outbound CHILD_SA %s{%d} established " + "with SPIs %.8x_i %.8x_o and TS %#R === %#R", + child_sa->get_name(child_sa), + child_sa->get_unique_id(child_sa), + ntohl(child_sa->get_spi(child_sa, TRUE)), + ntohl(child_sa->get_spi(child_sa, FALSE)), + my_ts, other_ts); + + my_ts->destroy(my_ts); + other_ts->destroy(other_ts); +} + /** * read in payloads and find the children to delete */ @@ -197,6 +248,7 @@ static void process_payloads(private_child_delete_t *this, message_t *message) /* fall through */ case CHILD_REKEYING: /* we reply as usual, rekeying will fail */ + case CHILD_INSTALLED_INBOUND: case CHILD_INSTALLED: if (!this->initiator) { @@ -234,7 +286,7 @@ static status_t destroy_and_reestablish(private_child_delete_t *this) child_sa_t *child_sa; child_cfg_t *child_cfg; protocol_id_t protocol; - uint32_t spi, reqid; + uint32_t spi, reqid, rekey_spi; action_t action; status_t status = SUCCESS; @@ -242,13 +294,21 @@ static status_t destroy_and_reestablish(private_child_delete_t *this) while (enumerator->enumerate(enumerator, (void**)&child_sa)) { /* signal child down event if we weren't rekeying */ + protocol = child_sa->get_protocol(child_sa); if (!this->rekeyed) { charon->bus->child_updown(charon->bus, child_sa, FALSE); } + else + { + rekey_spi = child_sa->get_rekey_spi(child_sa); + if (rekey_spi) + { + install_outbound(this, protocol, rekey_spi); + } + } spi = child_sa->get_spi(child_sa, TRUE); reqid = child_sa->get_reqid(child_sa); - protocol = child_sa->get_protocol(child_sa); child_cfg = child_sa->get_config(child_sa); child_cfg->get_ref(child_cfg); action = child_sa->get_close_action(child_sa); diff --git a/src/libcharon/sa/ikev2/tasks/child_rekey.c b/src/libcharon/sa/ikev2/tasks/child_rekey.c index c04ec141f..afc4644a2 100644 --- a/src/libcharon/sa/ikev2/tasks/child_rekey.c +++ b/src/libcharon/sa/ikev2/tasks/child_rekey.c @@ -227,6 +227,7 @@ METHOD(task_t, build_r, status_t, child_cfg_t *config; uint32_t reqid; child_sa_state_t state; + child_sa_t *child_sa; if (!this->child_sa) { @@ -260,7 +261,10 @@ METHOD(task_t, build_r, status_t, return SUCCESS; } + child_sa = this->child_create->get_child(this->child_create); this->child_sa->set_state(this->child_sa, CHILD_REKEYED); + this->child_sa->set_rekey_spi(this->child_sa, + child_sa->get_spi(child_sa, FALSE)); /* invoke rekey hook */ charon->bus->child_rekey(charon->bus, this->child_sa, @@ -472,7 +476,8 @@ METHOD(child_rekey_t, collide, void, /* ignore passive tasks that did not successfully create a CHILD_SA */ other_child = rekey->child_create->get_child(rekey->child_create); if (!other_child || - other_child->get_state(other_child) != CHILD_INSTALLED) + (other_child->get_state(other_child) != CHILD_INSTALLED && + other_child->get_state(other_child) != CHILD_INSTALLED_INBOUND)) { other->destroy(other); return; diff --git a/src/libcharon/tests/suites/test_child_rekey.c b/src/libcharon/tests/suites/test_child_rekey.c index fcac49388..19e5f784a 100644 --- a/src/libcharon/tests/suites/test_child_rekey.c +++ b/src/libcharon/tests/suites/test_child_rekey.c @@ -62,7 +62,7 @@ START_TEST(test_regular) assert_notify(IN, REKEY_SA); exchange_test_helper->process_message(exchange_test_helper, b, NULL); assert_child_sa_state(b, spi_b, CHILD_REKEYED); - assert_child_sa_state(b, 4, CHILD_INSTALLED); + assert_child_sa_state(b, 4, CHILD_INSTALLED_INBOUND); assert_hook(); /* <-- CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } */ @@ -150,7 +150,7 @@ START_TEST(test_regular_ke_invalid) assert_notify(IN, REKEY_SA); exchange_test_helper->process_message(exchange_test_helper, b, NULL); assert_child_sa_state(b, spi_b, CHILD_REKEYED); - assert_child_sa_state(b, 6, CHILD_INSTALLED); + assert_child_sa_state(b, 6, CHILD_INSTALLED_INBOUND); assert_hook(); /* <-- CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } */ @@ -204,7 +204,7 @@ START_TEST(test_regular_responder_ignore_soft_expire) assert_notify(IN, REKEY_SA); exchange_test_helper->process_message(exchange_test_helper, b, NULL); assert_child_sa_state(b, 2, CHILD_REKEYED); - assert_child_sa_state(b, 4, CHILD_INSTALLED); + assert_child_sa_state(b, 4, CHILD_INSTALLED_INBOUND); assert_hook(); /* <-- CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } */ @@ -263,7 +263,7 @@ START_TEST(test_regular_responder_handle_hard_expire) assert_notify(IN, REKEY_SA); exchange_test_helper->process_message(exchange_test_helper, b, NULL); assert_child_sa_state(b, 2, CHILD_REKEYED); - assert_child_sa_state(b, 4, CHILD_INSTALLED); + assert_child_sa_state(b, 4, CHILD_INSTALLED_INBOUND); assert_hook(); /* <-- CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } */ @@ -284,7 +284,7 @@ START_TEST(test_regular_responder_handle_hard_expire) /* INFORMATIONAL { D } --> */ assert_single_payload(IN, PLV2_DELETE); exchange_test_helper->process_message(exchange_test_helper, b, NULL); - assert_child_sa_state(b, 4, CHILD_INSTALLED); + assert_child_sa_state(b, 4, CHILD_INSTALLED_INBOUND); assert_child_sa_state(a, 2, CHILD_DELETING); /* <-- INFORMATIONAL { D } */ assert_single_payload(IN, PLV2_DELETE); @@ -361,14 +361,14 @@ START_TEST(test_collision) assert_hook_rekey(child_rekey, 2, 5); exchange_test_helper->process_message(exchange_test_helper, b, NULL); assert_child_sa_state(b, 2, CHILD_REKEYED); - assert_child_sa_state(b, 5, CHILD_INSTALLED); + assert_child_sa_state(b, 5, CHILD_INSTALLED_INBOUND); assert_hook(); /* <-- CREATE_CHILD_SA { N(REKEY_SA), SA, Ni, [KEi,] TSi, TSr } */ exchange_test_helper->nonce_first_byte = data[_i].nonces[3]; assert_hook_rekey(child_rekey, 1, 6); exchange_test_helper->process_message(exchange_test_helper, a, NULL); assert_child_sa_state(a, 1, CHILD_REKEYED); - assert_child_sa_state(a, 6, CHILD_INSTALLED); + assert_child_sa_state(a, 6, CHILD_INSTALLED_INBOUND); assert_hook(); /* <-- CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } */ @@ -387,7 +387,9 @@ START_TEST(test_collision) } assert_child_sa_state(a, data[_i].spi_del_a, CHILD_DELETING); assert_child_sa_state(a, data[_i].spi_del_b, CHILD_REKEYED); - assert_child_sa_state(a, data[_i].spi_a, CHILD_INSTALLED); + assert_child_sa_state(a, data[_i].spi_a, + data[_i].spi_del_a == 1 ? CHILD_INSTALLED + : CHILD_INSTALLED_INBOUND); /* CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } --> */ if (data[_i].spi_del_b == 2) { @@ -403,7 +405,9 @@ START_TEST(test_collision) } assert_child_sa_state(b, data[_i].spi_del_b, CHILD_DELETING); assert_child_sa_state(b, data[_i].spi_del_a, CHILD_REKEYED); - assert_child_sa_state(b, data[_i].spi_b, CHILD_INSTALLED); + assert_child_sa_state(b, data[_i].spi_b, + data[_i].spi_del_b == 2 ? CHILD_INSTALLED + : CHILD_INSTALLED_INBOUND); /* we don't expect this hook to get called anymore */ assert_hook_not_called(child_rekey); @@ -494,14 +498,14 @@ START_TEST(test_collision_delayed_response) assert_hook_rekey(child_rekey, 2, 5); exchange_test_helper->process_message(exchange_test_helper, b, NULL); assert_child_sa_state(b, 2, CHILD_REKEYED); - assert_child_sa_state(b, 5, CHILD_INSTALLED); + assert_child_sa_state(b, 5, CHILD_INSTALLED_INBOUND); assert_hook(); /* <-- CREATE_CHILD_SA { N(REKEY_SA), SA, Ni, [KEi,] TSi, TSr } */ exchange_test_helper->nonce_first_byte = data[_i].nonces[3]; assert_hook_rekey(child_rekey, 1, 6); exchange_test_helper->process_message(exchange_test_helper, a, NULL); assert_child_sa_state(a, 1, CHILD_REKEYED); - assert_child_sa_state(a, 6, CHILD_INSTALLED); + assert_child_sa_state(a, 6, CHILD_INSTALLED_INBOUND); assert_hook(); /* delay the CREATE_CHILD_SA response from b to a */ @@ -522,7 +526,9 @@ START_TEST(test_collision_delayed_response) } assert_child_sa_state(b, data[_i].spi_del_b, CHILD_DELETING); assert_child_sa_state(b, data[_i].spi_del_a, CHILD_REKEYED); - assert_child_sa_state(b, data[_i].spi_b, CHILD_INSTALLED); + assert_child_sa_state(b, data[_i].spi_b, + data[_i].spi_del_b == 2 ? CHILD_INSTALLED + : CHILD_INSTALLED_INBOUND); /* <-- INFORMATIONAL { D } */ assert_hook_not_called(child_rekey); @@ -540,7 +546,9 @@ START_TEST(test_collision_delayed_response) /* INFORMATIONAL { D } --> */ exchange_test_helper->process_message(exchange_test_helper, b, NULL); assert_child_sa_state(b, data[_i].spi_del_a, CHILD_REKEYED); - assert_child_sa_state(b, data[_i].spi_b, CHILD_INSTALLED); + assert_child_sa_state(b, data[_i].spi_b, + data[_i].spi_del_b == 2 ? CHILD_INSTALLED + : CHILD_INSTALLED_INBOUND); assert_child_sa_count(b, 2); assert_hook(); @@ -635,7 +643,7 @@ START_TEST(test_collision_delayed_request) assert_hook_rekey(child_rekey, 1, 5); exchange_test_helper->process_message(exchange_test_helper, a, NULL); assert_child_sa_state(a, 1, CHILD_REKEYED); - assert_child_sa_state(a, 5, CHILD_INSTALLED); + assert_child_sa_state(a, 5, CHILD_INSTALLED_INBOUND); assert_hook(); /* CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } --> */ assert_hook_rekey(child_rekey, 2, 4); @@ -736,7 +744,7 @@ START_TEST(test_collision_delayed_request_more) assert_hook_rekey(child_rekey, 1, 5); exchange_test_helper->process_message(exchange_test_helper, a, NULL); assert_child_sa_state(a, 1, CHILD_REKEYED); - assert_child_sa_state(a, 5, CHILD_INSTALLED); + assert_child_sa_state(a, 5, CHILD_INSTALLED_INBOUND); assert_hook(); /* CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } --> */ assert_hook_rekey(child_rekey, 2, 4); @@ -874,14 +882,14 @@ START_TEST(test_collision_ke_invalid) assert_hook_rekey(child_rekey, 2, 9); exchange_test_helper->process_message(exchange_test_helper, b, NULL); assert_child_sa_state(b, 2, CHILD_REKEYED); - assert_child_sa_state(b, 9, CHILD_INSTALLED); + assert_child_sa_state(b, 9, CHILD_INSTALLED_INBOUND); assert_hook(); /* <-- CREATE_CHILD_SA { N(REKEY_SA), SA, Ni, [KEi,] TSi, TSr } */ exchange_test_helper->nonce_first_byte = data[_i].nonces[3]; assert_hook_rekey(child_rekey, 1, 10); exchange_test_helper->process_message(exchange_test_helper, a, NULL); assert_child_sa_state(a, 1, CHILD_REKEYED); - assert_child_sa_state(a,10, CHILD_INSTALLED); + assert_child_sa_state(a,10, CHILD_INSTALLED_INBOUND); assert_hook(); /* <-- CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } */ @@ -898,7 +906,9 @@ START_TEST(test_collision_ke_invalid) } assert_child_sa_state(a, data[_i].spi_del_a, CHILD_DELETING); assert_child_sa_state(a, data[_i].spi_del_b, CHILD_REKEYED); - assert_child_sa_state(a, data[_i].spi_a, CHILD_INSTALLED); + assert_child_sa_state(a, data[_i].spi_a, + data[_i].spi_del_a == 1 ? CHILD_INSTALLED + : CHILD_INSTALLED_INBOUND); /* CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } --> */ if (data[_i].spi_del_b == 2) { @@ -912,7 +922,9 @@ START_TEST(test_collision_ke_invalid) } assert_child_sa_state(b, data[_i].spi_del_b, CHILD_DELETING); assert_child_sa_state(b, data[_i].spi_del_a, CHILD_REKEYED); - assert_child_sa_state(b, data[_i].spi_b, CHILD_INSTALLED); + assert_child_sa_state(b, data[_i].spi_b, + data[_i].spi_del_b == 2 ? CHILD_INSTALLED + : CHILD_INSTALLED_INBOUND); /* we don't expect this hook to get called anymore */ assert_hook_not_called(child_rekey); @@ -1039,7 +1051,7 @@ START_TEST(test_collision_ke_invalid_delayed_retry) assert_hook_rekey(child_rekey, 1, 9); exchange_test_helper->process_message(exchange_test_helper, a, NULL); assert_child_sa_state(a, 1, CHILD_REKEYED); - assert_child_sa_state(a, 9, CHILD_INSTALLED); + assert_child_sa_state(a, 9, CHILD_INSTALLED_INBOUND); assert_hook(); /* CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } --> */ assert_hook_rekey(child_rekey, 2, 8);