Re: [RFC PATCH v2 3/8] landlock: Fix inconsistency of errors for TCP actions

["Mickaël Salaün" <mic@digikod.net>]

Something is wrong with this patch.

On Thu, Oct 17, 2024 at 07:04:49PM +0800, Mikhail Ivanov wrote:
> Add two helpers for TCP bind/connect accesses, which will serve to perform
> action-specific network stack level checks and safely extract the port from
> the address.
> 
> Return -EAFNOSUPPORT instead of -EINVAL in sin_family checks.
> 
> Check socket state before validating address for TCP connect access. This
> is necessary to follow the error order of network stack.
> 
> Read sk_family value from socket structure with READ_ONCE to safely handle
> IPV6_ADDRFORM case (see [1]).
> 
> [1] https://lore.kernel.org/all/20240202095404.183274-1-edumazet@google.com/
> 
> Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect")
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> ---
>  security/landlock/net.c | 543 +++++++++++++++++++++++-----------------
>  1 file changed, 315 insertions(+), 228 deletions(-)
>  rewrite security/landlock/net.c (37%)
> 
> diff --git a/security/landlock/net.c b/security/landlock/net.c
> dissimilarity index 37%
> index a3142f9b15ee..06791aba9196 100644
> --- a/security/landlock/net.c
> +++ b/security/landlock/net.c
> @@ -1,228 +1,315 @@
> -// SPDX-License-Identifier: GPL-2.0-only
> -/*
> - * Landlock LSM - Network management and hooks
> - *
> - * Copyright © 2022-2023 Huawei Tech. Co., Ltd.
> - * Copyright © 2022-2023 Microsoft Corporation
> - */
> -
> -#include <linux/in.h>
> -#include <linux/net.h>
> -#include <linux/socket.h>
> -#include <net/ipv6.h>
> -
> -#include "common.h"
> -#include "cred.h"
> -#include "limits.h"
> -#include "net.h"
> -#include "ruleset.h"
> -
> -int landlock_append_net_rule(struct landlock_ruleset *const ruleset,
> -			     const u16 port, access_mask_t access_rights)
> -{
> -	int err;
> -	const struct landlock_id id = {
> -		.key.data = (__force uintptr_t)htons(port),
> -		.type = LANDLOCK_KEY_NET_PORT,
> -	};
> -
> -	BUILD_BUG_ON(sizeof(port) > sizeof(id.key.data));
> -
> -	/* Transforms relative access rights to absolute ones. */
> -	access_rights |= LANDLOCK_MASK_ACCESS_NET &
> -			 ~landlock_get_net_access_mask(ruleset, 0);
> -
> -	mutex_lock(&ruleset->lock);
> -	err = landlock_insert_rule(ruleset, id, access_rights);
> -	mutex_unlock(&ruleset->lock);
> -
> -	return err;
> -}
> -
> -static const struct landlock_ruleset *get_current_net_domain(void)
> -{
> -	const union access_masks any_net = {
> -		.net = ~0,
> -	};
> -
> -	return landlock_match_ruleset(landlock_get_current_domain(), any_net);
> -}
> -
> -static int check_access_port(const struct landlock_ruleset *const dom,
> -			     __be16 port, access_mask_t access_request)
> -{
> -	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;
> -}
> -
> -static int hook_socket_bind(struct socket *const sock,
> -			    struct sockaddr *const address, const int addrlen)
> -{
> -	__be16 port;
> -	struct sock *const sk = sock->sk;
> -	const struct landlock_ruleset *const dom = get_current_net_domain();
> -
> -	if (!dom)
> -		return 0;
> -	if (WARN_ON_ONCE(dom->num_layers < 1))
> -		return -EACCES;
> -
> -	if (sk_is_tcp(sk)) {
> -		/* Checks for minimal header length to safely read sa_family. */
> -		if (addrlen < offsetofend(typeof(*address), sa_family))
> -			return -EINVAL;
> -
> -		switch (address->sa_family) {
> -		case AF_UNSPEC:
> -		case AF_INET:
> -			if (addrlen < sizeof(struct sockaddr_in))
> -				return -EINVAL;
> -			port = ((struct sockaddr_in *)address)->sin_port;
> -			break;
> -
> -#if IS_ENABLED(CONFIG_IPV6)
> -		case AF_INET6:
> -			if (addrlen < SIN6_LEN_RFC2133)
> -				return -EINVAL;
> -			port = ((struct sockaddr_in6 *)address)->sin6_port;
> -			break;
> -#endif /* IS_ENABLED(CONFIG_IPV6) */
> -
> -		default:
> -			return 0;
> -		}
> -
> -		/*
> -		 * For compatibility reason, accept AF_UNSPEC for bind
> -		 * accesses (mapped to AF_INET) only if the address is
> -		 * INADDR_ANY (cf. __inet_bind).  Checking the address is
> -		 * required to not wrongfully return -EACCES instead of
> -		 * -EAFNOSUPPORT.
> -		 *
> -		 * We could return 0 and let the network stack handle these
> -		 * checks, but it is safer to return a proper error and test
> -		 * consistency thanks to kselftest.
> -		 */
> -		if (address->sa_family == AF_UNSPEC) {
> -			/* addrlen has already been checked for AF_UNSPEC. */
> -			const struct sockaddr_in *const sockaddr =
> -				(struct sockaddr_in *)address;
> -
> -			if (sk->sk_family != AF_INET)
> -				return -EINVAL;
> -
> -			if (sockaddr->sin_addr.s_addr != htonl(INADDR_ANY))
> -				return -EAFNOSUPPORT;
> -		} else {
> -			/*
> -			 * Checks sa_family consistency to not wrongfully return
> -			 * -EACCES instead of -EINVAL.  Valid sa_family changes are
> -			 * only (from AF_INET or AF_INET6) to AF_UNSPEC.
> -			 *
> -			 * We could return 0 and let the network stack handle this
> -			 * check, but it is safer to return a proper error and test
> -			 * consistency thanks to kselftest.
> -			 */
> -			if (address->sa_family != sk->sk_family)
> -				return -EINVAL;
> -		}
> -		return check_access_port(dom, port,
> -					 LANDLOCK_ACCESS_NET_BIND_TCP);
> -	}
> -	return 0;
> -}
> -
> -static int hook_socket_connect(struct socket *const sock,
> -			       struct sockaddr *const address,
> -			       const int addrlen)
> -{
> -	__be16 port;
> -	struct sock *const sk = sock->sk;
> -	const struct landlock_ruleset *const dom = get_current_net_domain();
> -
> -	if (!dom)
> -		return 0;
> -	if (WARN_ON_ONCE(dom->num_layers < 1))
> -		return -EACCES;
> -
> -	if (sk_is_tcp(sk)) {
> -		/* Checks for minimal header length to safely read sa_family. */
> -		if (addrlen < offsetofend(typeof(*address), sa_family))
> -			return -EINVAL;
> -
> -		switch (address->sa_family) {
> -		case AF_UNSPEC:
> -		case AF_INET:
> -			if (addrlen < sizeof(struct sockaddr_in))
> -				return -EINVAL;
> -			port = ((struct sockaddr_in *)address)->sin_port;
> -			break;
> -
> -#if IS_ENABLED(CONFIG_IPV6)
> -		case AF_INET6:
> -			if (addrlen < SIN6_LEN_RFC2133)
> -				return -EINVAL;
> -			port = ((struct sockaddr_in6 *)address)->sin6_port;
> -			break;
> -#endif /* IS_ENABLED(CONFIG_IPV6) */
> -
> -		default:
> -			return 0;
> -		}
> -
> -		/*
> -		 * Connecting to an address with AF_UNSPEC dissolves the TCP
> -		 * association, which have the same effect as closing the
> -		 * connection while retaining the socket object (i.e., the file
> -		 * descriptor).  As for dropping privileges, closing
> -		 * connections is always allowed.
> -		 *
> -		 * For a TCP access control system, this request is legitimate.
> -		 * Let the network stack handle potential inconsistencies and
> -		 * return -EINVAL if needed.
> -		 */
> -		if (address->sa_family == AF_UNSPEC)
> -			return 0;
> -		/*
> -		 * Checks sa_family consistency to not wrongfully return
> -		 * -EACCES instead of -EINVAL.  Valid sa_family changes are
> -		 * only (from AF_INET or AF_INET6) to AF_UNSPEC.
> -		 *
> -		 * We could return 0 and let the network stack handle this
> -		 * check, but it is safer to return a proper error and test
> -		 * consistency thanks to kselftest.
> -		 */
> -		if (address->sa_family != sk->sk_family)
> -			return -EINVAL;
> -
> -		return check_access_port(dom, port,
> -					 LANDLOCK_ACCESS_NET_CONNECT_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),
> -};
> -
> -__init void landlock_add_net_hooks(void)
> -{
> -	security_add_hooks(landlock_hooks, ARRAY_SIZE(landlock_hooks),
> -			   &landlock_lsmid);
> -}
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Landlock LSM - Network management and hooks
> + *
> + * Copyright © 2022-2023 Huawei Tech. Co., Ltd.
> + * Copyright © 2022-2023 Microsoft Corporation
> + */
> +
> +#include <linux/in.h>
> +#include <linux/net.h>
> +#include <linux/socket.h>
> +#include <net/ipv6.h>
> +
> +#include "common.h"
> +#include "cred.h"
> +#include "limits.h"
> +#include "net.h"
> +#include "ruleset.h"
> +
> +int landlock_append_net_rule(struct landlock_ruleset *const ruleset,
> +			     const u16 port, access_mask_t access_rights)
> +{
> +	int err;
> +	const struct landlock_id id = {
> +		.key.data = (__force uintptr_t)htons(port),
> +		.type = LANDLOCK_KEY_NET_PORT,
> +	};
> +
> +	BUILD_BUG_ON(sizeof(port) > sizeof(id.key.data));
> +
> +	/* Transforms relative access rights to absolute ones. */
> +	access_rights |= LANDLOCK_MASK_ACCESS_NET &
> +			 ~landlock_get_net_access_mask(ruleset, 0);
> +
> +	mutex_lock(&ruleset->lock);
> +	err = landlock_insert_rule(ruleset, id, access_rights);
> +	mutex_unlock(&ruleset->lock);
> +
> +	return err;
> +}
> +
> +static const struct landlock_ruleset *get_current_net_domain(void)
> +{
> +	const union access_masks any_net = {
> +		.net = ~0,
> +	};
> +
> +	return landlock_match_ruleset(landlock_get_current_domain(), any_net);
> +}
> +
> +static int check_access_port(const struct landlock_ruleset *const dom,
> +			     __be16 port, access_mask_t access_request)
> +{
> +	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;
> +}
> +
> +/*
> + * Checks that TCP @sock and @address attributes are correct for bind(2).
> + *
> + * On success, extracts port from @address in @port and returns 0.
> + *
> + * This validation is consistent with network stack and returns the error
> + * in the order corresponding to the order of errors from the network stack.
> + * It's required to not wrongfully return -EACCES instead of meaningful network
> + * stack level errors. Consistency is tested with kselftest.
> + *
> + * This helper does not provide consistency of error codes for BPF filter
> + * (if any).
> + */
> +static int
> +check_tcp_bind_consistency_and_get_port(struct socket *const sock,
> +					struct sockaddr *const address,
> +					const int addrlen, __be16 *port)
> +{
> +	/* IPV6_ADDRFORM can change sk->sk_family under us. */
> +	switch (READ_ONCE(sock->sk->sk_family)) {
> +	case AF_INET:
> +		const struct sockaddr_in *const addr =
> +			(struct sockaddr_in *)address;
> +
> +		/* Cf. inet_bind_sk(). */
> +		if (addrlen < sizeof(struct sockaddr_in))
> +			return -EINVAL;
> +		/*
> +		 * For compatibility reason, accept AF_UNSPEC for bind
> +		 * accesses (mapped to AF_INET) only if the address is
> +		 * INADDR_ANY (cf. __inet_bind).
> +		 */
> +		if (addr->sin_family != AF_INET) {
> +			if (addr->sin_family != AF_UNSPEC ||
> +			    addr->sin_addr.s_addr != htonl(INADDR_ANY))
> +				return -EAFNOSUPPORT;
> +		}
> +		*port = ((struct sockaddr_in *)address)->sin_port;
> +		break;
> +#if IS_ENABLED(CONFIG_IPV6)
> +	case AF_INET6:
> +		/* Cf. inet6_bind_sk(). */
> +		if (addrlen < SIN6_LEN_RFC2133)
> +			return -EINVAL;
> +		/* Cf. __inet6_bind(). */
> +		if (address->sa_family != AF_INET6)
> +			return -EAFNOSUPPORT;
> +		*port = ((struct sockaddr_in6 *)address)->sin6_port;
> +		break;
> +#endif /* IS_ENABLED(CONFIG_IPV6) */
> +	default:
> +		WARN_ON_ONCE(0);
> +		return -EACCES;
> +	}
> +	return 0;
> +}
> +
> +/*
> + * Checks that TCP @sock and @address attributes are correct for connect(2).
> + *
> + * On success, extracts port from @address in @port and returns 0.
> + *
> + * This validation is consistent with network stack and returns the error
> + * in the order corresponding to the order of errors from the network stack.
> + * It's required to not wrongfully return -EACCES instead of meaningful network
> + * stack level error. Consistency is partially tested with kselftest.
> + *
> + * This helper does not provide consistency of error codes for BPF filter
> + * (if any).
> + *
> + * The function holds socket lock while checking the socket state.
> + */
> +static int
> +check_tcp_connect_consistency_and_get_port(struct socket *const sock,
> +					   struct sockaddr *const address,
> +					   const int addrlen, __be16 *port)
> +{
> +	int err = 0;
> +	struct sock *const sk = sock->sk;
> +
> +	/* Cf. __inet_stream_connect(). */
> +	lock_sock(sk);
> +	switch (sock->state) {
> +	default:
> +		err = -EINVAL;
> +		break;
> +	case SS_CONNECTED:
> +		err = -EISCONN;
> +		break;
> +	case SS_CONNECTING:
> +		/*
> +		 * Calling connect(2) on nonblocking socket with SYN_SENT or SYN_RECV
> +		 * state immediately returns -EISCONN and -EALREADY (Cf. __inet_stream_connect()).
> +		 *
> +		 * This check is not tested with kselftests.
> +		 */
> +		if ((sock->file->f_flags & O_NONBLOCK) &&
> +		    ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV))) {
> +			if (inet_test_bit(DEFER_CONNECT, sk))
> +				err = -EISCONN;
> +			else
> +				err = -EALREADY;
> +			break;
> +		}
> +
> +		/*
> +		 * Current state is possible in two cases:
> +		 * 1. connect(2) is called upon nonblocking socket and previous
> +		 *    connection attempt was closed by RST packet (therefore socket is
> +		 *    in TCP_CLOSE state). In this case connect(2) calls
> +		 *    sk_prot->disconnect(), changes socket state and increases number
> +		 *    of disconnects.
> +		 * 2. connect(2) is called twice upon socket with TCP_FASTOPEN_CONNECT
> +		 *    option set. If socket state is TCP_CLOSE connect(2) does the
> +		 *    same logic as in point 1 case. Otherwise connect(2) may freeze
> +		 *    after inet_wait_for_connect() call since SYN was never sent.
> +		 *
> +		 * For both this cases Landlock cannot provide error consistency since
> +		 * 1. Both cases involve executing some network stack logic and changing
> +		 *    the socket state.
> +		 * 2. It cannot omit access check and allow network stack handle error
> +		 *    consistency since socket can change its state to SS_UNCONNECTED
> +		 *    before it will be locked again in inet_stream_connect().
> +		 *
> +		 * Therefore it is only possible to return 0 and check access right with
> +		 * check_access_port() helper.
> +		 */
> +		release_sock(sk);
> +		return 0;
> +	case SS_UNCONNECTED:
> +		if (sk->sk_state != TCP_CLOSE)
> +			err = -EISCONN;
> +		break;
> +	}
> +	release_sock(sk);
> +
> +	if (err)
> +		return err;
> +
> +	/* IPV6_ADDRFORM can change sk->sk_family under us. */
> +	switch (READ_ONCE(sk->sk_family)) {
> +	case AF_INET:
> +		/* Cf. tcp_v4_connect(). */
> +		if (addrlen < sizeof(struct sockaddr_in))
> +			return -EINVAL;
> +		if (address->sa_family != AF_INET)
> +			return -EAFNOSUPPORT;
> +
> +		*port = ((struct sockaddr_in *)address)->sin_port;
> +		break;
> +#if IS_ENABLED(CONFIG_IPV6)
> +	case AF_INET6:
> +		/* Cf. tcp_v6_connect(). */
> +		if (addrlen < SIN6_LEN_RFC2133)
> +			return -EINVAL;
> +		if (address->sa_family != AF_INET6)
> +			return -EAFNOSUPPORT;
> +
> +		*port = ((struct sockaddr_in6 *)address)->sin6_port;
> +		break;
> +#endif /* IS_ENABLED(CONFIG_IPV6) */
> +	default:
> +		WARN_ON_ONCE(0);
> +		return -EACCES;
> +	}
> +
> +	return 0;
> +}
> +
> +static int hook_socket_bind(struct socket *const sock,
> +			    struct sockaddr *const address, const int addrlen)
> +{
> +	int err;
> +	__be16 port;
> +	const struct landlock_ruleset *const dom = get_current_net_domain();
> +
> +	if (!dom)
> +		return 0;
> +	if (WARN_ON_ONCE(dom->num_layers < 1))
> +		return -EACCES;
> +
> +	if (sk_is_tcp(sock->sk)) {
> +		err = check_tcp_bind_consistency_and_get_port(sock, address,
> +							      addrlen, &port);
> +		if (err)
> +			return err;
> +		return check_access_port(dom, port,
> +					 LANDLOCK_ACCESS_NET_BIND_TCP);
> +	}
> +	return 0;
> +}
> +
> +static int hook_socket_connect(struct socket *const sock,
> +			       struct sockaddr *const address,
> +			       const int addrlen)
> +{
> +	int err;
> +	__be16 port;
> +	const struct landlock_ruleset *const dom = get_current_net_domain();
> +
> +	if (!dom)
> +		return 0;
> +	if (WARN_ON_ONCE(dom->num_layers < 1))
> +		return -EACCES;
> +
> +	if (sk_is_tcp(sock->sk)) {
> +		/* Checks for minimal header length to safely read sa_family. */
> +		if (addrlen < sizeof(address->sa_family))
> +			return -EINVAL;
> +		/*
> +		 * Connecting to an address with AF_UNSPEC dissolves the TCP
> +		 * association, which have the same effect as closing the
> +		 * connection while retaining the socket object (i.e., the file
> +		 * descriptor).  As for dropping privileges, closing
> +		 * connections is always allowed.
> +		 *
> +		 * For a TCP access control system, this request is legitimate.
> +		 * Let the network stack handle potential inconsistencies and
> +		 * return -EINVAL if needed.
> +		 */
> +		if (address->sa_family == AF_UNSPEC)
> +			return 0;
> +
> +		err = check_tcp_connect_consistency_and_get_port(
> +			sock, address, addrlen, &port);
> +		if (err)
> +			return err;
> +		return check_access_port(dom, port,
> +					 LANDLOCK_ACCESS_NET_CONNECT_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),
> +};
> +
> +__init void landlock_add_net_hooks(void)
> +{
> +	security_add_hooks(landlock_hooks, ARRAY_SIZE(landlock_hooks),
> +			   &landlock_lsmid);
> +}
> -- 
> 2.34.1
> 
>