Date: Thu, 20 Jun 2024 10:00:36 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: Günther Noack <gnoack@...gle.com>, 
	Eric Dumazet <edumazet@...gle.com>
Cc: Ivanov Mikhail <ivanov.mikhail1@...wei-partners.com>, 
	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: [PATCH 1/2] landlock: Add hook on socket_listen()

On Wed, Jun 19, 2024 at 09:05:03PM +0200, Günther Noack wrote:
> I agree with Mickaël's comment: this seems like an important fix.
> 
> Mostly for completeness: I played with the "socket type" patch set in a "TCP
> server" example, where *all* possible operations are restricted with Landlock,
> including the ones from the "socket type" patch set V2 with the little fix we
> discussed.
> 
>  - socket()
>  - bind()
>  - enforce a landlock ruleset restricting:
>    - file system access
>    - all TCP bind and connect
>    - socket creation
>  - listen()
>  - accept()
> 
> From the connection handler (which would be the place where an attacker can
> usually provide input), it is now still possible to bind a socket due to this
> problem.  The steps are:
> 
>   1) connect() on client_fd with AF_UNSPEC to disassociate the client FD
>   2) listen() on the client_fd
> 
> This succeeds and it listens on an ephemeral port.
> 
> The code is at [1], if you are interested.
> 
> [1] https://github.com/gnoack/landlock-examples/blob/main/tcpserver.c
> 
> 
> On Mon, May 13, 2024 at 03:15:50PM +0300, Ivanov Mikhail wrote:
> > 4/30/2024 4:36 PM, Mickaël Salaün wrote:
> > > On Mon, Apr 08, 2024 at 05:47:46PM +0800, Ivanov Mikhail wrote:
> > > > Make hook for socket_listen(). It will check that the socket protocol is
> > > > TCP, and if the socket's local port number is 0 (which means,
> > > > that listen(2) was called without any previous bind(2) call),
> > > > then listen(2) call will be legitimate only if there is a rule for bind(2)
> > > > allowing binding to port 0 (or if LANDLOCK_ACCESS_NET_BIND_TCP is not
> > > > supported by the sandbox).
> > > 
> > > Thanks for this patch and sorry for the late full review.  The code is
> > > good overall.
> > > 
> > > We should either consider this patch as a fix or add a new flag/access
> > > right to Landlock syscalls for compatibility reason.  I think this
> > > should be a fix.  Calling listen(2) without a previous call to bind(2)
> > > is a corner case that we should properly handle.  The commit message
> > > should make that explicit and highlight the goal of the patch: first
> > > explain why, and then how.
> > 
> > Yeap, this is fix-patch. I have covered motivation and proposed solution
> > in cover letter. Do you have any suggestions on how i can improve this?
> 
> Without wanting to turn around the direction of this code review now, I am still
> slightly concerned about the assymetry of this special case being implemented
> for listen() but not for connect().
> 
> The reason is this: My colleague Mr. B. recently pointed out to me that you can
> also do a bind() on a socket before a connect(!). The steps are:
> 
> * create socket with socket()
> * bind() to a local port 9090
> * connect() to a remote port 8080
> 
> This gives you a connection between ports 9090 and 8080.

Yes, this should not be an issue, but something to keep in mind.

> 
> A regular connect() without an explicit bind() is of course the more usual
> scenario.  In that case, we are also using up ("implicitly binding") one of the
> ephemeral ports.
> 
> It seems that, with respect to the port binding, listen() and connect() work
> quite similarly then?  This being considered, maybe it *is* the listen()
> operation on a port which we should be restricting, and not bind()?

I agree that we should be able to control listen according to the binded
port, see https://github.com/landlock-lsm/linux/issues/15
In a nutshell, the LANDLOCK_ACCESS_NET_LISTEN_TCP should make more sense
for most use cases, but I think LANDLOCK_ACCESS_NET_BIND_TCP is also
useful to limit opened (well-known) ports and port spoofing.

> 
> With some luck, that would then also free us from having to implement the
> check_tcp_socket_can_listen() logic, which is seemingly emulating logic from
> elsewhere in the kernel?

An alternative could be to only use LANDLOCK_ACCESS_NET_BIND_TCP for
explicit binding (i.e. current state, but with appropriate
documentation), and delegate to LANDLOCK_ACCESS_NET_LISTEN_TCP the
control of binding with listen(2).  That should free us from
implementing check_tcp_socket_can_listen().  The rationale would be that
a malicious sandboxed process could not explicitly bind to a
well-specified port, but only to a range of dedicated random ports (the
same range use for auto-binding with connect).  That could also help
developers by staying close to the kernel syscall ABI (principle of
least astonishment).

> 
> (I am by far not an expert in Linux networking, so I'll put this out for
> consideration and will happily stand corrected if I am misunderstanding
> something.  Maybe someone with more networking background can chime in?)

That would be good indeed.  Netfilter or network folks? Eric?

> 
> 
> > > > +		/* Socket is alredy binded to some port. */
> > > 
> > > This kind of spelling issue can be found by scripts/checkpatch.pl
> > 
> > will be fixed
> 
> P.S. there are two typos here, the obvious one in "alredy",
> but also the passive of "to bind" is "bound", not "binded".
> (That is also mis-spelled in a few more places I think.)
> 
> —Günther
> 

Powered by blists - more mailing lists