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 >
