Message-ID: <20240425.Soot5eNeexol@digikod.net>
Date: Tue, 30 Apr 2024 15:36:28 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: Ivanov Mikhail <ivanov.mikhail1@...wei-partners.com>
Cc: willemdebruijn.kernel@...il.com, gnoack3000@...il.com, 
	linux-security-module@...r.kernel.org, netdev@...r.kernel.org, netfilter-devel@...r.kernel.org, 
	yusongping@...wei.com, artem.kuzin@...wei.com, konstantin.meskhidze@...wei.com, 
	Günther Noack <gnoack@...gle.com>
Subject: Re: [PATCH 1/2] landlock: Add hook on socket_listen()

On Mon, Apr 08, 2024 at 05:47:46PM +0800, Ivanov Mikhail wrote:
> Make hook for socket_listen(). It will check that the socket protocol is
> TCP, and if the socket's local port number is 0 (which means,
> that listen(2) was called without any previous bind(2) call),
> then listen(2) call will be legitimate only if there is a rule for bind(2)
> allowing binding to port 0 (or if LANDLOCK_ACCESS_NET_BIND_TCP is not
> supported by the sandbox).

Thanks for this patch and sorry for the late full review.  The code is
good overall.

We should either consider this patch as a fix or add a new flag/access
right to Landlock syscalls for compatibility reason.  I think this
should be a fix.  Calling listen(2) without a previous call to bind(2)
is a corner case that we should properly handle.  The commit message
should make that explicit and highlight the goal of the patch: first
explain why, and then how.

We also need to update the user documentation to explain that
LANDLOCK_ACCESS_NET_BIND_TCP also handles this case.

> 
> Create a new check_access_socket() function to prevent useless copy paste.
> It should be called by hook handlers after they perform special checks and
> calculate socket port value.

You can add this tag:
Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect")

> 
> Signed-off-by: Ivanov Mikhail <ivanov.mikhail1@...wei-partners.com>
> Reviewed-by: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
> ---
>  security/landlock/net.c | 104 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 88 insertions(+), 16 deletions(-)
> 
> diff --git a/security/landlock/net.c b/security/landlock/net.c
> index c8bcd29bde09..c6ae4092cfd6 100644
> --- a/security/landlock/net.c
> +++ b/security/landlock/net.c
> @@ -10,6 +10,7 @@
>  #include <linux/net.h>
>  #include <linux/socket.h>
>  #include <net/ipv6.h>
> +#include <net/tcp.h>
>  
>  #include "common.h"
>  #include "cred.h"
> @@ -61,17 +62,36 @@ static const struct landlock_ruleset *get_current_net_domain(void)
>  	return dom;
>  }
>  
> -static int current_check_access_socket(struct socket *const sock,
> -				       struct sockaddr *const address,
> -				       const int addrlen,
> -				       access_mask_t access_request)
> +static int check_access_socket(const struct landlock_ruleset *const dom,
> +			  __be16 port,
> +			  access_mask_t access_request)

Please format all patches with clang-format.

>  {
> -	__be16 port;
>  	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_NET] = {};
>  	const struct landlock_rule *rule;
>  	struct landlock_id id = {
>  		.type = LANDLOCK_KEY_NET_PORT,
>  	};
> +
> +	id.key.data = (__force uintptr_t)port;
> +	BUILD_BUG_ON(sizeof(port) > sizeof(id.key.data));
> +
> +	rule = landlock_find_rule(dom, id);
> +	access_request = landlock_init_layer_masks(
> +		dom, access_request, &layer_masks, LANDLOCK_KEY_NET_PORT);
> +
> +	if (landlock_unmask_layers(rule, access_request, &layer_masks,
> +				   ARRAY_SIZE(layer_masks)))
> +		return 0;
> +
> +	return -EACCES;
> +}

This check_access_socket() refactoring should be in a dedicated patch.

