Message-ID: <20241018.Kahdeik0aaCh@digikod.net>
Date: Fri, 18 Oct 2024 20:08:10 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: Matthieu Baerts <matttbe@...nel.org>
Cc: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>, 
	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>
Subject: Re: [RFC PATCH v2 1/8] landlock: Fix non-TCP sockets restriction

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.

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

We need to take a closer look at the other stream protocols indeed.

> 
> 
> > 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?

Indeed.  The main issue was with the semantic/definition of
LANDLOCK_ACCESS_FS_NET_{CONNECT,BIND}_TCP.  We need to synchronize the
code with the documentation, one way or the other, preferably following
the principle of least astonishment.

> 
> (Personally, I would understand such behaviour change when upgrading to
> a major version, and still, maybe only if there were alternatives to

This "fix" needs to be backported, but we're not clear yet on what it
should be. :)

> 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
> :) )

The documentation and the initial idea was to control TCP bind and
connect.  The kernel implementation does more than that, so we need to
synthronize somehow.

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

Powered by blists - more mailing lists