Message-ID: <3631edfd-7f41-4ff1-9f30-20dcaa17b726@buffet.re>
Date: Sat, 19 Oct 2024 14:47:48 +0200
From: Matthieu Buffet <matthieu@...fet.re>
To: Mickaël Salaün <mic@...ikod.net>
Cc: Günther Noack <gnoack@...gle.com>,
 Paul Moore <paul@...l-moore.com>, James Morris <jmorris@...ei.org>,
 "Serge E . Hallyn" <serge@...lyn.com>,
 linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org,
 netdev@...r.kernel.org,
 Konstantin Meskhidze <konstantin.meskhidze@...wei.com>,
 Ivanov Mikhail <ivanov.mikhail1@...wei-partners.com>
Subject: Re: [RFC PATCH v1 4/7] landlock: Add UDP send+recv access control

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

Powered by blists - more mailing lists