From 1551d8b13d14028fc26fb1a363c33aa3a1200882 Mon Sep 17 00:00:00 2001 From: Martin Willi Date: Mon, 3 Jun 2013 18:13:27 +0200 Subject: [PATCH 1/5] kernel-netlink: reject policy refcount if the reqid differs Previously we silently replaced an existing policy with a new one if the reqid changed for the same selectors. This will break an old policy in the favour of the new one (for example if two clients behind the same NAT use transport mode). With this change any new policy gets rejected if the reqid differs. This will make sure we break no existing policy. For rekeying and acquires we still can have overlapping policies (as we use the same reqid), but for unrelated connections this is not true anymore (it wasn't actually before, we just silently broke the existing policy). --- .../kernel_netlink/kernel_netlink_ipsec.c | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/libhydra/plugins/kernel_netlink/kernel_netlink_ipsec.c b/src/libhydra/plugins/kernel_netlink/kernel_netlink_ipsec.c index a20804507..47e725c1c 100644 --- a/src/libhydra/plugins/kernel_netlink/kernel_netlink_ipsec.c +++ b/src/libhydra/plugins/kernel_netlink/kernel_netlink_ipsec.c @@ -558,6 +558,9 @@ struct policy_entry_t { /** List of SAs this policy is used by, ordered by priority */ linked_list_t *used_by; + + /** reqid for this policy */ + u_int32_t reqid; }; /** @@ -2048,7 +2051,7 @@ static status_t add_policy_internal(private_kernel_netlink_ipsec_t *this, { continue; } - tmpl->reqid = ipsec->cfg.reqid; + tmpl->reqid = policy->reqid; tmpl->id.proto = protos[i].proto; tmpl->aalgos = tmpl->ealgos = tmpl->calgos = ~0; tmpl->mode = mode2kernel(proto_mode); @@ -2204,6 +2207,7 @@ METHOD(kernel_ipsec_t, add_policy, status_t, .sel = ts2selector(src_ts, dst_ts), .mark = mark.value & mark.mask, .direction = direction, + .reqid = sa->reqid, ); /* find the policy, which matches EXACTLY */ @@ -2211,6 +2215,16 @@ METHOD(kernel_ipsec_t, add_policy, status_t, current = this->policies->get(this->policies, policy); if (current) { + if (current->reqid != sa->reqid) + { + DBG1(DBG_CFG, "unable to install policy %R === %R %N (mark " + "%u/0x%08x) for reqid %u, the same policy for reqid %u exists", + src_ts, dst_ts, policy_dir_names, direction, + mark.value, mark.mask, sa->reqid, current->reqid); + policy_entry_destroy(this, policy); + this->mutex->unlock(this->mutex); + return INVALID_STATE; + } /* use existing policy */ DBG2(DBG_KNL, "policy %R === %R %N (mark %u/0x%08x) " "already exists, increasing refcount", @@ -2382,7 +2396,7 @@ METHOD(kernel_ipsec_t, del_policy, status_t, /* find the policy */ this->mutex->lock(this->mutex); current = this->policies->get(this->policies, &policy); - if (!current) + if (!current || current->reqid != reqid) { if (mark.value) { @@ -2405,8 +2419,7 @@ METHOD(kernel_ipsec_t, del_policy, status_t, enumerator = current->used_by->create_enumerator(current->used_by); while (enumerator->enumerate(enumerator, (void**)&mapping)) { - if (reqid == mapping->sa->cfg.reqid && - priority == mapping->priority) + if (priority == mapping->priority) { current->used_by->remove_at(current->used_by, enumerator); policy_sa_destroy(mapping, &direction, this); From ae131e6bac1da271b45a15b0248b0b377cf449f8 Mon Sep 17 00:00:00 2001 From: Martin Willi Date: Wed, 5 Jun 2013 14:36:44 +0200 Subject: [PATCH 2/5] trap-manager: fix a memleak when installing a trap to %any --- src/libcharon/sa/trap_manager.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libcharon/sa/trap_manager.c b/src/libcharon/sa/trap_manager.c index 6c0ae19c7..7a4567cdf 100644 --- a/src/libcharon/sa/trap_manager.c +++ b/src/libcharon/sa/trap_manager.c @@ -109,6 +109,7 @@ METHOD(trap_manager_t, install, u_int32_t, 0, ike_cfg->get_other_port(ike_cfg)); if (!other || other->is_anyaddr(other)) { + DESTROY_IF(other); DBG1(DBG_CFG, "installing trap failed, remote address unknown"); return 0; } From f4e822c1b422e31edbf3170b5bcec58b80a7f4bb Mon Sep 17 00:00:00 2001 From: Martin Willi Date: Thu, 6 Jun 2013 10:54:06 +0200 Subject: [PATCH 3/5] trap-manager: don't check-in nonexisting IKE_SA if acquire fails --- src/libcharon/sa/trap_manager.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/libcharon/sa/trap_manager.c b/src/libcharon/sa/trap_manager.c index 7a4567cdf..5d947ece5 100644 --- a/src/libcharon/sa/trap_manager.c +++ b/src/libcharon/sa/trap_manager.c @@ -320,8 +320,7 @@ METHOD(trap_manager_t, acquire, void, } else { - charon->ike_sa_manager->checkin_and_destroy( - charon->ike_sa_manager, ike_sa); + ike_sa->destroy(ike_sa); } } peer->destroy(peer); From 2dcfc6983bc2f73bbc355f9204dda6a2901a3a38 Mon Sep 17 00:00:00 2001 From: Martin Willi Date: Wed, 5 Jun 2013 14:20:44 +0200 Subject: [PATCH 4/5] trap-manager: add a method to find reqid for installed traps by config --- src/libcharon/sa/trap_manager.c | 32 ++++++++++++++++++++++++++++++-- src/libcharon/sa/trap_manager.h | 8 ++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/src/libcharon/sa/trap_manager.c b/src/libcharon/sa/trap_manager.c index 5d947ece5..ab638ff3e 100644 --- a/src/libcharon/sa/trap_manager.c +++ b/src/libcharon/sa/trap_manager.c @@ -142,6 +142,8 @@ METHOD(trap_manager_t, install, u_int32_t, } } enumerator->destroy(enumerator); + this->lock->unlock(this->lock); + if (found) { /* config might have changed so update everything */ DBG1(DBG_CFG, "updating already routed CHILD_SA '%s'", @@ -180,10 +182,11 @@ METHOD(trap_manager_t, install, u_int32_t, .child_sa = child_sa, .peer_cfg = peer->get_ref(peer), ); + this->lock->write_lock(this->lock); this->traps->insert_last(this->traps, entry); + this->lock->unlock(this->lock); reqid = child_sa->get_reqid(child_sa); } - this->lock->unlock(this->lock); if (status != SUCCESS) { @@ -252,6 +255,31 @@ METHOD(trap_manager_t, create_enumerator, enumerator_t*, (void*)this->lock->unlock); } +METHOD(trap_manager_t, find_reqid, u_int32_t, + private_trap_manager_t *this, child_cfg_t *child) +{ + enumerator_t *enumerator; + child_cfg_t *current; + entry_t *entry; + u_int32_t reqid = 0; + + this->lock->read_lock(this->lock); + enumerator = this->traps->create_enumerator(this->traps); + while (enumerator->enumerate(enumerator, &entry)) + { + current = entry->child_sa->get_config(entry->child_sa); + if (streq(current->get_name(current), child->get_name(child))) + { + reqid = entry->child_sa->get_reqid(entry->child_sa); + break; + } + } + enumerator->destroy(enumerator); + this->lock->unlock(this->lock); + + return reqid; +} + METHOD(trap_manager_t, acquire, void, private_trap_manager_t *this, u_int32_t reqid, traffic_selector_t *src, traffic_selector_t *dst) @@ -417,6 +445,7 @@ trap_manager_t *trap_manager_create(void) .install = _install, .uninstall = _uninstall, .create_enumerator = _create_enumerator, + .find_reqid = _find_reqid, .acquire = _acquire, .flush = _flush, .destroy = _destroy, @@ -435,4 +464,3 @@ trap_manager_t *trap_manager_create(void) return &this->public; } - diff --git a/src/libcharon/sa/trap_manager.h b/src/libcharon/sa/trap_manager.h index e3d355662..97de45645 100644 --- a/src/libcharon/sa/trap_manager.h +++ b/src/libcharon/sa/trap_manager.h @@ -57,6 +57,14 @@ struct trap_manager_t { */ enumerator_t* (*create_enumerator)(trap_manager_t *this); + /** + * Find the reqid of a child config installed as a trap. + * + * @param child CHILD_SA config to get the reqid for + * @return reqid of trap, 0 if not found + */ + u_int32_t (*find_reqid)(trap_manager_t *this, child_cfg_t *child); + /** * Acquire an SA triggered by an installed trap. * From a7bc0bf4a6c091637e81eec0c268e5947f5c1e21 Mon Sep 17 00:00:00 2001 From: Martin Willi Date: Tue, 11 Jun 2013 16:11:14 +0200 Subject: [PATCH 5/5] ike: reuse the reqid of an installed trap having the same config When we have a trap installed, but a CHILD_SA gets established for the same config from the peer, we should reuse the same reqid. Otherwise we would have two identical policies using different reqids, what we can't handle in our kernel backend. --- src/libcharon/sa/child_sa.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libcharon/sa/child_sa.c b/src/libcharon/sa/child_sa.c index f50e32ff1..1069b2d91 100644 --- a/src/libcharon/sa/child_sa.c +++ b/src/libcharon/sa/child_sa.c @@ -1149,7 +1149,11 @@ child_sa_t * child_sa_create(host_t *me, host_t* other, } else { - this->reqid = ref_get(&reqid); + this->reqid = charon->traps->find_reqid(charon->traps, config); + if (!this->reqid) + { + this->reqid = ref_get(&reqid); + } } }