From: Tobias Brunner Date: Wed, 1 Mar 2017 17:02:38 +0000 (+0100) Subject: ikev2: Delay installation of outbound SAs during rekeying on the responder X-Git-Tag: 5.5.3~25^2~12 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=dc3710e987b49c96ee2e81c1979fb2a6133bd30d;p=thirdparty%2Fstrongswan.git 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. --- diff --git a/src/libcharon/sa/ikev2/tasks/child_create.c b/src/libcharon/sa/ikev2/tasks/child_create.c index d4d05c9c1c..db57ee815b 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 6fa8836acf..d69f44b0d9 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 c04ec141fd..afc4644a29 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 fcac49388e..19e5f784a2 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);