Merge branch 'mobike-nat'

These changes improve MOBIKE task queuing. In particular we don't want to ignore the response to an update (with NAT-D payloads) if only an address list update or DPD is queued as that could prevent use from updating the UDP encapsulation in the kernel. A new optional roam trigger is added to the kernel-netlink plugin based on routing rule changes. This only works properly, though, if the kernel based route lookup is used as the kernel-netlink plugin does currently not consider routing rules for its own route lookup. Another change prevents acquires during address updates if we have to update IPsec SAs by deleting and readding them. Because the outbound policy is still installed an acquire and temporary SA might get triggered in the short time no IPsec SA is installed, which could subsequently prevent the reinstallation of the SA. To this end we install drop policies before updating the policies and SAs. These also replace the fallback drop policies we previously used to prevent plaintext leaks during policy updates (which reduces the overhead in cases where addresses never or rarely change as additional policies will only have to be tracked during address updates). Fixes #2518.
2025-10-17 00:00:59 -04:00 · 2018-02-09 15:54:36 +01:00 · 2018-02-09 15:54:36 +01:00 · d58a84e0f7
commit d58a84e0f7
parent 626e8a31d0 044b0f048b
18 changed files with 503 additions and 242 deletions
--- a/conf/plugins/kernel-netlink.opt
+++ b/conf/plugins/kernel-netlink.opt
@ -47,6 +47,13 @@ charon.plugins.kernel-netlink.port_bypass = no
 	port based policies use global XFRM bypass policies for the used IKE UDP
 	ports.

+charon.plugins.kernel-netlink.process_rules = no
+	Whether to process changes in routing rules to trigger roam events.
+
+	Whether to process changes in routing rules to trigger roam events. This is
+	currently only useful if the kernel based route lookup is used (i.e. if
+	route installation is disabled or an inverted fwmark match is configured).
+
 charon.plugins.kernel-netlink.receive_buffer_size = 0
 	Maximum Netlink socket receive buffer in bytes.

--- a/src/libcharon/plugins/kernel_netlink/kernel_netlink_net.c
+++ b/src/libcharon/plugins/kernel_netlink/kernel_netlink_net.c
@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2008-2016 Tobias Brunner
+ * Copyright (C) 2008-2018 Tobias Brunner
 * Copyright (C) 2005-2008 Martin Willi
 * HSR Hochschule fuer Technik Rapperswil
 *
@ -78,6 +78,9 @@
 #define ROUTING_TABLE_PRIO 0
 #endif

+/** multicast groups (for groups > 31 setsockopt has to be used) */
+#define nl_group(group) (1 << (group - 1))
+
 ENUM(rt_msg_names, RTM_NEWLINK, RTM_GETRULE,
 	"RTM_NEWLINK",
 	"RTM_DELLINK",
@ -472,6 +475,11 @@ struct private_kernel_netlink_net_t {
 	 */
 	bool process_route;

+	/**
+	 * whether to react to RTM_NEWRULE or RTM_DELRULE events
+	 */
+	bool process_rules;
+
 	/**
 	 * whether to trigger roam events
 	 */
@ -1451,6 +1459,45 @@ static void process_route(private_kernel_netlink_net_t *this, struct nlmsghdr *h
 	host->destroy(host);
 }

+/**
+ * process RTM_NEW|DELRULE from kernel
+ */
+static void process_rule(private_kernel_netlink_net_t *this, struct nlmsghdr *hdr)
+{
+#ifdef HAVE_LINUX_FIB_RULES_H
+	struct rtmsg* msg = NLMSG_DATA(hdr);
+	struct rtattr *rta = RTM_RTA(msg);
+	size_t rtasize = RTM_PAYLOAD(hdr);
+	uint32_t table = 0;
+
+	/* ignore rules added by us or in the local routing table (local addrs) */
+	if (msg->rtm_table && (msg->rtm_table == this->routing_table ||
+						   msg->rtm_table == RT_TABLE_LOCAL))
+	{
+		return;
+	}
+
+	while (RTA_OK(rta, rtasize))
+	{
+		switch (rta->rta_type)
+		{
+			case FRA_TABLE:
+				if (RTA_PAYLOAD(rta) == sizeof(table))
+				{
+					table = *(uint32_t*)RTA_DATA(rta);
+				}
+				break;
+		}
+		rta = RTA_NEXT(rta, rtasize);
+	}
+	if (table && table == this->routing_table)
+	{	/* also check against extended table ID */
+		return;
+	}
+	fire_roam_event(this, FALSE);
+#endif
+}
+
 /**
 * Receives events from kernel
 */
@ -1508,6 +1555,13 @@ static bool receive_events(private_kernel_netlink_net_t *this, int fd,
 					process_route(this, hdr);
 				}
 				break;
+			case RTM_NEWRULE:
+			case RTM_DELRULE:
+				if (this->process_rules)
+				{
+					process_rule(this, hdr);
+				}
+				break;
 			default:
 				break;
 		}
@ -2985,6 +3039,8 @@ kernel_netlink_net_t *kernel_netlink_net_create()
 						"%s.prefer_temporary_addrs", FALSE, lib->ns),
 		.roam_events = lib->settings->get_bool(lib->settings,
 						"%s.plugins.kernel-netlink.roam_events", TRUE, lib->ns),
