From bb3899739d74356b2f365b73917cd0c8eedbdc92 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Sat, 28 May 2016 09:34:29 +0200 Subject: [PATCH] ikev2: Add a new state to track rekeyed IKE_SAs This makes handling such IKE_SAs more specifically compared to keeping them in state IKE_CONNECTING or IKE_ESTABLISHED (which we did when we lost a collision - even triggering the ike_updown event), or using IKE_REKEYING for them, which would also be ambiguous. For instance, we can now reject anything but DELETES for such SAs. --- src/libcharon/sa/ike_sa.c | 5 ++- src/libcharon/sa/ike_sa.h | 5 +++ src/libcharon/sa/ike_sa_manager.c | 3 +- src/libcharon/sa/ikev2/task_manager_v2.c | 35 +++++++++++++----- src/libcharon/sa/ikev2/tasks/ike_delete.c | 6 ++- src/libcharon/sa/ikev2/tasks/ike_rekey.c | 41 ++++++--------------- src/libcharon/tests/suites/test_ike_rekey.c | 2 +- 7 files changed, 53 insertions(+), 44 deletions(-) diff --git a/src/libcharon/sa/ike_sa.c b/src/libcharon/sa/ike_sa.c index b7d71e4d6..e81f6ceef 100644 --- a/src/libcharon/sa/ike_sa.c +++ b/src/libcharon/sa/ike_sa.c @@ -71,6 +71,7 @@ ENUM(ike_sa_state_names, IKE_CREATED, IKE_DESTROYING, "ESTABLISHED", "PASSIVE", "REKEYING", + "REKEYED", "DELETING", "DESTROYING", ); @@ -2356,7 +2357,8 @@ METHOD(ike_sa_t, retransmit, status_t, reestablish(this); break; } - if (this->state != IKE_CONNECTING) + if (this->state != IKE_CONNECTING && + this->state != IKE_REKEYED) { charon->bus->ike_updown(charon->bus, &this->public, FALSE); } @@ -2508,6 +2510,7 @@ METHOD(ike_sa_t, roam, status_t, case IKE_DELETING: case IKE_DESTROYING: case IKE_PASSIVE: + case IKE_REKEYED: return SUCCESS; default: break; diff --git a/src/libcharon/sa/ike_sa.h b/src/libcharon/sa/ike_sa.h index 2122867de..199a32c98 100644 --- a/src/libcharon/sa/ike_sa.h +++ b/src/libcharon/sa/ike_sa.h @@ -308,6 +308,11 @@ enum ike_sa_state_t { */ IKE_REKEYING, + /** + * IKE_SA has been rekeyed (or is redundant) + */ + IKE_REKEYED, + /** * IKE_SA is in progress of deletion */ diff --git a/src/libcharon/sa/ike_sa_manager.c b/src/libcharon/sa/ike_sa_manager.c index 63c9fdfa8..ce44207c4 100644 --- a/src/libcharon/sa/ike_sa_manager.c +++ b/src/libcharon/sa/ike_sa_manager.c @@ -1415,7 +1415,8 @@ METHOD(ike_sa_manager_t, checkout_by_config, ike_sa_t*, { continue; } - if (entry->ike_sa->get_state(entry->ike_sa) == IKE_DELETING) + if (entry->ike_sa->get_state(entry->ike_sa) == IKE_DELETING || + entry->ike_sa->get_state(entry->ike_sa) == IKE_REKEYED) { /* skip IKE_SAs which are not usable, wake other waiting threads */ entry->condvar->signal(entry->condvar); continue; diff --git a/src/libcharon/sa/ikev2/task_manager_v2.c b/src/libcharon/sa/ikev2/task_manager_v2.c index 702c383b1..26e5b48a8 100644 --- a/src/libcharon/sa/ikev2/task_manager_v2.c +++ b/src/libcharon/sa/ikev2/task_manager_v2.c @@ -535,6 +535,7 @@ METHOD(task_manager_t, initiate, status_t, break; } case IKE_REKEYING: + case IKE_REKEYED: if (activate_task(this, TASK_IKE_DELETE)) { exchange = INFORMATIONAL; @@ -611,7 +612,8 @@ METHOD(task_manager_t, initiate, status_t, case FAILED: default: this->initiating.type = EXCHANGE_TYPE_UNDEFINED; - if (this->ike_sa->get_state(this->ike_sa) != IKE_CONNECTING) + if (this->ike_sa->get_state(this->ike_sa) != IKE_CONNECTING && + this->ike_sa->get_state(this->ike_sa) != IKE_REKEYED) { charon->bus->ike_updown(charon->bus, this->ike_sa, FALSE); } @@ -909,9 +911,11 @@ static status_t process_request(private_task_manager_t *this, payload_t *payload; notify_payload_t *notify; delete_payload_t *delete; + ike_sa_state_t state; if (array_count(this->passive_tasks) == 0) { /* create tasks depending on request type, if not already some queued */ + state = this->ike_sa->get_state(this->ike_sa); switch (message->get_exchange_type(message)) { case IKE_SA_INIT: @@ -947,8 +951,8 @@ static status_t process_request(private_task_manager_t *this, { /* FIXME: we should prevent this on mediation connections */ bool notify_found = FALSE, ts_found = FALSE; - if (this->ike_sa->get_state(this->ike_sa) == IKE_CREATED || - this->ike_sa->get_state(this->ike_sa) == IKE_CONNECTING) + if (state == IKE_CREATED || + state == IKE_CONNECTING) { DBG1(DBG_IKE, "received CREATE_CHILD_SA request for " "unestablished IKE_SA, rejected"); @@ -1013,6 +1017,14 @@ static status_t process_request(private_task_manager_t *this, case PLV2_NOTIFY: { notify = (notify_payload_t*)payload; + if (state == IKE_REKEYED) + { + DBG1(DBG_IKE, "received unexpected notify %N " + "for rekeyed IKE_SA, ignored", + notify_type_names, + notify->get_notify_type(notify)); + break; + } switch (notify->get_notify_type(notify)) { case ADDITIONAL_IP4_ADDRESS: @@ -1355,6 +1367,8 @@ METHOD(task_manager_t, process_message, status_t, status_t status; uint32_t mid; bool schedule_delete_job = FALSE; + ike_sa_state_t state; + exchange_type_t type; charon->bus->message(charon->bus, msg, TRUE, FALSE); status = parse_message(this, msg); @@ -1395,15 +1409,16 @@ METHOD(task_manager_t, process_message, status_t, { if (mid == this->responding.mid) { - /* reject initial messages if not received in specific states */ - if ((msg->get_exchange_type(msg) == IKE_SA_INIT && - this->ike_sa->get_state(this->ike_sa) != IKE_CREATED) || - (msg->get_exchange_type(msg) == IKE_AUTH && - this->ike_sa->get_state(this->ike_sa) != IKE_CONNECTING)) + /* reject initial messages if not received in specific states, + * after rekeying we only expect a DELETE in an INFORMATIONAL */ + type = msg->get_exchange_type(msg); + state = this->ike_sa->get_state(this->ike_sa); + if ((type == IKE_SA_INIT && state != IKE_CREATED) || + (type == IKE_AUTH && state != IKE_CONNECTING) || + (state == IKE_REKEYED && type != INFORMATIONAL)) { DBG1(DBG_IKE, "ignoring %N in IKE_SA state %N", - exchange_type_names, msg->get_exchange_type(msg), - ike_sa_state_names, this->ike_sa->get_state(this->ike_sa)); + exchange_type_names, type, ike_sa_state_names, state); return FAILED; } if (!this->ike_sa->supports_extension(this->ike_sa, EXT_MOBIKE)) diff --git a/src/libcharon/sa/ikev2/tasks/ike_delete.c b/src/libcharon/sa/ikev2/tasks/ike_delete.c index e972dba07..72d656aae 100644 --- a/src/libcharon/sa/ikev2/tasks/ike_delete.c +++ b/src/libcharon/sa/ikev2/tasks/ike_delete.c @@ -68,7 +68,8 @@ METHOD(task_t, build_i, status_t, delete_payload = delete_payload_create(PLV2_DELETE, PROTO_IKE); message->add_payload(message, (payload_t*)delete_payload); - if (this->ike_sa->get_state(this->ike_sa) == IKE_REKEYING) + if (this->ike_sa->get_state(this->ike_sa) == IKE_REKEYING || + this->ike_sa->get_state(this->ike_sa) == IKE_REKEYED) { this->rekeyed = TRUE; } @@ -119,11 +120,12 @@ METHOD(task_t, process_r, status_t, switch (this->ike_sa->get_state(this->ike_sa)) { + case IKE_REKEYING: case IKE_ESTABLISHED: this->ike_sa->set_state(this->ike_sa, IKE_DELETING); this->ike_sa->reestablish(this->ike_sa); return NEED_MORE; - case IKE_REKEYING: + case IKE_REKEYED: this->rekeyed = TRUE; break; case IKE_DELETING: diff --git a/src/libcharon/sa/ikev2/tasks/ike_rekey.c b/src/libcharon/sa/ikev2/tasks/ike_rekey.c index b67f28f67..334749a61 100644 --- a/src/libcharon/sa/ikev2/tasks/ike_rekey.c +++ b/src/libcharon/sa/ikev2/tasks/ike_rekey.c @@ -84,7 +84,6 @@ static job_t* check_queued_tasks(ike_sa_t *ike_sa) job = (job_t*)initiate_tasks_job_create(ike_sa->get_id(ike_sa)); } enumerator->destroy(enumerator); - return job; } @@ -118,21 +117,11 @@ static void establish_new(private_ike_rekey_t *this) } this->new_sa = NULL; charon->bus->set_sa(charon->bus, this->ike_sa); + + this->ike_sa->set_state(this->ike_sa, IKE_REKEYED); } } -METHOD(task_t, process_r_delete, status_t, - private_ike_rekey_t *this, message_t *message) -{ - return this->ike_delete->task.process(&this->ike_delete->task, message); -} - -METHOD(task_t, build_r_delete, status_t, - private_ike_rekey_t *this, message_t *message) -{ - return this->ike_delete->task.build(&this->ike_delete->task, message); -} - METHOD(task_t, build_i_delete, status_t, private_ike_rekey_t *this, message_t *message) { @@ -236,21 +225,12 @@ METHOD(task_t, build_r, status_t, if (this->ike_sa->get_state(this->ike_sa) != IKE_REKEYING) { /* in case of a collision we let the initiating task handle this */ establish_new(this); - this->ike_sa->set_state(this->ike_sa, IKE_REKEYING); + /* make sure the IKE_SA is gone in case the peer fails to delete it */ + lib->scheduler->schedule_job(lib->scheduler, (job_t*) + delete_ike_sa_job_create(this->ike_sa->get_id(this->ike_sa), TRUE), + 90); } - - /* rekeying successful, delete the IKE_SA using a subtask */ - this->ike_delete = ike_delete_create(this->ike_sa, FALSE); - this->public.task.build = _build_r_delete; - this->public.task.process = _process_r_delete; - - /* the peer does have to delete the IKE_SA. If it does not, we get a - * unusable IKE_SA in REKEYING state without a replacement. We consider - * this a timeout condition by the peer, and trigger a delete actively. */ - lib->scheduler->schedule_job(lib->scheduler, (job_t*) - delete_ike_sa_job_create(this->ike_sa->get_id(this->ike_sa), TRUE), 90); - - return NEED_MORE; + return SUCCESS; } METHOD(task_t, process_i, status_t, @@ -316,11 +296,13 @@ METHOD(task_t, process_i, status_t, /* peer should delete this SA. Add a timeout just in case. */ job_t *job = (job_t*)delete_ike_sa_job_create( other->new_sa->get_id(other->new_sa), TRUE); - lib->scheduler->schedule_job(lib->scheduler, job, 10); + lib->scheduler->schedule_job(lib->scheduler, job, + HALF_OPEN_IKE_SA_TIMEOUT); DBG1(DBG_IKE, "IKE_SA rekey collision won, waiting for delete " "for redundant IKE_SA %s[%d]", other->new_sa->get_name(other->new_sa), other->new_sa->get_unique_id(other->new_sa)); + other->new_sa->set_state(other->new_sa, IKE_REKEYED); charon->ike_sa_manager->checkin(charon->ike_sa_manager, other->new_sa); other->new_sa = NULL; @@ -335,7 +317,8 @@ METHOD(task_t, process_i, status_t, this->new_sa->set_my_host(this->new_sa, host->clone(host)); host = this->ike_sa->get_other_host(this->ike_sa); this->new_sa->set_other_host(this->new_sa, host->clone(host)); - this->ike_sa->set_state(this->ike_sa, IKE_ESTABLISHED); + /* IKE_SAs in state IKE_REKEYED are silently deleted, so we use + * IKE_REKEYING */ this->new_sa->set_state(this->new_sa, IKE_REKEYING); if (this->new_sa->delete(this->new_sa) == DESTROY_ME) { diff --git a/src/libcharon/tests/suites/test_ike_rekey.c b/src/libcharon/tests/suites/test_ike_rekey.c index d2e365ed1..8fa6ea315 100644 --- a/src/libcharon/tests/suites/test_ike_rekey.c +++ b/src/libcharon/tests/suites/test_ike_rekey.c @@ -51,7 +51,7 @@ START_TEST(test_regular) assert_hook_rekey(ike_rekey, 1, 3); assert_no_notify(IN, REKEY_SA); exchange_test_helper->process_message(exchange_test_helper, b, NULL); - assert_ike_sa_state(b, IKE_REKEYING); + assert_ike_sa_state(b, IKE_REKEYED); assert_child_sa_count(b, 0); new_sa = assert_ike_sa_checkout(3, 4, FALSE); assert_ike_sa_state(new_sa, IKE_ESTABLISHED);