> +
> +static int current_check_access_socket(struct socket *const sock,
> +				       struct sockaddr *const address,
> +				       const int addrlen,
> +				       access_mask_t access_request)
> +{
> +	__be16 port;
>  	const struct landlock_ruleset *const dom = get_current_net_domain();
>  
>  	if (!dom)
> @@ -159,17 +179,7 @@ static int current_check_access_socket(struct socket *const sock,
>  			return -EINVAL;
>  	}
>  
> -	id.key.data = (__force uintptr_t)port;
> -	BUILD_BUG_ON(sizeof(port) > sizeof(id.key.data));
> -
> -	rule = landlock_find_rule(dom, id);
> -	access_request = landlock_init_layer_masks(
> -		dom, access_request, &layer_masks, LANDLOCK_KEY_NET_PORT);
> -	if (landlock_unmask_layers(rule, access_request, &layer_masks,
> -				   ARRAY_SIZE(layer_masks)))
> -		return 0;
> -
> -	return -EACCES;
> +	return check_access_socket(dom, port, access_request);
>  }
>  
>  static int hook_socket_bind(struct socket *const sock,
> @@ -187,9 +197,71 @@ static int hook_socket_connect(struct socket *const sock,
>  					   LANDLOCK_ACCESS_NET_CONNECT_TCP);
>  }
>  
> +/*
> + * Check that socket state and attributes are correct for listen.
> + * It is required to not wrongfully return -EACCES instead of -EINVAL.
> + */
> +static int check_tcp_socket_can_listen(struct socket *const sock)
> +{
> +	struct sock *sk = sock->sk;
> +	unsigned char cur_sk_state = sk->sk_state;
> +	const struct inet_connection_sock *icsk;
> +
> +	/* Allow only unconnected TCP socket to listen(cf. inet_listen). */

nit: Missing space.

The other comments in Landlock are written with the third person
(in theory everywhere): "Allows..."

> +	if (sock->state != SS_UNCONNECTED)
> +		return -EINVAL;
> +
> +	/* Check sock state consistency. */

Can you explain exactly what is going on here (in the comment)? Linking
to a kernel function would help.

> +	if (!((1 << cur_sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
> +		return -EINVAL;
> +
> +	/* Sockets can listen only if ULP control hook has clone method. */

What is ULP?

> +	icsk = inet_csk(sk);
> +	if (icsk->icsk_ulp_ops && !icsk->icsk_ulp_ops->clone)
> +		return -EINVAL;

Can you please add tests covering all these error cases?

> +	return 0;
> +}
> +
> +static int hook_socket_listen(struct socket *const sock,
> +			  const int backlog)
> +{
> +	int err;
> +	int family;
> +	const struct landlock_ruleset *const dom = get_current_net_domain();
> +
> +	if (!dom)
> +		return 0;
> +	if (WARN_ON_ONCE(dom->num_layers < 1))
> +		return -EACCES;
> +
> +	/*
> +	 * listen() on a TCP socket without pre-binding is allowed only
> +	 * if binding to port 0 is allowed.
> +	 */

This comment should be just before the inet_sk(sock->sk)->inet_num
check.

> +	family = sock->sk->__sk_common.skc_family;
> +
> +	if (family == AF_INET || family == AF_INET6) {

This would make the code simpler:

if (family != AF_INET && family != AF_INET6)
	return 0;


What would be the effect of listen() on an AF_UNSPEC socket?

> +		/* Checks if it's a (potential) TCP socket. */
> +		if (sock->type != SOCK_STREAM)
> +			return 0;

As for current_check_access_socket() this kind of check should be at the
beginning of the function (before the family check) to exit early and
simplify code.

> +
> +		/* Socket is alredy binded to some port. */

This kind of spelling issue can be found by scripts/checkpatch.pl

> +		if (inet_sk(sock->sk)->inet_num != 0)

Why do we want to allow listen() on any socket that is binded?

> +			return 0;
> +
> +		err = check_tcp_socket_can_listen(sock);
> +		if (unlikely(err))
> +			return err;
> +
> +		return check_access_socket(dom, 0, LANDLOCK_ACCESS_NET_BIND_TCP);
> +	}
> +	return 0;
> +}
> +
>  static struct security_hook_list landlock_hooks[] __ro_after_init = {
>  	LSM_HOOK_INIT(socket_bind, hook_socket_bind),
>  	LSM_HOOK_INIT(socket_connect, hook_socket_connect),
> +	LSM_HOOK_INIT(socket_listen, hook_socket_listen),
>  };
>  
>  __init void landlock_add_net_hooks(void)
> -- 
> 2.34.1
> 
> 

Powered by blists - more mailing lists