From: Tobias Brunner Date: Thu, 3 Feb 2022 16:10:46 +0000 (+0100) Subject: child-sa: Allocate a new reqid if dynamic traffic selectors are updated X-Git-Tag: 5.9.6rc1~3^2~16 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8f4cfe96691e7cff55f9c9c5d2b55ee242f94551;p=thirdparty%2Fstrongswan.git child-sa: Allocate a new reqid if dynamic traffic selectors are updated If update_sa() is called and dynamic traffic selectors are changed using new addresses, this might cause issues if we continue to use a reqid that doesn't match the updated traffic selectors. For instance, if the initiator then uses make-before-break reauth from the new IP. It's also a particular problem in the SELinux case where multiple CHILD_SAs with specific labels all share the same (trap) policy with generic label. However, SAs created after the update would not match due to the new reqid. --- diff --git a/src/libcharon/sa/child_sa.c b/src/libcharon/sa/child_sa.c index 0956c40248..eb583944f1 100644 --- a/src/libcharon/sa/child_sa.c +++ b/src/libcharon/sa/child_sa.c @@ -558,8 +558,12 @@ METHOD(enumerator_t, policy_destroy, void, free(this); } -METHOD(child_sa_t, create_policy_enumerator, enumerator_t*, - private_child_sa_t *this) +/** + * Create an enumerator over two lists of traffic selectors, returning all the + * pairs of traffic selectors from the first and second list. + */ +static enumerator_t *create_policy_enumerator_internal(array_t *my_ts, + array_t *other_ts) { policy_enumerator_t *e; @@ -569,15 +573,21 @@ METHOD(child_sa_t, create_policy_enumerator, enumerator_t*, .venumerate = _policy_enumerate, .destroy = _policy_destroy, }, - .mine = array_create_enumerator(this->my_ts), - .other = array_create_enumerator(this->other_ts), - .array = this->other_ts, + .mine = array_create_enumerator(my_ts), + .other = array_create_enumerator(other_ts), + .array = other_ts, .ts = NULL, ); return &e->public; } +METHOD(child_sa_t, create_policy_enumerator, enumerator_t*, + private_child_sa_t *this) +{ + return create_policy_enumerator_internal(this->my_ts, this->other_ts); +} + /** * update the cached usebytes * returns SUCCESS if the usebytes have changed, FAILED if not or no SPIs @@ -1119,6 +1129,7 @@ static status_t install_policies_outbound(private_child_sa_t *this, .dst = other_addr, .sa = other_sa, }; + uint32_t reqid = other_sa->reqid; status_t status = SUCCESS; status |= charon->kernel->add_policy(charon->kernel, &out_id, &out_policy); @@ -1142,7 +1153,7 @@ static status_t install_policies_outbound(private_child_sa_t *this, status |= charon->kernel->add_policy(charon->kernel, &out_id, &out_policy); /* reset the reqid for any other further policies */ - other_sa->reqid = this->reqid; + other_sa->reqid = reqid; } return status; } @@ -1229,6 +1240,7 @@ static void del_policies_outbound(private_child_sa_t *this, .dst = other_addr, .sa = other_sa, }; + uint32_t reqid = other_sa->reqid; charon->kernel->del_policy(charon->kernel, &out_id, &out_policy); @@ -1241,7 +1253,7 @@ static void del_policies_outbound(private_child_sa_t *this, out_policy.prio = POLICY_PRIORITY_ROUTED; } charon->kernel->del_policy(charon->kernel, &out_id, &out_policy); - other_sa->reqid = this->reqid; + other_sa->reqid = reqid; } } @@ -1299,28 +1311,38 @@ METHOD(child_sa_t, set_policies, void, array_sort(this->other_ts, (void*)traffic_selector_cmp, NULL); } +/** + * Allocate a reqid for the given local and remote traffic selectors. + */ +static status_t alloc_reqid(private_child_sa_t *this, array_t *my_ts, + array_t *other_ts, uint32_t *reqid) +{ + linked_list_t *my_ts_list, *other_ts_list; + status_t status; + + my_ts_list = linked_list_create_from_enumerator(array_create_enumerator(my_ts)); + other_ts_list = linked_list_create_from_enumerator(array_create_enumerator(other_ts)); + status = charon->kernel->alloc_reqid( + charon->kernel, my_ts_list, other_ts_list, + this->mark_in, this->mark_out, this->if_id_in, + this->if_id_out, label_for(this, LABEL_USE_REQID), + reqid); + my_ts_list->destroy(my_ts_list); + other_ts_list->destroy(other_ts_list); + return status; +} + METHOD(child_sa_t, install_policies, status_t, private_child_sa_t *this) { enumerator_t *enumerator; - linked_list_t *my_ts_list, *other_ts_list; traffic_selector_t *my_ts, *other_ts; status_t status = SUCCESS; bool install_outbound = FALSE; if (!this->reqid_allocated && !this->static_reqid) { - my_ts_list = linked_list_create_from_enumerator( - array_create_enumerator(this->my_ts)); - other_ts_list = linked_list_create_from_enumerator( - array_create_enumerator(this->other_ts)); - status = charon->kernel->alloc_reqid( - charon->kernel, my_ts_list, other_ts_list, - this->mark_in, this->mark_out, this->if_id_in, - this->if_id_out, label_for(this, LABEL_USE_REQID), - &this->reqid); - my_ts_list->destroy(my_ts_list); - other_ts_list->destroy(other_ts_list); + status = alloc_reqid(this, this->my_ts, this->other_ts, &this->reqid); if (status != SUCCESS) { return status; @@ -1554,7 +1576,7 @@ CALLBACK(reinstall_vip, void, * Update addresses and encap state of IPsec SAs in the kernel */ static status_t update_sas(private_child_sa_t *this, host_t *me, host_t *other, - bool encap) + bool encap, uint32_t reqid) { /* update our (initiator) SA */ if (this->my_spi && this->inbound_installed) @@ -1573,6 +1595,7 @@ static status_t update_sas(private_child_sa_t *this, host_t *me, host_t *other, .new_dst = me, .encap = this->encap, .new_encap = encap, + .new_reqid = reqid, }; if (charon->kernel->update_sa(charon->kernel, &id, &sa) == NOT_SUPPORTED) @@ -1598,6 +1621,7 @@ static status_t update_sas(private_child_sa_t *this, host_t *me, host_t *other, .new_dst = other, .encap = this->encap, .new_encap = encap, + .new_reqid = reqid, }; if (charon->kernel->update_sa(charon->kernel, &id, &sa) == NOT_SUPPORTED) @@ -1609,6 +1633,30 @@ static status_t update_sas(private_child_sa_t *this, host_t *me, host_t *other, return SUCCESS; } +/** + * Fill the second list with copies of the given traffic selectors updating + * dynamic traffic selectors based on the given addresses. + */ +static void update_ts(host_t *old_host, host_t *new_host, array_t *old_list, + array_t *new_list) +{ + enumerator_t *enumerator; + traffic_selector_t *old_ts, *new_ts; + + enumerator = array_create_enumerator(old_list); + while (enumerator->enumerate(enumerator, &old_ts)) + { + new_ts = old_ts->clone(old_ts); + if (new_ts->is_host(new_ts, old_host)) + { + new_ts->set_address(new_ts, new_host); + } + array_insert(new_list, ARRAY_TAIL, new_ts); + } + enumerator->destroy(enumerator); + array_sort(new_list, (void*)traffic_selector_cmp, NULL); +} + METHOD(child_sa_t, update, status_t, private_child_sa_t *this, host_t *me, host_t *other, linked_list_t *vips, bool encap) @@ -1636,8 +1684,9 @@ METHOD(child_sa_t, update, status_t, ipsec_sa_cfg_t my_sa, other_sa; enumerator_t *enumerator; traffic_selector_t *my_ts, *other_ts; + array_t *new_my_ts = NULL, *new_other_ts = NULL; policy_priority_t priority; - uint32_t manual_prio; + uint32_t manual_prio, new_reqid = 0; status_t state; bool outbound; @@ -1650,6 +1699,16 @@ METHOD(child_sa_t, update, status_t, enumerator = create_policy_enumerator(this); while (enumerator->enumerate(enumerator, &my_ts, &other_ts)) { + if (!new_my_ts && !me->ip_equals(me, this->my_addr) && + my_ts->is_host(my_ts, this->my_addr)) + { + new_my_ts = array_create(0, 0); + } + if (!new_other_ts && !other->ip_equals(other, this->other_addr) && + other_ts->is_host(other_ts, this->other_addr)) + { + new_other_ts = array_create(0, 0); + } /* install drop policy to avoid traffic leaks, acquires etc. */ if (outbound) { @@ -1664,14 +1723,61 @@ METHOD(child_sa_t, update, status_t, } enumerator->destroy(enumerator); + if (new_my_ts) + { + update_ts(this->my_addr, me, this->my_ts, new_my_ts); + } + if (new_other_ts) + { + update_ts(this->other_addr, other, this->other_ts, new_other_ts); + } + if (this->reqid_allocated && (new_my_ts || new_other_ts)) + { + /* if we allocated a reqid with the previous TS, we have to get a + * new one that matches the updated TS */ + if (alloc_reqid(this, new_my_ts ?: this->my_ts, + new_other_ts ?: this->other_ts, &new_reqid) != SUCCESS) + { + DBG1(DBG_CHD, "allocating new reqid for updated SA failed"); + } + } + /* update the IPsec SAs */ - state = update_sas(this, me, other, encap); + state = update_sas(this, me, other, encap, new_reqid); + + /* install new/updated policies only if we were able to update the + * SAs, otherwise we reinstall the old policies further below */ + if (state != NOT_SUPPORTED) + { + /* we reinstall the virtual IP to handle interface roaming + * correctly */ + if (vips) + { + vips->invoke_function(vips, reinstall_vip, me); + } + if (new_reqid) + { + my_sa.reqid = other_sa.reqid = new_reqid; + } + enumerator = create_policy_enumerator_internal( + new_my_ts ?: this->my_ts, + new_other_ts ?: this->other_ts); + while (enumerator->enumerate(enumerator, &my_ts, &other_ts)) + { + install_policies_internal(this, me, other, my_ts, other_ts, + &my_sa, &other_sa, POLICY_IPSEC, + priority, manual_prio, outbound); + } + enumerator->destroy(enumerator); + if (new_reqid) + { + my_sa.reqid = other_sa.reqid = this->reqid; + } + } enumerator = create_policy_enumerator(this); while (enumerator->enumerate(enumerator, &my_ts, &other_ts)) { - traffic_selector_t *old_my_ts = NULL, *old_other_ts = NULL; - /* reinstall the previous policies if we can't update the SAs */ if (state == NOT_SUPPORTED) { @@ -1679,58 +1785,64 @@ METHOD(child_sa_t, update, status_t, my_ts, other_ts, &my_sa, &other_sa, POLICY_IPSEC, priority, manual_prio, outbound); } - else - { - /* check if we have to update a "dynamic" traffic selector */ - if (!me->ip_equals(me, this->my_addr) && - my_ts->is_host(my_ts, this->my_addr)) - { - old_my_ts = my_ts->clone(my_ts); - my_ts->set_address(my_ts, me); - } - if (!other->ip_equals(other, this->other_addr) && - other_ts->is_host(other_ts, this->other_addr)) - { - old_other_ts = other_ts->clone(other_ts); - other_ts->set_address(other_ts, other); - } - - /* we reinstall the virtual IP to handle interface roaming - * correctly */ - if (vips) - { - vips->invoke_function(vips, reinstall_vip, me); - } - - /* reinstall updated policies */ - install_policies_internal(this, me, other, my_ts, other_ts, - &my_sa, &other_sa, POLICY_IPSEC, - priority, manual_prio, outbound); - } /* remove the drop policy */ if (outbound) { del_policies_outbound(this, this->my_addr, this->other_addr, - old_my_ts ?: my_ts, old_other_ts ?: other_ts, - &my_sa, &other_sa, POLICY_DROP, + my_ts, other_ts, &my_sa, &other_sa, POLICY_DROP, POLICY_PRIORITY_DEFAULT, manual_prio); } - - DESTROY_IF(old_my_ts); - DESTROY_IF(old_other_ts); } enumerator->destroy(enumerator); if (state == NOT_SUPPORTED) { + if (new_reqid && + charon->kernel->release_reqid(charon->kernel, + new_reqid, this->mark_in, this->mark_out, + this->if_id_in, this->if_id_out, + label_for(this, LABEL_USE_REQID)) != SUCCESS) + { + DBG1(DBG_CHD, "releasing reqid %u failed", new_reqid); + } + array_destroy_offset(new_my_ts, + offsetof(traffic_selector_t, destroy)); + array_destroy_offset(new_other_ts, + offsetof(traffic_selector_t, destroy)); set_state(this, old); return NOT_SUPPORTED; } + if (new_reqid) + { + if (charon->kernel->release_reqid(charon->kernel, + this->reqid, this->mark_in, this->mark_out, + this->if_id_in, this->if_id_out, + label_for(this, LABEL_USE_REQID)) != SUCCESS) + { + DBG1(DBG_CHD, "releasing reqid %u failed", this->reqid); + } + DBG1(DBG_CHD, "replaced reqid %u with reqid %u for updated " + "CHILD_SA %s{%d}", this->reqid, new_reqid, get_name(this), + this->unique_id); + this->reqid = new_reqid; + } + if (new_my_ts) + { + array_destroy_offset(this->my_ts, + offsetof(traffic_selector_t, destroy)); + this->my_ts = new_my_ts; + } + if (new_other_ts) + { + array_destroy_offset(this->other_ts, + offsetof(traffic_selector_t, destroy)); + this->other_ts = new_other_ts; + } } else if (!transport_proxy_mode) { - if (update_sas(this, me, other, encap) == NOT_SUPPORTED) + if (update_sas(this, me, other, encap, 0) == NOT_SUPPORTED) { set_state(this, old); return NOT_SUPPORTED;