child-sa: Allocate a new reqid if dynamic traffic selectors are updated

If update_sa() is called and dynamic traffic selectors are changed using new addresses, this might cause issues if we continue to use a reqid that doesn't match the updated traffic selectors. For instance, if the initiator then uses make-before-break reauth from the new IP. It's also a particular problem in the SELinux case where multiple CHILD_SAs with specific labels all share the same (trap) policy with generic label. However, SAs created after the update would not match due to the new reqid.
2022-04-14 18:42:01 +02:00
parent 96ecc39cd0
commit 8f4cfe9669
1 changed files with 170 additions and 58 deletions
@@ -558,8 +558,12 @@ METHOD(enumerator_t, policy_destroy, void,
 	free(this);
 }

-METHOD(child_sa_t, create_policy_enumerator, enumerator_t*,
-	   private_child_sa_t *this)
+/**
+ * Create an enumerator over two lists of traffic selectors, returning all the
+ * pairs of traffic selectors from the first and second list.
+ */
+static enumerator_t *create_policy_enumerator_internal(array_t *my_ts,
+													   array_t *other_ts)
 {
 	policy_enumerator_t *e;

@@ -569,15 +573,21 @@ METHOD(child_sa_t, create_policy_enumerator, enumerator_t*,
 			.venumerate = _policy_enumerate,
 			.destroy = _policy_destroy,
 		},
-		.mine = array_create_enumerator(this->my_ts),
-		.other = array_create_enumerator(this->other_ts),
-		.array = this->other_ts,
+		.mine = array_create_enumerator(my_ts),
+		.other = array_create_enumerator(other_ts),
+		.array = other_ts,
 		.ts = NULL,
 	);

 	return &e->public;
 }

+METHOD(child_sa_t, create_policy_enumerator, enumerator_t*,
+	   private_child_sa_t *this)
+{
+	return create_policy_enumerator_internal(this->my_ts, this->other_ts);
+}
+
 /**
 * update the cached usebytes
 * returns SUCCESS if the usebytes have changed, FAILED if not or no SPIs
@@ -1119,6 +1129,7 @@ static status_t install_policies_outbound(private_child_sa_t *this,
 		.dst = other_addr,
 		.sa = other_sa,
 	};
+	uint32_t reqid = other_sa->reqid;
 	status_t status = SUCCESS;

 	status |= charon->kernel->add_policy(charon->kernel, &out_id, &out_policy);
@@ -1142,7 +1153,7 @@ static status_t install_policies_outbound(private_child_sa_t *this,
 		status |= charon->kernel->add_policy(charon->kernel, &out_id,
 											 &out_policy);
 		/* reset the reqid for any other further policies */
-		other_sa->reqid = this->reqid;
+		other_sa->reqid = reqid;
 	}
 	return status;
 }
@@ -1229,6 +1240,7 @@ static void del_policies_outbound(private_child_sa_t *this,
 		.dst = other_addr,
 		.sa = other_sa,
 	};
+	uint32_t reqid = other_sa->reqid;

 	charon->kernel->del_policy(charon->kernel, &out_id, &out_policy);

@@ -1241,7 +1253,7 @@ static void del_policies_outbound(private_child_sa_t *this,
 			out_policy.prio = POLICY_PRIORITY_ROUTED;
 		}
 		charon->kernel->del_policy(charon->kernel, &out_id, &out_policy);
-		other_sa->reqid = this->reqid;
+		other_sa->reqid = reqid;
 	}
 }

@@ -1299,28 +1311,38 @@ METHOD(child_sa_t, set_policies, void,
 	array_sort(this->other_ts, (void*)traffic_selector_cmp, NULL);
 }

