Message-ID: <b8726b37-8819-2289-40ec-81d875b13eba@huawei-partners.com>
Date: Wed, 11 Dec 2024 18:24:53 +0300
From: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
To: Mickaël Salaün <mic@...ikod.net>
CC: Matthieu Baerts <matttbe@...nel.org>, <gnoack@...gle.com>,
	<willemdebruijn.kernel@...il.com>, <matthieu@...fet.re>,
	<linux-security-module@...r.kernel.org>, <netdev@...r.kernel.org>,
	<netfilter-devel@...r.kernel.org>, <yusongping@...wei.com>,
	<artem.kuzin@...wei.com>, <konstantin.meskhidze@...wei.com>, MPTCP Linux
	<mptcp@...ts.linux.dev>, David Laight <David.Laight@...lab.com>
Subject: Re: [RFC PATCH v2 1/8] landlock: Fix non-TCP sockets restriction

On 12/10/2024 9:05 PM, Mickaël Salaün wrote:
> On Tue, Dec 10, 2024 at 07:04:15PM +0100, Mickaël Salaün wrote:
>> On Mon, Dec 09, 2024 at 01:19:19PM +0300, Mikhail Ivanov wrote:
>>> On 12/4/2024 10:35 PM, Mickaël Salaün wrote:
>>>> On Wed, Dec 04, 2024 at 08:27:58PM +0100, Mickaël Salaün wrote:
>>>>> On Fri, Oct 18, 2024 at 08:08:12PM +0200, Mickaël Salaün wrote:
>>>>>> On Thu, Oct 17, 2024 at 02:59:48PM +0200, Matthieu Baerts wrote:
>>>>>>> Hi Mikhail and Landlock maintainers,
>>>>>>>
>>>>>>> +cc MPTCP list.
>>>>>>
>>>>>> Thanks, we should include this list in the next series.
>>>>>>
>>>>>>>
>>>>>>> On 17/10/2024 13:04, Mikhail Ivanov wrote:
>>>>>>>> Do not check TCP access right if socket protocol is not IPPROTO_TCP.
>>>>>>>> LANDLOCK_ACCESS_NET_BIND_TCP and LANDLOCK_ACCESS_NET_CONNECT_TCP
>>>>>>>> should not restrict bind(2) and connect(2) for non-TCP protocols
>>>>>>>> (SCTP, MPTCP, SMC).
>>>>>>>
>>>>>>> Thank you for the patch!
>>>>>>>
>>>>>>> I'm part of the MPTCP team, and I'm wondering if MPTCP should not be
>>>>>>> treated like TCP here. MPTCP is an extension to TCP: on the wire, we can
>>>>>>> see TCP packets with extra TCP options. On Linux, there is indeed a
>>>>>>> dedicated MPTCP socket (IPPROTO_MPTCP), but that's just internal,
>>>>>>> because we needed such dedicated socket to talk to the userspace.
>>>>>>>
>>>>>>> I don't know Landlock well, but I think it is important to know that an
>>>>>>> MPTCP socket can be used to discuss with "plain" TCP packets: the kernel
>>>>>>> will do a fallback to "plain" TCP if MPTCP is not supported by the other
>>>>>>> peer or by a middlebox. It means that with this patch, if TCP is blocked
>>>>>>> by Landlock, someone can simply force an application to create an MPTCP
>>>>>>> socket -- e.g. via LD_PRELOAD -- and bypass the restrictions. It will
>>>>>>> certainly work, even when connecting to a peer not supporting MPTCP.
>>>>>>>
>>>>>>> Please note that I'm not against this modification -- especially here
>>>>>>> when we remove restrictions around MPTCP sockets :) -- I'm just saying
>>>>>>> it might be less confusing for users if MPTCP is considered as being
>>>>>>> part of TCP. A bit similar to what someone would do with a firewall: if
>>>>>>> TCP is blocked, MPTCP is blocked as well.
>>>>>>
>>>>>> Good point!  I don't know well MPTCP but I think you're right.  Given
>>>>>> it's close relationship with TCP and the fallback mechanism, it would
>>>>>> make sense for users to not make a difference and it would avoid bypass
>>>>>> of misleading restrictions.  Moreover the Landlock rules are simple and
>>>>>> only control TCP ports, not peer addresses, which seems to be the main
>>>>>> evolution of MPTCP.
>>>>>
>>>>> Thinking more about this, this makes sense from the point of view of the
>>>>> network stack, but looking at external (potentially bogus) firewalls or
>>>>> malware detection systems, it is something different.  If we don't
>>>>> provide a way for users to differenciate the control of SCTP from TCP,
>>>>> malicious use of SCTP could still bypass this kind of bogus security
>>>>> appliances.  It would then be safer to stick to the protocol semantic by
>>>>> clearly differenciating TCP from MPTCP (or any other protocol).
>>>
>>> You mean that these firewals have protocol granularity (e.g. different
>>> restrictions for MPTCP and TCP sockets)?
>>
>> Yes, and more importantly they can miss the MTCP semantic and then not
>> properly filter such packet, which can be use to escape the network
>> policy.  See some issues here:
>> https://en.wikipedia.org/wiki/Multipath_TCP
>>
>> The point is that we cannot assume anything about other networking
>> stacks, and if Landlock can properly differentiate between TCP and MTCP
>> (e.g. with new LANDLOCK_ACCESS_NET_CONNECT_MTCP) users of such firewalls
>> could still limit the impact of their firewall's bugs.  However, if
>> Landlock treats TCP and MTCP the same way, we'll not be able to only
>> deny MTCP.  In most use cases, the network policy should treat both TCP
>> and MTCP the same way though, but we should let users decide according
>> to their context.
>>
>>  From an implementation point of view, adding MTCP support should be
>> simple, mainly tests will grow.
> 
> s/MTCP/MPTCP/g of course.

