Message-ID: <20241021.Abohbuph8eet@digikod.net>
Date: Mon, 21 Oct 2024 11:57:53 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: Matthieu Buffet <matthieu@...fet.re>
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

On Sat, Oct 19, 2024 at 02:47:48PM +0200, Matthieu Buffet wrote:
> 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}.

Not exactly, here is the rewording with my thinking:
...the semantics of LANDLOCK_ACCESS_NET_CONNECT_UDP to be {connect()}
and LANDLOCK_ACCESS_NET_SENDMSG_UDP to be {sendmsg() with explicit
address}.

sendmsg() without explicit address should always be allowed,
whatever the Landlock policy (similarly as write(2) on a write-opened
file descriptor).  In a nutshell, sendmsg(2) without explicit address
should be handled the same as a write(2) call on a connected socket (I
guess the kernel handles such action on connected datagram sockets the
same as on connected stream sockets).

I think it is more important to first have a simple model that
enables developers to initialize a socket with connect(2) and then
sandbox the process to only be able to use this socket to communicate
with the configured peer, similar to what can be enforced with TCP
sockets.

sendmsg(2) can do two different thinks: (optionally) configure a
peer/port and write data. recvmsg(2) only reads data.  We should first
start by controlling exchange from/to peers, and maybe later controlling
data flow.

An alternative approach would be to not add a sendmsg specific access
right but only LANDLOCK_ACCESS_NET_CONNECT_UDP because connect should
be a superset of sendmsg.  This would make it impossible to specifically
deny (shared) socket's configuration change though.  I think it's
better to stick to the kernel semantic with 3 dedicated access rights,
and it should make more sense for users too.  What do you think?


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

My proposal would indeed makes this use case impossible to enforce only
with the proposed access rights, and we would need to restrict write(2)
too BTW.  However, I think it would make sense to add complementary
access rights to restrict reading or writing to a socket.  I guess this
semantic would be useful for non-UDP protocols too with
LANDLOCK_ACCESS_NET_{READ,WRITE}_{TCP,UDP} access rights set at
socket-creation time and stored in the socket object (instead of looking
at a Landlock domain for each read/write call, similarly to the truncate
and ioctl_dev access rights).  What do you think?

I wonder what happens if we call recvmsg(2) on a newly created (and then
unconfigured) socket.  Without prior call to bind(2) I would guess that
the recvmsg(2) call fail but I'm not so sure with a datagram socket.  We
should check that.

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

Correct (I was a bit confused with recvmsg, but it doesn't set the
receiving address/port).

> Similarly, this makes it impossible to allow a send-only
> program to bind() to set a source port without allowing it to recvmsg()
> traffic.

Correct with the current access rights.  We would need a
LANDLOCK_ACCESS_NET_READ_UDP.

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

Yes

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

Yes

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

The documentation should highlight that these flags grants the right to
configure a socket, but indeed, no restrictions are enforced on reading
or writing on sockets.

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

The first and third rights are confusing.  I prefer simple names such as
LANDLOCK_ACCESS_NET_CONNECT_UDP and LANDLOCK_ACCESS_NET_BIND_UDP.
Moreover, LANDLOCK_ACCESS_NET_CONNECT_UDP would not impact sendmsg(2)
calls at all.

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

This tagging mechanism looks like a good idea to implement
LANDLOCK_ACCESS_NET_{READ,WRITE}_{TCP,UDP}, but that should be a
future separate patch series.

> 
> >> +	/*
> >> +	 * 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.

OK

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