netdev - Re: [PATCHv2 ipsec] xfrm: fix a warning in xfrm_policy_insert

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <70458451-6ece-5222-c46f-87c708eee81e@strongswan.org>
Date:   Mon, 8 Jun 2020 14:02:45 +0200
From:   Tobias Brunner <tobias@...ongswan.org>
To:     Xin Long <lucien.xin@...il.com>, netdev@...r.kernel.org
Cc:     Steffen Klassert <steffen.klassert@...unet.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        Sabrina Dubroca <sd@...asysnail.net>, yuehaibing@...wei.com,
        Andreas Steffen <andreas.steffen@...ongswan.org>
Subject: Re: [PATCHv2 ipsec] xfrm: fix a warning in xfrm_policy_insert_list

Hi Steffen, Xin,

This change could be problematic.  Actually, it's not really this one
but the original one that causes the issue:
> Fixes: 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and different priorities")

However, because the code in xfrm_policy_mark_match() treated policies
with the same mark/mask equal without considering the priority before
this change, it wasn't apparent.  The problem is that the code can now
lead to duplicate policies, which can not correctly be removed or queried.

That's because the priority is sent only in xfrm_userpolicy_info, which
XFRM_MSG_NEWPOLICY and XFRM_MSG_UPDPOLICY expect, but not in
xfrm_userpolicy_id, which is used to query and delete policies with
XFRM_MSG_GETPOLICY and XFRM_MSG_DELPOLICY, respectively (the mark is
sent with a separate attribute, which can be supplied to all commands).
 So we can only query/delete the duplicate policy with the highest
priority.  Such duplicates can even be created inadvertently via
XFRM_MSG_UPDPOLICY if the priority of an existing policy should be
changed, which worked fine so far.

The latter currently happens when strongSwan e.g. replaces a trap or
block policy with one that has templates assigned (those we install with
a higher priority, by default), which uses XFRM_MSG_UPDPOLICY that
doesn't update the existing policy anymore but creates a duplicate
instead.  Since only one XFRM_MSG_DELPOLICY is sent later when the
policy is deleted, a duplicate policy remains (and we couldn't even
delete the exact policy we wanted - the trap might be removed by the
user before the regular policy - due to the missing priority field in
xfrm_userpolicy_id).

I guess we could workaround this issue in strongSwan by installing
policies that share the same mark and selector with the same priority,
so only one instance is ever installed in the kernel.  But the inability
to address the exact policy when querying/deleting still looks like a
problem to me in general.

Regards,
Tobias