From: Tobias Brunner <tobias@strongswan.org>
Date: Fri, 26 Aug 2022 15:29:00 +0000 (+0200)
Subject: ike-sa: Always set ike_cfg_t when setting peer_cfg_t
X-Git-Tag: 5.9.8dr4~7
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=69995ed2c42be0dbb52a61b906ec63b9bbd1099f;p=thirdparty%2Fstrongswan.git

ike-sa: Always set ike_cfg_t when setting peer_cfg_t

This is more consistent and e.g. allows to properly take into account
some settings that are also relevant during IKE_AUTH (e.g. childless).

We also already use the peer_cfg_t's ike_cfg_t when rekeying,
reauthenticating and reestablishing an IKE_SA (and e.g. for DSCP).

Also changed are some IKEv1 cases where get_ike_cfg() is called before
set_peer_cfg() without taking a reference to the ike_cfg_t that might
get replaced/destroyed (none of the cases were problematic, though, but
it also wasn't necessary to keep the ike_cfg_t around).

Closes strongswan/strongswan#1238
---

diff --git a/src/libcharon/sa/ike_sa.c b/src/libcharon/sa/ike_sa.c
index 0d554204d2..b0b2ab27d6 100644
--- a/src/libcharon/sa/ike_sa.c
+++ b/src/libcharon/sa/ike_sa.c
@@ -436,11 +436,9 @@ METHOD(ike_sa_t, set_peer_cfg, void,
 	DESTROY_IF(this->peer_cfg);
 	this->peer_cfg = peer_cfg;
 
-	if (!this->ike_cfg)
-	{
-		this->ike_cfg = peer_cfg->get_ike_cfg(peer_cfg);
-		this->ike_cfg->get_ref(this->ike_cfg);
-	}
+	DESTROY_IF(this->ike_cfg);
+	this->ike_cfg = peer_cfg->get_ike_cfg(peer_cfg);
+	this->ike_cfg->get_ref(this->ike_cfg);
 
 	this->if_id_in = peer_cfg->get_if_id(peer_cfg, TRUE);
 	this->if_id_out = peer_cfg->get_if_id(peer_cfg, FALSE);
@@ -644,21 +642,9 @@ METHOD(ike_sa_t, get_message_id, uint32_t,
  */
 static void set_dscp(private_ike_sa_t *this, packet_t *packet)
 {
-	ike_cfg_t *ike_cfg;
-
-	/* prefer IKE config on peer_cfg, as its selection is more accurate
-	 * then the initial IKE config */
-	if (this->peer_cfg)
-	{
-		ike_cfg = this->peer_cfg->get_ike_cfg(this->peer_cfg);
-	}
-	else
-	{
-		ike_cfg = this->ike_cfg;
-	}
-	if (ike_cfg)
+	if (this->ike_cfg)
 	{
-		packet->set_dscp(packet, ike_cfg->get_dscp(ike_cfg));
+		packet->set_dscp(packet, this->ike_cfg->get_dscp(this->ike_cfg));
 	}
 }
 
diff --git a/src/libcharon/sa/ikev1/tasks/aggressive_mode.c b/src/libcharon/sa/ikev1/tasks/aggressive_mode.c
index 517843d3c8..6dbaabe9cb 100644
--- a/src/libcharon/sa/ikev1/tasks/aggressive_mode.c
+++ b/src/libcharon/sa/ikev1/tasks/aggressive_mode.c
@@ -58,11 +58,6 @@ struct private_aggressive_mode_t {
 	 */
 	phase1_t *ph1;
 
-	/**
-	 * IKE config to establish
-	 */
-	ike_cfg_t *ike_cfg;
-
 	/**
 	 * Peer config to use
 	 */
@@ -213,6 +208,7 @@ METHOD(task_t, build_i, status_t,
 	{
 		case AM_INIT:
 		{
+			ike_cfg_t *ike_cfg;
 			sa_payload_t *sa_payload;
 			id_payload_t *id_payload;
 			linked_list_t *proposals;
@@ -226,7 +222,7 @@ METHOD(task_t, build_i, status_t,
 				 this->ike_sa->get_other_host(this->ike_sa));
 			this->ike_sa->set_state(this->ike_sa, IKE_CONNECTING);
 
-			this->ike_cfg = this->ike_sa->get_ike_cfg(this->ike_sa);
+			ike_cfg = this->ike_sa->get_ike_cfg(this->ike_sa);
 			this->peer_cfg = this->ike_sa->get_peer_cfg(this->ike_sa);
 			this->peer_cfg->get_ref(this->peer_cfg);
 
@@ -244,7 +240,7 @@ METHOD(task_t, build_i, status_t,
 																 FALSE);
 			}
 			this->lifetime += this->peer_cfg->get_over_time(this->peer_cfg);
-			proposals = this->ike_cfg->get_proposals(this->ike_cfg);
+			proposals = ike_cfg->get_proposals(ike_cfg);
 			sa_payload = sa_payload_create_from_proposals_v1(proposals,
 									this->lifetime, 0, this->method, MODE_NONE,
 									ENCAP_NONE, 0);
@@ -252,8 +248,7 @@ METHOD(task_t, build_i, status_t,
 
 			message->add_payload(message, &sa_payload->payload_interface);
 
-			group = this->ike_cfg->get_algorithm(this->ike_cfg,
-												 KEY_EXCHANGE_METHOD);
+			group = ike_cfg->get_algorithm(ike_cfg, KEY_EXCHANGE_METHOD);
 			if (!group)
 			{
 				DBG1(DBG_IKE, "DH group selection failed");
@@ -370,6 +365,7 @@ METHOD(task_t, process_r, status_t,
 	{
 		case AM_INIT:
 		{
+			ike_cfg_t *ike_cfg;
 			sa_payload_t *sa_payload;
 			id_payload_t *id_payload;
 			identification_t *id;
@@ -377,7 +373,7 @@ METHOD(task_t, process_r, status_t,
 			proposal_selection_flag_t flags = PROPOSAL_SKIP_PRIVATE;
 			uint16_t group;
 
-			this->ike_cfg = this->ike_sa->get_ike_cfg(this->ike_sa);
+			ike_cfg = this->ike_sa->get_ike_cfg(this->ike_sa);
 			DBG0(DBG_IKE, "%H is initiating a Aggressive Mode IKE_SA",
 				 message->get_source(message));
 			this->ike_sa->set_state(this->ike_sa, IKE_CONNECTING);
@@ -405,8 +401,7 @@ METHOD(task_t, process_r, status_t,
 			{
 				flags = PROPOSAL_PREFER_SUPPLIED;
 			}
-			this->proposal = this->ike_cfg->select_proposal(this->ike_cfg, list,
-															flags);
+			this->proposal = ike_cfg->select_proposal(ike_cfg, list, flags);
 			list->destroy_offset(list, offsetof(proposal_t, destroy));
 			if (!this->proposal)
 			{
@@ -635,6 +630,7 @@ METHOD(task_t, process_i, status_t,
 		auth_method_t method;
 		sa_payload_t *sa_payload;
 		id_payload_t *id_payload;
+		ike_cfg_t *ike_cfg;
 		identification_t *id, *cid;
 		linked_list_t *list;
 		uint32_t lifetime;
@@ -647,7 +643,8 @@ METHOD(task_t, process_i, status_t,
 			return send_notify(this, INVALID_PAYLOAD_TYPE);
 		}
 		list = sa_payload->get_proposals(sa_payload);
-		this->proposal = this->ike_cfg->select_proposal(this->ike_cfg, list, 0);
+		ike_cfg = this->ike_sa->get_ike_cfg(this->ike_sa);
+		this->proposal = ike_cfg->select_proposal(ike_cfg, list, 0);
 		list->destroy_offset(list, offsetof(proposal_t, destroy));
 		if (!this->proposal)
 		{
diff --git a/src/libcharon/sa/ikev1/tasks/main_mode.c b/src/libcharon/sa/ikev1/tasks/main_mode.c
index 9ab34ce92b..4248700d5e 100644
--- a/src/libcharon/sa/ikev1/tasks/main_mode.c
+++ b/src/libcharon/sa/ikev1/tasks/main_mode.c
@@ -58,11 +58,6 @@ struct private_main_mode_t {
 	 */
 	phase1_t *ph1;
 
-	/**
-	 * IKE config to establish
-	 */
-	ike_cfg_t *ike_cfg;
-
 	/**
 	 * Peer config to use
 	 */
@@ -247,6 +242,7 @@ METHOD(task_t, build_i, status_t,
 	{
 		case MM_INIT:
 		{
+			ike_cfg_t *ike_cfg;
 			sa_payload_t *sa_payload;
 			linked_list_t *proposals;
 			packet_t *packet;
@@ -257,7 +253,7 @@ METHOD(task_t, build_i, status_t,
 				 this->ike_sa->get_other_host(this->ike_sa));
 			this->ike_sa->set_state(this->ike_sa, IKE_CONNECTING);
 
-			this->ike_cfg = this->ike_sa->get_ike_cfg(this->ike_sa);
+			ike_cfg = this->ike_sa->get_ike_cfg(this->ike_sa);
 			this->peer_cfg = this->ike_sa->get_peer_cfg(this->ike_sa);
 			this->peer_cfg->get_ref(this->peer_cfg);
 
@@ -275,7 +271,7 @@ METHOD(task_t, build_i, status_t,
 																 FALSE);
 			}
 			this->lifetime += this->peer_cfg->get_over_time(this->peer_cfg);
-			proposals = this->ike_cfg->get_proposals(this->ike_cfg);
+			proposals = ike_cfg->get_proposals(ike_cfg);
 			sa_payload = sa_payload_create_from_proposals_v1(proposals,
 									this->lifetime, 0, this->method, MODE_NONE,
 									ENCAP_NONE, 0);
@@ -364,11 +360,12 @@ METHOD(task_t, process_r, status_t,
 	{
 		case MM_INIT:
 		{
+			ike_cfg_t *ike_cfg;
 			linked_list_t *list;
 			sa_payload_t *sa_payload;
 			proposal_selection_flag_t flags = 0;
 
-			this->ike_cfg = this->ike_sa->get_ike_cfg(this->ike_sa);
+			ike_cfg = this->ike_sa->get_ike_cfg(this->ike_sa);
 			DBG0(DBG_IKE, "%H is initiating a Main Mode IKE_SA",
 				 message->get_source(message));
 			this->ike_sa->set_state(this->ike_sa, IKE_CONNECTING);
@@ -402,8 +399,7 @@ METHOD(task_t, process_r, status_t,
 			{
 				flags |= PROPOSAL_PREFER_SUPPLIED;
 			}
-			this->proposal = this->ike_cfg->select_proposal(this->ike_cfg,
-											list, flags);
+			this->proposal = ike_cfg->select_proposal(ike_cfg, list, flags);
 			list->destroy_offset(list, offsetof(proposal_t, destroy));
 			if (!this->proposal)
 			{
@@ -636,6 +632,7 @@ METHOD(task_t, process_i, status_t,
 		{
 			linked_list_t *list;
 			sa_payload_t *sa_payload;
+			ike_cfg_t *ike_cfg;
 			auth_method_t method;
 			proposal_selection_flag_t flags = 0;
 			uint32_t lifetime;
@@ -654,8 +651,8 @@ METHOD(task_t, process_i, status_t,
 			{
 				flags |= PROPOSAL_SKIP_PRIVATE;
 			}
-			this->proposal = this->ike_cfg->select_proposal(this->ike_cfg,
-															list, flags);
+			ike_cfg = this->ike_sa->get_ike_cfg(this->ike_sa);
+			this->proposal = ike_cfg->select_proposal(ike_cfg, list, flags);
 			list->destroy_offset(list, offsetof(proposal_t, destroy));
 			if (!this->proposal)
 			{