[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240620.teeFoot6gaeX@digikod.net>
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