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

[Ivanov Mikhail]

7/1/2024 6:47 PM, Günther Noack wrote:
> 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. :)

Indeed, implementing a new type of rule seems to be an optimal approach
for this.

>       
>   (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?).

I thought about something like that, yeah) Also, I tried to find out
if there are cases where port remains bound to the client socket after
the connection is completed. In this case, listen() can be called to a
socket with a port bound via connect() call. In the case of TCP, such
scenario is not possible, port is always deallocated in tcp_set_state()
method.

So, I don't see any realworld vulnerabilities, we can leave this case
until someone comes up with a real issue.

> 
> 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.

Agreed. Anyway, if someone finds any interesting cases with
auto-binding via sendto(), we can implement a new rule, as you suggested
for connect().

Thanks you for your research and ideas, Günther!
I'll prepare the LANDLOCK_ACCESS_NET_LISTEN_TCP patchset for the review.

> 
> Thanks,
> —Günther