[RFC PATCH v2 1/8] landlock: Fix non-TCP sockets restriction

[Mikhail Ivanov]

On 12/12/2024 9:43 PM, Mickaël Salaün wrote:
> On Wed, Dec 11, 2024 at 06:24:53PM +0300, Mikhail Ivanov wrote:
>> 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/
> 
> Let's continue the discussion on this thread.
> 
>>
>>>
>>>>
>>>>>
>>>>>>>
>>>>>>> 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?
> 
> The changes are not clear to me so I don't know.  If a lot of parts are
> changed, maybe splitting this patch into a few patches would help.  I'm
> a bit worried that too much parts are changed though.

Mostly, there are just bind() and connect() related checks moved to
hook_socket_{connect, bind}.

I think I'd better move all refactoring-related fixes to a separate
patchset.

> 
> When I try to apply this series I get:
> 
>    Patch failed at 0002 landlock: Make network stack layer checks explicit
>    for each TCP action
>    error: patch failed: security/landlock/net.c:1
>    error: security/landlock/net.c: patch does not apply

Sorry, it looks like patches created using the --break-rewrites option
of git format can only be applied manually. I'll try to split this patch
in v3 so that it can be applied automatically.