That's reasonable, thanks for explanation!

We should also consider control of other protocols that use TCP
internally [1], since it should be easy to bypass TCP restriction by
using them (e.g. provoking a fallback of MPTCP or SMC connection to
TCP).

The simplest solution is to implement separate access rights for SMC and
RDS, as well as for MPTCP. I think we should stick to it.

I was worried if there was a case where it would be useful to allow only
SMC (and deny TCP). If there are any, it would be more correct to
restrict only the fallback SMC -> TCP with TCP access rights. But such
logic seems too complicated for the kernel and implicit for SMC
applications that can rely on a TCP connection.

[1] 
https://lore.kernel.org/all/62336067-18c2-3493-d0ec-6dd6a6d3a1b5@huawei-partners.com/

> 
>>
>>>
>>>>>
>>>>> Mikhail, could you please send a new patch series containing one patch
>>>>> to fix the kernel and another to extend tests?
>>>>
>>>> No need to squash them in one, please keep the current split of the test
>>>> patches.  However, it would be good to be able to easily backport them,
>>>> or at least the most relevant for this fix, which means to avoid
>>>> extended refactoring.
>>>
>>> No problem, I'll remove the fix of error consistency from this patchset.
>>> BTW, what do you think about second and third commits? Should I send the
>>> new version of them as well (in separate patch)?
>>
>> According to the description, patch 2 may be included in this series if
>> it can be tested with any other LSM, but I cannot read these patches:
>> https://lore.kernel.org/all/20241017110454.265818-3-ivanov.mikhail1@huawei-partners.com/

Ok I'll do this, since this patch doesn't make any functional changes.

About readability, a lot of code blocks were moved in this patch, and
because of this, the regular diff file has become too unreadable.
So, I decided to re-generate it with --break-rewrites option of git
format- patch. Do you have any advice on how best to compose a diff for
this patch?

Powered by blists - more mailing lists