child-sa: Install drop policies while updating IPsec SAs and policies

If we have to remove and reinstall SAs for address updates (as with the
Linux kernel) there is a short time where there is no SA installed.  If
we keep the policies installed they (or any traps) might cause acquires
and temporary kernel states that could prevent the updated SA from
getting installed again.

This replaces the previous workaround to avoid plaintext traffic leaks
during policy updates, which used low-priority drop policies.

src/libcharon/sa/child_sa.c

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2006-2017 Tobias Brunner
+ * Copyright (C) 2006-2018 Tobias Brunner
  * Copyright (C) 2016 Andreas Steffen
  * Copyright (C) 2005-2008 Martin Willi
  * Copyright (C) 2006 Daniel Roethlisberger
@@ -1249,17 +1249,6 @@ METHOD(child_sa_t, install_policies, status_t,
 		enumerator = create_policy_enumerator(this);
 		while (enumerator->enumerate(enumerator, &my_ts, &other_ts))
 		{
-			/* install outbound drop policy to avoid packets leaving unencrypted
-			 * when updating policies */
-			if (priority == POLICY_PRIORITY_DEFAULT && manual_prio == 0 &&
-				require_policy_update() && install_outbound)
-			{
-				status |= install_policies_outbound(this, this->my_addr,
-									this->other_addr, my_ts, other_ts,
-									&my_sa, &other_sa, POLICY_DROP,
-									POLICY_PRIORITY_FALLBACK, 0);
-			}
-
 			status |= install_policies_inbound(this, this->my_addr,
 									this->other_addr, my_ts, other_ts,
 									&my_sa, &other_sa, POLICY_IPSEC,
@@ -1350,15 +1339,6 @@ METHOD(child_sa_t, install_outbound, status_t,
 		enumerator = create_policy_enumerator(this);
 		while (enumerator->enumerate(enumerator, &my_ts, &other_ts))
 		{
-			/* install outbound drop policy to avoid packets leaving unencrypted
-			 * when updating policies */
-			if (manual_prio == 0 &&	require_policy_update())
-			{
-				status |= install_policies_outbound(this, this->my_addr,
-									this->other_addr, my_ts, other_ts,
-									&my_sa, &other_sa, POLICY_DROP,
-									POLICY_PRIORITY_FALLBACK, 0);
-			}
 			status |= install_policies_outbound(this, this->my_addr,
 									this->other_addr, my_ts, other_ts,
 									&my_sa, &other_sa, POLICY_IPSEC,
@@ -1407,12 +1387,6 @@ METHOD(child_sa_t, remove_outbound, void,
 								  my_ts, other_ts, &my_sa, &other_sa,
 								  POLICY_IPSEC, POLICY_PRIORITY_DEFAULT,
 								  manual_prio);
-			if (manual_prio == 0 && require_policy_update())
-			{
-				del_policies_outbound(this, this->my_addr, this->other_addr,
-									  my_ts, other_ts, &my_sa, &other_sa,
-									  POLICY_DROP, POLICY_PRIORITY_FALLBACK, 0);
-			}
 		}
 		enumerator->destroy(enumerator);
 	}
@@ -1458,27 +1432,11 @@ CALLBACK(reinstall_vip, void,
 	}
 }
 
-METHOD(child_sa_t, update, status_t,
+/**
-	private_child_sa_t *this,  host_t *me, host_t *other, linked_list_t *vips,
+ * 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)
-{
-	child_sa_state_t old;
-	bool transport_proxy_mode;
-
-	/* anything changed at all? */
-	if (me->equals(me, this->my_addr) &&
-		other->equals(other, this->other_addr) && this->encap == encap)
-	{
-		return SUCCESS;
-	}
-
-	old = this->state;
-	set_state(this, CHILD_UPDATING);
-	transport_proxy_mode = this->mode == MODE_TRANSPORT &&
-						   this->config->has_option(this->config,
-													OPT_PROXY_MODE);
-
-	if (!transport_proxy_mode)
 {
 	/* update our (initiator) SA */
 	if (this->my_spi)
@@ -1500,7 +1458,6 @@ METHOD(child_sa_t, update, status_t,
 		if (charon->kernel->update_sa(charon->kernel, &id,
 									  &sa) == NOT_SUPPORTED)
 		{
-				set_state(this, old);
 			return NOT_SUPPORTED;
 		}
 	}
@@ -1525,37 +1482,77 @@ METHOD(child_sa_t, update, status_t,
 		if (charon->kernel->update_sa(charon->kernel, &id,
 									  &sa) == NOT_SUPPORTED)
 		{
-				set_state(this, old);
 			return NOT_SUPPORTED;
 		}
 	}
+	/* we currently ignore the actual return values above */
+	return SUCCESS;
 }
 
+METHOD(child_sa_t, update, status_t,
+	private_child_sa_t *this, host_t *me, host_t *other, linked_list_t *vips,
+	bool encap)
+{
+	child_sa_state_t old;
+	bool transport_proxy_mode;
+
+	/* anything changed at all? */
+	if (me->equals(me, this->my_addr) &&
+		other->equals(other, this->other_addr) && this->encap == encap)
+	{
+		return SUCCESS;
+	}
+
+	old = this->state;
+	set_state(this, CHILD_UPDATING);
+	transport_proxy_mode = this->mode == MODE_TRANSPORT &&
+						   this->config->has_option(this->config,
+													OPT_PROXY_MODE);
+
 	if (!this->config->has_option(this->config, OPT_NO_POLICIES) &&
 		require_policy_update())
-	{
-		if (!me->ip_equals(me, this->my_addr) ||
-			!other->ip_equals(other, this->other_addr))
 	{
 		ipsec_sa_cfg_t my_sa, other_sa;
 		enumerator_t *enumerator;
 		traffic_selector_t *my_ts, *other_ts;
 		uint32_t manual_prio;
+		status_t state;
 
 		prepare_sa_cfg(this, &my_sa, &other_sa);
 		manual_prio = this->config->get_manual_prio(this->config);
 
-			/* always use high priorities, as hosts getting updated are INSTALLED */
+		enumerator = create_policy_enumerator(this);
+		while (enumerator->enumerate(enumerator, &my_ts, &other_ts))
+		{
+			/* install drop policy to avoid traffic leaks, acquires etc. */
+			install_policies_outbound(this, this->my_addr, this->other_addr,
+						my_ts, other_ts, &my_sa, &other_sa, POLICY_DROP,
+						POLICY_PRIORITY_DEFAULT, manual_prio);
+
+			/* remove old policies */
+			del_policies_internal(this, this->my_addr, this->other_addr,
+						my_ts, other_ts, &my_sa, &other_sa, POLICY_IPSEC,
+						POLICY_PRIORITY_DEFAULT, manual_prio);
+		}
+		enumerator->destroy(enumerator);
+
+		/* update the IPsec SAs */
+		state = update_sas(this, me, other, encap);
+
 		enumerator = create_policy_enumerator(this);
 		while (enumerator->enumerate(enumerator, &my_ts, &other_ts))
 		{
 			traffic_selector_t *old_my_ts = NULL, *old_other_ts = NULL;
 
-				/* remove old policies first */
+			/* reinstall the previous policies if we can't update the SAs */
-				del_policies_internal(this, this->my_addr, this->other_addr,
+			if (state == NOT_SUPPORTED)
-							my_ts, other_ts, &my_sa, &other_sa, POLICY_IPSEC,
+			{
-							POLICY_PRIORITY_DEFAULT, manual_prio);
+				install_policies_internal(this, this->my_addr, this->other_addr,
-
+						my_ts, other_ts, &my_sa, &other_sa,
+						POLICY_IPSEC, POLICY_PRIORITY_DEFAULT, manual_prio);
+			}
+			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))
@@ -1578,23 +1575,32 @@ METHOD(child_sa_t, update, status_t,
 				install_policies_internal(this, me, other, my_ts, other_ts,
 										  &my_sa, &other_sa, POLICY_IPSEC,
 										  POLICY_PRIORITY_DEFAULT, manual_prio);
-
+			}
-				/* update fallback policies after the new policy is in place */
+			/* remove the drop policy */
-				if (manual_prio == 0)
-				{
 			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,
-										  POLICY_PRIORITY_FALLBACK, 0);
+								  POLICY_PRIORITY_DEFAULT, 0);
-					install_policies_outbound(this, me, other, my_ts, other_ts,
+
-										  &my_sa, &other_sa, POLICY_DROP,
-										  POLICY_PRIORITY_FALLBACK, 0);
-				}
 			DESTROY_IF(old_my_ts);
 			DESTROY_IF(old_other_ts);
 		}
 		enumerator->destroy(enumerator);
+
+		if (state == NOT_SUPPORTED)
+		{
+			set_state(this, old);
+			return NOT_SUPPORTED;
+		}
+
+	}
+	else if (!transport_proxy_mode)
+	{
+		if (update_sas(this, me, other, encap) == NOT_SUPPORTED)
+		{
+			set_state(this, old);
+			return NOT_SUPPORTED;
 		}
 	}
 
@@ -1655,13 +1661,6 @@ METHOD(child_sa_t, destroy, void,
 			del_policies_inbound(this, this->my_addr, this->other_addr,
 								 my_ts, other_ts, &my_sa, &other_sa,
 								 POLICY_IPSEC, priority, manual_prio);
-			if (!this->trap && manual_prio == 0 && require_policy_update() &&
-				del_outbound)
-			{
-				del_policies_outbound(this, this->my_addr, this->other_addr,
-									  my_ts, other_ts, &my_sa, &other_sa,
-									  POLICY_DROP, POLICY_PRIORITY_FALLBACK, 0);
-			}
 		}
 		enumerator->destroy(enumerator);
 	}