netdev - Re: [PATCH ipsec] xfrm: fix "disable_policy" flag use when arriving from different devices

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] [day] [month] [year] [list]

Message-ID: <56903463-9d73-8cb1-52ee-07d12430ed7a@6wind.com>
Date:   Fri, 13 May 2022 17:12:36 +0200
From:   Nicolas Dichtel <nicolas.dichtel@...nd.com>
To:     Eyal Birger <eyal.birger@...il.com>
Cc:     davem@...emloft.net, yoshfuji@...ux-ipv6.org, dsahern@...nel.org,
        edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
        steffen.klassert@...unet.com, herbert@...dor.apana.org.au,
        netdev@...r.kernel.org, Shmulik Ladkani <shmulik.ladkani@...il.com>
Subject: Re: [PATCH ipsec] xfrm: fix "disable_policy" flag use when arriving
 from different devices




Le 12/05/2022 à 18:29, Eyal Birger a écrit :
> On Thu, May 12, 2022 at 7:09 PM Nicolas Dichtel
> <nicolas.dichtel@...nd.com> wrote:
>>
>>
>> Le 12/05/2022 à 12:48, Eyal Birger a écrit :
>>> In IPv4 setting the "disable_policy" flag on a device means no policy
>>> should be enforced for traffic originating from the device. This was
>>> implemented by seting the DST_NOPOLICY flag in the dst based on the
>>> originating device.
>>>
>>> However, dsts are cached in nexthops regardless of the originating
>>> devices, in which case, the DST_NOPOLICY flag value may be incorrect.
>>>
>>> Consider the following setup:
>>>
>>>                      +------------------------------+
>>>                      | ROUTER                       |
>>>   +-------------+    | +-----------------+          |
>>>   | ipsec src   |----|-|ipsec0           |          |
>>>   +-------------+    | |disable_policy=0 |   +----+ |
>>>                      | +-----------------+   |eth1|-|-----
>>>   +-------------+    | +-----------------+   +----+ |
>>>   | noipsec src |----|-|eth0             |          |
>>>   +-------------+    | |disable_policy=1 |          |
>>>                      | +-----------------+          |
>>>                      +------------------------------+
>>>
>>> Where ROUTER has a default route towards eth1.
>>>
>>> dst entries for traffic arriving from eth0 would have DST_NOPOLICY
>>> and would be cached and therefore can be reused by traffic originating
>>> from ipsec0, skipping policy check.
>>>
>>> Fix by setting a IPSKB_NOPOLICY flag in IPCB and observing it instead
>>> of the DST in IN/FWD IPv4 policy checks.
>>>
>>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>>> Reported-by: Shmulik Ladkani <shmulik.ladkani@...il.com>
>>> Signed-off-by: Eyal Birger <eyal.birger@...il.com>
>>> ---
>>
>> [snip]
>>
>>> @@ -1852,8 +1856,7 @@ static int __mkroute_input(struct sk_buff *skb,
>>>               }
>>>       }
>>>
>>> -     rth = rt_dst_alloc(out_dev->dev, 0, res->type,
>>> -                        IN_DEV_ORCONF(in_dev, NOPOLICY),
>>> +     rth = rt_dst_alloc(out_dev->dev, 0, res->type, no_policy,>                         IN_DEV_ORCONF(out_dev, NOXFRM));
>> no_policy / DST_NOPOLICY is still needed in the dst entry after this patch?
> 
> I see it's being set in the outbound direction in IPv4 - though I don't see
> where it's actually used in that direction.
> 
> Maybe it could be cleaned up as a follow up, but I wanted to scope this
> patch to the bugfix.
Sure, the cleanup should be put in a separate patch. Removing the flag for ipv4
could help to avoid ambiguity.


Thank you,
Nicolas