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:

quoted

4/30/2024 4:36 PM, Mickaël Salaün wrote:

quoted

On Mon, Apr 08, 2024 at 05:47:46PM +0800, Ivanov Mikhail wrote:

quoted

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?

quoted

quoted

quoted

+		/* 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