+/**
+ * Allocate a reqid for the given local and remote traffic selectors.
+ */
+static status_t alloc_reqid(private_child_sa_t *this, array_t *my_ts,
+							array_t *other_ts, uint32_t *reqid)
+{
+	linked_list_t *my_ts_list, *other_ts_list;
+	status_t status;
+
+	my_ts_list = linked_list_create_from_enumerator(array_create_enumerator(my_ts));
+	other_ts_list = linked_list_create_from_enumerator(array_create_enumerator(other_ts));
+	status = charon->kernel->alloc_reqid(
+							charon->kernel, my_ts_list, other_ts_list,
+							this->mark_in, this->mark_out, this->if_id_in,
+							this->if_id_out, label_for(this, LABEL_USE_REQID),
+							reqid);
+	my_ts_list->destroy(my_ts_list);
+	other_ts_list->destroy(other_ts_list);
+	return status;
+}
+
 METHOD(child_sa_t, install_policies, status_t,
 	   private_child_sa_t *this)
 {
 	enumerator_t *enumerator;
-	linked_list_t *my_ts_list, *other_ts_list;
 	traffic_selector_t *my_ts, *other_ts;
 	status_t status = SUCCESS;
 	bool install_outbound = FALSE;

 	if (!this->reqid_allocated && !this->static_reqid)
 	{
-		my_ts_list = linked_list_create_from_enumerator(
-									array_create_enumerator(this->my_ts));
-		other_ts_list = linked_list_create_from_enumerator(
-									array_create_enumerator(this->other_ts));
-		status = charon->kernel->alloc_reqid(
-							charon->kernel, my_ts_list, other_ts_list,
-							this->mark_in, this->mark_out, this->if_id_in,
-							this->if_id_out, label_for(this, LABEL_USE_REQID),
-							&this->reqid);
-		my_ts_list->destroy(my_ts_list);
-		other_ts_list->destroy(other_ts_list);
+		status = alloc_reqid(this, this->my_ts, this->other_ts, &this->reqid);
 		if (status != SUCCESS)
 		{
 			return status;
@@ -1554,7 +1576,7 @@ CALLBACK(reinstall_vip, void,
 * Update addresses and encap state of IPsec SAs in the kernel
 */
 static status_t update_sas(private_child_sa_t *this, host_t *me, host_t *other,
-						   bool encap)
+						   bool encap, uint32_t reqid)
 {
 	/* update our (initiator) SA */
 	if (this->my_spi && this->inbound_installed)
@@ -1573,6 +1595,7 @@ static status_t update_sas(private_child_sa_t *this, host_t *me, host_t *other,
 			.new_dst = me,
 			.encap = this->encap,
 			.new_encap = encap,
+			.new_reqid = reqid,
 		};
 		if (charon->kernel->update_sa(charon->kernel, &id,
 									  &sa) == NOT_SUPPORTED)
@@ -1598,6 +1621,7 @@ static status_t update_sas(private_child_sa_t *this, host_t *me, host_t *other,
 			.new_dst = other,
 			.encap = this->encap,
 			.new_encap = encap,
+			.new_reqid = reqid,
 		};
 		if (charon->kernel->update_sa(charon->kernel, &id,
 									  &sa) == NOT_SUPPORTED)
@@ -1609,6 +1633,30 @@ static status_t update_sas(private_child_sa_t *this, host_t *me, host_t *other,
 	return SUCCESS;
 }

+/**
+ * Fill the second list with copies of the given traffic selectors updating
+ * dynamic traffic selectors based on the given addresses.
+ */
+static void update_ts(host_t *old_host, host_t *new_host, array_t *old_list,
+					  array_t *new_list)
+{
+	enumerator_t *enumerator;
+	traffic_selector_t *old_ts, *new_ts;
+
+	enumerator = array_create_enumerator(old_list);
+	while (enumerator->enumerate(enumerator, &old_ts))
+	{
+		new_ts = old_ts->clone(old_ts);
+		if (new_ts->is_host(new_ts, old_host))
+		{
+			new_ts->set_address(new_ts, new_host);
+		}
+		array_insert(new_list, ARRAY_TAIL, new_ts);
+	}
+	enumerator->destroy(enumerator);
+	array_sort(new_list, (void*)traffic_selector_cmp, NULL);
+}
+
 METHOD(child_sa_t, update, status_t,
 	private_child_sa_t *this, host_t *me, host_t *other, linked_list_t *vips,
 	bool encap)
@@ -1636,8 +1684,9 @@ METHOD(child_sa_t, update, status_t,
 		ipsec_sa_cfg_t my_sa, other_sa;
 		enumerator_t *enumerator;
 		traffic_selector_t *my_ts, *other_ts;
+		array_t *new_my_ts = NULL, *new_other_ts = NULL;
 		policy_priority_t priority;
-		uint32_t manual_prio;
+		uint32_t manual_prio, new_reqid = 0;
 		status_t state;
 		bool outbound;

@@ -1650,6 +1699,16 @@ METHOD(child_sa_t, update, status_t,
 		enumerator = create_policy_enumerator(this);
 		while (enumerator->enumerate(enumerator, &my_ts, &other_ts))
 		{
+			if (!new_my_ts && !me->ip_equals(me, this->my_addr) &&
+				my_ts->is_host(my_ts, this->my_addr))
+			{
+				new_my_ts = array_create(0, 0);
+			}
+			if (!new_other_ts && !other->ip_equals(other, this->other_addr) &&
+				other_ts->is_host(other_ts, this->other_addr))
+			{
+				new_other_ts = array_create(0, 0);
+			}
 			/* install drop policy to avoid traffic leaks, acquires etc. */
 			if (outbound)
 			{
@@ -1664,14 +1723,61 @@ METHOD(child_sa_t, update, status_t,
 		}
 		enumerator->destroy(enumerator);

+		if (new_my_ts)
+		{
+			update_ts(this->my_addr, me, this->my_ts, new_my_ts);
+		}
+		if (new_other_ts)
+		{
+			update_ts(this->other_addr, other, this->other_ts, new_other_ts);
+		}
+		if (this->reqid_allocated && (new_my_ts || new_other_ts))
+		{
+			/* if we allocated a reqid with the previous TS, we have to get a
+			 * new one that matches the updated TS */
+			if (alloc_reqid(this, new_my_ts ?: this->my_ts,
+							new_other_ts ?: this->other_ts, &new_reqid) != SUCCESS)
+			{
+				DBG1(DBG_CHD, "allocating new reqid for updated SA failed");
+			}
+		}
+
 		/* update the IPsec SAs */
-		state = update_sas(this, me, other, encap);
+		state = update_sas(this, me, other, encap, new_reqid);
+
+		/* install new/updated policies only if we were able to update the
+		 * SAs, otherwise we reinstall the old policies further below */
+		if (state != NOT_SUPPORTED)
+		{
+			/* we reinstall the virtual IP to handle interface roaming
+			 * correctly */
+			if (vips)
+			{
+				vips->invoke_function(vips, reinstall_vip, me);
+			}
+			if (new_reqid)
+			{
+				my_sa.reqid = other_sa.reqid = new_reqid;
+			}
+			enumerator = create_policy_enumerator_internal(
+												new_my_ts ?: this->my_ts,
+												new_other_ts ?: this->other_ts);
+			while (enumerator->enumerate(enumerator, &my_ts, &other_ts))
+			{
+				install_policies_internal(this, me, other, my_ts, other_ts,
+										  &my_sa, &other_sa, POLICY_IPSEC,
+										  priority, manual_prio, outbound);
+			}
+			enumerator->destroy(enumerator);
+			if (new_reqid)
+			{
+				my_sa.reqid = other_sa.reqid = this->reqid;
+			}
+		}

 		enumerator = create_policy_enumerator(this);
 		while (enumerator->enumerate(enumerator, &my_ts, &other_ts))
 		{
-			traffic_selector_t *old_my_ts = NULL, *old_other_ts = NULL;
-
 			/* reinstall the previous policies if we can't update the SAs */
 			if (state == NOT_SUPPORTED)
 			{
@@ -1679,58 +1785,64 @@ METHOD(child_sa_t, update, status_t,
 						my_ts, other_ts, &my_sa, &other_sa, POLICY_IPSEC,
 						priority, manual_prio, outbound);
 			}
-			else
-			{
-				/* check if we have to update a "dynamic" traffic selector */
-				if (!me->ip_equals(me, this->my_addr) &&
-					my_ts->is_host(my_ts, this->my_addr))
-				{
-					old_my_ts = my_ts->clone(my_ts);
-					my_ts->set_address(my_ts, me);
-				}
-				if (!other->ip_equals(other, this->other_addr) &&
-					other_ts->is_host(other_ts, this->other_addr))
-				{
-					old_other_ts = other_ts->clone(other_ts);
-					other_ts->set_address(other_ts, other);
-				}
-
-				/* we reinstall the virtual IP to handle interface roaming
-				 * correctly */
-				if (vips)
-				{
-					vips->invoke_function(vips, reinstall_vip, me);
-				}
-
-				/* reinstall updated policies */
-				install_policies_internal(this, me, other, my_ts, other_ts,
-						&my_sa, &other_sa, POLICY_IPSEC,
-						priority, manual_prio, outbound);
-			}
 			/* remove the drop policy */
 			if (outbound)
 			{
 				del_policies_outbound(this, this->my_addr, this->other_addr,
-						old_my_ts ?: my_ts, old_other_ts ?: other_ts,
-						&my_sa, &other_sa, POLICY_DROP,
+						my_ts, other_ts, &my_sa, &other_sa, POLICY_DROP,
 						POLICY_PRIORITY_DEFAULT, manual_prio);
 			}
-
-			DESTROY_IF(old_my_ts);
-			DESTROY_IF(old_other_ts);
 		}
 		enumerator->destroy(enumerator);

 		if (state == NOT_SUPPORTED)
 		{
+			if (new_reqid &&
+				charon->kernel->release_reqid(charon->kernel,
+						new_reqid, this->mark_in, this->mark_out,
+						this->if_id_in, this->if_id_out,
+						label_for(this, LABEL_USE_REQID)) != SUCCESS)
+			{
+				DBG1(DBG_CHD, "releasing reqid %u failed", new_reqid);
+			}
+			array_destroy_offset(new_my_ts,
+								 offsetof(traffic_selector_t, destroy));
+			array_destroy_offset(new_other_ts,
+								 offsetof(traffic_selector_t, destroy));
 			set_state(this, old);
 			return NOT_SUPPORTED;
 		}

+		if (new_reqid)
+		{
+			if (charon->kernel->release_reqid(charon->kernel,
+						this->reqid, this->mark_in, this->mark_out,
+						this->if_id_in, this->if_id_out,
+						label_for(this, LABEL_USE_REQID)) != SUCCESS)
+			{
+				DBG1(DBG_CHD, "releasing reqid %u failed", this->reqid);
+			}
+			DBG1(DBG_CHD, "replaced reqid %u with reqid %u for updated "
+				 "CHILD_SA %s{%d}", this->reqid, new_reqid, get_name(this),
+				 this->unique_id);
+			this->reqid = new_reqid;
+		}
+		if (new_my_ts)
+		{
+			array_destroy_offset(this->my_ts,
+								 offsetof(traffic_selector_t, destroy));
+			this->my_ts = new_my_ts;
+		}
+		if (new_other_ts)
+		{
+			array_destroy_offset(this->other_ts,
+								 offsetof(traffic_selector_t, destroy));
+			this->other_ts = new_other_ts;
+		}
 	}
 	else if (!transport_proxy_mode)
 	{
-		if (update_sas(this, me, other, encap) == NOT_SUPPORTED)
+		if (update_sas(this, me, other, encap, 0) == NOT_SUPPORTED)
 		{
 			set_state(this, old);
 			return NOT_SUPPORTED;