child-rekey: Support CHILD_SA rekeying with multiple key exchanges

src/libcharon/sa/ikev2/tasks/child_create.c

@@ -2196,6 +2196,10 @@ static status_t delete_failed_sa(private_child_create_t *this)
 	{
 		this->public.task.build = _build_i_delete;
 		this->public.task.process = (void*)return_success;
+		/* destroying it here allows the rekey task to differentiate between
+		 * this and the multi-KE case */
+		this->child_sa->destroy(this->child_sa);
+		this->child_sa = NULL;
 		return NEED_MORE;
 	}
 	return SUCCESS;

src/libcharon/sa/ikev2/tasks/child_delete.c

@@ -187,11 +187,6 @@ static void install_outbound(private_child_delete_t *this,
 		DBG1(DBG_IKE, "CHILD_SA not found after rekeying");
 		return;
 	}
-	if (this->initiator && is_redundant(this, child_sa))
-	{	/* if we won the rekey collision we don't want to install the
-		 * redundant SA created by the peer */
-		return;
-	}
 
 	status = child_sa->install_outbound(child_sa);
 	if (status != SUCCESS)

src/libcharon/sa/ikev2/tasks/child_rekey.c

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2009-2018 Tobias Brunner
+ * Copyright (C) 2009-2023 Tobias Brunner
  * Copyright (C) 2005-2007 Martin Willi
  * Copyright (C) 2005 Jan Hutter
  *
