From 9ae6b507b972e7c36253109f5fcd617e0bec005a Mon Sep 17 00:00:00 2001 From: Adrian-Ken Rueegsegger Date: Wed, 8 Apr 2015 19:52:44 +0200 Subject: [PATCH 01/12] charon-tkm: Fix SAD insertion when adding ESA Commit f5fc592 added the reqid to the SAD. The insert call swapped the order of the esa_id and reqid parameters. --- src/charon-tkm/src/tkm/tkm_kernel_ipsec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c b/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c index 734b1ec55..30c8e1022 100644 --- a/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c +++ b/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c @@ -132,7 +132,7 @@ METHOD(kernel_ipsec_t, add_sa, status_t, } esa_id = tkm->idmgr->acquire_id(tkm->idmgr, TKM_CTX_ESA); - if (!tkm->sad->insert(tkm->sad, reqid, esa_id, local, peer, spi_rem, + if (!tkm->sad->insert(tkm->sad, esa_id, reqid, local, peer, spi_rem, protocol)) { DBG1(DBG_KNL, "unable to add entry (%llu) to SAD", esa_id); From 8262be3cdc7275015c59af0e341c5526e2472d99 Mon Sep 17 00:00:00 2001 From: Adrian-Ken Rueegsegger Date: Wed, 8 Apr 2015 18:06:51 +0200 Subject: [PATCH 02/12] Add bool param to ALERT_KEEP_ON_CHILD_SA_FAILURE alert The parameter indicates if the alert is raised upon failure to establish the first CHILD SA of an IKE SA. --- src/libcharon/bus/bus.h | 3 ++- src/libcharon/sa/ikev2/tasks/child_create.c | 8 ++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/libcharon/bus/bus.h b/src/libcharon/bus/bus.h index 051c429f9..47b8820d3 100644 --- a/src/libcharon/bus/bus.h +++ b/src/libcharon/bus/bus.h @@ -130,7 +130,8 @@ enum alert_t { ALERT_UNIQUE_REPLACE, /** IKE_SA deleted because of "keep" unique policy, no argument */ ALERT_UNIQUE_KEEP, - /** IKE_SA kept on failed child SA establishment, no argument */ + /** IKE_SA kept on failed child SA establishment, argument is an int (!=0 if + * first child SA) */ ALERT_KEEP_ON_CHILD_SA_FAILURE, /** allocating virtual IP failed, linked_list_t of host_t requested */ ALERT_VIP_FAILURE, diff --git a/src/libcharon/sa/ikev2/tasks/child_create.c b/src/libcharon/sa/ikev2/tasks/child_create.c index 6d9132a68..6e00ebada 100644 --- a/src/libcharon/sa/ikev2/tasks/child_create.c +++ b/src/libcharon/sa/ikev2/tasks/child_create.c @@ -1092,7 +1092,10 @@ METHOD(task_t, process_r, status_t, static void handle_child_sa_failure(private_child_create_t *this, message_t *message) { - if (message->get_exchange_type(message) == IKE_AUTH && + bool is_first; + + is_first = message->get_exchange_type(message) == IKE_AUTH; + if (is_first && lib->settings->get_bool(lib->settings, "%s.close_ike_on_child_failure", FALSE, lib->ns)) { @@ -1106,7 +1109,8 @@ static void handle_child_sa_failure(private_child_create_t *this, else { DBG1(DBG_IKE, "failed to establish CHILD_SA, keeping IKE_SA"); - charon->bus->alert(charon->bus, ALERT_KEEP_ON_CHILD_SA_FAILURE); + charon->bus->alert(charon->bus, ALERT_KEEP_ON_CHILD_SA_FAILURE, + is_first); } } From ded14df60336e6c5769bb8a4242ffa625da117aa Mon Sep 17 00:00:00 2001 From: Adrian-Ken Rueegsegger Date: Wed, 8 Apr 2015 18:11:20 +0200 Subject: [PATCH 03/12] charon-tkm: Only skip creation of first child SA Use the new is_first boolean parameter of the ALERT_KEEP_ON_CHILD_SA_FAILURE alert to determine if the failure was caused by the first CHILD SA. --- src/charon-tkm/src/tkm/tkm_listener.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/charon-tkm/src/tkm/tkm_listener.c b/src/charon-tkm/src/tkm/tkm_listener.c index bb1218266..f57527602 100644 --- a/src/charon-tkm/src/tkm/tkm_listener.c +++ b/src/charon-tkm/src/tkm/tkm_listener.c @@ -14,6 +14,8 @@ * for more details. */ +#include + #include #include #include @@ -209,6 +211,13 @@ METHOD(listener_t, alert, bool, { tkm_keymat_t *keymat; isa_id_type isa_id; + int is_first; + + is_first = va_arg(args, int); + if (!is_first) + { + return TRUE; + } keymat = (tkm_keymat_t*)ike_sa->get_keymat(ike_sa); isa_id = keymat->get_isa_id(keymat); From c7ce0d96cd89b8071637cd59879776b01053f28a Mon Sep 17 00:00:00 2001 From: Adrian-Ken Rueegsegger Date: Wed, 8 Apr 2015 18:54:54 +0200 Subject: [PATCH 04/12] charon-tkm: Add get_other_esa_id function to TKM kernel SAD The function gets the ESA id for another entry associated with the same security policy as the specified ESA. --- src/charon-tkm/src/tkm/tkm_kernel_sad.c | 57 +++++++++++++++++++++++++ src/charon-tkm/src/tkm/tkm_kernel_sad.h | 10 +++++ src/charon-tkm/tests/kernel_sad_tests.c | 36 ++++++++++++++++ 3 files changed, 103 insertions(+) diff --git a/src/charon-tkm/src/tkm/tkm_kernel_sad.c b/src/charon-tkm/src/tkm/tkm_kernel_sad.c index 3394b58af..619ebeeb6 100644 --- a/src/charon-tkm/src/tkm/tkm_kernel_sad.c +++ b/src/charon-tkm/src/tkm/tkm_kernel_sad.c @@ -126,6 +126,26 @@ static bool sad_entry_match_dst(sad_entry_t * const entry, entry->proto == *proto; } +/** + * Find a list entry with given esa id. + */ +static bool sad_entry_match_esa_id(sad_entry_t * const entry, + const esa_id_type * const esa_id) +{ + return entry->esa_id == *esa_id; +} + +/** + * Find a list entry with given reqid and different esa id. + */ +static bool sad_entry_match_other_esa(sad_entry_t * const entry, + const esa_id_type * const esa_id, + const u_int32_t * const reqid) +{ + return entry->reqid == *reqid && + entry->esa_id != *esa_id; +} + /** * Compare two SAD entries for equality. */ @@ -209,6 +229,42 @@ METHOD(tkm_kernel_sad_t, get_esa_id, esa_id_type, return id; } +METHOD(tkm_kernel_sad_t, get_other_esa_id, esa_id_type, + private_tkm_kernel_sad_t * const this, const esa_id_type esa_id) +{ + esa_id_type id = 0; + sad_entry_t *entry = NULL; + u_int32_t reqid; + status_t res; + + this->mutex->lock(this->mutex); + res = this->data->find_first(this->data, + (linked_list_match_t)sad_entry_match_esa_id, + (void**)&entry, &esa_id); + if (res == SUCCESS && entry) + { + reqid = entry->reqid; + } + else + { + DBG3(DBG_KNL, "no SAD entry found for ESA id %llu", esa_id); + this->mutex->unlock(this->mutex); + return id; + } + + res = this->data->find_first(this->data, + (linked_list_match_t)sad_entry_match_other_esa, + (void**)&entry, &esa_id, &reqid); + if (res == SUCCESS && entry) + { + id = entry->esa_id; + DBG3(DBG_KNL, "returning ESA id %llu of other SAD entry with reqid %u", + id, reqid); + } + this->mutex->unlock(this->mutex); + return id; +} + METHOD(tkm_kernel_sad_t, get_dst_host, host_t *, private_tkm_kernel_sad_t * const this, const u_int32_t reqid, const u_int32_t spi, const u_int8_t proto) @@ -289,6 +345,7 @@ tkm_kernel_sad_t *tkm_kernel_sad_create() .public = { .insert = _insert, .get_esa_id = _get_esa_id, + .get_other_esa_id = _get_other_esa_id, .get_dst_host = _get_dst_host, .remove = __remove, .destroy = _destroy, diff --git a/src/charon-tkm/src/tkm/tkm_kernel_sad.h b/src/charon-tkm/src/tkm/tkm_kernel_sad.h index 38b19dd01..fd4830805 100644 --- a/src/charon-tkm/src/tkm/tkm_kernel_sad.h +++ b/src/charon-tkm/src/tkm/tkm_kernel_sad.h @@ -62,6 +62,16 @@ struct tkm_kernel_sad_t { const host_t * const src, const host_t * const dst, const u_int32_t spi, const u_int8_t proto); + /** + * Get ESA id for entry associated with same security policy as the + * specified ESA. + * + * @param esa_id id of ESA identifying the security policy + * @return ESA id of entry if found, 0 otherwise + */ + esa_id_type (*get_other_esa_id)(tkm_kernel_sad_t * const this, + const esa_id_type esa_id); + /** * Get destination host for entry with given parameters. * diff --git a/src/charon-tkm/tests/kernel_sad_tests.c b/src/charon-tkm/tests/kernel_sad_tests.c index b9ab3cb5e..91ccdd4dd 100644 --- a/src/charon-tkm/tests/kernel_sad_tests.c +++ b/src/charon-tkm/tests/kernel_sad_tests.c @@ -81,6 +81,37 @@ START_TEST(test_get_esa_id_nonexistent) } END_TEST +START_TEST(test_get_other_esa_id) +{ + host_t *addr = host_create_from_string("127.0.0.1", 1024); + tkm_kernel_sad_t *sad = tkm_kernel_sad_create(); + fail_unless(sad->insert(sad, 23, 54, addr, addr, 42, 50), + "Error inserting SAD entry"); + fail_unless(sad->insert(sad, 24, 54, addr, addr, 42, 50), + "Error inserting SAD entry"); + fail_unless(sad->get_other_esa_id(sad, 23) == 24, + "Error getting other esa id"); + sad->destroy(sad); + addr->destroy(addr); +} +END_TEST + +START_TEST(test_get_other_esa_id_nonexistent) +{ + host_t *addr = host_create_from_string("127.0.0.1", 1024); + tkm_kernel_sad_t *sad = tkm_kernel_sad_create(); + fail_unless(sad->get_other_esa_id(sad, 1) == 0, + "Got other esa id for nonexistent SAD entry"); + fail_unless(sad->insert(sad, 23, 54, addr, addr, 42, 50), + "Error inserting SAD entry"); + fail_unless(sad->get_other_esa_id(sad, 23) == 0, + "Got own esa id"); + + sad->destroy(sad); + addr->destroy(addr); +} +END_TEST + START_TEST(test_get_dst_host) { host_t *addr = host_create_from_string("127.0.0.1", 1024); @@ -151,6 +182,11 @@ Suite *make_kernel_sad_tests() tcase_add_test(tc, test_get_esa_id_nonexistent); suite_add_tcase(s, tc); + tc = tcase_create("get_other_esa_id"); + tcase_add_test(tc, test_get_other_esa_id); + tcase_add_test(tc, test_get_other_esa_id_nonexistent); + suite_add_tcase(s, tc); + tc = tcase_create("get_dst_host"); tcase_add_test(tc, test_get_dst_host); tcase_add_test(tc, test_get_dst_host_nonexistent); From 2783bd17a401f961d4cf2e126f28f2ea5dcc41d6 Mon Sep 17 00:00:00 2001 From: Adrian-Ken Rueegsegger Date: Wed, 8 Apr 2015 18:56:23 +0200 Subject: [PATCH 05/12] charon-tkm: Select other ESA if any is present upon deletion In the case that multiple ESAs exist (e.g. rekey collision) for a security policy, make sure to select one of the remaining ESAs. --- src/charon-tkm/src/tkm/tkm_kernel_ipsec.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c b/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c index 30c8e1022..e94a3b5c2 100644 --- a/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c +++ b/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c @@ -217,11 +217,22 @@ METHOD(kernel_ipsec_t, del_sa, status_t, private_tkm_kernel_ipsec_t *this, host_t *src, host_t *dst, u_int32_t spi, u_int8_t protocol, u_int16_t cpi, mark_t mark) { - esa_id_type esa_id; + esa_id_type esa_id, other_esa_id; esa_id = tkm->sad->get_esa_id(tkm->sad, src, dst, spi, protocol); if (esa_id) { + other_esa_id = tkm->sad->get_other_esa_id(tkm->sad, esa_id); + if (other_esa_id) + { + DBG1(DBG_KNL, "selecting child SA (esa: %llu)", other_esa_id); + if (ike_esa_select(other_esa_id) != TKM_OK) + { + DBG1(DBG_KNL, "error selecting other child SA (esa: %llu)", + other_esa_id); + } + } + DBG1(DBG_KNL, "deleting child SA (esa: %llu, spi: %x)", esa_id, ntohl(spi)); if (ike_esa_reset(esa_id) != TKM_OK) From a0cf92a65056588bc8729f44fa687618b8bc1214 Mon Sep 17 00:00:00 2001 From: Reto Buerki Date: Wed, 22 Apr 2015 15:52:43 +0200 Subject: [PATCH 06/12] charon-tkm: Drop obsolete TKM_LIMIT define --- src/charon-tkm/src/tkm/tkm_id_manager.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/charon-tkm/src/tkm/tkm_id_manager.c b/src/charon-tkm/src/tkm/tkm_id_manager.c index e6d571b83..d8ff6753f 100644 --- a/src/charon-tkm/src/tkm/tkm_id_manager.c +++ b/src/charon-tkm/src/tkm/tkm_id_manager.c @@ -19,8 +19,6 @@ #include #include -#define TKM_LIMIT 100 - ENUM_BEGIN(tkm_context_kind_names, TKM_CTX_NONCE, TKM_CTX_ESA, "NONCE_CONTEXT", "DH_CONTEXT", From 5460098ccee93ad1a80a7593a502642dad2568bd Mon Sep 17 00:00:00 2001 From: Adrian-Ken Rueegsegger Date: Wed, 22 Apr 2015 16:37:23 +0200 Subject: [PATCH 07/12] charon-tkm: Remove ESA nonce mappings from chunk map --- src/charon-tkm/src/tkm/tkm_kernel_ipsec.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c b/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c index e94a3b5c2..d751a06cd 100644 --- a/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c +++ b/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c @@ -164,6 +164,7 @@ METHOD(kernel_ipsec_t, add_sa, status_t, DBG1(DBG_KNL, "child SA (%llu, no PFS) creation failed", esa_id); goto failure; } + tkm->chunk_map->remove(tkm->chunk_map, nonce_loc); tkm->idmgr->release_id(tkm->idmgr, TKM_CTX_NONCE, nonce_loc_id); } /* creation of subsequent child SA with PFS: nonce and dh context are set */ @@ -176,6 +177,7 @@ METHOD(kernel_ipsec_t, add_sa, status_t, DBG1(DBG_KNL, "child SA (%llu) creation failed", esa_id); goto failure; } + tkm->chunk_map->remove(tkm->chunk_map, nonce_loc); tkm->idmgr->release_id(tkm->idmgr, TKM_CTX_NONCE, nonce_loc_id); } if (ike_esa_select(esa_id) != TKM_OK) From a8ca50e6358cf245806ebbf15e83039bd9daf085 Mon Sep 17 00:00:00 2001 From: Reto Buerki Date: Thu, 23 Apr 2015 10:06:49 +0200 Subject: [PATCH 08/12] charon-tkm: Drop unneeded nonceg get_id function --- src/charon-tkm/src/tkm/tkm_nonceg.c | 7 ------- src/charon-tkm/src/tkm/tkm_nonceg.h | 8 -------- src/charon-tkm/tests/nonceg_tests.c | 1 - 3 files changed, 16 deletions(-) diff --git a/src/charon-tkm/src/tkm/tkm_nonceg.c b/src/charon-tkm/src/tkm/tkm_nonceg.c index a07326798..89baab7ce 100644 --- a/src/charon-tkm/src/tkm/tkm_nonceg.c +++ b/src/charon-tkm/src/tkm/tkm_nonceg.c @@ -71,12 +71,6 @@ METHOD(nonce_gen_t, destroy, void, free(this); } -METHOD(tkm_nonceg_t, get_id, nc_id_type, - private_tkm_nonceg_t *this) -{ - return this->context_id; -} - /* * Described in header. */ @@ -91,7 +85,6 @@ tkm_nonceg_t *tkm_nonceg_create() .allocate_nonce = _allocate_nonce, .destroy = _destroy, }, - .get_id = _get_id, }, .context_id = tkm->idmgr->acquire_id(tkm->idmgr, TKM_CTX_NONCE), ); diff --git a/src/charon-tkm/src/tkm/tkm_nonceg.h b/src/charon-tkm/src/tkm/tkm_nonceg.h index ceadb081f..d158551fe 100644 --- a/src/charon-tkm/src/tkm/tkm_nonceg.h +++ b/src/charon-tkm/src/tkm/tkm_nonceg.h @@ -36,14 +36,6 @@ struct tkm_nonceg_t { * Implements nonce_gen_t. */ nonce_gen_t nonce_gen; - - /** - * Get nonce context id. - * - * @return context id of this nonce generator. - */ - nc_id_type (*get_id)(tkm_nonceg_t * const this); - }; /** diff --git a/src/charon-tkm/tests/nonceg_tests.c b/src/charon-tkm/tests/nonceg_tests.c index 6f524cb22..d150891eb 100644 --- a/src/charon-tkm/tests/nonceg_tests.c +++ b/src/charon-tkm/tests/nonceg_tests.c @@ -27,7 +27,6 @@ START_TEST(test_nonceg_creation) ng = tkm_nonceg_create(); fail_if(ng == NULL, "Error creating tkm nonce generator"); - fail_if(ng->get_id(ng) == 0, "Invalid context id (0)"); ng->nonce_gen.destroy(&ng->nonce_gen); } From 8cdc563258db508ce7a848f63f68cbe37b4f40e0 Mon Sep 17 00:00:00 2001 From: Reto Buerki Date: Thu, 23 Apr 2015 09:41:12 +0200 Subject: [PATCH 09/12] charon-tkm: Reset stale nonce contexts If the nonce generator detects a stale nonce upon destroy(), it resets the context in the TKM and releases associated resources in the ID manager and chunk map. Also, do not acquire the nonce context ID in tkm_nonceg_create function but rather when the nonce is actually created by get_nonce(). The nonces created with get_nonce must also be registered in the chunk map. --- src/charon-tkm/src/tkm/tkm_nonceg.c | 46 ++++++++++++++++++----------- src/charon-tkm/tests/keymat_tests.c | 2 +- 2 files changed, 30 insertions(+), 18 deletions(-) diff --git a/src/charon-tkm/src/tkm/tkm_nonceg.c b/src/charon-tkm/src/tkm/tkm_nonceg.c index 89baab7ce..336f16ecd 100644 --- a/src/charon-tkm/src/tkm/tkm_nonceg.c +++ b/src/charon-tkm/src/tkm/tkm_nonceg.c @@ -33,23 +33,32 @@ struct private_tkm_nonceg_t { tkm_nonceg_t public; /** - * Context id. + * Nonce chunk. */ - nc_id_type context_id; - + chunk_t nonce; }; METHOD(nonce_gen_t, get_nonce, bool, private_tkm_nonceg_t *this, size_t size, u_int8_t *buffer) { nonce_type nonce; + uint64_t nc_id; - if (ike_nc_create(this->context_id, size, &nonce) != TKM_OK) + nc_id = tkm->idmgr->acquire_id(tkm->idmgr, TKM_CTX_NONCE); + if (!nc_id) { return FALSE; } + if (ike_nc_create(nc_id, size, &nonce) != TKM_OK) + { + tkm->idmgr->release_id(tkm->idmgr, TKM_CTX_NONCE, nc_id); + return FALSE; + } + memcpy(buffer, &nonce.data, size); + this->nonce = chunk_clone(chunk_create(buffer, size)); + tkm->chunk_map->insert(tkm->chunk_map, &this->nonce, nc_id); return TRUE; } @@ -57,17 +66,27 @@ METHOD(nonce_gen_t, allocate_nonce, bool, private_tkm_nonceg_t *this, size_t size, chunk_t *chunk) { *chunk = chunk_alloc(size); - if (get_nonce(this, chunk->len, chunk->ptr)) - { - tkm->chunk_map->insert(tkm->chunk_map, chunk, this->context_id); - return TRUE; - } - return FALSE; + return get_nonce(this, chunk->len, chunk->ptr); } METHOD(nonce_gen_t, destroy, void, private_tkm_nonceg_t *this) { + uint64_t nc_id; + + nc_id = tkm->chunk_map->get_id(tkm->chunk_map, &this->nonce); + if (nc_id) + { + DBG1(DBG_IKE, "resetting stale nonce context %llu", nc_id); + + if (ike_nc_reset(nc_id) != TKM_OK) + { + DBG1(DBG_IKE, "failed to reset nonce context %llu", nc_id); + } + tkm->idmgr->release_id(tkm->idmgr, TKM_CTX_NONCE, nc_id); + tkm->chunk_map->remove(tkm->chunk_map, &this->nonce); + } + chunk_free(&this->nonce); free(this); } @@ -86,14 +105,7 @@ tkm_nonceg_t *tkm_nonceg_create() .destroy = _destroy, }, }, - .context_id = tkm->idmgr->acquire_id(tkm->idmgr, TKM_CTX_NONCE), ); - if (!this->context_id) - { - free(this); - return NULL; - } - return &this->public; } diff --git a/src/charon-tkm/tests/keymat_tests.c b/src/charon-tkm/tests/keymat_tests.c index 889965a78..d087bee3f 100644 --- a/src/charon-tkm/tests/keymat_tests.c +++ b/src/charon-tkm/tests/keymat_tests.c @@ -46,7 +46,6 @@ START_TEST(test_derive_ike_keys) fail_if(!ng, "Unable to create nonce generator"); fail_unless(ng->nonce_gen.allocate_nonce(&ng->nonce_gen, 32, &nonce), "Unable to allocate nonce"); - ng->nonce_gen.destroy(&ng->nonce_gen); tkm_diffie_hellman_t *dh = tkm_diffie_hellman_create(MODP_4096_BIT); fail_if(!dh, "Unable to create DH"); @@ -69,6 +68,7 @@ START_TEST(test_derive_ike_keys) fail_if(aead->get_block_size(aead) != 16, "Block size mismatch %d", aead->get_block_size(aead)); + ng->nonce_gen.destroy(&ng->nonce_gen); proposal->destroy(proposal); dh->dh.destroy(&dh->dh); ike_sa_id->destroy(ike_sa_id); From 72376234cbc449b062c68a24837aeefacad44ade Mon Sep 17 00:00:00 2001 From: Reto Buerki Date: Thu, 23 Apr 2015 08:46:18 +0200 Subject: [PATCH 10/12] child-create: Make nonceg a member of child_create struct This allows to control the life-cycle of a nonce in the context of the child create task. In the TKM use-case, it is required to reset the nonce context if the created nonce is not consumed. This happens if the child SA negotiation fails and it is detected before the SA is established via the TKM kernel plugin (i.e. rekey collision). --- src/libcharon/sa/ikev2/tasks/child_create.c | 28 ++++++++++++--------- 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/src/libcharon/sa/ikev2/tasks/child_create.c b/src/libcharon/sa/ikev2/tasks/child_create.c index 6e00ebada..868f27164 100644 --- a/src/libcharon/sa/ikev2/tasks/child_create.c +++ b/src/libcharon/sa/ikev2/tasks/child_create.c @@ -64,6 +64,11 @@ struct private_child_create_t { */ chunk_t other_nonce; + /** + * nonce generator + */ + nonce_gen_t *nonceg; + /** * config to create the CHILD_SA from */ @@ -216,22 +221,12 @@ static status_t get_nonce(message_t *message, chunk_t *nonce) */ static status_t generate_nonce(private_child_create_t *this) { - nonce_gen_t *nonceg; - - nonceg = this->keymat->keymat.create_nonce_gen(&this->keymat->keymat); - if (!nonceg) - { - DBG1(DBG_IKE, "no nonce generator found to create nonce"); - return FAILED; - } - if (!nonceg->allocate_nonce(nonceg, NONCE_SIZE, &this->my_nonce)) + if (!this->nonceg->allocate_nonce(this->nonceg, NONCE_SIZE, + &this->my_nonce)) { DBG1(DBG_IKE, "nonce allocation failed"); - nonceg->destroy(nonceg); return FAILED; } - nonceg->destroy(nonceg); - return SUCCESS; } @@ -1631,6 +1626,7 @@ METHOD(task_t, destroy, void, } DESTROY_IF(this->config); + DESTROY_IF(this->nonceg); free(this); } @@ -1670,6 +1666,14 @@ child_create_t *child_create_create(ike_sa_t *ike_sa, .retry = FALSE, ); + this->nonceg = this->keymat->keymat.create_nonce_gen(&this->keymat->keymat); + if (!this->nonceg) + { + DBG1(DBG_IKE, "no nonce generator found to create nonce"); + free(this); + return NULL; + } + if (config) { this->public.task.build = _build_i; From 0b308faf6d546e01f7cb58db1377d4c26a5ee26b Mon Sep 17 00:00:00 2001 From: Reto Buerki Date: Thu, 23 Apr 2015 11:19:24 +0200 Subject: [PATCH 11/12] ike-init: Make nonceg a member of ike_init struct This allows to control the life-cycle of a nonce in the context of the ike init task. In the TKM use-case the nonce generator cannot be destroyed before the ike init task is finalized, otherwise the created nonce is detected as stale. --- src/libcharon/sa/ikev2/tasks/ike_init.c | 37 ++++++++++++------------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/src/libcharon/sa/ikev2/tasks/ike_init.c b/src/libcharon/sa/ikev2/tasks/ike_init.c index 0d5700ef2..2d9bf518d 100644 --- a/src/libcharon/sa/ikev2/tasks/ike_init.c +++ b/src/libcharon/sa/ikev2/tasks/ike_init.c @@ -89,6 +89,11 @@ struct private_ike_init_t { */ chunk_t other_nonce; + /** + * nonce generator + */ + nonce_gen_t *nonceg; + /** * Negotiated proposal used for IKE_SA */ @@ -428,21 +433,12 @@ METHOD(task_t, build_i, status_t, /* generate nonce only when we are trying the first time */ if (this->my_nonce.ptr == NULL) { - nonce_gen_t *nonceg; - - nonceg = this->keymat->keymat.create_nonce_gen(&this->keymat->keymat); - if (!nonceg) - { - DBG1(DBG_IKE, "no nonce generator found to create nonce"); - return FAILED; - } - if (!nonceg->allocate_nonce(nonceg, NONCE_SIZE, &this->my_nonce)) + if (!this->nonceg->allocate_nonce(this->nonceg, NONCE_SIZE, + &this->my_nonce)) { DBG1(DBG_IKE, "nonce allocation failed"); - nonceg->destroy(nonceg); return FAILED; } - nonceg->destroy(nonceg); } if (this->cookie.ptr) @@ -477,19 +473,11 @@ METHOD(task_t, process_r, status_t, DBG0(DBG_IKE, "%H is initiating an IKE_SA", message->get_source(message)); this->ike_sa->set_state(this->ike_sa, IKE_CONNECTING); - nonceg = this->keymat->keymat.create_nonce_gen(&this->keymat->keymat); - if (!nonceg) - { - DBG1(DBG_IKE, "no nonce generator found to create nonce"); - return FAILED; - } - if (!nonceg->allocate_nonce(nonceg, NONCE_SIZE, &this->my_nonce)) + if (!this->nonceg->allocate_nonce(this->nonceg, NONCE_SIZE, &this->my_nonce)) { DBG1(DBG_IKE, "nonce allocation failed"); - nonceg->destroy(nonceg); return FAILED; } - nonceg->destroy(nonceg); #ifdef ME { @@ -756,6 +744,7 @@ METHOD(task_t, destroy, void, { DESTROY_IF(this->dh); DESTROY_IF(this->proposal); + DESTROY_IF(this->nonceg); chunk_free(&this->my_nonce); chunk_free(&this->other_nonce); chunk_free(&this->cookie); @@ -801,6 +790,14 @@ ike_init_t *ike_init_create(ike_sa_t *ike_sa, bool initiator, ike_sa_t *old_sa) "%s.signature_authentication", TRUE, lib->ns), ); + this->nonceg = this->keymat->keymat.create_nonce_gen(&this->keymat->keymat); + if (!this->nonceg) + { + DBG1(DBG_IKE, "no nonce generator found to create nonce"); + free(this); + return FAILED; + } + if (initiator) { this->public.task.build = _build_i; From 38b65d71860a1a725c7bab48500380866cb379d6 Mon Sep 17 00:00:00 2001 From: Adrian-Ken Rueegsegger Date: Tue, 21 Apr 2015 16:34:06 +0200 Subject: [PATCH 12/12] charon-tkm: Also store local SPI in SAD --- src/charon-tkm/src/tkm/tkm_kernel_ipsec.c | 2 +- src/charon-tkm/src/tkm/tkm_kernel_sad.c | 32 ++++++++++++++--------- src/charon-tkm/src/tkm/tkm_kernel_sad.h | 11 ++++---- src/charon-tkm/tests/kernel_sad_tests.c | 18 ++++++------- 4 files changed, 36 insertions(+), 27 deletions(-) diff --git a/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c b/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c index d751a06cd..7a0672aa8 100644 --- a/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c +++ b/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c @@ -132,7 +132,7 @@ METHOD(kernel_ipsec_t, add_sa, status_t, } esa_id = tkm->idmgr->acquire_id(tkm->idmgr, TKM_CTX_ESA); - if (!tkm->sad->insert(tkm->sad, esa_id, reqid, local, peer, spi_rem, + if (!tkm->sad->insert(tkm->sad, esa_id, reqid, local, peer, spi_loc, spi_rem, protocol)) { DBG1(DBG_KNL, "unable to add entry (%llu) to SAD", esa_id); diff --git a/src/charon-tkm/src/tkm/tkm_kernel_sad.c b/src/charon-tkm/src/tkm/tkm_kernel_sad.c index 619ebeeb6..2556f6b8b 100644 --- a/src/charon-tkm/src/tkm/tkm_kernel_sad.c +++ b/src/charon-tkm/src/tkm/tkm_kernel_sad.c @@ -72,9 +72,14 @@ struct sad_entry_t { host_t *dst; /** - * SPI of CHILD SA. + * Local SPI of CHILD SA. */ - u_int32_t spi; + u_int32_t spi_loc; + + /** + * Remote SPI of CHILD SA. + */ + u_int32_t spi_rem; /** * Protocol of CHILD SA (ESP/AH). @@ -97,7 +102,7 @@ static void sad_entry_destroy(sad_entry_t *entry) } /** - * Find a list entry with given src, dst, spi and proto values. + * Find a list entry with given src, dst, (remote) spi and proto values. */ static bool sad_entry_match(sad_entry_t * const entry, const host_t * const src, const host_t * const dst, const u_int32_t * const spi, @@ -110,7 +115,7 @@ static bool sad_entry_match(sad_entry_t * const entry, const host_t * const src, return src->ip_equals(entry->src, (host_t *)src) && dst->ip_equals(entry->dst, (host_t *)dst) && - entry->spi == *spi && entry->proto == *proto; + entry->spi_rem == *spi && entry->proto == *proto; } /** @@ -121,9 +126,9 @@ static bool sad_entry_match_dst(sad_entry_t * const entry, const u_int32_t * const spi, const u_int8_t * const proto) { - return entry->reqid == *reqid && - entry->spi == *spi && - entry->proto == *proto; + return entry->reqid == *reqid && + entry->spi_rem == *spi && + entry->proto == *proto; } /** @@ -160,13 +165,15 @@ static bool sad_entry_equal(sad_entry_t * const left, sad_entry_t * const right) left->reqid == right->reqid && left->src->ip_equals(left->src, right->src) && left->dst->ip_equals(left->dst, right->dst) && - left->spi == right->spi && left->proto == right->proto; + left->spi_loc == right->spi_loc && + left->spi_rem == right->spi_rem && + left->proto == right->proto; } METHOD(tkm_kernel_sad_t, insert, bool, private_tkm_kernel_sad_t * const this, const esa_id_type esa_id, const u_int32_t reqid, const host_t * const src, const host_t * const dst, - const u_int32_t spi, const u_int8_t proto) + const u_int32_t spi_loc, const u_int32_t spi_rem, const u_int8_t proto) { status_t result; sad_entry_t *new_entry; @@ -176,7 +183,8 @@ METHOD(tkm_kernel_sad_t, insert, bool, .reqid = reqid, .src = (host_t *)src, .dst = (host_t *)dst, - .spi = spi, + .spi_loc = spi_loc, + .spi_rem = spi_rem, .proto = proto, ); @@ -187,8 +195,8 @@ METHOD(tkm_kernel_sad_t, insert, bool, if (result == NOT_FOUND) { DBG3(DBG_KNL, "inserting SAD entry (esa: %llu, reqid: %u, src: %H, " - "dst: %H, spi: %x, proto: %u)", esa_id, reqid, src, dst, - ntohl(spi), proto); + "dst: %H, spi_loc: %x, spi_rem: %x,proto: %u)", esa_id, reqid, src, + dst, ntohl(spi_loc), ntohl(spi_rem), proto); new_entry->src = src->clone((host_t *)src); new_entry->dst = dst->clone((host_t *)dst); this->data->insert_last(this->data, new_entry); diff --git a/src/charon-tkm/src/tkm/tkm_kernel_sad.h b/src/charon-tkm/src/tkm/tkm_kernel_sad.h index fd4830805..3a84deffc 100644 --- a/src/charon-tkm/src/tkm/tkm_kernel_sad.h +++ b/src/charon-tkm/src/tkm/tkm_kernel_sad.h @@ -40,21 +40,22 @@ struct tkm_kernel_sad_t { * @param reqid reqid of the SA * @param src source address of CHILD SA * @param dst destination address of CHILD SA - * @param spi SPI of CHILD SA + * @param spi_loc Local SPI of CHILD SA + * @param spi_rem Remote SPI of CHILD SA * @param proto protocol of CHILD SA (ESP/AH) * @return TRUE if entry was inserted, FALSE otherwise */ bool (*insert)(tkm_kernel_sad_t * const this, const esa_id_type esa_id, const u_int32_t reqid, const host_t * const src, - const host_t * const dst, const u_int32_t spi, - const u_int8_t proto); + const host_t * const dst, const u_int32_t spi_loc, + const u_int32_t spi_rem, const u_int8_t proto); /** * Get ESA id for entry with given parameters. * * @param src source address of CHILD SA * @param dst destination address of CHILD SA - * @param spi SPI of CHILD SA + * @param spi Remote SPI of CHILD SA * @param proto protocol of CHILD SA (ESP/AH) * @return ESA id of entry if found, 0 otherwise */ @@ -76,7 +77,7 @@ struct tkm_kernel_sad_t { * Get destination host for entry with given parameters. * * @param reqid reqid of CHILD SA - * @param spi SPI of CHILD SA + * @param spi Remote SPI of CHILD SA * @param proto protocol of CHILD SA (ESP/AH) * @return destination host of entry if found, NULL otherwise */ diff --git a/src/charon-tkm/tests/kernel_sad_tests.c b/src/charon-tkm/tests/kernel_sad_tests.c index 91ccdd4dd..2a033d237 100644 --- a/src/charon-tkm/tests/kernel_sad_tests.c +++ b/src/charon-tkm/tests/kernel_sad_tests.c @@ -34,7 +34,7 @@ START_TEST(test_insert) host_t *addr = host_create_from_string("127.0.0.1", 1024); tkm_kernel_sad_t *sad = tkm_kernel_sad_create(); - fail_unless(sad->insert(sad, 1, 2, addr, addr, 42, 50), + fail_unless(sad->insert(sad, 1, 2, addr, addr, 27, 42, 50), "Error inserting SAD entry"); sad->destroy(sad); @@ -47,9 +47,9 @@ START_TEST(test_insert_duplicate) host_t *addr = host_create_from_string("127.0.0.1", 1024); tkm_kernel_sad_t *sad = tkm_kernel_sad_create(); - fail_unless(sad->insert(sad, 1, 2, addr, addr, 42, 50), + fail_unless(sad->insert(sad, 1, 2, addr, addr, 27, 42, 50), "Error inserting SAD entry"); - fail_if(sad->insert(sad, 1, 2, addr, addr, 42, 50), + fail_if(sad->insert(sad, 1, 2, addr, addr, 27, 42, 50), "Expected error inserting duplicate entry"); sad->destroy(sad); @@ -61,7 +61,7 @@ START_TEST(test_get_esa_id) { host_t *addr = host_create_from_string("127.0.0.1", 1024); tkm_kernel_sad_t *sad = tkm_kernel_sad_create(); - fail_unless(sad->insert(sad, 23, 54, addr, addr, 42, 50), + fail_unless(sad->insert(sad, 23, 54, addr, addr, 27, 42, 50), "Error inserting SAD entry"); fail_unless(sad->get_esa_id(sad, addr, addr, 42, 50) == 23, "Error getting esa id"); @@ -85,9 +85,9 @@ START_TEST(test_get_other_esa_id) { host_t *addr = host_create_from_string("127.0.0.1", 1024); tkm_kernel_sad_t *sad = tkm_kernel_sad_create(); - fail_unless(sad->insert(sad, 23, 54, addr, addr, 42, 50), + fail_unless(sad->insert(sad, 23, 54, addr, addr, 27, 42, 50), "Error inserting SAD entry"); - fail_unless(sad->insert(sad, 24, 54, addr, addr, 42, 50), + fail_unless(sad->insert(sad, 24, 54, addr, addr, 27, 42, 50), "Error inserting SAD entry"); fail_unless(sad->get_other_esa_id(sad, 23) == 24, "Error getting other esa id"); @@ -102,7 +102,7 @@ START_TEST(test_get_other_esa_id_nonexistent) tkm_kernel_sad_t *sad = tkm_kernel_sad_create(); fail_unless(sad->get_other_esa_id(sad, 1) == 0, "Got other esa id for nonexistent SAD entry"); - fail_unless(sad->insert(sad, 23, 54, addr, addr, 42, 50), + fail_unless(sad->insert(sad, 23, 54, addr, addr, 27, 42, 50), "Error inserting SAD entry"); fail_unless(sad->get_other_esa_id(sad, 23) == 0, "Got own esa id"); @@ -116,7 +116,7 @@ START_TEST(test_get_dst_host) { host_t *addr = host_create_from_string("127.0.0.1", 1024); tkm_kernel_sad_t *sad = tkm_kernel_sad_create(); - fail_unless(sad->insert(sad, 23, 54, addr, addr, 42, 50), + fail_unless(sad->insert(sad, 23, 54, addr, addr, 27, 42, 50), "Error inserting SAD entry"); host_t *dst = sad->get_dst_host(sad, 54, 42, 50); @@ -139,7 +139,7 @@ START_TEST(test_remove) { host_t *addr = host_create_from_string("127.0.0.1", 1024); tkm_kernel_sad_t *sad = tkm_kernel_sad_create(); - fail_unless(sad->insert(sad, 23, 54, addr, addr, 42, 50), + fail_unless(sad->insert(sad, 23, 54, addr, addr, 27, 42, 50), "Error inserting SAD entry"); fail_unless(sad->get_esa_id(sad, addr, addr, 42, 50) == 23, "Error getting esa id");