From: Tobias Brunner Date: Mon, 29 Jun 2020 12:12:05 +0000 (+0200) Subject: child-rekey: Support CHILD_SA rekeying with multiple key exchanges X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b37c738feba287f6bc89fae1b582e89ec0c543c9;p=thirdparty%2Fstrongswan.git child-rekey: Support CHILD_SA rekeying with multiple key exchanges --- diff --git a/src/libcharon/sa/ikev2/tasks/child_create.c b/src/libcharon/sa/ikev2/tasks/child_create.c index 9c99739aca..d2eb92193a 100644 --- a/src/libcharon/sa/ikev2/tasks/child_create.c +++ b/src/libcharon/sa/ikev2/tasks/child_create.c @@ -2193,6 +2193,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; diff --git a/src/libcharon/sa/ikev2/tasks/child_delete.c b/src/libcharon/sa/ikev2/tasks/child_delete.c index 54aba2c506..b9a0a509de 100644 --- a/src/libcharon/sa/ikev2/tasks/child_delete.c +++ b/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) diff --git a/src/libcharon/sa/ikev2/tasks/child_rekey.c b/src/libcharon/sa/ikev2/tasks/child_rekey.c index 9281c6339d..79362cdf78 100644 --- a/src/libcharon/sa/ikev2/tasks/child_rekey.c +++ b/src/libcharon/sa/ikev2/tasks/child_rekey.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2018 Tobias Brunner + * Copyright (C) 2009-2020 Tobias Brunner * Copyright (C) 2005-2007 Martin Willi * Copyright (C) 2005 Jan Hutter * @@ -79,13 +79,32 @@ struct private_child_rekey_t { task_t *collision; /** - * Indicate that peer destroyed the redundant child from collision. - * 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. + * State flags */ - 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; }; /** @@ -159,8 +178,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); @@ -184,15 +201,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 dh_group; + 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); @@ -202,17 +220,17 @@ METHOD(task_t, build_i, status_t, { /* reuse the DH group negotiated previously */ this->child_create->use_dh_group(this->child_create, dh_group); } - } - reqid = this->child_sa->get_reqid(this->child_sa); - this->child_create->use_reqid(this->child_create, reqid); - this->child_create->use_marks(this->child_create, + reqid = this->child_sa->get_reqid(this->child_sa); + this->child_create->use_reqid(this->child_create, 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_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) @@ -245,13 +263,47 @@ 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) { 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) { @@ -265,114 +317,191 @@ 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) + { + DBG1(DBG_IKE, "peer initiated rekeying, but we did too and already " + "sent IKE_FOLLOWUP_KE"); + message->add_notify(message, TRUE, TEMPORARY_FAILURE, chunk_empty); + return SUCCESS; + } - /* let the CHILD_CREATE task build the response */ - reqid = this->child_sa->get_reqid(this->child_sa); - this->child_create->use_reqid(this->child_create, reqid); - this->child_create->use_marks(this->child_create, + if (message->get_exchange_type(message) == CREATE_CHILD_SA) + { + reqid = this->child_sa->get_reqid(this->child_sa); + this->child_create->use_reqid(this->child_create, 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_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)); - 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); + 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); + } - state = this->child_sa->get_state(this->child_sa); - this->child_sa->set_state(this->child_sa, CHILD_REKEYING); + if (this->child_create->task.build(&this->child_create->task, + message) == NEED_MORE) + { + /* 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)); - if (message->get_payload(message, PLV2_SECURITY_ASSOCIATION) == NULL) + /* 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; } + return SUCCESS; +} - child_sa = this->child_create->get_child(this->child_create); - 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)); +/** + * 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; - /* invoke rekey hook */ - charon->bus->child_rekey(charon->bus, this->child_sa, - this->child_create->get_child(this->child_create)); - return SUCCESS; + 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_DELETE) + { /* CHILD_DELETE, which we only adopt if it is for the CHILD_SA we are + * ourselves rekeying */ + to_delete = this->child_create->get_child(this->child_create); + if (multi_ke) + { + DBG1(DBG_IKE, "CHILD_SA rekey/delete collision, abort incomplete " + "multi-KE rekeying"); + } + else + { + 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; + } - if (this->collision->get_type(this->collision) == TASK_CHILD_REKEY) + 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) { - chunk_t this_nonce, other_nonce; - private_child_rekey_t *other = (private_child_rekey_t*)this->collision; + 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; + } - this_nonce = this->child_create->get_lower_nonce(this->child_create); - other_nonce = other->child_create->get_lower_nonce(other->child_create); + *to_install = this->child_create->get_child(this->child_create); + to_delete = this->child_sa; - /* 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) + /* 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)) { - child_sa_t *child_sa; - - *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 the + * other is expected to delete */ + child_sa = other->child_create->get_child(other->child_create); + if (child_sa) { - /* 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_close_action(child_sa, ACTION_NONE); - if (child_sa->get_state(child_sa) != CHILD_REKEYED) - { - child_sa->set_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 { - 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), + 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 - { /* 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) + { + /* 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) { - 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)); + 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 { - 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)); + 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; } @@ -381,7 +510,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)) { @@ -432,25 +561,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 */ - return NEED_MORE; + if (message->get_notify(message, INVALID_KE_PAYLOAD) || + !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 { @@ -544,9 +691,16 @@ 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 || - other_child->get_state(other_child) != CHILD_INSTALLED) + if (!other_child) + { + 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; } } @@ -555,7 +709,7 @@ METHOD(child_rekey_t, collide, bool, 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) @@ -569,9 +723,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; } @@ -588,7 +748,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; @@ -605,7 +768,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); + } free(this); }