Message-ID: <20240719150618.197991-2-mic@digikod.net>
Date: Fri, 19 Jul 2024 17:06:16 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: Günther Noack <gnoack@...gle.com>,
	Ivanov Mikhail <ivanov.mikhail1@...wei-partners.com>,
	Konstantin Meskhidze <konstantin.meskhidze@...wei.com>,
	Paul Moore <paul@...l-moore.com>
Cc: Mickaël Salaün <mic@...ikod.net>,
	Casey Schaufler <casey@...aufler-ca.com>,
	Jeff Xu <jeffxu@...gle.com>,
	Kees Cook <keescook@...omium.org>,
	"Serge E . Hallyn" <serge@...lyn.com>,
	Shervin Oloumi <enlightened@...omium.org>,
	Tahera Fahimi <fahimitahera@...il.com>,
	linux-kernel@...r.kernel.org,
	linux-security-module@...r.kernel.org,
	stable@...r.kernel.org
Subject: [RFC PATCH v1 1/3] landlock: Use socket's domain instead of current's domain

Before this change, network restrictions were enforced according to the
calling thread's Landlock domain, leading to potential inconsistent
results when the same socket was used by different threads or processes
(with different domains).  This change fixes such access control
inconsistency by enforcing the socket's Landlock domain instead of the
caller's Landlock domain.

Socket's Landlock domain is inherited from the thread that created this
socket.  This means that a socket created without sandboxing will be
free to connect and bind without limitation.  This also means that a
socket created by a sandboxed thread will inherit the thread's policy,
which will be enforced on this socket even when used by another thread
or passed to another process.

The initial rationale [1] was that a socket does not directly grants
access to data, but it is an object used to define an access (e.g.
connection to a peer).  Contrary to my initial assumption, we can
identify to which protocol/port a newly created socket can give access
to with the socket's file->f_cred inherited from its creator.  Moreover,
from a kernel point of view, especially for shared objects, we need a
more consistent access model.  This means that the same action on the
same socket performed by different threads will have the same effect.
This follows the same approach as for file descriptors tied to the file
system (e.g. LANDLOCK_ACCESS_FS_TRUNCATE).

One potential risk of this change is for unsandboxed processes to send
socket file descriptors to sandboxed processes, which could give
unrestricted network access to the sandboxed process (by reconfigure the
socket).  While it makes sense for processes to transfer (AF_UNIX)
socketpairs, which is OK because they can only exchange data between
themselves, it should be rare for processes to legitimately pass other
kind of sockets (e.g. AF_INET).

Another potential risk of this approach is socket file descriptor leaks.
This is the same risk as with regular file descriptor leaks giving
access to the content of a file, which is well known and documented.
This could be mitigated with a future complementary restriction on
received or inherited file descriptors.

One interesting side effect of this new approach is that a process can
create a socket that will only allow to connect to a set of ports.  This
can be done by creating a thread, sandboxing it, creating a socket, and
using the related file descriptor (in the same process).  Passing this
restricted socket to a more sandboxed process makes it possible to have
a more dynamic security policy.

This new approach aligns with SELinux and Smack instead of AppArmor and
Tomoyo.  It is also in line with capability-based security mechanisms
such as Capsicum.

This slight semantic change is important for current and future
Landlock's consistency, and it must be backported.

Current tests are still OK because this behavior wasn't covered.  A
following commit adds new tests.

Cc: Günther Noack <gnoack@...gle.com>
Cc: Ivanov Mikhail <ivanov.mikhail1@...wei-partners.com>
Cc: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
Cc: Paul Moore <paul@...l-moore.com>
Cc: Tahera Fahimi <fahimitahera@...il.com>
Cc: <stable@...r.kernel.org> # 6.7.x: 088e2efaf3d2: landlock: Simplify current_check_access_socket()
Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect")
Link: https://lore.kernel.org/r/263c1eb3-602f-57fe-8450-3f138581bee7@digikod.net [1]
Signed-off-by: Mickaël Salaün <mic@...ikod.net>
Link: https://lore.kernel.org/r/20240719150618.197991-2-mic@digikod.net
---
 security/landlock/net.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/security/landlock/net.c b/security/landlock/net.c
index c8bcd29bde09..78e027a74819 100644
--- a/security/landlock/net.c
+++ b/security/landlock/net.c
@@ -50,10 +50,11 @@ get_raw_handled_net_accesses(const struct landlock_ruleset *const domain)
 	return access_dom;
 }
 
-static const struct landlock_ruleset *get_current_net_domain(void)
+static const struct landlock_ruleset *
+get_socket_net_domain(const struct socket *const sock)
 {
 	const struct landlock_ruleset *const dom =
-		landlock_get_current_domain();
+		landlock_cred(sock->file->f_cred)->domain;
 
 	if (!dom || !get_raw_handled_net_accesses(dom))
 		return NULL;
@@ -61,10 +62,9 @@ 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(struct socket *const sock,
+			       struct sockaddr *const address,
+			       const int addrlen, access_mask_t access_request)
 {
 	__be16 port;
 	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_NET] = {};
@@ -72,7 +72,7 @@ static int current_check_access_socket(struct socket *const sock,
 	struct landlock_id id = {
 		.type = LANDLOCK_KEY_NET_PORT,
 	};
-	const struct landlock_ruleset *const dom = get_current_net_domain();
+	const struct landlock_ruleset *const dom = get_socket_net_domain(sock);
 
 	if (!dom)
 		return 0;
@@ -175,16 +175,16 @@ static int current_check_access_socket(struct socket *const sock,
 static int hook_socket_bind(struct socket *const sock,
 			    struct sockaddr *const address, const int addrlen)
 {
-	return current_check_access_socket(sock, address, addrlen,
-					   LANDLOCK_ACCESS_NET_BIND_TCP);
+	return check_access_socket(sock, address, addrlen,
+				   LANDLOCK_ACCESS_NET_BIND_TCP);
 }
 
 static int hook_socket_connect(struct socket *const sock,
 			       struct sockaddr *const address,
 			       const int addrlen)
 {
-	return current_check_access_socket(sock, address, addrlen,
-					   LANDLOCK_ACCESS_NET_CONNECT_TCP);
+	return check_access_socket(sock, address, addrlen,
+				   LANDLOCK_ACCESS_NET_CONNECT_TCP);
 }
 
 static struct security_hook_list landlock_hooks[] __ro_after_init = {
-- 
2.45.2

Powered by blists - more mailing lists