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:

quoted

On Tue, Dec 10, 2024 at 07:04:15PM +0100, Mickaël Salaün wrote:

quoted

On Mon, Dec 09, 2024 at 01:19:19PM +0300, Mikhail Ivanov wrote:

quoted

On 12/4/2024 10:35 PM, Mickaël Salaün wrote:

quoted

On Wed, Dec 04, 2024 at 08:27:58PM +0100, Mickaël Salaün wrote:

quoted

On Fri, Oct 18, 2024 at 08:08:12PM +0200, Mickaël Salaün wrote:

quoted

On Thu, Oct 17, 2024 at 02:59:48PM +0200, Matthieu Baerts wrote:

quoted

Hi Mikhail and Landlock maintainers,

+cc MPTCP list.

Thanks, we should include this list in the next series.

quoted

On 17/10/2024 13:04, Mikhail Ivanov wrote:

quoted

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/ (local)

Let's continue the discussion on this thread.

quoted

quoted

quoted

quoted

quoted

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/ (local)

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.

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