Message-ID: <20241017110454.265818-4-ivanov.mikhail1@huawei-partners.com>
Date: Thu, 17 Oct 2024 19:04:49 +0800
From: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
To: <mic@...ikod.net>, <gnoack@...gle.com>
CC: <willemdebruijn.kernel@...il.com>, <matthieu@...fet.re>,
	<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>
Subject: [RFC PATCH v2 3/8] landlock: Fix inconsistency of errors for TCP actions

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@...wei-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

Powered by blists - more mailing lists