@@ -84,13 +84,32 @@ struct private_child_rekey_t {
 	task_t *collision;
 
 	/**
-	 * Indicate that peer destroyed the redundant child from collision.
+	 * State flags
-	 * This happens if a peer's delete notification for the redundant
-	 * child gets processed before the rekey job. If so, we must not
-	 * touch the child created in the collision since it points to
-	 * memory already freed.
 	 */
-	bool other_child_destroyed;
+	enum {
+
+		/**
+		 * Set if we use multiple key exchanges and already processed the
+		 * CREATE_CHILD_SA response and started sending IKE_FOLLOWUP_KEs.
+		 */
+		CHILD_REKEY_FOLLOWUP_KE = (1<<0),
+
+		/**
+		 * Set if we adopted a completed passive task, otherwise (i.e. for
+		 * multi-KE rekeyings) we just reference it.
+		 */
+		CHILD_REKEY_ADOPTED_PASSIVE = (1<<1),
+
+		/**
+		 * Indicate that the peer destroyed the redundant child from a
+		 * collision. This happens if a peer's delete notification for the
+		 * redundant child gets processed before the active rekey job is
+		 * complete. If so, we must not touch the child created in the collision
+		 * since it points to memory already freed.
+		 */
+		CHILD_REKEY_OTHER_DESTROYED = (1<<2),
+
+	} flags;
 };
 
 /**
@@ -169,8 +188,6 @@ METHOD(task_t, build_i, status_t,
 	private_child_rekey_t *this, message_t *message)
 {
 	notify_payload_t *notify;
-	uint32_t reqid;
-	child_cfg_t *config;
 
 	this->child_sa = this->ike_sa->get_child_sa(this->ike_sa, this->protocol,
 												this->spi, TRUE);
@@ -194,15 +211,16 @@ METHOD(task_t, build_i, status_t,
 		message->set_exchange_type(message, EXCHANGE_TYPE_UNDEFINED);
 		return SUCCESS;
 	}
-	config = this->child_sa->get_config(this->child_sa);
-
 
 	/* our CHILD_CREATE task does the hard work for us */
 	if (!this->child_create)
 	{
+		child_cfg_t *config;
 		proposal_t *proposal;
 		uint16_t ke_method;
+		uint32_t reqid;
 
+		config = this->child_sa->get_config(this->child_sa);
 		this->child_create = child_create_create(this->ike_sa,
 									config->get_ref(config), TRUE, NULL, NULL);
 
@@ -212,21 +230,21 @@ METHOD(task_t, build_i, status_t,
 		{	/* reuse the KE method negotiated previously */
 			this->child_create->use_ke_method(this->child_create, ke_method);
 		}
-	}
+		reqid = this->child_sa->get_reqid_ref(this->child_sa);
-	reqid = this->child_sa->get_reqid_ref(this->child_sa);
+		if (reqid)
-	if (reqid)
+		{
-	{
+			this->child_create->use_reqid(this->child_create, reqid);
-		this->child_create->use_reqid(this->child_create, reqid);
+			charon->kernel->release_reqid(charon->kernel, reqid);
-		charon->kernel->release_reqid(charon->kernel, reqid);
+		}
-	}
+		this->child_create->use_marks(this->child_create,
-	this->child_create->use_marks(this->child_create,
 						this->child_sa->get_mark(this->child_sa, TRUE).value,
 						this->child_sa->get_mark(this->child_sa, FALSE).value);
-	this->child_create->use_if_ids(this->child_create,
+		this->child_create->use_if_ids(this->child_create,
 						this->child_sa->get_if_id(this->child_sa, TRUE),
 						this->child_sa->get_if_id(this->child_sa, FALSE));
-	this->child_create->use_label(this->child_create,
+		this->child_create->use_label(this->child_create,
 						this->child_sa->get_label(this->child_sa));
+	}
 
 	if (this->child_create->task.build(&this->child_create->task,
 									   message) != NEED_MORE)
@@ -259,14 +277,48 @@ METHOD(task_t, process_r, status_t,
 	return NEED_MORE;
 }
 
+/**
+ * Check if we are actively rekeying and, optionally, if we already sent an
+ * IKE_FOLLOWUP_KE message.
+ */
+static bool actively_rekeying(private_child_rekey_t *this, bool *followup_sent)
+{
+	enumerator_t *enumerator;
+	task_t *task;
+	bool found = FALSE;
+
+	enumerator = this->ike_sa->create_task_enumerator(this->ike_sa,
+													  TASK_QUEUE_ACTIVE);
+	while (enumerator->enumerate(enumerator, (void**)&task))
+	{
+		if (task->get_type(task) == TASK_CHILD_REKEY)
+		{
+			private_child_rekey_t *rekey = (private_child_rekey_t*)task;
+
+			if (this->child_sa == rekey->child_sa)
+			{
+				if (followup_sent)
+				{
+					*followup_sent = rekey->flags & CHILD_REKEY_FOLLOWUP_KE;
+				}
+				found = TRUE;
+			}
+			break;
+		}
+	}
+	enumerator->destroy(enumerator);
+	return found;
+}
+
 METHOD(task_t, build_r, status_t,
 	private_child_rekey_t *this, message_t *message)
 {
 	notify_payload_t *notify;
 	child_cfg_t *config;
-	uint32_t reqid;
-	child_sa_state_t state;
 	child_sa_t *child_sa;
+	child_sa_state_t state = CHILD_INSTALLED;
+	uint32_t reqid;
+	bool followup_sent;
 
 	if (!this->child_sa)
 	{
@@ -284,118 +336,195 @@ METHOD(task_t, build_r, status_t,
 		message->add_notify(message, TRUE, TEMPORARY_FAILURE, chunk_empty);
 		return SUCCESS;
 	}
-
+	if (actively_rekeying(this, &followup_sent) && followup_sent)
-	/* let the CHILD_CREATE task build the response */
-	reqid = this->child_sa->get_reqid_ref(this->child_sa);
-	if (reqid)
 	{
-		this->child_create->use_reqid(this->child_create, reqid);
+		DBG1(DBG_IKE, "peer initiated rekeying, but we did too and already "
-		charon->kernel->release_reqid(charon->kernel, reqid);
+			 "sent IKE_FOLLOWUP_KE");
-	}
+		message->add_notify(message, TRUE, TEMPORARY_FAILURE, chunk_empty);
-	this->child_create->use_marks(this->child_create,
-						this->child_sa->get_mark(this->child_sa, TRUE).value,
-						this->child_sa->get_mark(this->child_sa, FALSE).value);
-	this->child_create->use_if_ids(this->child_create,
-						this->child_sa->get_if_id(this->child_sa, TRUE),
-						this->child_sa->get_if_id(this->child_sa, FALSE));
-	this->child_create->use_label(this->child_create,
-						this->child_sa->get_label(this->child_sa));
-	config = this->child_sa->get_config(this->child_sa);
-	this->child_create->set_config(this->child_create, config->get_ref(config));
-	this->child_create->task.build(&this->child_create->task, message);
-
-	state = this->child_sa->get_state(this->child_sa);
-	this->child_sa->set_state(this->child_sa, CHILD_REKEYING);
-
-	if (message->get_payload(message, PLV2_SECURITY_ASSOCIATION) == NULL)
-	{	/* rekeying failed, reuse old child */
-		this->child_sa->set_state(this->child_sa, state);
 		return SUCCESS;
 	}
 
-	child_sa = this->child_create->get_child(this->child_create);
+	if (message->get_exchange_type(message) == CREATE_CHILD_SA)
-	this->child_sa->set_state(this->child_sa, CHILD_REKEYED);
+	{
-	this->child_sa->set_rekey_spi(this->child_sa,
+		reqid = this->child_sa->get_reqid_ref(this->child_sa);
-								  child_sa->get_spi(child_sa, FALSE));
+		if (reqid)
+		{
+			this->child_create->use_reqid(this->child_create, reqid);
+			charon->kernel->release_reqid(charon->kernel, reqid);
+		}
+		this->child_create->use_marks(this->child_create,
+						this->child_sa->get_mark(this->child_sa, TRUE).value,
+						this->child_sa->get_mark(this->child_sa, FALSE).value);
+		this->child_create->use_if_ids(this->child_create,
+						this->child_sa->get_if_id(this->child_sa, TRUE),
+						this->child_sa->get_if_id(this->child_sa, FALSE));
+		this->child_create->use_label(this->child_create,
+						this->child_sa->get_label(this->child_sa));
+		config = this->child_sa->get_config(this->child_sa);
+		this->child_create->set_config(this->child_create,
+									   config->get_ref(config));
+		state = this->child_sa->get_state(this->child_sa);
+		this->child_sa->set_state(this->child_sa, CHILD_REKEYING);
+	}
 
-	/* invoke rekey hook */
+	if (this->child_create->task.build(&this->child_create->task,
-	charon->bus->child_rekey(charon->bus, this->child_sa,
+									   message) == NEED_MORE)
-							 this->child_create->get_child(this->child_create));
+	{
+		/* additional key exchanges */
+		this->flags |= CHILD_REKEY_FOLLOWUP_KE;
+		return NEED_MORE;
+	}
+
+	child_sa = this->child_create->get_child(this->child_create);
+	if (child_sa && child_sa->get_state(child_sa) == CHILD_INSTALLED)
+	{
+		this->child_sa->set_state(this->child_sa, CHILD_REKEYED);
+		this->child_sa->set_rekey_spi(this->child_sa,
+									  child_sa->get_spi(child_sa, FALSE));
+
+		/* FIXME: this might trigger twice if there was a collision */
+		charon->bus->child_rekey(charon->bus, this->child_sa, child_sa);
+	}
+	else if (this->child_sa->get_state(this->child_sa) == CHILD_REKEYING)
+	{	/* rekeying failed, reuse old child */
+		this->child_sa->set_state(this->child_sa, state);
+	}
 	return SUCCESS;
 }
 
+/**
+ * Remove the passive rekey task that's waiting for IKE_FOLLOWUP_KE requests
+ * that will never come.
+ */
+static void remove_passive_rekey_task(private_child_rekey_t *this)
+{
+	enumerator_t *enumerator;
+	task_t *task;
+
+	enumerator = this->ike_sa->create_task_enumerator(this->ike_sa,
+													  TASK_QUEUE_PASSIVE);
+	while (enumerator->enumerate(enumerator, &task))
+	{
+		if (task->get_type(task) == TASK_CHILD_REKEY)
+		{
+			this->ike_sa->remove_task(this->ike_sa, enumerator);
+			task->destroy(task);
+			break;
+		}
+	}
+	enumerator->destroy(enumerator);
+}
+
 /**
  * Handle a rekey collision
  */
 static child_sa_t *handle_collision(private_child_rekey_t *this,
-									child_sa_t **to_install)
+									child_sa_t **to_install, bool multi_ke)
 {
-	child_sa_t *to_delete;
+	private_child_rekey_t *other = (private_child_rekey_t*)this->collision;
+	chunk_t this_nonce, other_nonce;
+	child_sa_t *to_delete, *child_sa;
 
-	if (this->collision->get_type(this->collision) == TASK_CHILD_REKEY)
+	if (this->collision->get_type(this->collision) == TASK_CHILD_DELETE)
-	{
+	{	/* CHILD_DELETE, which we only adopt if it is for the CHILD_SA we are
-		chunk_t this_nonce, other_nonce;
+		 * ourselves rekeying */
-		private_child_rekey_t *other = (private_child_rekey_t*)this->collision;
+		to_delete = this->child_create->get_child(this->child_create);
-
+		if (multi_ke)
-		this_nonce = this->child_create->get_lower_nonce(this->child_create);
-		other_nonce = other->child_create->get_lower_nonce(other->child_create);
-
-		/* if we have the lower nonce, delete rekeyed SA. If not, delete
-		 * the redundant. */
-		if (memcmp(this_nonce.ptr, other_nonce.ptr,
-				   min(this_nonce.len, other_nonce.len)) > 0)
 		{
-			child_sa_t *child_sa;
+			DBG1(DBG_IKE, "CHILD_SA rekey/delete collision, abort incomplete "
-
+				 "multi-KE rekeying");
-			*to_install = this->child_create->get_child(this->child_create);
-			to_delete = this->child_sa;
-			DBG1(DBG_IKE, "CHILD_SA rekey collision won, deleting old child "
-				 "%s{%d}", to_delete->get_name(to_delete),
-				 to_delete->get_unique_id(to_delete));
-			/* don't touch child other created, it has already been deleted */
-			if (!this->other_child_destroyed)
-			{
-				/* disable close action and updown event for redundant child */
-				child_sa = other->child_create->get_child(other->child_create);
-				if (child_sa)
-				{
-					child_sa->set_close_action(child_sa, ACTION_NONE);
-					if (child_sa->get_state(child_sa) != CHILD_REKEYED)
-					{
-						child_sa->set_state(child_sa, CHILD_REKEYED);
-					}
-				}
-			}
 		}
 		else
 		{
-			to_delete = this->child_create->get_child(this->child_create);
-			DBG1(DBG_IKE, "CHILD_SA rekey collision lost, deleting redundant "
-				 "child %s{%d}", to_delete->get_name(to_delete),
-				 to_delete->get_unique_id(to_delete));
-		}
-	}
-	else
-	{	/* CHILD_DELETE */
-		child_delete_t *del = (child_delete_t*)this->collision;
-
-		/* we didn't had a chance to compare the nonces, so we delete
-		 * the CHILD_SA the other is not deleting. */
-		if (del->get_child(del) != this->child_sa)
-		{
-			to_delete = this->child_sa;
-			DBG1(DBG_IKE, "CHILD_SA rekey/delete collision, deleting old child "
-				 "%s{%d}", to_delete->get_name(to_delete),
-				 to_delete->get_unique_id(to_delete));
-		}
-		else
-		{
-			to_delete = this->child_create->get_child(this->child_create);
 			DBG1(DBG_IKE, "CHILD_SA rekey/delete collision, deleting redundant "
 				 "child %s{%d}", to_delete->get_name(to_delete),
 				 to_delete->get_unique_id(to_delete));
 		}
+		return to_delete;
 	}
+
+	this_nonce = this->child_create->get_lower_nonce(this->child_create);
+	other_nonce = other->child_create->get_lower_nonce(other->child_create);
+
+	/* the SA with the lowest nonce should be deleted (if already complete),
+	 * check if we or the peer created it */
+	if (memcmp(this_nonce.ptr, other_nonce.ptr,
+			   min(this_nonce.len, other_nonce.len)) < 0)
+	{
+		to_delete = this->child_create->get_child(this->child_create);
+		if (multi_ke)
+		{
+			DBG1(DBG_IKE, "CHILD_SA rekey collision lost, abort incomplete "
+				 "multi-KE rekeying");
+		}
+		else
+		{
+			DBG1(DBG_IKE, "CHILD_SA rekey collision lost, deleting "
+				 "redundant child %s{%d}", to_delete->get_name(to_delete),
+				 to_delete->get_unique_id(to_delete));
+		}
+		return to_delete;
+	}
+
+	*to_install = this->child_create->get_child(this->child_create);
+	to_delete = this->child_sa;
+
+	/* the passive rekeying is complete only if it was single-KE.  otherwise,
+	 * the peer would either have stopped before sending IKE_FOLLOWUP_KE when
+	 * it noticed it lost, or it responded with TEMPORARY_FAILURE to our
+	 * CREATE_CHILD_SA request if it already started sending them. */
+	if (this->flags & CHILD_REKEY_ADOPTED_PASSIVE)
+	{
+		/* we don't want to install the peer's redundant outbound SA */
+		this->child_sa->set_rekey_spi(this->child_sa, 0);
+		/* don't touch child other created if it has already been deleted */
+		if (!(this->flags & CHILD_REKEY_OTHER_DESTROYED))
+		{
+			/* disable close action and updown event for redundant child the
+			 * other is expected to delete */
+			child_sa = other->child_create->get_child(other->child_create);
+			if (child_sa)
+			{
+				child_sa->set_close_action(child_sa, ACTION_NONE);
+				if (child_sa->get_state(child_sa) != CHILD_REKEYED)
+				{
+					child_sa->set_state(child_sa, CHILD_REKEYED);
+				}
+			}
+		}
+		if (multi_ke)
+		{
+			DBG1(DBG_IKE, "CHILD_SA rekey collision won, continue with "
+				 "multi-KE rekeying");
+			/* change the state back, we are not done rekeying yet */
+			this->child_sa->set_state(this->child_sa, CHILD_REKEYING);
+		}
+		else
+		{
+			DBG1(DBG_IKE, "CHILD_SA rekey collision won, deleting old child "
+				 "%s{%d}", to_delete->get_name(to_delete),
+				 to_delete->get_unique_id(to_delete));
+		}
+		this->collision->destroy(this->collision);
+	}
+	else
+	{
+		/* the peer will not continue with its multi-KE rekeying, so we must
+		 * remove the passive task that's waiting for IKE_FOLLOWUP_KEs */
+		if (multi_ke)
+		{
+			DBG1(DBG_IKE, "CHILD_SA rekey collision won, continue with "
+				 "multi-KE rekeying and remove passive %N task",
+				 task_type_names, TASK_CHILD_REKEY);
+		}
+		else
+		{
+			DBG1(DBG_IKE, "CHILD_SA rekey collision won, remove passive %N "
+				 "task", task_type_names, TASK_CHILD_REKEY);
+		}
+		remove_passive_rekey_task(this);
+	}
+	this->collision = NULL;
 	return to_delete;
 }
 
@@ -404,7 +533,7 @@ METHOD(task_t, process_i, status_t,
 {
 	protocol_id_t protocol;
 	uint32_t spi;
-	child_sa_t *to_delete, *to_install = NULL;
+	child_sa_t *child_sa, *to_delete = NULL, *to_install = NULL;
 
 	if (message->get_notify(message, NO_ADDITIONAL_SAS))
 	{
@@ -459,25 +588,43 @@ METHOD(task_t, process_i, status_t,
 	if (this->child_create->task.process(&this->child_create->task,
 										 message) == NEED_MORE)
 	{
-		/* bad DH group while rekeying, retry, or failure requiring deletion */
+		if (message->get_notify(message, INVALID_KE_PAYLOAD) ||
-		return NEED_MORE;
+			!this->child_create->get_child(this->child_create))
+		{	/* bad key exchange mechanism, retry, or failure requiring delete */
+			return NEED_MORE;
+		}
+		/* multiple key exchanges */
+		this->flags |= CHILD_REKEY_FOLLOWUP_KE;
+		/* there will only be a collision while we process a CREATE_CHILD_SA
+		 * response, later we just respond with TEMPORARY_FAILURE and ignore
+		 * the passive task - if we lost, the returned SA is the one we created
+		 * in this task, since it's not complete yet, we abort the task */
+		if (this->collision)
+		{
+			to_delete = handle_collision(this, &to_install, TRUE);
+		}
+		return (to_delete && to_delete != this->child_sa) ? SUCCESS : NEED_MORE;
 	}
-	if (message->get_payload(message, PLV2_SECURITY_ASSOCIATION) == NULL)
+
+	child_sa = this->child_create->get_child(this->child_create);
+	if (!child_sa || child_sa->get_state(child_sa) != CHILD_INSTALLED)
 	{
 		/* establishing new child failed, reuse old and try again. but not when
-		 * we received a delete in the meantime */
+		 * we received a delete in the meantime or passively rekeyed the SA */
 		if (!this->collision ||
-			 this->collision->get_type(this->collision) != TASK_CHILD_DELETE)
+			(this->collision->get_type(this->collision) != TASK_CHILD_DELETE &&
+			 !(this->flags & CHILD_REKEY_ADOPTED_PASSIVE)))
 		{
 			schedule_delayed_rekey(this);
 		}
 		return SUCCESS;
 	}
 
-	/* check for rekey collisions */
+	/* there won't be a collision if this task is for a multi-KE rekeying, as a
+	 * collision during CREATE_CHILD_SA was cleaned up above */
 	if (this->collision)
 	{
-		to_delete = handle_collision(this, &to_install);
+		to_delete = handle_collision(this, &to_install, FALSE);
 	}
 	else
 	{
@@ -571,18 +718,25 @@ METHOD(child_rekey_t, collide, bool,
 		}
 		/* ignore passive tasks that did not successfully create a CHILD_SA */
 		other_child = rekey->child_create->get_child(rekey->child_create);
-		if (!other_child ||
+		if (!other_child)
-			 other_child->get_state(other_child) != CHILD_INSTALLED)
 		{
 			return FALSE;
 		}
+		if (other_child->get_state(other_child) != CHILD_INSTALLED)
+		{
+			DBG1(DBG_IKE, "colliding passive rekeying is not yet complete",
+				 task_type_names, TASK_CHILD_REKEY);
+			/* we do reference the task to check its state later */
+			this->collision = other;
+			return FALSE;
+		}
 	}
 	else if (other->get_type(other) == TASK_CHILD_DELETE)
 	{
 		child_delete_t *del = (child_delete_t*)other;
 		if (is_redundant(this, del->get_child(del)))
 		{
-			this->other_child_destroyed = TRUE;
+			this->flags |= CHILD_REKEY_OTHER_DESTROYED;
 			return FALSE;
 		}
 		if (del->get_child(del) != this->child_sa)
@@ -596,9 +750,15 @@ METHOD(child_rekey_t, collide, bool,
 		/* shouldn't happen */
 		return FALSE;
 	}
+
 	DBG1(DBG_IKE, "detected %N collision with %N", task_type_names,
 		 TASK_CHILD_REKEY, task_type_names, other->get_type(other));
-	DESTROY_IF(this->collision);
+
+	if (this->flags & CHILD_REKEY_ADOPTED_PASSIVE)
+	{
+		DESTROY_IF(this->collision);
+	}
+	this->flags |= CHILD_REKEY_ADOPTED_PASSIVE;
 	this->collision = other;
 	return TRUE;
 }
@@ -615,7 +775,10 @@ METHOD(task_t, migrate, void,
 	{
 		this->child_create->task.migrate(&this->child_create->task, ike_sa);
 	}
-	DESTROY_IF(this->collision);
+	if (this->flags & CHILD_REKEY_ADOPTED_PASSIVE)
+	{
+		DESTROY_IF(this->collision);
+	}
 
 	this->ike_sa = ike_sa;
 	this->collision = NULL;
@@ -632,7 +795,10 @@ METHOD(task_t, destroy, void,
 	{
 		this->child_delete->task.destroy(&this->child_delete->task);
 	}
-	DESTROY_IF(this->collision);
+	if (this->flags & CHILD_REKEY_ADOPTED_PASSIVE)
+	{
+		DESTROY_IF(this->collision);
+	}
 	chunk_free(&this->spi_data);
 	free(this);
 }