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]

Date:   Tue, 9 Jun 2020 16:19:57 +0800
From:   Xin Long <lucien.xin@...il.com>
To:     Tobias Brunner <tobias@...ongswan.org>
Cc:     network dev <netdev@...r.kernel.org>,
        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 <yuehaibing@...wei.com>,
        Andreas Steffen <andreas.steffen@...ongswan.org>
Subject: Re: [PATCHv2 ipsec] xfrm: fix a warning in xfrm_policy_insert_list

 a, .

On Mon, Jun 8, 2020 at 8:02 PM Tobias Brunner <tobias@...ongswan.org> wrote:
>
> 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.
It's a problem that can't be removed, but not for being 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.
priority doesn't exist in xfrm_userpolicy_id, and we can add
XFRMA_PRIORITY like XFRMA_MARK to fix it.
since we also take priority to make a policy unique, we can't update
this attribute.

>
> 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 see.

>
> 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.
>
For deleting, yes, but for querying, I think it makes sense not to pass
the priority, and always get the policy with the highest priority.

We can separate the deleting path from the querying path when
XFRMA_PRIORITY attribute is set.

Is that enough for your case to only fix for the policy deleting?