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

[Matthieu Baerts <matttbe@kernel.org>]

Hi Mikhail and Landlock maintainers,

+cc MPTCP list.

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.

I understand that a future goal might probably be to have dedicated
restrictions for MPTCP and the other stream protocols (and/or for all
stream protocols like it was before this patch), but in the meantime, it
might be less confusing considering MPTCP as being part of TCP (I'm not
sure about the other stream protocols).


> sk_is_tcp() is used for this to check address family of the socket
> before doing INET-specific address length validation. This is required
> for error consistency.
> 
> Closes: https://github.com/landlock-lsm/linux/issues/40
> Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect")

I don't know how fixes are considered in Landlock, but should this patch
be considered as a fix? It might be surprising for someone who thought
all "stream" connections were blocked to have them unblocked when
updating to a minor kernel version, no?

(Personally, I would understand such behaviour change when upgrading to
a major version, and still, maybe only if there were alternatives to
continue having the same behaviour, e.g. a way to restrict all stream
sockets the same way, or something per stream socket. But that's just me
:) )

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.