+		.process_rules = lib->settings->get_bool(lib->settings,
+						"%s.plugins.kernel-netlink.process_rules", FALSE, lib->ns),
 		.mtu = lib->settings->get_int(lib->settings,
 						"%s.plugins.kernel-netlink.mtu", 0, lib->ns),
 		.mss = lib->settings->get_int(lib->settings,
@ -3037,8 +3093,19 @@ kernel_netlink_net_t *kernel_netlink_net_create()
 			destroy(this);
 			return NULL;
 		}
-		addr.nl_groups = RTMGRP_IPV4_IFADDR | RTMGRP_IPV6_IFADDR |
-						 RTMGRP_IPV4_ROUTE | RTMGRP_IPV6_ROUTE | RTMGRP_LINK;
+		addr.nl_groups = nl_group(RTNLGRP_IPV4_IFADDR) |
+						 nl_group(RTNLGRP_IPV6_IFADDR) |
+						 nl_group(RTNLGRP_LINK);
+		if (this->process_route)
+		{
+			addr.nl_groups |= nl_group(RTNLGRP_IPV4_ROUTE) |
+							  nl_group(RTNLGRP_IPV6_ROUTE);
+		}
+		if (this->process_rules)
+		{
+			addr.nl_groups |= nl_group(RTNLGRP_IPV4_RULE) |
+							  nl_group(RTNLGRP_IPV6_RULE);
+		}
 		if (bind(this->socket_events, (struct sockaddr*)&addr, sizeof(addr)))
 		{
 			DBG1(DBG_KNL, "unable to bind RT event socket: %s (%d)",
--- a/src/libcharon/sa/child_sa.c
+++ b/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,8 +1432,65 @@ 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)
+{
+	/* update our (initiator) SA */
+	if (this->my_spi)
+	{
+		kernel_ipsec_sa_id_t id = {
+			.src = this->other_addr,
+			.dst = this->my_addr,
+			.spi = this->my_spi,
+			.proto = proto_ike2ip(this->protocol),
+			.mark = mark_in_sa(this),
+		};
+		kernel_ipsec_update_sa_t sa = {
+			.cpi = this->ipcomp != IPCOMP_NONE ? this->my_cpi : 0,
+			.new_src = other,
+			.new_dst = me,
+			.encap = this->encap,
+			.new_encap = encap,
+		};
+		if (charon->kernel->update_sa(charon->kernel, &id,
+									  &sa) == NOT_SUPPORTED)
+		{
+			return NOT_SUPPORTED;
+		}
+	}
+
+	/* update his (responder) SA */
+	if (this->other_spi)
+	{
+		kernel_ipsec_sa_id_t id = {
+			.src = this->my_addr,
+			.dst = this->other_addr,
+			.spi = this->other_spi,
+			.proto = proto_ike2ip(this->protocol),
+			.mark = this->mark_out,
+		};
+		kernel_ipsec_update_sa_t sa = {
+			.cpi = this->ipcomp != IPCOMP_NONE ? this->other_cpi : 0,
+			.new_src = me,
+			.new_dst = other,
+			.encap = this->encap,
+			.new_encap = encap,
+		};
+		if (charon->kernel->update_sa(charon->kernel, &id,
+									  &sa) == NOT_SUPPORTED)
+		{
+			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,
+	private_child_sa_t *this, host_t *me, host_t *other, linked_list_t *vips,
 	bool encap)
 {
 	child_sa_state_t old;
@ -1478,84 +1509,50 @@ METHOD(child_sa_t, update, status_t,
 						   this->config->has_option(this->config,
 													OPT_PROXY_MODE);

-	if (!transport_proxy_mode)
-	{
-		/* update our (initiator) SA */
-		if (this->my_spi)
-		{
-			kernel_ipsec_sa_id_t id = {
-				.src = this->other_addr,
-				.dst = this->my_addr,
-				.spi = this->my_spi,
-				.proto = proto_ike2ip(this->protocol),
-				.mark = mark_in_sa(this),
-			};
-			kernel_ipsec_update_sa_t sa = {
-				.cpi = this->ipcomp != IPCOMP_NONE ? this->my_cpi : 0,
-				.new_src = other,
-				.new_dst = me,
-				.encap = this->encap,
-				.new_encap = encap,
-			};
-			if (charon->kernel->update_sa(charon->kernel, &id,
-										  &sa) == NOT_SUPPORTED)
-			{
-				set_state(this, old);
-				return NOT_SUPPORTED;
-			}
-		}
-
-		/* update his (responder) SA */
-		if (this->other_spi)
-		{
-			kernel_ipsec_sa_id_t id = {
-				.src = this->my_addr,
-				.dst = this->other_addr,
-				.spi = this->other_spi,
-				.proto = proto_ike2ip(this->protocol),
-				.mark = this->mark_out,
-			};
-			kernel_ipsec_update_sa_t sa = {
-				.cpi = this->ipcomp != IPCOMP_NONE ? this->other_cpi : 0,
-				.new_src = me,
-				.new_dst = other,
-				.encap = this->encap,
-				.new_encap = encap,
-			};
-			if (charon->kernel->update_sa(charon->kernel, &id,
-										  &sa) == NOT_SUPPORTED)
-			{
-				set_state(this, old);
-				return NOT_SUPPORTED;
-			}
-		}
-	}
-
 	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);
+
+		enumerator = create_policy_enumerator(this);
+		while (enumerator->enumerate(enumerator, &my_ts, &other_ts))
 		{
-			ipsec_sa_cfg_t my_sa, other_sa;
-			enumerator_t *enumerator;
-			traffic_selector_t *my_ts, *other_ts;
-			uint32_t manual_prio;
+			/* 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);

-			prepare_sa_cfg(this, &my_sa, &other_sa);
-			manual_prio = this->config->get_manual_prio(this->config);
+			/* 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);

-			/* always use high priorities, as hosts getting updated are INSTALLED */
-			enumerator = create_policy_enumerator(this);
-			while (enumerator->enumerate(enumerator, &my_ts, &other_ts))
+		/* 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;
+
+			/* reinstall the previous policies if we can't update the SAs */
+			if (state == NOT_SUPPORTED)
+			{
+				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
 			{
-				traffic_selector_t *old_my_ts = NULL, *old_other_ts = NULL;
-
-				/* remove old policies first */
-				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);
-
 				/* 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 */
-				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);
-					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);
+			/* remove the drop policy */
+			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_DEFAULT, 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);
 	}
--- a/src/libcharon/sa/ike_sa.c
+++ b/src/libcharon/sa/ike_sa.c
@ -231,11 +231,6 @@ struct private_ike_sa_t {
 	 */
 	chunk_t nat_detection_dest;

-	/**
-	 * number pending UPDATE_SA_ADDRESS (MOBIKE)
-	 */
-	uint32_t pending_updates;
-
 	/**
 	 * NAT keep alive interval
 	 */
@ -734,8 +729,11 @@ METHOD(ike_sa_t, set_condition, void,
 			switch (condition)
 			{
 				case COND_NAT_HERE:
-				case COND_NAT_FAKE:
 				case COND_NAT_THERE:
+					DBG1(DBG_IKE, "%s host is not behind NAT anymore",
+						 condition == COND_NAT_HERE ? "local" : "remote");
+					/* fall-through */
+				case COND_NAT_FAKE:
 					set_condition(this, COND_NAT_ANY,
 								  has_condition(this, COND_NAT_HERE) ||
 								  has_condition(this, COND_NAT_THERE) ||
@ -1052,18 +1050,6 @@ METHOD(ike_sa_t, has_mapping_changed, bool,
 	return TRUE;
 }

-METHOD(ike_sa_t, set_pending_updates, void,
-	private_ike_sa_t *this, uint32_t updates)
-{
-	this->pending_updates = updates;
-}
-
-METHOD(ike_sa_t, get_pending_updates, uint32_t,
-	private_ike_sa_t *this)
-{
-	return this->pending_updates;
-}
-
 METHOD(ike_sa_t, float_ports, void,
 	   private_ike_sa_t *this)
 {
@ -2970,8 +2956,6 @@ ike_sa_t * ike_sa_create(ike_sa_id_t *ike_sa_id, bool initiator,
 			.supports_extension = _supports_extension,
 			.set_condition = _set_condition,
 			.has_condition = _has_condition,
-			.set_pending_updates = _set_pending_updates,
-			.get_pending_updates = _get_pending_updates,
 			.create_peer_address_enumerator = _create_peer_address_enumerator,
 			.add_peer_address = _add_peer_address,
 			.clear_peer_addresses = _clear_peer_addresses,
--- a/src/libcharon/sa/ike_sa.h
+++ b/src/libcharon/sa/ike_sa.h
@ -646,20 +646,6 @@ struct ike_sa_t {
 	 */
 	bool (*has_condition) (ike_sa_t *this, ike_condition_t condition);

-	/**
-	 * Get the number of queued MOBIKE address updates.
-	 *
-	 * @return				number of pending updates
-	 */
-	uint32_t (*get_pending_updates)(ike_sa_t *this);
-
-	/**
-	 * Set the number of queued MOBIKE address updates.
-	 *
-	 * @param updates		number of pending updates
-	 */
-	void (*set_pending_updates)(ike_sa_t *this, uint32_t updates);
-
 #ifdef ME
 	/**
 	 * Activate mediation server functionality for this IKE_SA.
--- a/src/libcharon/sa/ikev2/task_manager_v2.c
+++ b/src/libcharon/sa/ikev2/task_manager_v2.c
@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2007-2016 Tobias Brunner
+ * Copyright (C) 2007-2018 Tobias Brunner
 * Copyright (C) 2007-2010 Martin Willi
 * HSR Hochschule fuer Technik Rapperswil
 *
@ -1642,24 +1642,9 @@ METHOD(task_manager_t, process_message, status_t,
 METHOD(task_manager_t, queue_task_delayed, void,
 	private_task_manager_t *this, task_t *task, uint32_t delay)
 {
-	enumerator_t *enumerator;
 	queued_task_t *queued;
 	timeval_t time;

-	if (task->get_type(task) == TASK_IKE_MOBIKE)
-	{	/*  there is no need to queue more than one mobike task */
-		enumerator = array_create_enumerator(this->queued_tasks);
-		while (enumerator->enumerate(enumerator, &queued))
-		{
-			if (queued->task->get_type(queued->task) == TASK_IKE_MOBIKE)
-			{
-				enumerator->destroy(enumerator);
-				task->destroy(task);
-				return;
-			}
-		}
-		enumerator->destroy(enumerator);
-	}
 	time_monotonic(&time);
 	if (delay)
 	{
@ -1877,12 +1862,41 @@ METHOD(task_manager_t, queue_ike_delete, void,
 	queue_task(this, (task_t*)ike_delete_create(this->ike_sa, TRUE));
 }

+/**
+ * There is no need to queue more than one mobike task, so this either returns
+ * an already queued task or queues one if there is none yet.
+ */
+static ike_mobike_t *queue_mobike_task(private_task_manager_t *this)
+{
+	enumerator_t *enumerator;
+	queued_task_t *queued;
+	ike_mobike_t *mobike = NULL;
+
+	enumerator = array_create_enumerator(this->queued_tasks);
+	while (enumerator->enumerate(enumerator, &queued))
+	{
+		if (queued->task->get_type(queued->task) == TASK_IKE_MOBIKE)
+		{
+			mobike = (ike_mobike_t*)queued->task;
+			break;
+		}
+	}
+	enumerator->destroy(enumerator);
+
+	if (!mobike)
+	{
+		mobike = ike_mobike_create(this->ike_sa, TRUE);
+		queue_task(this, &mobike->task);
+	}
+	return mobike;
+}
+
 METHOD(task_manager_t, queue_mobike, void,
 	private_task_manager_t *this, bool roam, bool address)
 {
 	ike_mobike_t *mobike;

-	mobike = ike_mobike_create(this->ike_sa, TRUE);
+	mobike = queue_mobike_task(this);
 	if (roam)
 	{
 		enumerator_t *enumerator;
@ -1909,7 +1923,31 @@ METHOD(task_manager_t, queue_mobike, void,
 	{
 		mobike->addresses(mobike);
 	}
-	queue_task(this, &mobike->task);
+}
+
+METHOD(task_manager_t, queue_dpd, void,
+	private_task_manager_t *this)
+{
+	ike_mobike_t *mobike;
+
+	if (this->ike_sa->supports_extension(this->ike_sa, EXT_MOBIKE) &&
+		this->ike_sa->has_condition(this->ike_sa, COND_NAT_HERE))
+	{
+#ifdef ME
+		peer_cfg_t *cfg = this->ike_sa->get_peer_cfg(this->ike_sa);
+		if (cfg->get_peer_id(cfg) ||
+			this->ike_sa->has_condition(this->ike_sa, COND_ORIGINAL_INITIATOR))
+#else
+		if (this->ike_sa->has_condition(this->ike_sa, COND_ORIGINAL_INITIATOR))
+#endif
+		{
+			/* use mobike enabled DPD to detect NAT mapping changes */
+			mobike = queue_mobike_task(this);
+			mobike->dpd(mobike);
+			return;
+		}
+	}
+	queue_task(this, (task_t*)ike_dpd_create(TRUE));
 }

 METHOD(task_manager_t, queue_child, void,
@ -1940,32 +1978,6 @@ METHOD(task_manager_t, queue_child_delete, void,
 												  protocol, spi, expired));
 }

-METHOD(task_manager_t, queue_dpd, void,
-	private_task_manager_t *this)
-{
-	ike_mobike_t *mobike;
-
-	if (this->ike_sa->supports_extension(this->ike_sa, EXT_MOBIKE) &&
-		this->ike_sa->has_condition(this->ike_sa, COND_NAT_HERE))
-	{
-#ifdef ME
-		peer_cfg_t *cfg = this->ike_sa->get_peer_cfg(this->ike_sa);
-		if (cfg->get_peer_id(cfg) ||
-			this->ike_sa->has_condition(this->ike_sa, COND_ORIGINAL_INITIATOR))
-#else
-		if (this->ike_sa->has_condition(this->ike_sa, COND_ORIGINAL_INITIATOR))
-#endif
-		{
-			/* use mobike enabled DPD to detect NAT mapping changes */
-			mobike = ike_mobike_create(this->ike_sa, TRUE);
-			mobike->dpd(mobike);
-			queue_task(this, &mobike->task);
-			return;
-		}
-	}
-	queue_task(this, (task_t*)ike_dpd_create(TRUE));
-}
-
 METHOD(task_manager_t, adopt_tasks, void,
 	private_task_manager_t *this, task_manager_t *other_public)
 {
--- a/src/libcharon/sa/ikev2/tasks/ike_mobike.c
+++ b/src/libcharon/sa/ikev2/tasks/ike_mobike.c
@ -1,7 +1,7 @@
 /*
- * Copyright (C) 2010-2014 Tobias Brunner
+ * Copyright (C) 2010-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
@ -76,13 +76,35 @@ struct private_ike_mobike_t {
 	 * additional addresses got updated
 	 */
 	bool addresses_updated;
-
-	/**
-	 * whether the pending updates counter was increased
-	 */
-	bool pending_update;
 };

+/**
+ * Check if a newer MOBIKE update task is queued
+ */
+static bool is_newer_update_queued(private_ike_mobike_t *this)
+{
+	enumerator_t *enumerator;
+	private_ike_mobike_t *mobike;
+	task_t *task;
+	bool found = FALSE;
+
+	enumerator = this->ike_sa->create_task_enumerator(this->ike_sa,
+													  TASK_QUEUE_QUEUED);
+	while (enumerator->enumerate(enumerator, &task))
+	{
+		if (task->get_type(task) == TASK_IKE_MOBIKE)
+		{
+			mobike = (private_ike_mobike_t*)task;
+			/* a queued check or update might invalidate the results of the
+			 * current task */
+			found = mobike->check || mobike->update;
+			break;
+		}
+	}
+	enumerator->destroy(enumerator);
+	return found;
+}
+
 /**
 * read notifys from message and evaluate them
 */
@ -526,9 +548,8 @@ METHOD(task_t, process_i, status_t,
 	}
 	else if (message->get_exchange_type(message) == INFORMATIONAL)
 	{
-		if (this->ike_sa->get_pending_updates(this->ike_sa) > 1)
+		if (is_newer_update_queued(this))
 		{
-			/* newer update queued, ignore this one */
 			return SUCCESS;
 		}
 		if (this->cookie2.ptr)
@ -553,7 +574,7 @@ METHOD(task_t, process_i, status_t,
 		if (this->natd)
 		{
 			this->natd->task.process(&this->natd->task, message);
-			if (this->natd->has_mapping_changed(this->natd))
+			if (!this->update && this->natd->has_mapping_changed(this->natd))
 			{
 				/* force an update if mappings have changed */
 				this->update = this->check = TRUE;
@ -615,25 +636,13 @@ METHOD(ike_mobike_t, addresses, void,
 	   private_ike_mobike_t *this)
 {
 	this->address = TRUE;
-	if (!this->pending_update)
-	{
-		this->pending_update = TRUE;
-		this->ike_sa->set_pending_updates(this->ike_sa,
-						this->ike_sa->get_pending_updates(this->ike_sa) + 1);
-	}
 }

 METHOD(ike_mobike_t, roam, void,
 	   private_ike_mobike_t *this, bool address)
 {
 	this->check = TRUE;
-	this->address = address;
-	if (!this->pending_update)
-	{
-		this->pending_update = TRUE;
-		this->ike_sa->set_pending_updates(this->ike_sa,
-						this->ike_sa->get_pending_updates(this->ike_sa) + 1);
-	}
+	this->address |= address;
 }

 METHOD(ike_mobike_t, dpd, void,
@ -643,12 +652,6 @@ METHOD(ike_mobike_t, dpd, void,
 	{
 		this->natd = ike_natd_create(this->ike_sa, this->initiator);
 	}
-	if (!this->pending_update)
-	{
-		this->pending_update = TRUE;
-		this->ike_sa->set_pending_updates(this->ike_sa,
-						this->ike_sa->get_pending_updates(this->ike_sa) + 1);
-	}
 }

 METHOD(ike_mobike_t, is_probing, bool,
@ -678,21 +681,11 @@ METHOD(task_t, migrate, void,
 	{
 		this->natd->task.migrate(&this->natd->task, ike_sa);
 	}
-	if (this->pending_update)
-	{
-		this->ike_sa->set_pending_updates(this->ike_sa,
-						this->ike_sa->get_pending_updates(this->ike_sa) + 1);
-	}
 }

 METHOD(task_t, destroy, void,
 	   private_ike_mobike_t *this)
 {
-	if (this->pending_update)
-	{
-		this->ike_sa->set_pending_updates(this->ike_sa,
-						this->ike_sa->get_pending_updates(this->ike_sa) - 1);
-	}
 	chunk_free(&this->cookie2);
 	if (this->natd)
 	{
--- a/testing/tests/ikev2/mobike-virtual-ip-nat/description.txt
+++ b/testing/tests/ikev2/mobike-virtual-ip-nat/description.txt
@ -0,0 +1,9 @@
+The roadwarrior <b>alice</b> is sitting behind the NAT router <b>moon</b> but
+at the outset of the scenariou is also directly connected to the 192.168.0.0/24 network
+via an additional <b>eth1</b> interface. <b>alice</b> builds up a tunnel to gateway <b>sun</b>
+in order to reach <b>bob</b> in the subnet behind. When the <b>eth1</b> interface
+goes away, <b>alice</b> switches to <b>eth0</b> and signals the IP address change
+via a MOBIKE ADDRESS_UPDATE notification to peer <b>sun</b>. Later the interface
+comes back up again and because the best path is preferred (charon.prefer_best_path)
+there is another switch to the directly connected path.  <b>alice</b> sets
+a virtual IP of 10.3.0.3, so that the IPsec policies don't have to be changed.
--- a/testing/tests/ikev2/mobike-virtual-ip-nat/evaltest.dat
+++ b/testing/tests/ikev2/mobike-virtual-ip-nat/evaltest.dat
@ -0,0 +1,31 @@
+alice::ipsec status 2> /dev/null::mobike.*ESTABLISHED.*192.168.0.50.*PH_IP_SUN::YES
+sun::  ipsec status 2> /dev/null::mobike.*ESTABLISHED.*PH_IP_SUN.*192.168.0.50::YES
+alice::ipsec status 2> /dev/null::mobike.*INSTALLED.*ESP SPIs::YES
+sun::  ipsec status 2> /dev/null::mobike.*INSTALLED.*ESP SPIs::YES
+alice::ipsec statusall 2> /dev/null::10.3.0.3/32 === 10.2.0.0/16::YES
+sun::  ipsec statusall 2> /dev/null::10.2.0.0/16 === 10.3.0.3/32::YES
+alice::ping -c 1 PH_IP_BOB::64 bytes from PH_IP_BOB: icmp_.eq=1::YES
+alice::ifdown eth1::No output expected::NO
+alice::sleep 1::No output expected::NO
+alice::ipsec status 2> /dev/null::mobike.*ESTABLISHED.*PH_IP_ALICE.*PH_IP_SUN::YES
+sun::  ipsec status 2> /dev/null::mobike.*ESTABLISHED.*PH_IP_SUN.*PH_IP_MOON::YES
+alice::ipsec status 2> /dev/null::mobike.*INSTALLED.*ESP in UDP SPIs::YES
+sun::  ipsec status 2> /dev/null::mobike.*INSTALLED.*ESP in UDP SPIs::YES
+alice::ipsec statusall 2> /dev/null::10.3.0.3/32 === 10.2.0.0/16::YES
+sun::  ipsec statusall 2> /dev/null::10.2.0.0/16 === 10.3.0.3/32::YES
+alice::ping -c 1 PH_IP_BOB::64 bytes from PH_IP_BOB: icmp_.eq=1::YES
+alice::ifup eth1::No output expected::NO
+alice::sleep 1::No output expected::NO
+alice::ping -c 1 PH_IP_BOB::64 bytes from PH_IP_BOB: icmp_.eq=1::YES
+alice::ipsec status 2> /dev/null::mobike.*ESTABLISHED.*192.168.0.50.*PH_IP_SUN::YES
+sun::  ipsec status 2> /dev/null::mobike.*ESTABLISHED.*PH_IP_SUN.*192.168.0.50::YES
+alice::ipsec status 2> /dev/null::mobike.*INSTALLED.*ESP SPIs::YES
+sun::  ipsec status 2> /dev/null::mobike.*INSTALLED.*ESP SPIs::YES
+alice::ipsec statusall 2> /dev/null::10.3.0.3/32 === 10.2.0.0/16::YES
+sun::  ipsec statusall 2> /dev/null::10.2.0.0/16 === 10.3.0.3/32::YES
+sun::tcpdump::alice1.strongswan.org.*sun.strongswan.org: ESP.*seq=0x1::YES
+sun::tcpdump::sun.strongswan.org.*alice1.strongswan.org: ESP.*seq=0x1::YES
+moon::tcpdump::moon.strongswan.org.*sun.strongswan.org.*: ESP.*seq=0x2::YES
+moon::tcpdump::sun.strongswan.org.*moon.strongswan.org.*: ESP.*seq=0x2::YES
+bob::tcpdump::10.3.0.3.*bob.strongswan.org.*ICMP echo request::3
+bob::tcpdump::bob.strongswan.org.*10.3.0.3.*ICMP echo reply::3
--- a/testing/tests/ikev2/mobike-virtual-ip-nat/hosts/alice/etc/ipsec.conf
+++ b/testing/tests/ikev2/mobike-virtual-ip-nat/hosts/alice/etc/ipsec.conf
@ -0,0 +1,19 @@
+# /etc/ipsec.conf - strongSwan IPsec configuration file
+
+config setup
+
+conn %default
+	ikelifetime=60m
+	keylife=20m
+	rekeymargin=3m
+	keyingtries=1
+	keyexchange=ikev2
+
+conn mobike
+	leftsourceip=%config
+	leftcert=aliceCert.pem
+	leftid=alice@strongswan.org
+	right=PH_IP_SUN
+	rightid=@sun.strongswan.org
+	rightsubnet=10.2.0.0/16
+	auto=add
--- a/testing/tests/ikev2/mobike-virtual-ip-nat/hosts/alice/etc/iptables.rules
+++ b/testing/tests/ikev2/mobike-virtual-ip-nat/hosts/alice/etc/iptables.rules
@ -0,0 +1,42 @@
+*filter
+
+# default policy is DROP
+-P INPUT DROP
+-P OUTPUT DROP
+-P FORWARD DROP
+
+# allow traffic on lo as ifup/ifdown call bind's rndc which accesses TCP 953
+-A OUTPUT -o lo -j ACCEPT
+-A INPUT -i lo -j ACCEPT
+
+# allow IPsec tunnel traffic
+-A INPUT  -m policy --dir in  --pol ipsec --proto esp -j ACCEPT
+-A OUTPUT -m policy --dir out --pol ipsec --proto esp -j ACCEPT
+
+# allow ESP
+-A INPUT  -i eth0 -p 50 -j ACCEPT
+-A INPUT  -i eth1 -p 50 -j ACCEPT
+-A OUTPUT -o eth0 -p 50 -j ACCEPT
+-A OUTPUT -o eth1 -p 50 -j ACCEPT
+
+# allow IKE
+-A INPUT  -i eth0 -p udp --sport 500 --dport 500 -j ACCEPT
+-A INPUT  -i eth1 -p udp --sport 500 --dport 500 -j ACCEPT
+-A OUTPUT -o eth0 -p udp --dport 500 --sport 500 -j ACCEPT
+-A OUTPUT -o eth1 -p udp --dport 500 --sport 500 -j ACCEPT
+
+# allow MobIKE
+-A INPUT  -i eth0 -p udp --sport 4500 --dport 4500 -j ACCEPT
+-A INPUT  -i eth1 -p udp --sport 4500 --dport 4500 -j ACCEPT
+-A OUTPUT -o eth0 -p udp --dport 4500 --sport 4500 -j ACCEPT
+-A OUTPUT -o eth1 -p udp --dport 4500 --sport 4500 -j ACCEPT
+
+# allow ssh
+-A INPUT  -p tcp --dport 22 -j ACCEPT
+-A OUTPUT -p tcp --sport 22 -j ACCEPT
+
+# allow crl fetch from winnetou
+-A INPUT  -p tcp --sport 80 -s PH_IP_WINNETOU -j ACCEPT
+-A OUTPUT -p tcp --dport 80 -d PH_IP_WINNETOU -j ACCEPT
+
+COMMIT
--- a/testing/tests/ikev2/mobike-virtual-ip-nat/hosts/alice/etc/strongswan.conf
+++ b/testing/tests/ikev2/mobike-virtual-ip-nat/hosts/alice/etc/strongswan.conf
@ -0,0 +1,12 @@
+# /etc/strongswan.conf - strongSwan configuration file
+
+charon {
+  load = random nonce aes sha1 sha2 pem pkcs1 curve25519 gmp x509 curl revocation hmac stroke kernel-netlink socket-default
+  prefer_best_path = yes
+
+  syslog {
+    daemon {
+      knl = 2
+    }
+  }
+}
--- a/testing/tests/ikev2/mobike-virtual-ip-nat/hosts/sun/etc/ipsec.conf
+++ b/testing/tests/ikev2/mobike-virtual-ip-nat/hosts/sun/etc/ipsec.conf
@ -0,0 +1,20 @@
+# /etc/ipsec.conf - strongSwan IPsec configuration file
+
+config setup
+
+conn %default
+	ikelifetime=60m
+	keylife=20m
+	rekeymargin=3m
+	keyingtries=1
+	keyexchange=ikev2
+
+conn mobike
+	left=PH_IP_SUN
+	leftcert=sunCert.pem
+	leftid=@sun.strongswan.org
+	leftsubnet=10.2.0.0/16
+	right=%any
+	rightsourceip=10.3.0.3
+	rightid=alice@strongswan.org
+	auto=add
--- a/testing/tests/ikev2/mobike-virtual-ip-nat/hosts/sun/etc/iptables.rules
+++ b/testing/tests/ikev2/mobike-virtual-ip-nat/hosts/sun/etc/iptables.rules
@ -0,0 +1,32 @@
+*filter
+
+# default policy is DROP
+-P INPUT DROP
+-P OUTPUT DROP
+-P FORWARD DROP
+
+# allow IPsec tunnel traffic
+-A FORWARD -m policy --dir in  --pol ipsec --proto esp -j ACCEPT
+-A FORWARD -m policy --dir out --pol ipsec --proto esp -j ACCEPT
+
+# allow ESP
+-A INPUT  -i eth0 -p 50 -j ACCEPT
+-A OUTPUT -o eth0 -p 50 -j ACCEPT
+
+# allow IKE
+-A INPUT  -i eth0 -p udp --dport 500 -j ACCEPT
+-A OUTPUT -o eth0 -p udp --sport 500 -j ACCEPT
+
+# allow MobIKE
+-A INPUT  -i eth0 -p udp --dport 4500 -j ACCEPT
+-A OUTPUT -o eth0 -p udp --sport 4500 -j ACCEPT
+
+# allow ssh
+-A INPUT  -p tcp --dport 22 -j ACCEPT
+-A OUTPUT -p tcp --sport 22 -j ACCEPT
+
+# allow crl fetch from winnetou
+-A INPUT  -i eth0 -p tcp --sport 80 -s PH_IP_WINNETOU -j ACCEPT
+-A OUTPUT -o eth0 -p tcp --dport 80 -d PH_IP_WINNETOU -j ACCEPT
+
+COMMIT
--- a/testing/tests/ikev2/mobike-virtual-ip-nat/hosts/sun/etc/strongswan.conf
+++ b/testing/tests/ikev2/mobike-virtual-ip-nat/hosts/sun/etc/strongswan.conf
@ -0,0 +1,11 @@
+# /etc/strongswan.conf - strongSwan configuration file
+
+charon {
+  load = random nonce aes sha1 sha2 pem pkcs1 curve25519 gmp x509 curl revocation hmac stroke kernel-netlink socket-default
+
+  syslog {
+    daemon {
+      knl = 2
+    }
+  }
+}
--- a/testing/tests/ikev2/mobike-virtual-ip-nat/posttest.dat
+++ b/testing/tests/ikev2/mobike-virtual-ip-nat/posttest.dat
@ -0,0 +1,6 @@
+alice::ipsec stop
+sun::ipsec stop
+alice::ifdown eth1
+alice::iptables-restore < /etc/iptables.flush
+sun::iptables-restore < /etc/iptables.flush
+moon::iptables -t nat -F
--- a/testing/tests/ikev2/mobike-virtual-ip-nat/pretest.dat
+++ b/testing/tests/ikev2/mobike-virtual-ip-nat/pretest.dat
@ -0,0 +1,10 @@
+alice::ifup eth1
+alice::iptables-restore < /etc/iptables.rules
+sun::iptables-restore < /etc/iptables.rules
+moon::iptables -t nat -A POSTROUTING -o eth0 -s 10.1.0.0/16 -p udp -j SNAT --to-source PH_IP_MOON:1024-1100
+moon::iptables -t nat -A POSTROUTING -o eth0 -s 10.1.0.0/16 -p tcp -j SNAT --to-source PH_IP_MOON:2000-2100
+alice::ipsec start
+sun::ipsec start
+alice::expect-connection mobike
+sun::expect-connection mobike
+alice::ipsec up mobike
--- a/testing/tests/ikev2/mobike-virtual-ip-nat/test.conf
+++ b/testing/tests/ikev2/mobike-virtual-ip-nat/test.conf
@ -0,0 +1,21 @@
+#!/bin/bash
+#
+# This configuration file provides information on the
+# guest instances used for this test
+
+# All guest instances that are required for this test
+#
+VIRTHOSTS="alice moon winnetou sun bob"
+
+# Corresponding block diagram
+#
+DIAGRAM="a-m-w-s-b.png"
+
+# Guest instances on which tcpdump is to be started
+#
+TCPDUMPHOSTS="bob moon sun"
+
+# Guest instances on which IPsec is started
+# Used for IPsec logging purposes
+#
+IPSECHOSTS="alice sun"