Message-ID: <Zvhh3CRj9T7_KIhC@google.com>
Date: Sat, 28 Sep 2024 22:06:52 +0200
From: "Günther Noack" <gnoack@...gle.com>
To: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
Cc: mic@...ikod.net, willemdebruijn.kernel@...il.com, gnoack3000@...il.com, 
	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
Subject: Re: [RFC PATCH v3 14/19] selftests/landlock: Test socketpair(2) restriction

On Fri, Sep 27, 2024 at 11:48:22AM +0200, Günther Noack wrote:
> On Mon, Sep 23, 2024 at 03:57:47PM +0300, Mikhail Ivanov wrote:
> > (Btw I think that disassociation control can be really useful. If
> > it were possible to restrict this action for each protocol, we would
> > have stricter control over the protocols used.)
> 
> In my understanding, the disassociation support is closely intertwined with the
> transport layer - the last paragraph of DESCRIPTION in connect(2) is listing
> TCP, UDP and Unix Domain sockets in datagram mode. -- The relevant code in in
> net/ipv4/af_inet.c in inet_dgram_connect() and __inet_stream_connect(), where
> AF_UNSPEC is handled.
> 
> I would love to find a way to restrict this independent of the specific
> transport protocol as well.
> 
> Remark on the side - in af_inet.c in inet_shutdown(), I also found a worrying
> scenario where the same sk->sk_prot->disconnect() function is called and
> sock->state also gets reset to SS_UNCONNECTED.  I have done a naive attempt to
> hit that code path by calling shutdown() on a passive TCP socket, but was not
> able to reuse the socket for new connections afterwards. (Have not debugged it
> further though.)  I wonder whether this is a scnenario that we also need to
> cover?

FYI, **this does turn out to work** (I just fumbled in my first experiment). --
It is possible to reset a listening socket with shutdown() into a state where it
can be used for at least a new connect(2), and maybe also for new listen(2)s.

The same might also be possible if a socket is in the TCP_SYN_SENT state at the
time of shutdown() (although that is a bit trickier to try out).

So a complete disassociation control for TCP/IP might not only need to have
LANDLOCK_ACCESS_SOCKET_CONNECT_UNSPEC (or however we'd call it), but also
LANDLOCK_ACCESS_SOCKET_PASSIVE_SHUTDOWN and maybe even another one for the
TCP_SYN_SENT case...? *

It makes me uneasy to think that I only looked at AF_INET{,6} and TCP so far,
and that other protocols would need a similarly close look.  It will be
difficult to cover all the "disassociation" cases in all the different
protocols, and even more difficult to detect new ones when they pop up.  If we
discover new ones and they'd need new Landlock access rights, it would also
potentially mean that existing Landlock users would have to update their rules
to spell that out.

It might be easier after all to not rely on "disassociation" control too much
and instead to design the network-related access rights in a way so that we can
provide the desired sandboxing guarantees by restricting the "constructive"
operations (the ones that initiate new network connections or that listen on the
network).

Mikhail, in your email I am quoting above, you are saying that "disassociation
control can be really useful"; do you know of any cases where a restriction of
connect/listen is *not* enough and where you'd still want the disassociation
control?

(In my mind, the disassociation control would have mainly been needed if we had
gone with Mickaël's "Use socket's Landlock domain" RFC [1]?  Mickaël and me have
discussed this patch set at LSS and I am also now coming around to the
realization that this would have introduced more complication.  - It might have
been a more "pure" approach, but comes at the expense of complicating Landlock
usage.)

—Günther

[1] https://lore.kernel.org/all/20240719150618.197991-1-mic@digikod.net/

* for later reference, my reasoning in the code is: net/ipv4/af_inet.c
  implements the entry points for connect() and listen() at the address family
  layer.  Both operations require that the sock->state is SS_UNCONNECTED.  So
  the rest is going through the other occurrences of SS_UNCONNECTED in that same
  file to see if there are any places where the socket can get back into that
  state.  The places I found where it is set to that state are:
  
  1. inet_create (right after creation, expected)
  2. __inet_stream_connect in the AF_UNSPEC case (known issue)
  3. __inet_stream_connect in the case of a failed connect (expected)
  4. inet_shutdown in the case of TCP_LISTEN or TCP_SYN_SENT (mentioned above)

Powered by blists - more mailing lists