From 207ae20aa611d0ab6d330042295f9ce278868ac3 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Thu, 25 Jun 2026 18:56:23 +0200 Subject: [PATCH] mediation-manager: Avoid potential use-after-free when checking online status This is unlikely to be an issue in practice because only one caller actually uses the ID and it does so immediately afterwards. So there is only a tiny window in which the peer could terminate or rekey its SA to cause the returned ID to get destroyed. Fixes: d5cc1758332e ("experimental P2P-NAT-T for IKEv2 merged back from branch") --- src/libcharon/processing/jobs/mediation_job.c | 4 +++- src/libcharon/sa/ikev2/mediation_manager.c | 4 ++-- src/libcharon/sa/ikev2/mediation_manager.h | 8 ++------ src/libcharon/sa/ikev2/tasks/ike_me.c | 1 + 4 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/libcharon/processing/jobs/mediation_job.c b/src/libcharon/processing/jobs/mediation_job.c index ac30aec80..6d38d467f 100644 --- a/src/libcharon/processing/jobs/mediation_job.c +++ b/src/libcharon/processing/jobs/mediation_job.c @@ -83,12 +83,14 @@ METHOD(job_t, execute, job_requeue_t, { ike_sa_id_t *target_sa_id; - target_sa_id = charon->mediation_manager->check(charon->mediation_manager, this->target); + target_sa_id = charon->mediation_manager->check(charon->mediation_manager, + this->target); if (target_sa_id) { ike_sa_t *target_sa = charon->ike_sa_manager->checkout(charon->ike_sa_manager, target_sa_id); + target_sa_id->destroy(target_sa_id); if (target_sa) { if (this->callback) diff --git a/src/libcharon/sa/ikev2/mediation_manager.c b/src/libcharon/sa/ikev2/mediation_manager.c index 820e748ee..25e22acea 100644 --- a/src/libcharon/sa/ikev2/mediation_manager.c +++ b/src/libcharon/sa/ikev2/mediation_manager.c @@ -259,7 +259,7 @@ METHOD(mediation_manager_t, check, ike_sa_id_t*, return NULL; } - ike_sa_id = peer->ike_sa_id; + ike_sa_id = peer->ike_sa_id ? peer->ike_sa_id->clone(peer->ike_sa_id) : NULL; this->mutex->unlock(this->mutex); @@ -292,7 +292,7 @@ METHOD(mediation_manager_t, check_and_register, ike_sa_id_t*, return NULL; } - ike_sa_id = peer->ike_sa_id; + ike_sa_id = peer->ike_sa_id->clone(peer->ike_sa_id); this->mutex->unlock(this->mutex); diff --git a/src/libcharon/sa/ikev2/mediation_manager.h b/src/libcharon/sa/ikev2/mediation_manager.h index 752306adc..04e52741b 100644 --- a/src/libcharon/sa/ikev2/mediation_manager.h +++ b/src/libcharon/sa/ikev2/mediation_manager.h @@ -54,9 +54,7 @@ struct mediation_manager_t { * Checks if a specific peer is online. * * @param peer_id the peer's ID - * @returns - * - IKE_SA ID of the peer's SA. - * - NULL, if the peer is not online. + * @returns ID of the peer's SA (cloned), NULL if offline */ ike_sa_id_t* (*check) (mediation_manager_t* this, identification_t *peer_id); @@ -67,9 +65,7 @@ struct mediation_manager_t { * * @param peer_id the peer's ID * @param requester the requesters ID - * @returns - * - IKE_SA ID of the peer's SA. - * - NULL, if the peer is not online. + * @returns ID of the peer's SA (cloned), NULL if offline */ ike_sa_id_t* (*check_and_register) (mediation_manager_t* this, identification_t *peer_id, diff --git a/src/libcharon/sa/ikev2/tasks/ike_me.c b/src/libcharon/sa/ikev2/tasks/ike_me.c index 2d30ea1d9..e8e597561 100644 --- a/src/libcharon/sa/ikev2/tasks/ike_me.c +++ b/src/libcharon/sa/ikev2/tasks/ike_me.c @@ -701,6 +701,7 @@ METHOD(task_t, build_r_ms, status_t, chunk_empty); break; } + peer_sa->destroy(peer_sa); job_t *job = (job_t*)mediation_job_create(this->peer_id, this->ike_sa->get_other_id(this->ike_sa), this->connect_id,