Re: [PATCH 1/2] landlock: Add hook on socket_listen()

["Günther Noack" <gnoack@google.com>]

On Mon, Jul 01, 2024 at 04:10:27PM +0300, Ivanov Mikhail wrote:
> Thanks for the great explanation! We're on the same page.
> 
> Considering that binding to ephemeral ports can be done not only with
> bind() or listen(), I think your approach is more correct.
> Restricting any possible binding to ephemeral ports just using
> LANDLOCK_ACCESS_NET_BIND_TCP wouldn't allow sandboxed processes
> to deny listen() without pre-binding (which is quite unsafe) and
> use connect() in the usuall way (without pre-binding).
> 
> Controlling ephemeral ports allocation for listen() can be done in the
> same way as for LANDLOCK_ACCESS_NET_BIND_TCP in the patch with
> LANDLOCK_ACCESS_NET_LISTEN_TCP access right implementation.

That sounds good, yes! 👍


> I'm only concerned about controlling the auto-binding for other
> operations (such as connect() and sendto() for UDP). As I said, I think
> this can also be useful: users will be able to control which processes
> are allowed to use ephemeral ports from ip_local_port_range and which
> are not, and they must assign ports for each operation explicitly. If
> you agree that such control is reasonable, we'll probably  have to
> consider some API changes, since such control is currently not possible.
> 
> We should clarify this before I send a patch with the
> LANDLOCK_ACCESS_NET_LISTEN_TCP implementation. WDYT?

LANDLOCK_ACCESS_NET_LISTEN_TCP seems like the most important to me.

For connect() and sendto(), I think the access rights are less urgent:

connect(): We already have LANDLOCK_ACCESS_NET_CONNECT_TCP, but that one is
getting restricted for the *remote* port number.

 (a) I think it would be possible to do the same for the *local* port number, by
     introducing a separate LANDLOCK_ACCESS_NET_CONNECT_TCP_LOCALPORT right.
     (Yes, the name is absolutely horrible, this is just for the example :))
     hook_socket_connect() would then need to do both a check for the remote
     port using LANDLOCK_ACCESS_NET_CONNECT_TCP, as it already does today, as
     well as a check for the (previously bound?) local port using
     LANDLOCK_ACCESS_NET_CONNECT_TCP_LOCALPORT.
     
     So I think it is extensible in that direction, in principle, even though I
     don't currently have a good name for that access right. :)
     
 (b) Compared to what LANDLOCK_ACCESS_NET_BIND_TCP already restricts, a
     hypothetical LANDLOCK_ACCESS_NET_CONNECT_TCP_LOCALPORT right would only
     additionally restrict the use of ephemeral ports.  I'm currently having a
     hard time seeing what an attacker could do with that (use up all ephemeral
     ports?).

sendto(): I think this is not relevant yet, because as the documentation said,
ephemeral ports are only handed out when sendto() is used with datagram (UDP)
sockets.

Once Landlock starts having UDP support, this would become relevant, but for
this patch set, I think that the TCP server use case as discussed further above
in this thread is very compelling.

Thanks,
—Günther