Re: [RFC PATCH v1 4/7] landlock: Add UDP send+recv access control

From: Matthieu Buffet
Date: Sat Oct 19 2024 - 08:55:34 EST


Hi Mickaël,

I've almost finished merging your review (thanks for all the feedback), with the exception of this main point. Just making sure we agree on the limitations before I merge this into a new version.

On 9/21/2024 12:23 PM, Mickaël Salaün wrote:
>> + /*
>> + * If there is a more specific address in the message, it will take
>> + * precedence over any connect()ed address. Base our access check on it.
>> + */
>> + if (address) {
>> + const bool in_udpv6_sendmsg =
>> + (sock->sk->sk_prot == &udpv6_prot);
>> +
>> + err = get_addr_port(address, msg->msg_namelen, in_udpv6_sendmsg,
>> + &port);
>> + if (err != 0)
>> + return err;
>> +
>> + /*
>> + * In `udpv6_sendmsg`, AF_UNSPEC is interpreted as "no address".
>> + * In that case, the call above will succeed but without
>> + * returning a port.
>> + */
>> + if (in_udpv6_sendmsg && address->sa_family == AF_UNSPEC)
>> + address = NULL;
>> + }
>> +
>> + /*
>> + * Without a message-specific destination address, the socket must be
>> + * connect()ed to an address, base our access check on that one.
>> + */
>> + if (!address) {
>
> If the address is not specified, I think we should just allow the
> request and let the network stack handle the rest. The advantage of
> this approach would be that if the socket was previously allowed to be
> connected, the check is only done once and they will be almost no
> performance impact when calling sendto/write/recvfrom/read on this
> "connected" socket.
> [...]
> What about something like this (with the appropriate comments)?
>
> if (!address)
> return 0;
>
> if (address->sa_family == AF_UNSPEC && sock->sk->sk_prot ==
> &udpv6_prot)
> return 0;
>
> err = get_addr_port(address, msg->msg_namelen, &port);
> if (err)
> return err;
>
> return check_access_port(dom, LANDLOCK_ACCESS_NET_SENDMSG_UDP, port);

If I understand correctly, you would like the semantics of LANDLOCK_ACCESS_NET_CONNECT_UDP to be {connect(), and sendmsg() without explicit address} and LANDLOCK_ACCESS_NET_SENDMSG_UDP to be {sendmsg() with explicit address}. This makes it impossible to allow a landlocked server to connect() in order to receive traffic only from a specific client while at the same time preventing it from sending traffic (that is, a receive-only client-specific socket ala Cloudflare's "established-over-unconnected"[1]).

>> + err = check_access_port(dom, LANDLOCK_ACCESS_NET_RECVMSG_UDP,
>> + port_bigendian);
>> + if (err != -EACCES)
>> + return err;
>
> We should be able to follow the same policy for "connected" sockets.

Again if I understand correctly, to fully merge semantics of LANDLOCK_ACCESS_NET_BIND_UDP and LANDLOCK_ACCESS_NET_RECVMSG_UDP (since if the access check is performed at bind() time, there is nothing to check in recvmsg() anymore). Similarly, this makes it impossible to allow a send-only program to bind() to set a source port without allowing it to recvmsg() traffic.

I do not know of real-life programs that might want to sandbox their network workers *that* precisely, nor how much we want to be future-proof and support it. If not, I can merge your feedback and:
- remove LANDLOCK_ACCESS_NET_RECVMSG_UDP and the recvmsg() hook;
- change the doc for LANDLOCK_ACCESS_NET_SENDMSG_UDP to mention that it is not required if the app uses connect() and then sendmsg() without explicit addresses;
- change the doc for LANDLOCK_ACCESS_NET_CONNECT_UDP to mention that it grants the right to send traffic (and similarly for LANDLOCK_ACCESS_NET_BIND_UDP to receive traffic), and the reason (performance, though I haven't managed to get a benchmark);
- rename to LANDLOCK_ACCESS_NET_CONNECT_SENDMSG_UDP, LANDLOCK_ACCESS_NET_SENDMSG_UDP, and LANDLOCK_ACCESS_NET_BIND_RECVMSG_UDP, what do you think?

If merging semantics is a problem, I mentioned socket tagging in [2] to reduce the performance impact (e.g. tag whether it can send traffic at connect() time, and tag whether it can recv at bind() time). So another option could be to keep precise semantics and explore that?

>> + /*
>> + * Slow path: socket is bound to an ephemeral port. Need a second check
>> + * on port 0 with different semantics ("any ephemeral port").
>> + */
>> + inet_sk_get_local_port_range(sk, &ephemeral_low, &ephemeral_high);
>
> Is it to handle recvmsg(with port 0)?

If you mean recvmsg() on a socket that was previously bind(0), yep. This second rule lookup added a different meaning to rules on port 0. Without this, one would not be able to allow "all ephemeral ports", making the feature unusable for servers that bind on ephemeral ports. All this disappears if we remove LANDLOCK_ACCESS_NET_RECVMSG_UDP.

Matthieu

[1] https://blog.cloudflare.com/everything-you-ever-wanted-to-know-about-udp-sockets-but-were-afraid-to-ask-part-1/
[2] https://github.com/landlock-lsm/linux/issues/10#issuecomment-2267871144