diff --git a/src/libcharon/sa/ikev2/tasks/child_create.c b/src/libcharon/sa/ikev2/tasks/child_create.c index 4d4d72e0b..5161ca1ca 100644 --- a/src/libcharon/sa/ikev2/tasks/child_create.c +++ b/src/libcharon/sa/ikev2/tasks/child_create.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2008-2017 Tobias Brunner + * Copyright (C) 2008-2018 Tobias Brunner * Copyright (C) 2005-2008 Martin Willi * Copyright (C) 2005 Jan Hutter * HSR Hochschule fuer Technik Rapperswil @@ -277,12 +277,13 @@ static bool ts_list_is_host(linked_list_t *list, host_t *host) } /** - * Allocate SPIs and update proposals + * Allocate SPIs and update proposals, we also promote the selected DH group */ static bool allocate_spi(private_child_create_t *this) { enumerator_t *enumerator; proposal_t *proposal; + linked_list_t *other_dh_groups; if (this->initiator) { @@ -304,12 +305,29 @@ static bool allocate_spi(private_child_create_t *this) { if (this->initiator) { + other_dh_groups = linked_list_create(); enumerator = this->proposals->create_enumerator(this->proposals); while (enumerator->enumerate(enumerator, &proposal)) { proposal->set_spi(proposal, this->my_spi); + + /* move the selected DH group to the front, if any */ + if (this->dh_group != MODP_NONE && + !proposal->promote_dh_group(proposal, this->dh_group)) + { /* proposals that don't contain the selected group are + * moved to the back */ + this->proposals->remove_at(this->proposals, enumerator); + other_dh_groups->insert_last(other_dh_groups, proposal); + } } enumerator->destroy(enumerator); + enumerator = other_dh_groups->create_enumerator(other_dh_groups); + while (enumerator->enumerate(enumerator, (void**)&proposal)) + { /* no need to remove from the list as we destroy it anyway*/ + this->proposals->insert_last(this->proposals, proposal); + } + enumerator->destroy(enumerator); + other_dh_groups->destroy(other_dh_groups); } else { @@ -1006,8 +1024,8 @@ METHOD(task_t, build_i, status_t, chunk_empty); return SUCCESS; } - if (!this->retry) - { + if (!this->retry && this->dh_group == MODP_NONE) + { /* during a rekeying the group might already be set */ this->dh_group = this->config->get_dh_group(this->config); } break; @@ -1615,6 +1633,12 @@ METHOD(child_create_t, use_marks, void, this->mark_out = out; } +METHOD(child_create_t, use_dh_group, void, + private_child_create_t *this, diffie_hellman_group_t dh_group) +{ + this->dh_group = dh_group; +} + METHOD(child_create_t, get_child, child_sa_t*, private_child_create_t *this) { @@ -1736,6 +1760,7 @@ child_create_t *child_create_create(ike_sa_t *ike_sa, .get_lower_nonce = _get_lower_nonce, .use_reqid = _use_reqid, .use_marks = _use_marks, + .use_dh_group = _use_dh_group, .task = { .get_type = _get_type, .migrate = _migrate, diff --git a/src/libcharon/sa/ikev2/tasks/child_create.h b/src/libcharon/sa/ikev2/tasks/child_create.h index f48d7b0a9..59fc6d2d9 100644 --- a/src/libcharon/sa/ikev2/tasks/child_create.h +++ b/src/libcharon/sa/ikev2/tasks/child_create.h @@ -1,6 +1,7 @@ /* + * Copyright (C) 2018 Tobias Brunner * Copyright (C) 2007 Martin Willi - * Hochschule fuer Technik Rapperswil + * HSR Hochschule fuer Technik Rapperswil * * This program is free software; you can redistribute it and/or modify it * under the terms of the GNU General Public License as published by the @@ -59,6 +60,15 @@ struct child_create_t { */ void (*use_marks)(child_create_t *this, u_int in, u_int out); + /** + * Initially propose a specific DH group to override configuration. + * + * This is used during rekeying to prefer the previously negotiated group. + * + * @param dh_group DH group to use + */ + void (*use_dh_group)(child_create_t *this, diffie_hellman_group_t dh_group); + /** * Get the lower of the two nonces, used for rekey collisions. * diff --git a/src/libcharon/sa/ikev2/tasks/child_rekey.c b/src/libcharon/sa/ikev2/tasks/child_rekey.c index b67e9b80f..f90056658 100644 --- a/src/libcharon/sa/ikev2/tasks/child_rekey.c +++ b/src/libcharon/sa/ikev2/tasks/child_rekey.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2017 Tobias Brunner + * Copyright (C) 2009-2018 Tobias Brunner * Copyright (C) 2005-2007 Martin Willi * Copyright (C) 2005 Jan Hutter * HSR Hochschule fuer Technik Rapperswil @@ -190,8 +190,18 @@ METHOD(task_t, build_i, status_t, /* our CHILD_CREATE task does the hard work for us */ if (!this->child_create) { + proposal_t *proposal; + uint16_t dh_group; + this->child_create = child_create_create(this->ike_sa, config->get_ref(config), TRUE, NULL, NULL); + + proposal = this->child_sa->get_proposal(this->child_sa); + if (proposal->get_algorithm(proposal, DIFFIE_HELLMAN_GROUP, + &dh_group, NULL)) + { /* reuse the DH group negotiated previously */ + this->child_create->use_dh_group(this->child_create, dh_group); + } } reqid = this->child_sa->get_reqid(this->child_sa); this->child_create->use_reqid(this->child_create, reqid); diff --git a/src/libcharon/sa/ikev2/tasks/ike_init.c b/src/libcharon/sa/ikev2/tasks/ike_init.c index d75d21715..3d73d728b 100644 --- a/src/libcharon/sa/ikev2/tasks/ike_init.c +++ b/src/libcharon/sa/ikev2/tasks/ike_init.c @@ -1,8 +1,8 @@ /* - * Copyright (C) 2008-2015 Tobias Brunner + * Copyright (C) 2008-2018 Tobias Brunner * Copyright (C) 2005-2008 Martin Willi * Copyright (C) 2005 Jan Hutter - * Hochschule fuer Technik Rapperswil + * HSR Hochschule fuer Technik Rapperswil * * This program is free software; you can redistribute it and/or modify it * under the terms of the GNU General Public License as published by the @@ -282,7 +282,7 @@ static bool build_payloads(private_ike_init_t *this, message_t *message) sa_payload_t *sa_payload; ke_payload_t *ke_payload; nonce_payload_t *nonce_payload; - linked_list_t *proposal_list; + linked_list_t *proposal_list, *other_dh_groups; ike_sa_id_t *id; proposal_t *proposal; enumerator_t *enumerator; @@ -294,16 +294,31 @@ static bool build_payloads(private_ike_init_t *this, message_t *message) if (this->initiator) { proposal_list = this->config->get_proposals(this->config); - if (this->old_sa) + other_dh_groups = linked_list_create(); + enumerator = proposal_list->create_enumerator(proposal_list); + while (enumerator->enumerate(enumerator, (void**)&proposal)) { /* include SPI of new IKE_SA when we are rekeying */ - enumerator = proposal_list->create_enumerator(proposal_list); - while (enumerator->enumerate(enumerator, (void**)&proposal)) + if (this->old_sa) { proposal->set_spi(proposal, id->get_initiator_spi(id)); } - enumerator->destroy(enumerator); + /* move the selected DH group to the front of the proposal */ + if (!proposal->promote_dh_group(proposal, this->dh_group)) + { /* the proposal does not include the group, move to the back */ + proposal_list->remove_at(proposal_list, enumerator); + other_dh_groups->insert_last(other_dh_groups, proposal); + } } + enumerator->destroy(enumerator); + /* add proposals that don't contain the selected group */ + enumerator = other_dh_groups->create_enumerator(other_dh_groups); + while (enumerator->enumerate(enumerator, (void**)&proposal)) + { /* no need to remove from the list as we destroy it anyway*/ + proposal_list->insert_last(proposal_list, proposal); + } + enumerator->destroy(enumerator); + other_dh_groups->destroy(other_dh_groups); sa_payload = sa_payload_create_from_proposals_v2(proposal_list); proposal_list->destroy_offset(proposal_list, offsetof(proposal_t, destroy)); @@ -531,10 +546,30 @@ METHOD(task_t, build_i, status_t, return FAILED; } - /* if the DH group is set via use_dh_group(), we already have a DH object */ + /* if we are retrying after an INVALID_KE_PAYLOAD we already have one */ if (!this->dh) { - this->dh_group = this->config->get_dh_group(this->config); + if (this->old_sa && lib->settings->get_bool(lib->settings, + "%s.prefer_previous_dh_group", TRUE, lib->ns)) + { /* reuse the DH group we used for the old IKE_SA when rekeying */ + proposal_t *proposal; + uint16_t dh_group; + + proposal = this->old_sa->get_proposal(this->old_sa); + if (proposal->get_algorithm(proposal, DIFFIE_HELLMAN_GROUP, + &dh_group, NULL)) + { + this->dh_group = dh_group; + } + else + { /* this shouldn't happen, but let's be safe */ + this->dh_group = this->config->get_dh_group(this->config); + } + } + else + { + this->dh_group = this->config->get_dh_group(this->config); + } this->dh = this->keymat->keymat.create_dh(&this->keymat->keymat, this->dh_group); if (!this->dh) @@ -544,6 +579,18 @@ METHOD(task_t, build_i, status_t, return FAILED; } } + else if (this->dh->get_dh_group(this->dh) != this->dh_group) + { /* reset DH instance if group changed (INVALID_KE_PAYLOAD) */ + this->dh->destroy(this->dh); + this->dh = this->keymat->keymat.create_dh(&this->keymat->keymat, + this->dh_group); + if (!this->dh) + { + DBG1(DBG_IKE, "requested DH group %N not supported", + diffie_hellman_group_names, this->dh_group); + return FAILED; + } + } /* generate nonce only when we are trying the first time */ if (this->my_nonce.ptr == NULL) @@ -929,12 +976,6 @@ METHOD(task_t, migrate, void, this->keymat = (keymat_v2_t*)ike_sa->get_keymat(ike_sa); this->proposal = NULL; this->dh_failed = FALSE; - if (this->dh && this->dh->get_dh_group(this->dh) != this->dh_group) - { /* reset DH value only if group changed (INVALID_KE_PAYLOAD) */ - this->dh->destroy(this->dh); - this->dh = this->keymat->keymat.create_dh(&this->keymat->keymat, - this->dh_group); - } } METHOD(task_t, destroy, void, diff --git a/src/libcharon/tests/suites/test_child_rekey.c b/src/libcharon/tests/suites/test_child_rekey.c index ac169723f..44d004ab7 100644 --- a/src/libcharon/tests/suites/test_child_rekey.c +++ b/src/libcharon/tests/suites/test_child_rekey.c @@ -231,6 +231,61 @@ START_TEST(test_regular_ke_invalid) /* child_updown */ assert_hook(); + /* because the DH group should get reused another rekeying should complete + * without additional exchange */ + initiate_rekey(a, 5); + /* this should never get called as this results in a successful rekeying */ + assert_hook_not_called(child_updown); + + /* CREATE_CHILD_SA { N(REKEY_SA), SA, Ni, [KEi,] TSi, TSr } --> */ + assert_hook_called(child_rekey); + assert_notify(IN, REKEY_SA); + exchange_test_helper->process_message(exchange_test_helper, b, NULL); + assert_child_sa_state(b, 6, CHILD_REKEYED, CHILD_OUTBOUND_INSTALLED); + assert_child_sa_state(b, 8, CHILD_INSTALLED, CHILD_OUTBOUND_REGISTERED); + assert_ipsec_sas_installed(b, 5, 6, 8); + assert_hook(); + + /* <-- CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } */ + assert_hook_called(child_rekey); + assert_no_notify(IN, REKEY_SA); + exchange_test_helper->process_message(exchange_test_helper, a, NULL); + assert_child_sa_state(a, 5, CHILD_DELETING, CHILD_OUTBOUND_INSTALLED); + assert_child_sa_state(a, 7, CHILD_INSTALLED, CHILD_OUTBOUND_INSTALLED); + assert_ipsec_sas_installed(a, 5, 6, 7, 8); + assert_hook(); + + /* INFORMATIONAL { D } --> */ + assert_hook_not_called(child_rekey); + assert_single_payload(IN, PLV2_DELETE); + exchange_test_helper->process_message(exchange_test_helper, b, NULL); + assert_child_sa_state(b, 6, CHILD_DELETING, CHILD_OUTBOUND_NONE); + assert_child_sa_state(b, 8, CHILD_INSTALLED, CHILD_OUTBOUND_INSTALLED); + assert_child_sa_count(b, 2); + assert_ipsec_sas_installed(b, 6, 7, 8); + assert_hook(); + + /* <-- INFORMATIONAL { D } */ + assert_hook_not_called(child_rekey); + assert_single_payload(IN, PLV2_DELETE); + exchange_test_helper->process_message(exchange_test_helper, a, NULL); + assert_child_sa_state(a, 5, CHILD_DELETING, CHILD_OUTBOUND_NONE); + assert_child_sa_state(a, 7, CHILD_INSTALLED); + assert_child_sa_count(a, 2); + assert_ipsec_sas_installed(a, 5, 7, 8); + assert_hook(); + + /* simulate the execution of the scheduled jobs */ + destroy_rekeyed(a, 5); + assert_child_sa_count(a, 1); + assert_ipsec_sas_installed(a, 7, 8); + destroy_rekeyed(b, 6); + assert_child_sa_count(b, 1); + assert_ipsec_sas_installed(b, 7, 8); + + /* child_updown */ + assert_hook(); + call_ikesa(a, destroy); call_ikesa(b, destroy); } diff --git a/src/libcharon/tests/suites/test_ike_rekey.c b/src/libcharon/tests/suites/test_ike_rekey.c index ba39657a4..e22a0c288 100644 --- a/src/libcharon/tests/suites/test_ike_rekey.c +++ b/src/libcharon/tests/suites/test_ike_rekey.c @@ -138,6 +138,8 @@ START_TEST(test_regular_ke_invalid) lib->settings->set_bool(lib->settings, "%s.prefer_configured_proposals", TRUE, lib->ns); + lib->settings->set_bool(lib->settings, "%s.prefer_previous_dh_group", + FALSE, lib->ns); initiate_rekey(a); @@ -382,6 +384,8 @@ START_TEST(test_collision_ke_invalid) lib->settings->set_bool(lib->settings, "%s.prefer_configured_proposals", TRUE, lib->ns); + lib->settings->set_bool(lib->settings, "%s.prefer_previous_dh_group", + FALSE, lib->ns); /* Six nonces and SPIs are needed (SPI 1 and 2 are used for the initial * IKE_SA): @@ -591,6 +595,8 @@ START_TEST(test_collision_ke_invalid_delayed_retry) lib->settings->set_bool(lib->settings, "%s.prefer_configured_proposals", TRUE, lib->ns); + lib->settings->set_bool(lib->settings, "%s.prefer_previous_dh_group", + FALSE, lib->ns); /* Five nonces and SPIs are needed (SPI 1 and 2 are used for the initial * IKE_SA): diff --git a/src/libstrongswan/crypto/proposal/proposal.c b/src/libstrongswan/crypto/proposal/proposal.c index 221375f7b..7d857bff0 100644 --- a/src/libstrongswan/crypto/proposal/proposal.c +++ b/src/libstrongswan/crypto/proposal/proposal.c @@ -1,8 +1,8 @@ /* - * Copyright (C) 2008-2016 Tobias Brunner + * Copyright (C) 2008-2018 Tobias Brunner * Copyright (C) 2006-2010 Martin Willi * Copyright (C) 2013-2015 Andreas Steffen - * Hochschule fuer Technik Rapperswil + * HSR Hochschule fuer Technik Rapperswil * * This program is free software; you can redistribute it and/or modify it * under the terms of the GNU General Public License as published by the @@ -171,6 +171,36 @@ METHOD(proposal_t, has_dh_group, bool, return found; } +METHOD(proposal_t, promote_dh_group, bool, + private_proposal_t *this, diffie_hellman_group_t group) +{ + enumerator_t *enumerator; + entry_t *entry; + bool found = FALSE; + + enumerator = array_create_enumerator(this->transforms); + while (enumerator->enumerate(enumerator, &entry)) + { + if (entry->type == DIFFIE_HELLMAN_GROUP && + entry->alg == group) + { + array_remove_at(this->transforms, enumerator); + found = TRUE; + } + } + enumerator->destroy(enumerator); + + if (found) + { + entry_t entry = { + .type = DIFFIE_HELLMAN_GROUP, + .alg = group, + }; + array_insert(this->transforms, ARRAY_HEAD, &entry); + } + return found; +} + METHOD(proposal_t, strip_dh, void, private_proposal_t *this, diffie_hellman_group_t keep) { @@ -716,6 +746,7 @@ proposal_t *proposal_create(protocol_id_t protocol, u_int number) .create_enumerator = _create_enumerator, .get_algorithm = _get_algorithm, .has_dh_group = _has_dh_group, + .promote_dh_group = _promote_dh_group, .strip_dh = _strip_dh, .select = _select_proposal, .get_protocol = _get_protocol, diff --git a/src/libstrongswan/crypto/proposal/proposal.h b/src/libstrongswan/crypto/proposal/proposal.h index d9a2af79f..0052674b9 100644 --- a/src/libstrongswan/crypto/proposal/proposal.h +++ b/src/libstrongswan/crypto/proposal/proposal.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2016 Tobias Brunner + * Copyright (C) 2009-2018 Tobias Brunner * Copyright (C) 2006 Martin Willi * HSR Hochschule fuer Technik Rapperswil * @@ -108,7 +108,16 @@ struct proposal_t { * @param group group to check for * @return TRUE if algorithm included */ - bool (*has_dh_group) (proposal_t *this, diffie_hellman_group_t group); + bool (*has_dh_group)(proposal_t *this, diffie_hellman_group_t group); + + /** + * Move the given DH group to the front of the list if it was contained in + * the proposal. + * + * @param group group to promote + * @return TRUE if algorithm included + */ + bool (*promote_dh_group)(proposal_t *this, diffie_hellman_group_t group); /** * Strip DH groups from proposal to use it without PFS. diff --git a/src/libstrongswan/tests/suites/test_proposal.c b/src/libstrongswan/tests/suites/test_proposal.c index 91766266d..1a2f97d5f 100644 --- a/src/libstrongswan/tests/suites/test_proposal.c +++ b/src/libstrongswan/tests/suites/test_proposal.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2016 Tobias Brunner + * Copyright (C) 2016-2018 Tobias Brunner * HSR Hochschule fuer Technik Rapperswil * * This program is free software; you can redistribute it and/or modify it @@ -57,21 +57,27 @@ static struct { { PROTO_AH, "sha256-esn-noesn", "AH:HMAC_SHA2_256_128/EXT_SEQ/NO_EXT_SEQ" }, }; -START_TEST(test_create_from_string) +static void assert_proposal_eq(proposal_t *proposal, char *expected) { - proposal_t *proposal; char str[BUF_LEN]; - proposal = proposal_create_from_string(create_data[_i].proto, - create_data[_i].proposal); - if (!create_data[_i].expected) + if (!expected) { ck_assert(!proposal); return; } snprintf(str, sizeof(str), "%P", proposal); - ck_assert_str_eq(create_data[_i].expected, str); - proposal->destroy(proposal); + ck_assert_str_eq(expected, str); +} + +START_TEST(test_create_from_string) +{ + proposal_t *proposal; + + proposal = proposal_create_from_string(create_data[_i].proto, + create_data[_i].proposal); + assert_proposal_eq(proposal, create_data[_i].expected); + DESTROY_IF(proposal); } END_TEST @@ -151,6 +157,43 @@ START_TEST(test_select_spi) } END_TEST +START_TEST(test_promote_dh_group) +{ + proposal_t *proposal; + + proposal = proposal_create_from_string(PROTO_IKE, + "aes128-sha256-modp3072-ecp256"); + ck_assert(proposal->promote_dh_group(proposal, ECP_256_BIT)); + assert_proposal_eq(proposal, "IKE:AES_CBC_128/HMAC_SHA2_256_128/PRF_HMAC_SHA2_256/ECP_256/MODP_3072"); + proposal->destroy(proposal); +} +END_TEST + +START_TEST(test_promote_dh_group_already_front) +{ + proposal_t *proposal; + + proposal = proposal_create_from_string(PROTO_IKE, + "aes128-sha256-modp3072-ecp256"); + ck_assert(proposal->promote_dh_group(proposal, MODP_3072_BIT)); + assert_proposal_eq(proposal, "IKE:AES_CBC_128/HMAC_SHA2_256_128/PRF_HMAC_SHA2_256/MODP_3072/ECP_256"); + proposal->destroy(proposal); +} +END_TEST + +START_TEST(test_promote_dh_group_not_contained) +{ + proposal_t *proposal; + + proposal = proposal_create_from_string(PROTO_IKE, + "aes128-sha256-modp3072-ecp256"); + + ck_assert(!proposal->promote_dh_group(proposal, MODP_2048_BIT)); + assert_proposal_eq(proposal, "IKE:AES_CBC_128/HMAC_SHA2_256_128/PRF_HMAC_SHA2_256/MODP_3072/ECP_256"); + proposal->destroy(proposal); +} +END_TEST + Suite *proposal_suite_create() { Suite *s; @@ -167,5 +210,11 @@ Suite *proposal_suite_create() tcase_add_test(tc, test_select_spi); suite_add_tcase(s, tc); + tc = tcase_create("promote_dh_group"); + tcase_add_test(tc, test_promote_dh_group); + tcase_add_test(tc, test_promote_dh_group_already_front); + tcase_add_test(tc, test_promote_dh_group_not_contained); + suite_add_tcase(s, tc); + return s; }