Merge branch 'dh-group-rekey'

These changes improve rekeying after the peer initially selected a
different DH group than we proposed.  Instead of using the configured DH
group again, and causing another INVALID_KE_PAYLOAD notify, we now reuse
the previously negotiated group.  We also send the selected DH group
first in the proposals (and move proposals that don't contain the group
to the back) so that implementations that select the proposal first and
without consulting the KE payload (e.g. strongSwan when preferring the
client's proposals) will see the preferred group first.

Fixes #2526.

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,

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.
 	 *

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);

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,

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);
 }

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):

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,

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.

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;
 }