landlock: UNIX connect() control by pathname and scope

From: Justin Suess <[email protected]>

Add a LSM hook security_unix_find.

This hook is called to check the path of a named unix socket before a connection is initiated. The peer socket may be inspected as well.

Why existing hooks are unsuitable:

Existing socket hooks, security_unix_stream_connect(), security_unix_may_send(), and security_socket_connect() don't provide TOCTOU-free / namespace independent access to the paths of sockets.

(1) We cannot resolve the path from the struct sockaddr in existing hooks. This requires another path lookup. A change in the path between the two lookups will cause a TOCTOU bug.

(2) We cannot use the struct path from the listening socket, because it may be bound to a path in a different namespace than the caller, resulting in a path that cannot be referenced at policy creation time.

Cc Günther Noack <[email protected]>

Cc Tingmao Wang <[email protected]>

Signed-off-by Justin Suess <[email protected]>

---
 include/linux/lsm_hook_defs.h |  5 +++++
 include/linux/security.h      | 11 +++++++++++
 net/unix/af_unix.c            |  8 ++++++++
 security/security.c           | 20 ++++++++++++++++++++
 4 files changed, 44 insertions(+)

include/linux/lsm_hook_defs.h +5 -0

index 8c42b4bde09c..7a0fd3dbfa29 100644
@@ -317,6 +317,11 @@ LSM_HOOK(int, 0, post_notification, const struct cred *w_cred,
 LSM_HOOK(int, 0, watch_key, struct key *key)
 #endif /* CONFIG_SECURITY && CONFIG_KEY_NOTIFICATIONS */
 
+#if defined(CONFIG_SECURITY_NETWORK) && defined(CONFIG_SECURITY_PATH)
+LSM_HOOK(int, 0, unix_find, const struct path *path, struct sock *other,
+	 int flags)
+#endif /* CONFIG_SECURITY_NETWORK && CONFIG_SECURITY_PATH */
+
 #ifdef CONFIG_SECURITY_NETWORK
 LSM_HOOK(int, 0, unix_stream_connect, struct sock *sock, struct sock *other,
 	 struct sock *newsk)

include/linux/security.h +11 -0

index 83a646d72f6f..99a33d8eb28d 100644
@@ -1931,6 +1931,17 @@ static inline int security_mptcp_add_subflow(struct sock *sk, struct sock *ssk)
 }
 #endif	/* CONFIG_SECURITY_NETWORK */
 
+#if defined(CONFIG_SECURITY_NETWORK) && defined(CONFIG_SECURITY_PATH)
+
+int security_unix_find(const struct path *path, struct sock *other, int flags);
+
+#else /* CONFIG_SECURITY_NETWORK && CONFIG_SECURITY_PATH */
+static inline int security_unix_find(const struct path *path, struct sock *other, int flags)
+{
+	return 0;
+}
+#endif /* CONFIG_SECURITY_NETWORK && CONFIG_SECURITY_PATH */
+
 #ifdef CONFIG_SECURITY_INFINIBAND
 int security_ib_pkey_access(void *sec, u64 subnet_prefix, u16 pkey);
 int security_ib_endport_manage_subnet(void *sec, const char *name, u8 port_num);

net/unix/af_unix.c +0 -0

index d0511225799b..369812b79dd8 100644
@@ -1230,6 +1230,14 @@ static struct sock *unix_find_bsd(struct sockaddr_un *sunaddr, int addr_len,
 	if (!sk)
 		goto path_put;
 

net/unix/af_unix.c +0 -0

 		touch_atime(&path);

security/security.c +20 -0

index 67af9228c4e9..c73196b8db4b 100644
@@ -4731,6 +4731,26 @@ int security_mptcp_add_subflow(struct sock *sk, struct sock *ssk)
 
 #endif	/* CONFIG_SECURITY_NETWORK */
 
+#if defined(CONFIG_SECURITY_NETWORK) && defined(CONFIG_SECURITY_PATH)
+/**
+ * security_unix_find() - Check if a named AF_UNIX socket can connect
+ * @path: path of the socket being connected to
+ * @other: peer sock
+ * @flags: flags associated with the socket
+ *
+ * This hook is called to check permissions before connecting to a named
+ * AF_UNIX socket.
+ *
+ * Return: Returns 0 if permission is granted.
+ */
+int security_unix_find(const struct path *path, struct sock *other, int flags)
+{
+	return call_int_hook(unix_find, path, other, flags);
+}
+EXPORT_SYMBOL(security_unix_find);
+
+#endif	/* CONFIG_SECURITY_NETWORK && CONFIG_SECURITY_PATH */
+
 #ifdef CONFIG_SECURITY_INFINIBAND
 /**
  * security_ib_pkey_access() - Check if access to an IB pkey is allowed

Signature · 1 line

2.52.0

Quoted 77 lines · Günther Noack

From: Justin Suess <[email protected]>

Add a LSM hook security_unix_find.

This hook is called to check the path of a named unix socket before a connection is initiated. The peer socket may be inspected as well.

Why existing hooks are unsuitable:

Existing socket hooks, security_unix_stream_connect(), security_unix_may_send(), and security_socket_connect() don't provide TOCTOU-free / namespace independent access to the paths of sockets.

(1) We cannot resolve the path from the struct sockaddr in existing hooks. This requires another path lookup. A change in the path between the two lookups will cause a TOCTOU bug.

(2) We cannot use the struct path from the listening socket, because it may be bound to a path in a different namespace than the caller, resulting in a path that cannot be referenced at policy creation time.

Cc Günther Noack <[email protected]>

Cc Tingmao Wang <[email protected]>

Signed-off-by Justin Suess <[email protected]>

---
 include/linux/lsm_hook_defs.h |  5 +++++
 include/linux/security.h      | 11 +++++++++++
 net/unix/af_unix.c            |  8 ++++++++
 security/security.c           | 20 ++++++++++++++++++++
 4 files changed, 44 insertions(+)

include/linux/lsm_hook_defs.h +5 -0

index 8c42b4bde09c..7a0fd3dbfa29 100644
@@ -317,6 +317,11 @@ LSM_HOOK(int, 0, post_notification, const struct cred *w_cred,
 LSM_HOOK(int, 0, watch_key, struct key *key)
 #endif /* CONFIG_SECURITY && CONFIG_KEY_NOTIFICATIONS */
 
+#if defined(CONFIG_SECURITY_NETWORK) && defined(CONFIG_SECURITY_PATH)
+LSM_HOOK(int, 0, unix_find, const struct path *path, struct sock *other,
+	 int flags)
+#endif /* CONFIG_SECURITY_NETWORK && CONFIG_SECURITY_PATH */
+
 #ifdef CONFIG_SECURITY_NETWORK
 LSM_HOOK(int, 0, unix_stream_connect, struct sock *sock, struct sock *other,
 	 struct sock *newsk)

include/linux/security.h +11 -0

index 83a646d72f6f..99a33d8eb28d 100644
@@ -1931,6 +1931,17 @@ static inline int security_mptcp_add_subflow(struct sock *sk, struct sock *ssk)
 }
 #endif	/* CONFIG_SECURITY_NETWORK */
 
+#if defined(CONFIG_SECURITY_NETWORK) && defined(CONFIG_SECURITY_PATH)
+
+int security_unix_find(const struct path *path, struct sock *other, int flags);
+
+#else /* CONFIG_SECURITY_NETWORK && CONFIG_SECURITY_PATH */
+static inline int security_unix_find(const struct path *path, struct sock *other, int flags)
+{
+	return 0;
+}
+#endif /* CONFIG_SECURITY_NETWORK && CONFIG_SECURITY_PATH */
+
 #ifdef CONFIG_SECURITY_INFINIBAND
 int security_ib_pkey_access(void *sec, u64 subnet_prefix, u16 pkey);
 int security_ib_endport_manage_subnet(void *sec, const char *name, u8 port_num);

net/unix/af_unix.c +0 -0

index d0511225799b..369812b79dd8 100644
@@ -1230,6 +1230,14 @@ static struct sock *unix_find_bsd(struct sockaddr_un *sunaddr, int addr_len,
 	if (!sk)
 		goto path_put;
 

net/unix/af_unix.c static struct sock *unix_find_bsd(struct sockaddr_un *sunaddr, int addr_len, · line 1242

Quoted 10 lines

net/unix/af_unix.c +8 -0

+	/*
+	 * We call the hook because we know that the inode is a socket and we
+	 * hold a valid reference to it via the path.
+	 */
+	err = security_unix_find(&path, sk, flags);
+	if (err)
+		goto sock_put;
+
 	err = -EPROTOTYPE;
 	if (sk->sk_type == type)

I think this hook call should be moved here, just before the touch_atime() call for consistency with the socket type check, and to avoid doing useless check in the hook.

Quoted 36 lines

net/unix/af_unix.c +0 -0

 		touch_atime(&path);

security/security.c +20 -0

index 67af9228c4e9..c73196b8db4b 100644
@@ -4731,6 +4731,26 @@ int security_mptcp_add_subflow(struct sock *sk, struct sock *ssk)
 
 #endif	/* CONFIG_SECURITY_NETWORK */
 
+#if defined(CONFIG_SECURITY_NETWORK) && defined(CONFIG_SECURITY_PATH)
+/**
+ * security_unix_find() - Check if a named AF_UNIX socket can connect
+ * @path: path of the socket being connected to
+ * @other: peer sock
+ * @flags: flags associated with the socket
+ *
+ * This hook is called to check permissions before connecting to a named
+ * AF_UNIX socket.
+ *
+ * Return: Returns 0 if permission is granted.
+ */
+int security_unix_find(const struct path *path, struct sock *other, int flags)
+{
+	return call_int_hook(unix_find, path, other, flags);
+}
+EXPORT_SYMBOL(security_unix_find);
+
+#endif	/* CONFIG_SECURITY_NETWORK && CONFIG_SECURITY_PATH */
+
 #ifdef CONFIG_SECURITY_INFINIBAND
 /**
  * security_ib_pkey_access() - Check if access to an IB pkey is allowed

Signature · 1 line

2.52.0

Add a LSM hook security_unix_find.

This hook is called to check the path of a named unix socket before a connection is initiated. The peer socket may be inspected as well.

Why existing hooks are unsuitable:

Existing socket hooks, security_unix_stream_connect(), security_unix_may_send(), and security_socket_connect() don't provide TOCTOU-free / namespace independent access to the paths of sockets.

(1) We cannot resolve the path from the struct sockaddr in existing hooks. This requires another path lookup. A change in the path between the two lookups will cause a TOCTOU bug.

(2) We cannot use the struct path from the listening socket, because it may be bound to a path in a different namespace than the caller, resulting in a path that cannot be referenced at policy creation time.

Cc Günther Noack <[email protected]>

Cc Tingmao Wang <[email protected]>

Signed-off-by Justin Suess <[email protected]>

---
 include/linux/lsm_hook_defs.h |  5 +++++
 include/linux/security.h      | 11 +++++++++++
 net/unix/af_unix.c            | 13 ++++++++++---
 security/security.c           | 20 ++++++++++++++++++++
 4 files changed, 46 insertions(+), 3 deletions(-)

include/linux/lsm_hook_defs.h +5 -0

index 8c42b4bde09c..7a0fd3dbfa29 100644
@@ -317,6 +317,11 @@ LSM_HOOK(int, 0, post_notification, const struct cred *w_cred,
 LSM_HOOK(int, 0, watch_key, struct key *key)
 #endif /* CONFIG_SECURITY && CONFIG_KEY_NOTIFICATIONS */
 
+#if defined(CONFIG_SECURITY_NETWORK) && defined(CONFIG_SECURITY_PATH)
+LSM_HOOK(int, 0, unix_find, const struct path *path, struct sock *other,
+	 int flags)
+#endif /* CONFIG_SECURITY_NETWORK && CONFIG_SECURITY_PATH */
+
 #ifdef CONFIG_SECURITY_NETWORK
 LSM_HOOK(int, 0, unix_stream_connect, struct sock *sock, struct sock *other,
 	 struct sock *newsk)

include/linux/security.h +11 -0

index 83a646d72f6f..99a33d8eb28d 100644
@@ -1931,6 +1931,17 @@ static inline int security_mptcp_add_subflow(struct sock *sk, struct sock *ssk)
 }
 #endif	/* CONFIG_SECURITY_NETWORK */
 
+#if defined(CONFIG_SECURITY_NETWORK) && defined(CONFIG_SECURITY_PATH)
+
+int security_unix_find(const struct path *path, struct sock *other, int flags);
+
+#else /* CONFIG_SECURITY_NETWORK && CONFIG_SECURITY_PATH */
+static inline int security_unix_find(const struct path *path, struct sock *other, int flags)
+{
+	return 0;
+}
+#endif /* CONFIG_SECURITY_NETWORK && CONFIG_SECURITY_PATH */
+
 #ifdef CONFIG_SECURITY_INFINIBAND
 int security_ib_pkey_access(void *sec, u64 subnet_prefix, u16 pkey);
 int security_ib_endport_manage_subnet(void *sec, const char *name, u8 port_num);

net/unix/af_unix.c +10 -3

index f6d56e70c7a2..41698460194b 100644
@@ -1231,10 +1231,17 @@ static struct sock *unix_find_bsd(struct sockaddr_un *sunaddr, int addr_len,
 		goto path_put;
 
 	err = -EPROTOTYPE;
-	if (sk->sk_type == type)
-		touch_atime(&path);
-	else
+	if (sk->sk_type != type)
+		goto sock_put;
+
+	/*
+	 * We call the hook because we know that the inode is a socket and we
+	 * hold a valid reference to it via the path.
+	 */
+	err = security_unix_find(&path, sk, flags);
+	if (err)
 		goto sock_put;
+	touch_atime(&path);
 
 	path_put(&path);
 

security/security.c +20 -0

index 67af9228c4e9..c73196b8db4b 100644
@@ -4731,6 +4731,26 @@ int security_mptcp_add_subflow(struct sock *sk, struct sock *ssk)
 
 #endif	/* CONFIG_SECURITY_NETWORK */
 
+#if defined(CONFIG_SECURITY_NETWORK) && defined(CONFIG_SECURITY_PATH)
+/**
+ * security_unix_find() - Check if a named AF_UNIX socket can connect
+ * @path: path of the socket being connected to
+ * @other: peer sock
+ * @flags: flags associated with the socket
+ *
+ * This hook is called to check permissions before connecting to a named
+ * AF_UNIX socket.
+ *
+ * Return: Returns 0 if permission is granted.
+ */
+int security_unix_find(const struct path *path, struct sock *other, int flags)
+{
+	return call_int_hook(unix_find, path, other, flags);
+}
+EXPORT_SYMBOL(security_unix_find);
+
+#endif	/* CONFIG_SECURITY_NETWORK && CONFIG_SECURITY_PATH */
+
 #ifdef CONFIG_SECURITY_INFINIBAND
 /**
  * security_ib_pkey_access() - Check if access to an IB pkey is allowed

Signature · 1 line

2.52.0

Quoted 28 lines · Justin Suess

Add a LSM hook security_unix_find.

This hook is called to check the path of a named unix socket before a connection is initiated. The peer socket may be inspected as well.

Why existing hooks are unsuitable:

Existing socket hooks, security_unix_stream_connect(), security_unix_may_send(), and security_socket_connect() don't provide TOCTOU-free / namespace independent access to the paths of sockets.

(1) We cannot resolve the path from the struct sockaddr in existing hooks. This requires another path lookup. A change in the path between the two lookups will cause a TOCTOU bug.

(2) We cannot use the struct path from the listening socket, because it may be bound to a path in a different namespace than the caller, resulting in a path that cannot be referenced at policy creation time.

Cc Günther Noack <[email protected]>

Cc Tingmao Wang <[email protected]>

Signed-off-by Justin Suess <[email protected]>

---
 include/linux/lsm_hook_defs.h |  5 +++++
 include/linux/security.h      | 11 +++++++++++
 net/unix/af_unix.c            | 13 ++++++++++---
 security/security.c           | 20 ++++++++++++++++++++
 4 files changed, 46 insertions(+), 3 deletions(-)

Reviewed-by Günther Noack <[email protected]>

Thank you, this looks good. I'll include it in the next version of the Unix connect patch set again.

Quoted 29 lines · Günther Noack

Quoted 28 lines · Justin Suess

Add a LSM hook security_unix_find.

This hook is called to check the path of a named unix socket before a connection is initiated. The peer socket may be inspected as well.

Why existing hooks are unsuitable:

Existing socket hooks, security_unix_stream_connect(), security_unix_may_send(), and security_socket_connect() don't provide TOCTOU-free / namespace independent access to the paths of sockets.

(1) We cannot resolve the path from the struct sockaddr in existing hooks. This requires another path lookup. A change in the path between the two lookups will cause a TOCTOU bug.

(2) We cannot use the struct path from the listening socket, because it may be bound to a path in a different namespace than the caller, resulting in a path that cannot be referenced at policy creation time.

Cc Günther Noack <[email protected]>

Cc Tingmao Wang <[email protected]>

Signed-off-by Justin Suess <[email protected]>

---
 include/linux/lsm_hook_defs.h |  5 +++++
 include/linux/security.h      | 11 +++++++++++
 net/unix/af_unix.c            | 13 ++++++++++---
 security/security.c           | 20 ++++++++++++++++++++
 4 files changed, 46 insertions(+), 3 deletions(-)

...

Quoted 2 lines

Reviewed-by Günther Noack <[email protected]>

Quoted 2 lines

Thank you, this looks good. I'll include it in the next version of the Unix connect patch set again.

I'm looking for this patchset to review/ACK the new hook in context, but I'm not seeing it in my inbox or lore. Did I simply miss the patchset or is it still a work in progress? No worries if it hasn't been posted yet, I just wanted to make sure I wasn't holding this up any more than I already may have :)

Signature · 1 line

paul-moore.com

Quoted 35 lines · Paul Moore

Quoted 29 lines · Günther Noack

Quoted 28 lines · Justin Suess

Add a LSM hook security_unix_find.

This hook is called to check the path of a named unix socket before a connection is initiated. The peer socket may be inspected as well.

Why existing hooks are unsuitable:

Existing socket hooks, security_unix_stream_connect(), security_unix_may_send(), and security_socket_connect() don't provide TOCTOU-free / namespace independent access to the paths of sockets.

(1) We cannot resolve the path from the struct sockaddr in existing hooks. This requires another path lookup. A change in the path between the two lookups will cause a TOCTOU bug.

(2) We cannot use the struct path from the listening socket, because it may be bound to a path in a different namespace than the caller, resulting in a path that cannot be referenced at policy creation time.

Cc Günther Noack <[email protected]>

Cc Tingmao Wang <[email protected]>

Signed-off-by Justin Suess <[email protected]>

---
 include/linux/lsm_hook_defs.h |  5 +++++
 include/linux/security.h      | 11 +++++++++++
 net/unix/af_unix.c            | 13 ++++++++++---
 security/security.c           | 20 ++++++++++++++++++++
 4 files changed, 46 insertions(+), 3 deletions(-)

...

Quoted 2 lines

Reviewed-by Günther Noack <[email protected]>

Quoted 8 lines

Quoted 2 lines

Thank you, this looks good. I'll include it in the next version of the Unix connect patch set again.

I'm looking for this patchset to review/ACK the new hook in context, but I'm not seeing it in my inbox or lore. Did I simply miss the patchset or is it still a work in progress? No worries if it hasn't been posted yet, I just wanted to make sure I wasn't holding this up any more than I already may have :)

Good Morning Paul,

Can't speak to the rest of the patch, but I sent this LSM hook for review purposes before inclusion with the rest of the V6 of this patch.

Günther added his review tag, but I was asked to make some minor comment / commit message updates. I sent the same patch, with updated comments/commit to him in a follow up, off-list email to avoid spamming the list. No code changes were made, just comments.

I don't think this particular patch will change substantially, unless we find something unexpected. But the way we use the hook may change (esp wrt to locking and the SOCK_DEAD state), which is important for your review.

So you may want to hold off your review until the full V6 series gets sent so you can review the hook in context. There were some questions about locking that needed proper digging into. [1]

Quoted 56 lines · Justin Suess

Quoted 35 lines · Paul Moore

Quoted 29 lines · Günther Noack

Quoted 28 lines · Justin Suess

Add a LSM hook security_unix_find.

This hook is called to check the path of a named unix socket before a connection is initiated. The peer socket may be inspected as well.

Why existing hooks are unsuitable:

Existing socket hooks, security_unix_stream_connect(), security_unix_may_send(), and security_socket_connect() don't provide TOCTOU-free / namespace independent access to the paths of sockets.

(1) We cannot resolve the path from the struct sockaddr in existing hooks. This requires another path lookup. A change in the path between the two lookups will cause a TOCTOU bug.

(2) We cannot use the struct path from the listening socket, because it may be bound to a path in a different namespace than the caller, resulting in a path that cannot be referenced at policy creation time.

Cc Günther Noack <[email protected]>

Cc Tingmao Wang <[email protected]>

Signed-off-by Justin Suess <[email protected]>

---
 include/linux/lsm_hook_defs.h |  5 +++++
 include/linux/security.h      | 11 +++++++++++
 net/unix/af_unix.c            | 13 ++++++++++---
 security/security.c           | 20 ++++++++++++++++++++
 4 files changed, 46 insertions(+), 3 deletions(-)

...

Quoted 2 lines

Reviewed-by Günther Noack <[email protected]>

Quoted 8 lines

Quoted 2 lines

Thank you, this looks good. I'll include it in the next version of the Unix connect patch set again.

I'm looking for this patchset to review/ACK the new hook in context, but I'm not seeing it in my inbox or lore. Did I simply miss the patchset or is it still a work in progress? No worries if it hasn't been posted yet, I just wanted to make sure I wasn't holding this up any more than I already may have :)

Good Morning Paul,

Can't speak to the rest of the patch, but I sent this LSM hook for review purposes before inclusion with the rest of the V6 of this patch.

Günther added his review tag, but I was asked to make some minor comment / commit message updates. I sent the same patch, with updated comments/commit to him in a follow up, off-list email to avoid spamming the list. No code changes were made, just comments.

Quoted 7 lines

I don't think this particular patch will change substantially, unless we find something unexpected. But the way we use the hook may change (esp wrt to locking and the SOCK_DEAD state), which is important for your review.

So you may want to hold off your review until the full V6 series gets sent so you can review the hook in context. There were some questions about locking that needed proper digging into. [1]

Great, thanks for the update, that was helpful. As you recommend, I'll hold off on reviewing this further until we have the full context of the other patchset; we've already talked about this hook addition a few times anyway, and based on a quick look yesterday, nothing particularly evil jumped out at me.

security/security.c +0 -0

index 67af9228c4e9..c73196b8db4b 100644
@@ -4731,6 +4731,26 @@ int security_mptcp_add_subflow(struct sock *sk, struct sock *ssk)
 
 #endif	/* CONFIG_SECURITY_NETWORK */

Hello!

Quoted 7 lines · Justin Suess

security/security.c +0 -0

index 67af9228c4e9..c73196b8db4b 100644
@@ -4731,6 +4731,26 @@ int security_mptcp_add_subflow(struct sock *sk, struct sock *ssk)
 
 #endif	/* CONFIG_SECURITY_NETWORK */

security/security.c int security_mptcp_add_subflow(struct sock *sk, struct sock *ssk) · line 4742

Quoted 10 lines

security/security.c +9 -0

 
+#if defined(CONFIG_SECURITY_NETWORK) && defined(CONFIG_SECURITY_PATH)
+/**
+ * security_unix_find() - Check if a named AF_UNIX socket can connect
+ * @path: path of the socket being connected to
+ * @other: peer sock
+ * @flags: flags associated with the socket
+ *
+ * This hook is called to check permissions before connecting to a named
+ * AF_UNIX socket.

Nit: Could we please insert a sentence about locking here?

Something like:

The caller holds no locks on @other.

(Originally brought up by Mickaël in
https://lore.kernel.org/all/[email protected]/)

Thanks,
–Günther

Quoted 27 lines · Günther Noack

Hello!

Quoted 7 lines · Justin Suess

security/security.c +0 -0

index 67af9228c4e9..c73196b8db4b 100644
@@ -4731,6 +4731,26 @@ int security_mptcp_add_subflow(struct sock *sk, struct sock *ssk)
 
 #endif	/* CONFIG_SECURITY_NETWORK */

security/security.c int security_mptcp_add_subflow(struct sock *sk, struct sock *ssk) · line 4742

Quoted 10 lines

security/security.c +9 -0

 
+#if defined(CONFIG_SECURITY_NETWORK) && defined(CONFIG_SECURITY_PATH)
+/**
+ * security_unix_find() - Check if a named AF_UNIX socket can connect
+ * @path: path of the socket being connected to
+ * @other: peer sock
+ * @flags: flags associated with the socket
+ *
+ * This hook is called to check permissions before connecting to a named
+ * AF_UNIX socket.

Nit: Could we please insert a sentence about locking here?

Something like:

The caller holds no locks on @other.

Quoted 5 lines

(Originally brought up by Mickaël in
https://lore.kernel.org/all/[email protected]/)

Thanks,
–Günther

Sounds good. Would a "Link:" to the mentioned thread be appropriate in the commit message?

I feel like the reasoning for this is subtle but important for hook consumers.

Justin

* Add a new access right LANDLOCK_ACCESS_FS_RESOLVE_UNIX, which controls the look up operations for named UNIX domain sockets. The resolution happens during connect() and sendmsg() (depending on socket type).
* Hook into the path lookup in unix_find_bsd() in af_unix.c, using a LSM hook. Make policy decisions based on the new access rights
* Increment the Landlock ABI version.
* Minor test adaptions to keep the tests working.

With this access right, access is granted if either of the following conditions is met:

* The target socket's filesystem path was allow-listed using a LANDLOCK_RULE_PATH_BENEATH rule, *or*:
* The target socket was created in the same Landlock domain in which LANDLOCK_ACCESS_FS_RESOLVE_UNIX was restricted.

In case of a denial, connect() and sendmsg() return EACCES, which is the same error as it is returned if the user does not have the write bit in the traditional Unix file system permissions of that file.

This feature was created with substantial discussion and input from Justin Suess, Tingmao Wang and Mickaël Salaün.

Cc Tingmao Wang <[email protected]>

Cc Justin Suess <[email protected]>

Cc Mickaël Salaün <[email protected]>

Suggested-by Jann Horn <[email protected]>

Link https://github.com/landlock-lsm/linux/issues/36

Signed-off-by Günther Noack <[email protected]>

---
 include/uapi/linux/landlock.h                |  10 ++
 security/landlock/access.h                   |  11 +-
 security/landlock/audit.c                    |   1 +
 security/landlock/fs.c                       | 102 ++++++++++++++++++-
 security/landlock/limits.h                   |   2 +-
 security/landlock/syscalls.c                 |   2 +-
 tools/testing/selftests/landlock/base_test.c |   2 +-
 tools/testing/selftests/landlock/fs_test.c   |   5 +-
 8 files changed, 128 insertions(+), 7 deletions(-)

include/uapi/linux/landlock.h +0 -0

index f88fa1f68b77..3a8fc3af0d64 100644
@@ -248,6 +248,15 @@ struct landlock_net_port_attr {
  *
  *   This access right is available since the fifth version of the Landlock

include/uapi/linux/landlock.h +1 -0

  *
  * Whether an opened file can be truncated with :manpage:`ftruncate(2)` or used
  * with `ioctl(2)` is determined during :manpage:`open(2)`, in the same way as

@@ -333,6 +342,7 @@ struct landlock_net_port_attr {
 #define LANDLOCK_ACCESS_FS_REFER			(1ULL << 13)
 #define LANDLOCK_ACCESS_FS_TRUNCATE			(1ULL << 14)
 #define LANDLOCK_ACCESS_FS_IOCTL_DEV			(1ULL << 15)
+#define LANDLOCK_ACCESS_FS_RESOLVE_UNIX			(1ULL << 16)
 /* clang-format on */
 
 /**

security/landlock/access.h +10 -1

index 42c95747d7bd..9a2991688835 100644
@@ -34,7 +34,7 @@
 	LANDLOCK_ACCESS_FS_IOCTL_DEV)
 /* clang-format on */
 
-typedef u16 access_mask_t;
+typedef u32 access_mask_t;
 
 /* Makes sure all filesystem access rights can be stored. */
 static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);

@@ -76,6 +76,15 @@ struct layer_access_masks {
 	access_mask_t access[LANDLOCK_MAX_NUM_LAYERS];
 };
 
+static inline bool
+layer_access_masks_empty(const struct layer_access_masks *masks)
+{
+	for (size_t i = 0; i < ARRAY_SIZE(masks->access); i++)
+		if (masks->access[i])
+			return false;
+	return true;
+}
+
 /*
  * Tracks domains responsible of a denied access.  This avoids storing in each
  * object the full matrix of per-layer unfulfilled access rights, which is

security/landlock/audit.c +1 -0

index 60ff217ab95b..8d0edf94037d 100644
@@ -37,6 +37,7 @@ static const char *const fs_access_strings[] = {
 	[BIT_INDEX(LANDLOCK_ACCESS_FS_REFER)] = "fs.refer",
 	[BIT_INDEX(LANDLOCK_ACCESS_FS_TRUNCATE)] = "fs.truncate",
 	[BIT_INDEX(LANDLOCK_ACCESS_FS_IOCTL_DEV)] = "fs.ioctl_dev",
+	[BIT_INDEX(LANDLOCK_ACCESS_FS_RESOLVE_UNIX)] = "fs.resolve_unix",
 };
 
 static_assert(ARRAY_SIZE(fs_access_strings) == LANDLOCK_NUM_ACCESS_FS);

security/landlock/fs.c +9 -1

index e764470f588c..76035c6f2bf1 100644
@@ -27,6 +27,7 @@
 #include <linux/lsm_hooks.h>
 #include <linux/mount.h>
 #include <linux/namei.h>
+#include <linux/net.h>
 #include <linux/path.h>
 #include <linux/pid.h>
 #include <linux/rcupdate.h>

@@ -314,7 +315,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
 	LANDLOCK_ACCESS_FS_READ_FILE | \
 	LANDLOCK_ACCESS_FS_TRUNCATE | \
-	LANDLOCK_ACCESS_FS_IOCTL_DEV)
+	LANDLOCK_ACCESS_FS_IOCTL_DEV | \
+	LANDLOCK_ACCESS_FS_RESOLVE_UNIX)
 /* clang-format on */
 
 /*

@@ -1561,6 +1563,103 @@ static int hook_path_truncate(const struct path *const path)
 	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
 }
 
+/**
+ * unmask_scoped_access - Remove access right bits in @masks in all layers
+ *                        where @client and @server have the same domain
+ *
+ * This does the same as domain_is_scoped(), but unmasks bits in @masks.
+ * It can not return early as domain_is_scoped() does.

security/landlock/fs.c +55 -0

+{
+	int client_layer, server_layer;
+	const struct landlock_hierarchy *client_walker, *server_walker;
+
+	if (WARN_ON_ONCE(!client))
+		return; /* should not happen */
+
+	if (!server)
+		return; /* server has no Landlock domain; nothing to clear */
+
+	client_layer = client->num_layers - 1;
+	client_walker = client->hierarchy;
+	server_layer = server->num_layers - 1;
+	server_walker = server->hierarchy;
+
+	/*
+	 * Clears the access bits at all layers where the client domain is the
+	 * same as the server domain.  We start the walk at min(client_layer,
+	 * server_layer).  The layer bits until there can not be cleared because
+	 * either the client or the server domain is missing.
+	 */
+	for (; client_layer > server_layer; client_layer--)
+		client_walker = client_walker->parent;
+
+	for (; server_layer > client_layer; server_layer--)
+		server_walker = server_walker->parent;
+
+	for (; client_layer >= 0; client_layer--) {
+		if (masks->access[client_layer] & access &&
+		    client_walker == server_walker)
+			masks->access[client_layer] &= ~access;
+
+		client_walker = client_walker->parent;
+		server_walker = server_walker->parent;
+	}
+}
+
+static int hook_unix_find(const struct path *const path, struct sock *other,
+			  int flags)
+{
+	const struct landlock_ruleset *dom_other;
+	const struct landlock_cred_security *subject;
+	struct layer_access_masks layer_masks;
+	struct landlock_request request = {};
+	static const struct access_masks fs_resolve_unix = {
+		.fs = LANDLOCK_ACCESS_FS_RESOLVE_UNIX,
+	};
+
+	/* Lookup for the purpose of saving coredumps is OK. */
+	if (unlikely(flags & SOCK_COREDUMP))
+		return 0;
+
+	/* Access to the same (or a lower) domain is always allowed. */
+	subject = landlock_get_applicable_subject(current_cred(),
+						  fs_resolve_unix, NULL);

security/landlock/fs.c +17 -0

+	unmask_scoped_access(subject->domain, dom_other, &layer_masks,
+			     fs_resolve_unix.fs);
+
+	if (layer_access_masks_empty(&layer_masks))
+		return 0;
+
+	/* Checks the connections to allow-listed paths. */
+	if (is_access_to_paths_allowed(subject->domain, path,
+				       fs_resolve_unix.fs, &layer_masks,
+				       &request, NULL, 0, NULL, NULL, NULL))
+		return 0;
+
+	landlock_log_denial(subject, &request);
+	return -EACCES;
+}
+
 /* File hooks */
 
 /**

@@ -1838,6 +1937,7 @@ static struct security_hook_list landlock_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(path_unlink, hook_path_unlink),
 	LSM_HOOK_INIT(path_rmdir, hook_path_rmdir),
 	LSM_HOOK_INIT(path_truncate, hook_path_truncate),
+	LSM_HOOK_INIT(unix_find, hook_unix_find),
 
 	LSM_HOOK_INIT(file_alloc_security, hook_file_alloc_security),
 	LSM_HOOK_INIT(file_open, hook_file_open),

security/landlock/limits.h +1 -1

index eb584f47288d..b454ad73b15e 100644
@@ -19,7 +19,7 @@
 #define LANDLOCK_MAX_NUM_LAYERS		16
 #define LANDLOCK_MAX_NUM_RULES		U32_MAX
 
-#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_IOCTL_DEV
+#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_RESOLVE_UNIX
 #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
 #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
 

security/landlock/syscalls.c +1 -1

index 0d66a68677b7..933902d43241 100644
@@ -164,7 +164,7 @@ static const struct file_operations ruleset_fops = {
  * If the change involves a fix that requires userspace awareness, also update
  * the errata documentation in Documentation/userspace-api/landlock.rst .
  */
-const int landlock_abi_version = 8;
+const int landlock_abi_version = 9;
 
 /**
  * sys_landlock_create_ruleset - Create a new ruleset

tools/testing/selftests/landlock/base_test.c +1 -1

index 0fea236ef4bd..30d37234086c 100644
@@ -76,7 +76,7 @@ TEST(abi_version)
 	const struct landlock_ruleset_attr ruleset_attr = {
 		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
 	};
-	ASSERT_EQ(8, landlock_create_ruleset(NULL, 0,
+	ASSERT_EQ(9, landlock_create_ruleset(NULL, 0,
 					     LANDLOCK_CREATE_RULESET_VERSION));
 
 	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,

tools/testing/selftests/landlock/fs_test.c +3 -2

index 968a91c927a4..b318627e7561 100644
@@ -575,9 +575,10 @@ TEST_F_FORK(layout1, inval)
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
 	LANDLOCK_ACCESS_FS_READ_FILE | \
 	LANDLOCK_ACCESS_FS_TRUNCATE | \
-	LANDLOCK_ACCESS_FS_IOCTL_DEV)
+	LANDLOCK_ACCESS_FS_IOCTL_DEV | \
+	LANDLOCK_ACCESS_FS_RESOLVE_UNIX)
 
-#define ACCESS_LAST LANDLOCK_ACCESS_FS_IOCTL_DEV
+#define ACCESS_LAST LANDLOCK_ACCESS_FS_RESOLVE_UNIX
 
 #define ACCESS_ALL ( \
 	ACCESS_FILE | \

Signature · 1 line

2.52.0

Quoted 78 lines · Günther Noack

* Add a new access right LANDLOCK_ACCESS_FS_RESOLVE_UNIX, which controls the look up operations for named UNIX domain sockets. The resolution happens during connect() and sendmsg() (depending on socket type).
* Hook into the path lookup in unix_find_bsd() in af_unix.c, using a LSM hook. Make policy decisions based on the new access rights
* Increment the Landlock ABI version.
* Minor test adaptions to keep the tests working.

With this access right, access is granted if either of the following conditions is met:

* The target socket's filesystem path was allow-listed using a LANDLOCK_RULE_PATH_BENEATH rule, *or*:
* The target socket was created in the same Landlock domain in which LANDLOCK_ACCESS_FS_RESOLVE_UNIX was restricted.

In case of a denial, connect() and sendmsg() return EACCES, which is the same error as it is returned if the user does not have the write bit in the traditional Unix file system permissions of that file.

This feature was created with substantial discussion and input from Justin Suess, Tingmao Wang and Mickaël Salaün.

Cc Tingmao Wang <[email protected]>

Cc Justin Suess <[email protected]>

Cc Mickaël Salaün <[email protected]>

Suggested-by Jann Horn <[email protected]>

Link https://github.com/landlock-lsm/linux/issues/36

Signed-off-by Günther Noack <[email protected]>

---
 include/uapi/linux/landlock.h                |  10 ++
 security/landlock/access.h                   |  11 +-
 security/landlock/audit.c                    |   1 +
 security/landlock/fs.c                       | 102 ++++++++++++++++++-
 security/landlock/limits.h                   |   2 +-
 security/landlock/syscalls.c                 |   2 +-
 tools/testing/selftests/landlock/base_test.c |   2 +-
 tools/testing/selftests/landlock/fs_test.c   |   5 +-
 8 files changed, 128 insertions(+), 7 deletions(-)

index 60ff217ab95b..8d0edf94037d 100644

security/landlock/audit.c +1 -0

@@ -37,6 +37,7 @@ static const char *const fs_access_strings[] = {
 	[BIT_INDEX(LANDLOCK_ACCESS_FS_REFER)] = "fs.refer",
 	[BIT_INDEX(LANDLOCK_ACCESS_FS_TRUNCATE)] = "fs.truncate",
 	[BIT_INDEX(LANDLOCK_ACCESS_FS_IOCTL_DEV)] = "fs.ioctl_dev",
+	[BIT_INDEX(LANDLOCK_ACCESS_FS_RESOLVE_UNIX)] = "fs.resolve_unix",
 };
 
 static_assert(ARRAY_SIZE(fs_access_strings) == LANDLOCK_NUM_ACCESS_FS);

security/landlock/fs.c +3 -1

index e764470f588c..76035c6f2bf1 100644
@@ -27,6 +27,7 @@
 #include <linux/lsm_hooks.h>
 #include <linux/mount.h>
 #include <linux/namei.h>
+#include <linux/net.h>
 #include <linux/path.h>
 #include <linux/pid.h>
 #include <linux/rcupdate.h>

@@ -314,7 +315,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
 	LANDLOCK_ACCESS_FS_READ_FILE | \
 	LANDLOCK_ACCESS_FS_TRUNCATE | \
-	LANDLOCK_ACCESS_FS_IOCTL_DEV)
+	LANDLOCK_ACCESS_FS_IOCTL_DEV | \
+	LANDLOCK_ACCESS_FS_RESOLVE_UNIX)
 /* clang-format on */
 
 /*

@@ -1561,6 +1563,103 @@ static int hook_path_truncate(const struct path *const path)
 	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
 }
 

security/landlock/fs.c static int hook_path_truncate(const struct path *const path) · line 1571

Quoted 6 lines

security/landlock/fs.c +6 -0

+/**
+ * unmask_scoped_access - Remove access right bits in @masks in all layers
+ *                        where @client and @server have the same domain
+ *
+ * This does the same as domain_is_scoped(), but unmasks bits in @masks.
+ * It can not return early as domain_is_scoped() does.

Quoted 5 lines

security/landlock/fs.c +5 -0

+ *
+ * @client: Client domain
+ * @server: Server domain
+ * @masks: Layer access masks to unmask
+ * @access: Access bit that controls scoping

security/landlock/fs.c static int hook_path_truncate(const struct path *const path) · line 1581

Quoted 5 lines

security/landlock/fs.c +5 -0

+ */
+static void unmask_scoped_access(const struct landlock_ruleset *const client,
+				 const struct landlock_ruleset *const server,
+				 struct layer_access_masks *const masks,
+				 const access_mask_t access)

This helper should be moved to task.c and factored out with domain_is_scoped(). This should be a dedicated patch.

Quoted 37 lines

security/landlock/fs.c +36 -0

+{
+	int client_layer, server_layer;
+	const struct landlock_hierarchy *client_walker, *server_walker;
+
+	if (WARN_ON_ONCE(!client))
+		return; /* should not happen */
+
+	if (!server)
+		return; /* server has no Landlock domain; nothing to clear */
+
+	client_layer = client->num_layers - 1;
+	client_walker = client->hierarchy;
+	server_layer = server->num_layers - 1;
+	server_walker = server->hierarchy;
+
+	/*
+	 * Clears the access bits at all layers where the client domain is the
+	 * same as the server domain.  We start the walk at min(client_layer,
+	 * server_layer).  The layer bits until there can not be cleared because
+	 * either the client or the server domain is missing.
+	 */
+	for (; client_layer > server_layer; client_layer--)
+		client_walker = client_walker->parent;
+
+	for (; server_layer > client_layer; server_layer--)
+		server_walker = server_walker->parent;
+
+	for (; client_layer >= 0; client_layer--) {
+		if (masks->access[client_layer] & access &&
+		    client_walker == server_walker)
+			masks->access[client_layer] &= ~access;
+
+		client_walker = client_walker->parent;
+		server_walker = server_walker->parent;
+	}
+}

Quoted 79 lines · Mickaël Salaün

Quoted 78 lines · Günther Noack

* Add a new access right LANDLOCK_ACCESS_FS_RESOLVE_UNIX, which controls the look up operations for named UNIX domain sockets. The resolution happens during connect() and sendmsg() (depending on socket type).
* Hook into the path lookup in unix_find_bsd() in af_unix.c, using a LSM hook. Make policy decisions based on the new access rights
* Increment the Landlock ABI version.
* Minor test adaptions to keep the tests working.

With this access right, access is granted if either of the following conditions is met:

* The target socket's filesystem path was allow-listed using a LANDLOCK_RULE_PATH_BENEATH rule, *or*:
* The target socket was created in the same Landlock domain in which LANDLOCK_ACCESS_FS_RESOLVE_UNIX was restricted.

In case of a denial, connect() and sendmsg() return EACCES, which is the same error as it is returned if the user does not have the write bit in the traditional Unix file system permissions of that file.

This feature was created with substantial discussion and input from Justin Suess, Tingmao Wang and Mickaël Salaün.

Cc Tingmao Wang <[email protected]>

Cc Justin Suess <[email protected]>

Cc Mickaël Salaün <[email protected]>

Suggested-by Jann Horn <[email protected]>

Link https://github.com/landlock-lsm/linux/issues/36

Signed-off-by Günther Noack <[email protected]>

---
 include/uapi/linux/landlock.h                |  10 ++
 security/landlock/access.h                   |  11 +-
 security/landlock/audit.c                    |   1 +
 security/landlock/fs.c                       | 102 ++++++++++++++++++-
 security/landlock/limits.h                   |   2 +-
 security/landlock/syscalls.c                 |   2 +-
 tools/testing/selftests/landlock/base_test.c |   2 +-
 tools/testing/selftests/landlock/fs_test.c   |   5 +-
 8 files changed, 128 insertions(+), 7 deletions(-)

index 60ff217ab95b..8d0edf94037d 100644

security/landlock/audit.c +1 -0

@@ -37,6 +37,7 @@ static const char *const fs_access_strings[] = {
 	[BIT_INDEX(LANDLOCK_ACCESS_FS_REFER)] = "fs.refer",
 	[BIT_INDEX(LANDLOCK_ACCESS_FS_TRUNCATE)] = "fs.truncate",
 	[BIT_INDEX(LANDLOCK_ACCESS_FS_IOCTL_DEV)] = "fs.ioctl_dev",
+	[BIT_INDEX(LANDLOCK_ACCESS_FS_RESOLVE_UNIX)] = "fs.resolve_unix",
 };
 
 static_assert(ARRAY_SIZE(fs_access_strings) == LANDLOCK_NUM_ACCESS_FS);

security/landlock/fs.c +3 -1

index e764470f588c..76035c6f2bf1 100644
@@ -27,6 +27,7 @@
 #include <linux/lsm_hooks.h>
 #include <linux/mount.h>
 #include <linux/namei.h>
+#include <linux/net.h>
 #include <linux/path.h>
 #include <linux/pid.h>
 #include <linux/rcupdate.h>

@@ -314,7 +315,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
 	LANDLOCK_ACCESS_FS_READ_FILE | \
 	LANDLOCK_ACCESS_FS_TRUNCATE | \
-	LANDLOCK_ACCESS_FS_IOCTL_DEV)
+	LANDLOCK_ACCESS_FS_IOCTL_DEV | \
+	LANDLOCK_ACCESS_FS_RESOLVE_UNIX)
 /* clang-format on */
 
 /*

@@ -1561,6 +1563,103 @@ static int hook_path_truncate(const struct path *const path)
 	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
 }
 

Quoted 6 lines

security/landlock/fs.c static int hook_path_truncate(const struct path *const path) · line 1571

Quoted 6 lines

security/landlock/fs.c +6 -0

+/**
+ * unmask_scoped_access - Remove access right bits in @masks in all layers
+ *                        where @client and @server have the same domain
+ *
+ * This does the same as domain_is_scoped(), but unmasks bits in @masks.
+ * It can not return early as domain_is_scoped() does.

Why can't we use the same logic as for other scopes?

Quoted 5 lines

Quoted 5 lines

security/landlock/fs.c +5 -0

+ *
+ * @client: Client domain
+ * @server: Server domain
+ * @masks: Layer access masks to unmask
+ * @access: Access bit that controls scoping

Quoted 8 lines

security/landlock/fs.c static int hook_path_truncate(const struct path *const path) · line 1581

Quoted 5 lines

security/landlock/fs.c +5 -0

+ */
+static void unmask_scoped_access(const struct landlock_ruleset *const client,
+				 const struct landlock_ruleset *const server,
+				 struct layer_access_masks *const masks,
+				 const access_mask_t access)

This helper should be moved to task.c and factored out with domain_is_scoped(). This should be a dedicated patch.

Well, if domain_is_scoped() can be refactored and made generic, it would make more sense to move it to domain.c

Quoted 37 lines

Quoted 37 lines

security/landlock/fs.c +36 -0

+{
+	int client_layer, server_layer;
+	const struct landlock_hierarchy *client_walker, *server_walker;
+
+	if (WARN_ON_ONCE(!client))
+		return; /* should not happen */
+
+	if (!server)
+		return; /* server has no Landlock domain; nothing to clear */
+
+	client_layer = client->num_layers - 1;
+	client_walker = client->hierarchy;
+	server_layer = server->num_layers - 1;
+	server_walker = server->hierarchy;
+
+	/*
+	 * Clears the access bits at all layers where the client domain is the
+	 * same as the server domain.  We start the walk at min(client_layer,
+	 * server_layer).  The layer bits until there can not be cleared because
+	 * either the client or the server domain is missing.
+	 */
+	for (; client_layer > server_layer; client_layer--)
+		client_walker = client_walker->parent;
+
+	for (; server_layer > client_layer; server_layer--)
+		server_walker = server_walker->parent;
+
+	for (; client_layer >= 0; client_layer--) {
+		if (masks->access[client_layer] & access &&
+		    client_walker == server_walker)
+			masks->access[client_layer] &= ~access;
+
+		client_walker = client_walker->parent;
+		server_walker = server_walker->parent;
+	}
+}

Quoted 80 lines · Mickaël Salaün

Quoted 79 lines · Mickaël Salaün

Quoted 78 lines · Günther Noack

* Add a new access right LANDLOCK_ACCESS_FS_RESOLVE_UNIX, which controls the look up operations for named UNIX domain sockets. The resolution happens during connect() and sendmsg() (depending on socket type).
* Hook into the path lookup in unix_find_bsd() in af_unix.c, using a LSM hook. Make policy decisions based on the new access rights
* Increment the Landlock ABI version.
* Minor test adaptions to keep the tests working.

With this access right, access is granted if either of the following conditions is met:

* The target socket's filesystem path was allow-listed using a LANDLOCK_RULE_PATH_BENEATH rule, *or*:
* The target socket was created in the same Landlock domain in which LANDLOCK_ACCESS_FS_RESOLVE_UNIX was restricted.

In case of a denial, connect() and sendmsg() return EACCES, which is the same error as it is returned if the user does not have the write bit in the traditional Unix file system permissions of that file.

This feature was created with substantial discussion and input from Justin Suess, Tingmao Wang and Mickaël Salaün.

Cc Tingmao Wang <[email protected]>

Cc Justin Suess <[email protected]>

Cc Mickaël Salaün <[email protected]>

Suggested-by Jann Horn <[email protected]>

Link https://github.com/landlock-lsm/linux/issues/36

Signed-off-by Günther Noack <[email protected]>

---
 include/uapi/linux/landlock.h                |  10 ++
 security/landlock/access.h                   |  11 +-
 security/landlock/audit.c                    |   1 +
 security/landlock/fs.c                       | 102 ++++++++++++++++++-
 security/landlock/limits.h                   |   2 +-
 security/landlock/syscalls.c                 |   2 +-
 tools/testing/selftests/landlock/base_test.c |   2 +-
 tools/testing/selftests/landlock/fs_test.c   |   5 +-
 8 files changed, 128 insertions(+), 7 deletions(-)

index 60ff217ab95b..8d0edf94037d 100644

security/landlock/audit.c +1 -0

@@ -37,6 +37,7 @@ static const char *const fs_access_strings[] = {
 	[BIT_INDEX(LANDLOCK_ACCESS_FS_REFER)] = "fs.refer",
 	[BIT_INDEX(LANDLOCK_ACCESS_FS_TRUNCATE)] = "fs.truncate",
 	[BIT_INDEX(LANDLOCK_ACCESS_FS_IOCTL_DEV)] = "fs.ioctl_dev",
+	[BIT_INDEX(LANDLOCK_ACCESS_FS_RESOLVE_UNIX)] = "fs.resolve_unix",
 };
 
 static_assert(ARRAY_SIZE(fs_access_strings) == LANDLOCK_NUM_ACCESS_FS);

security/landlock/fs.c +3 -1

index e764470f588c..76035c6f2bf1 100644
@@ -27,6 +27,7 @@
 #include <linux/lsm_hooks.h>
 #include <linux/mount.h>
 #include <linux/namei.h>
+#include <linux/net.h>
 #include <linux/path.h>
 #include <linux/pid.h>
 #include <linux/rcupdate.h>

@@ -314,7 +315,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
 	LANDLOCK_ACCESS_FS_READ_FILE | \
 	LANDLOCK_ACCESS_FS_TRUNCATE | \
-	LANDLOCK_ACCESS_FS_IOCTL_DEV)
+	LANDLOCK_ACCESS_FS_IOCTL_DEV | \
+	LANDLOCK_ACCESS_FS_RESOLVE_UNIX)
 /* clang-format on */
 
 /*

@@ -1561,6 +1563,103 @@ static int hook_path_truncate(const struct path *const path)
 	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
 }
 

Quoted 8 lines

Quoted 6 lines

security/landlock/fs.c static int hook_path_truncate(const struct path *const path) · line 1571

Quoted 6 lines

security/landlock/fs.c +6 -0

+/**
+ * unmask_scoped_access - Remove access right bits in @masks in all layers
+ *                        where @client and @server have the same domain
+ *
+ * This does the same as domain_is_scoped(), but unmasks bits in @masks.
+ * It can not return early as domain_is_scoped() does.

Why can't we use the same logic as for other scopes?

The other scopes, for which this is implemented in domain_is_scoped(), do not need to do this layer-by-layer.

I have to admit, in my initial implementation, I was using domain_is_scoped() directly, and the logic at the end of the hook was roughly:

   --- BUGGY CODE START ---
       // ...
       
       if (!domain_is_scoped(..., ..., LANDLOCK_ACCESS_FS_RESOLVE_UNIX))
           return 0;  /* permitted */

       return current_check_access_path(path, LANDLOCK_ACCESS_FS_RESOLVE_UNIX)
   }
   --- BUGGY CODE END ---

Unfortunately, that is a logic error though -- it implements the formula

Access granted if:
(FOR-ALL l ∈ layers scoped-access-ok(l)) OR (FOR-ALL l ∈ layers path-access-ok(l)) (WRONG!)

but the formula we want is:

Access granted if:
FOR-ALL l ∈ layers (scoped-access-ok(l) OR path-access-ok(l)) (CORRECT!)

This makes a difference in the case where (pseudocode):

1. landlock_restrict_self(RESOLVE_UNIX) // d1
2. create_unix_server("./sock")
3. landlock_restrict_self(RESOLVE_UNIX, rule=Allow(".", RESOLVE_UNIX)) // d2
4. connect_unix("./sock")

,------------------------------------------------d1--,

   |                                                    |
   |    ./sock server                                   |
   |       ^                                            |
   |       |                                            |
   |  ,------------------------------------------d2--,  |
   |  |    |                                         |  |
   |  |  client                                      |  |
   |  |                                              |  |
   |  '----------------------------------------------'  |
   |                                                    |

'----------------------------------------------------'

(BTW, this scenario is covered in the selftests, that is why there is a variant of these selftests where instead of applying "no domain", we apply a domain with an exception rule like in step 3 in the pseudocode above. Applying that domain should behave the same as applying no domain at all.)

Intuitively, it is clear that the access should be granted:

- d1 does not restrict access to the server, because the socket was created within d1 itself.
- d2 does not restrict access to the server, because it has a rule to allow it

But the "buggy code" logic above comes to a different conclusion:

- the domain_is_scoped() check denies the access, because the server is in a more privileged domain relative to the client domain.
- the current_check_access_path() check denies the access as well, because the socket's path is not allow-listed in d1.

In the 'intuitive' reasoning above, we are checking d1 and d2 independently of each other. While Landlock is not implemented like that internally, we need to stay consistent with it so that domains compose correctly. The way to do that is to track is access check results on a per-layer basis again, and that is why unmask_scoped_access() uses a layer mask for tracking. The original domain_is_scoped() does not use a layer mask, but that also means that it can return early in some scenarios -- if for any of the relevant layer depths, the client and server domains are not the same, it exits early with failure because it's overall not fulfillable any more. In the RESOLVE_UNIX case though, we need to remember in which layers we failed (both high an low ones), because these layers can still be fulfilled with a PATH_BENEATH rule later.

Summary:

Option 1: We *can* unify this if you want. It just might come at a small performance penalty for domain_is_scoped(), which now uses the larger layer mask data structure and can't do the same early returns any more as before.

Option 2: Alternatively, if we move the two functions into the same module, we can keep them separate but still test them against each other to make sure they are in-line:

This invocation should return true...

domain_is_scoped(cli, srv, access)

...in the exactly the same situations where this invocation leaves any bits set in layer_masks:

  landlock_init_layer_masks(dom, access, &layer_masks, LL_KEY_INODE);
  unmask_scoped_access(cli, srv, &layer_masks, access);

What do you prefer?

Quoted 20 lines

Quoted 15 lines

security/landlock/fs.c static int hook_path_truncate(const struct path *const path) · line 1581

Quoted 12 lines

security/landlock/fs.c +10 -0

+ *
+ * @client: Client domain
+ * @server: Server domain
+ * @masks: Layer access masks to unmask
+ * @access: Access bit that controls scoping
+ */
+static void unmask_scoped_access(const struct landlock_ruleset *const client,
+				 const struct landlock_ruleset *const server,
+				 struct layer_access_masks *const masks,
+				 const access_mask_t access)

This helper should be moved to task.c and factored out with domain_is_scoped(). This should be a dedicated patch.

Well, if domain_is_scoped() can be refactored and made generic, it would make more sense to move it to domain.c

Quoted 1 line

Quoted 6 lines

Quoted 6 lines

security/landlock/fs.c static int hook_path_truncate(const struct path *const path) · line 1587

Quoted 6 lines

security/landlock/fs.c +6 -0

+{
+	int client_layer, server_layer;
+	const struct landlock_hierarchy *client_walker, *server_walker;
+
+	if (WARN_ON_ONCE(!client))
+		return; /* should not happen */

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

security/landlock/fs.c +16 -0

+
+	if (!server)
+		return; /* server has no Landlock domain; nothing to clear */
+
+	client_layer = client->num_layers - 1;
+	client_walker = client->hierarchy;
+	server_layer = server->num_layers - 1;
+	server_walker = server->hierarchy;
+
+	/*
+	 * Clears the access bits at all layers where the client domain is the
+	 * same as the server domain.  We start the walk at min(client_layer,
+	 * server_layer).  The layer bits until there can not be cleared because
+	 * either the client or the server domain is missing.
+	 */
+	for (; client_layer > server_layer; client_layer--)

Quoted 8 lines

Quoted 8 lines

security/landlock/fs.c static int hook_path_truncate(const struct path *const path) · line 1611

Quoted 8 lines

security/landlock/fs.c +8 -0

+		client_walker = client_walker->parent;
+
+	for (; server_layer > client_layer; server_layer--)
+		server_walker = server_walker->parent;
+
+	for (; client_layer >= 0; client_layer--) {
+		if (masks->access[client_layer] & access &&
+		    client_walker == server_walker)

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

security/landlock/fs.c +6 -0

+			masks->access[client_layer] &= ~access;
+
+		client_walker = client_walker->parent;
+		server_walker = server_walker->parent;
+	}
+}

Quoted 109 lines · Günther Noack

Quoted 80 lines · Mickaël Salaün

Quoted 79 lines · Mickaël Salaün

Quoted 78 lines · Günther Noack

* Add a new access right LANDLOCK_ACCESS_FS_RESOLVE_UNIX, which controls the look up operations for named UNIX domain sockets. The resolution happens during connect() and sendmsg() (depending on socket type).
* Hook into the path lookup in unix_find_bsd() in af_unix.c, using a LSM hook. Make policy decisions based on the new access rights
* Increment the Landlock ABI version.
* Minor test adaptions to keep the tests working.

With this access right, access is granted if either of the following conditions is met:

* The target socket's filesystem path was allow-listed using a LANDLOCK_RULE_PATH_BENEATH rule, *or*:
* The target socket was created in the same Landlock domain in which LANDLOCK_ACCESS_FS_RESOLVE_UNIX was restricted.

In case of a denial, connect() and sendmsg() return EACCES, which is the same error as it is returned if the user does not have the write bit in the traditional Unix file system permissions of that file.

This feature was created with substantial discussion and input from Justin Suess, Tingmao Wang and Mickaël Salaün.

Cc Tingmao Wang <[email protected]>

Cc Justin Suess <[email protected]>

Cc Mickaël Salaün <[email protected]>

Suggested-by Jann Horn <[email protected]>

Link https://github.com/landlock-lsm/linux/issues/36

Signed-off-by Günther Noack <[email protected]>

---
 include/uapi/linux/landlock.h                |  10 ++
 security/landlock/access.h                   |  11 +-
 security/landlock/audit.c                    |   1 +
 security/landlock/fs.c                       | 102 ++++++++++++++++++-
 security/landlock/limits.h                   |   2 +-
 security/landlock/syscalls.c                 |   2 +-
 tools/testing/selftests/landlock/base_test.c |   2 +-
 tools/testing/selftests/landlock/fs_test.c   |   5 +-
 8 files changed, 128 insertions(+), 7 deletions(-)

index 60ff217ab95b..8d0edf94037d 100644

security/landlock/audit.c +1 -0

@@ -37,6 +37,7 @@ static const char *const fs_access_strings[] = {
 	[BIT_INDEX(LANDLOCK_ACCESS_FS_REFER)] = "fs.refer",
 	[BIT_INDEX(LANDLOCK_ACCESS_FS_TRUNCATE)] = "fs.truncate",
 	[BIT_INDEX(LANDLOCK_ACCESS_FS_IOCTL_DEV)] = "fs.ioctl_dev",
+	[BIT_INDEX(LANDLOCK_ACCESS_FS_RESOLVE_UNIX)] = "fs.resolve_unix",
 };
 
 static_assert(ARRAY_SIZE(fs_access_strings) == LANDLOCK_NUM_ACCESS_FS);

security/landlock/fs.c +3 -1

index e764470f588c..76035c6f2bf1 100644
@@ -27,6 +27,7 @@
 #include <linux/lsm_hooks.h>
 #include <linux/mount.h>
 #include <linux/namei.h>
+#include <linux/net.h>
 #include <linux/path.h>
 #include <linux/pid.h>
 #include <linux/rcupdate.h>

@@ -314,7 +315,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
 	LANDLOCK_ACCESS_FS_READ_FILE | \
 	LANDLOCK_ACCESS_FS_TRUNCATE | \
-	LANDLOCK_ACCESS_FS_IOCTL_DEV)
+	LANDLOCK_ACCESS_FS_IOCTL_DEV | \
+	LANDLOCK_ACCESS_FS_RESOLVE_UNIX)
 /* clang-format on */
 
 /*

@@ -1561,6 +1563,103 @@ static int hook_path_truncate(const struct path *const path)
 	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
 }
 

Quoted 8 lines

Quoted 6 lines

security/landlock/fs.c static int hook_path_truncate(const struct path *const path) · line 1571

Quoted 6 lines

security/landlock/fs.c +6 -0

+/**
+ * unmask_scoped_access - Remove access right bits in @masks in all layers
+ *                        where @client and @server have the same domain
+ *
+ * This does the same as domain_is_scoped(), but unmasks bits in @masks.
+ * It can not return early as domain_is_scoped() does.

Why can't we use the same logic as for other scopes?

The other scopes, for which this is implemented in domain_is_scoped(), do not need to do this layer-by-layer.

I have to admit, in my initial implementation, I was using domain_is_scoped() directly, and the logic at the end of the hook was roughly:

   --- BUGGY CODE START ---
       // ...
       
       if (!domain_is_scoped(..., ..., LANDLOCK_ACCESS_FS_RESOLVE_UNIX))
           return 0;  /* permitted */

       return current_check_access_path(path, LANDLOCK_ACCESS_FS_RESOLVE_UNIX)
   }
   --- BUGGY CODE END ---

Unfortunately, that is a logic error though -- it implements the formula

Quoted 7 lines

Access granted if:
(FOR-ALL l ∈ layers scoped-access-ok(l)) OR (FOR-ALL l ∈ layers path-access-ok(l)) (WRONG!)

but the formula we want is:

Access granted if:
FOR-ALL l ∈ layers (scoped-access-ok(l) OR path-access-ok(l)) (CORRECT!)

It is worth it to add this explanation to the unmask_scoped_access() description, also pointing to the test that check this case.

Quoted 70 lines

This makes a difference in the case where (pseudocode):

1. landlock_restrict_self(RESOLVE_UNIX) // d1
2. create_unix_server("./sock")
3. landlock_restrict_self(RESOLVE_UNIX, rule=Allow(".", RESOLVE_UNIX)) // d2
4. connect_unix("./sock")

,------------------------------------------------d1--,

   |                                                    |
   |    ./sock server                                   |
   |       ^                                            |
   |       |                                            |
   |  ,------------------------------------------d2--,  |
   |  |    |                                         |  |
   |  |  client                                      |  |
   |  |                                              |  |
   |  '----------------------------------------------'  |
   |                                                    |

'----------------------------------------------------'

(BTW, this scenario is covered in the selftests, that is why there is a variant of these selftests where instead of applying "no domain", we apply a domain with an exception rule like in step 3 in the pseudocode above. Applying that domain should behave the same as applying no domain at all.)

Intuitively, it is clear that the access should be granted:

- d1 does not restrict access to the server, because the socket was created within d1 itself.
- d2 does not restrict access to the server, because it has a rule to allow it

But the "buggy code" logic above comes to a different conclusion:

- the domain_is_scoped() check denies the access, because the server is in a more privileged domain relative to the client domain.
- the current_check_access_path() check denies the access as well, because the socket's path is not allow-listed in d1.

In the 'intuitive' reasoning above, we are checking d1 and d2 independently of each other. While Landlock is not implemented like that internally, we need to stay consistent with it so that domains compose correctly. The way to do that is to track is access check results on a per-layer basis again, and that is why unmask_scoped_access() uses a layer mask for tracking. The original domain_is_scoped() does not use a layer mask, but that also means that it can return early in some scenarios -- if for any of the relevant layer depths, the client and server domains are not the same, it exits early with failure because it's overall not fulfillable any more. In the RESOLVE_UNIX case though, we need to remember in which layers we failed (both high an low ones), because these layers can still be fulfilled with a PATH_BENEATH rule later.

Summary:

Option 1: We *can* unify this if you want. It just might come at a small performance penalty for domain_is_scoped(), which now uses the larger layer mask data structure and can't do the same early returns any more as before.

Option 2: Alternatively, if we move the two functions into the same module, we can keep them separate but still test them against each other to make sure they are in-line:

This invocation should return true...

domain_is_scoped(cli, srv, access)

Quoted 7 lines

...in the exactly the same situations where this invocation leaves any bits set in layer_masks:

  landlock_init_layer_masks(dom, access, &layer_masks, LL_KEY_INODE);
  unmask_scoped_access(cli, srv, &layer_masks, access);

What do you prefer?

I was thinking about factoring out domain_is_scoped() with unmask_scoped_access() but, after some tests, it is not worth it. Your approach is simple and good.

Quoted 20 lines

Quoted 20 lines

Quoted 15 lines

security/landlock/fs.c static int hook_path_truncate(const struct path *const path) · line 1581

Quoted 12 lines

security/landlock/fs.c +10 -0

+ *
+ * @client: Client domain
+ * @server: Server domain
+ * @masks: Layer access masks to unmask
+ * @access: Access bit that controls scoping
+ */
+static void unmask_scoped_access(const struct landlock_ruleset *const client,
+				 const struct landlock_ruleset *const server,
+				 struct layer_access_masks *const masks,
+				 const access_mask_t access)

This helper should be moved to task.c and factored out with domain_is_scoped(). This should be a dedicated patch.

Well, if domain_is_scoped() can be refactored and made generic, it would make more sense to move it to domain.c

Quoted 1 line

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

security/landlock/fs.c static int hook_path_truncate(const struct path *const path) · line 1587

Quoted 6 lines

security/landlock/fs.c +6 -0

+{
+	int client_layer, server_layer;
+	const struct landlock_hierarchy *client_walker, *server_walker;
+
+	if (WARN_ON_ONCE(!client))
+		return; /* should not happen */

Please no comment after ";"

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

security/landlock/fs.c +16 -0

+
+	if (!server)
+		return; /* server has no Landlock domain; nothing to clear */
+
+	client_layer = client->num_layers - 1;
+	client_walker = client->hierarchy;
+	server_layer = server->num_layers - 1;
+	server_walker = server->hierarchy;
+
+	/*
+	 * Clears the access bits at all layers where the client domain is the
+	 * same as the server domain.  We start the walk at min(client_layer,
+	 * server_layer).  The layer bits until there can not be cleared because
+	 * either the client or the server domain is missing.
+	 */
+	for (; client_layer > server_layer; client_layer--)

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

security/landlock/fs.c static int hook_path_truncate(const struct path *const path) · line 1611

Quoted 8 lines

security/landlock/fs.c +8 -0

+		client_walker = client_walker->parent;
+
+	for (; server_layer > client_layer; server_layer--)
+		server_walker = server_walker->parent;
+
+	for (; client_layer >= 0; client_layer--) {
+		if (masks->access[client_layer] & access &&
+		    client_walker == server_walker)

I'd prefer to first check client_walker == server_walker and then the access. My main concern is that only one bit of access matching masks->access[client_layer] clear all the access request bits. In practice there is only one, for now, but this code should be more strict by following a defensive approach.

Quoted 1 line

Quoted 1 line

Quoted 1 line

security/landlock/fs.c static int hook_path_truncate(const struct path *const path) · line 1612

Quoted 1 line

security/landlock/fs.c +1 -0

+			masks->access[client_layer] &= ~access;

Quoted 6 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

security/landlock/fs.c +5 -0

+
+		client_walker = client_walker->parent;
+		server_walker = server_walker->parent;
+	}
+}

Quoted 254 lines · Mickaël Salaün

Quoted 109 lines · Günther Noack

Quoted 80 lines · Mickaël Salaün

Quoted 79 lines · Mickaël Salaün

Quoted 78 lines · Günther Noack

* Add a new access right LANDLOCK_ACCESS_FS_RESOLVE_UNIX, which controls the look up operations for named UNIX domain sockets. The resolution happens during connect() and sendmsg() (depending on socket type).
* Hook into the path lookup in unix_find_bsd() in af_unix.c, using a LSM hook. Make policy decisions based on the new access rights
* Increment the Landlock ABI version.
* Minor test adaptions to keep the tests working.

With this access right, access is granted if either of the following conditions is met:

* The target socket's filesystem path was allow-listed using a LANDLOCK_RULE_PATH_BENEATH rule, *or*:
* The target socket was created in the same Landlock domain in which LANDLOCK_ACCESS_FS_RESOLVE_UNIX was restricted.

In case of a denial, connect() and sendmsg() return EACCES, which is the same error as it is returned if the user does not have the write bit in the traditional Unix file system permissions of that file.

This feature was created with substantial discussion and input from Justin Suess, Tingmao Wang and Mickaël Salaün.

Cc Tingmao Wang <[email protected]>

Cc Justin Suess <[email protected]>

Cc Mickaël Salaün <[email protected]>

Suggested-by Jann Horn <[email protected]>

Link https://github.com/landlock-lsm/linux/issues/36

Signed-off-by Günther Noack <[email protected]>

---
 include/uapi/linux/landlock.h                |  10 ++
 security/landlock/access.h                   |  11 +-
 security/landlock/audit.c                    |   1 +
 security/landlock/fs.c                       | 102 ++++++++++++++++++-
 security/landlock/limits.h                   |   2 +-
 security/landlock/syscalls.c                 |   2 +-
 tools/testing/selftests/landlock/base_test.c |   2 +-
 tools/testing/selftests/landlock/fs_test.c   |   5 +-
 8 files changed, 128 insertions(+), 7 deletions(-)

index 60ff217ab95b..8d0edf94037d 100644

security/landlock/audit.c +1 -0

@@ -37,6 +37,7 @@ static const char *const fs_access_strings[] = {
 	[BIT_INDEX(LANDLOCK_ACCESS_FS_REFER)] = "fs.refer",
 	[BIT_INDEX(LANDLOCK_ACCESS_FS_TRUNCATE)] = "fs.truncate",
 	[BIT_INDEX(LANDLOCK_ACCESS_FS_IOCTL_DEV)] = "fs.ioctl_dev",
+	[BIT_INDEX(LANDLOCK_ACCESS_FS_RESOLVE_UNIX)] = "fs.resolve_unix",
 };
 
 static_assert(ARRAY_SIZE(fs_access_strings) == LANDLOCK_NUM_ACCESS_FS);

security/landlock/fs.c +3 -1

index e764470f588c..76035c6f2bf1 100644
@@ -27,6 +27,7 @@
 #include <linux/lsm_hooks.h>
 #include <linux/mount.h>
 #include <linux/namei.h>
+#include <linux/net.h>
 #include <linux/path.h>
 #include <linux/pid.h>
 #include <linux/rcupdate.h>

@@ -314,7 +315,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
 	LANDLOCK_ACCESS_FS_READ_FILE | \
 	LANDLOCK_ACCESS_FS_TRUNCATE | \
-	LANDLOCK_ACCESS_FS_IOCTL_DEV)
+	LANDLOCK_ACCESS_FS_IOCTL_DEV | \
+	LANDLOCK_ACCESS_FS_RESOLVE_UNIX)
 /* clang-format on */
 
 /*

@@ -1561,6 +1563,103 @@ static int hook_path_truncate(const struct path *const path)
 	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
 }
 

Quoted 8 lines

Quoted 6 lines

security/landlock/fs.c static int hook_path_truncate(const struct path *const path) · line 1571

Quoted 6 lines

security/landlock/fs.c +6 -0

+/**
+ * unmask_scoped_access - Remove access right bits in @masks in all layers
+ *                        where @client and @server have the same domain
+ *
+ * This does the same as domain_is_scoped(), but unmasks bits in @masks.
+ * It can not return early as domain_is_scoped() does.

Why can't we use the same logic as for other scopes?

The other scopes, for which this is implemented in domain_is_scoped(), do not need to do this layer-by-layer.

I have to admit, in my initial implementation, I was using domain_is_scoped() directly, and the logic at the end of the hook was roughly:

   --- BUGGY CODE START ---
       // ...
       
       if (!domain_is_scoped(..., ..., LANDLOCK_ACCESS_FS_RESOLVE_UNIX))
           return 0;  /* permitted */

       return current_check_access_path(path, LANDLOCK_ACCESS_FS_RESOLVE_UNIX)
   }
   --- BUGGY CODE END ---

Unfortunately, that is a logic error though -- it implements the formula

Quoted 7 lines

Access granted if:
(FOR-ALL l ∈ layers scoped-access-ok(l)) OR (FOR-ALL l ∈ layers path-access-ok(l)) (WRONG!)

but the formula we want is:

Access granted if:
FOR-ALL l ∈ layers (scoped-access-ok(l) OR path-access-ok(l)) (CORRECT!)

It is worth it to add this explanation to the unmask_scoped_access() description, also pointing to the test that check this case.

Quoted 70 lines

This makes a difference in the case where (pseudocode):

1. landlock_restrict_self(RESOLVE_UNIX) // d1
2. create_unix_server("./sock")
3. landlock_restrict_self(RESOLVE_UNIX, rule=Allow(".", RESOLVE_UNIX)) // d2
4. connect_unix("./sock")

,------------------------------------------------d1--,

   |                                                    |
   |    ./sock server                                   |
   |       ^                                            |
   |       |                                            |
   |  ,------------------------------------------d2--,  |
   |  |    |                                         |  |
   |  |  client                                      |  |
   |  |                                              |  |
   |  '----------------------------------------------'  |
   |                                                    |

'----------------------------------------------------'

(BTW, this scenario is covered in the selftests, that is why there is a variant of these selftests where instead of applying "no domain", we apply a domain with an exception rule like in step 3 in the pseudocode above. Applying that domain should behave the same as applying no domain at all.)

Intuitively, it is clear that the access should be granted:

- d1 does not restrict access to the server, because the socket was created within d1 itself.
- d2 does not restrict access to the server, because it has a rule to allow it

But the "buggy code" logic above comes to a different conclusion:

- the domain_is_scoped() check denies the access, because the server is in a more privileged domain relative to the client domain.
- the current_check_access_path() check denies the access as well, because the socket's path is not allow-listed in d1.

In the 'intuitive' reasoning above, we are checking d1 and d2 independently of each other. While Landlock is not implemented like that internally, we need to stay consistent with it so that domains compose correctly. The way to do that is to track is access check results on a per-layer basis again, and that is why unmask_scoped_access() uses a layer mask for tracking. The original domain_is_scoped() does not use a layer mask, but that also means that it can return early in some scenarios -- if for any of the relevant layer depths, the client and server domains are not the same, it exits early with failure because it's overall not fulfillable any more. In the RESOLVE_UNIX case though, we need to remember in which layers we failed (both high an low ones), because these layers can still be fulfilled with a PATH_BENEATH rule later.

Summary:

Option 1: We *can* unify this if you want. It just might come at a small performance penalty for domain_is_scoped(), which now uses the larger layer mask data structure and can't do the same early returns any more as before.

Option 2: Alternatively, if we move the two functions into the same module, we can keep them separate but still test them against each other to make sure they are in-line:

This invocation should return true...

domain_is_scoped(cli, srv, access)

Quoted 7 lines

...in the exactly the same situations where this invocation leaves any bits set in layer_masks:

  landlock_init_layer_masks(dom, access, &layer_masks, LL_KEY_INODE);
  unmask_scoped_access(cli, srv, &layer_masks, access);

What do you prefer?

I was thinking about factoring out domain_is_scoped() with unmask_scoped_access() but, after some tests, it is not worth it. Your approach is simple and good.

Quoted 20 lines

Quoted 20 lines

Quoted 15 lines

security/landlock/fs.c static int hook_path_truncate(const struct path *const path) · line 1581

Quoted 12 lines

security/landlock/fs.c +10 -0

+ *
+ * @client: Client domain
+ * @server: Server domain
+ * @masks: Layer access masks to unmask
+ * @access: Access bit that controls scoping
+ */
+static void unmask_scoped_access(const struct landlock_ruleset *const client,
+				 const struct landlock_ruleset *const server,
+				 struct layer_access_masks *const masks,
+				 const access_mask_t access)

This helper should be moved to task.c and factored out with domain_is_scoped(). This should be a dedicated patch.

Well, if domain_is_scoped() can be refactored and made generic, it would make more sense to move it to domain.c

Quoted 1 line

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

security/landlock/fs.c static int hook_path_truncate(const struct path *const path) · line 1587

Quoted 6 lines

security/landlock/fs.c +6 -0

+{
+	int client_layer, server_layer;
+	const struct landlock_hierarchy *client_walker, *server_walker;
+
+	if (WARN_ON_ONCE(!client))
+		return; /* should not happen */

Please no comment after ";"

Quoted 22 lines

Quoted 22 lines

Quoted 22 lines

Quoted 22 lines

security/landlock/fs.c +22 -0

+
+	if (!server)
+		return; /* server has no Landlock domain; nothing to clear */
+
+	client_layer = client->num_layers - 1;
+	client_walker = client->hierarchy;
+	server_layer = server->num_layers - 1;
+	server_walker = server->hierarchy;
+
+	/*
+	 * Clears the access bits at all layers where the client domain is the
+	 * same as the server domain.  We start the walk at min(client_layer,
+	 * server_layer).  The layer bits until there can not be cleared because
+	 * either the client or the server domain is missing.
+	 */
+	for (; client_layer > server_layer; client_layer--)
+		client_walker = client_walker->parent;
+
+	for (; server_layer > client_layer; server_layer--)
+		server_walker = server_walker->parent;
+
+	for (; client_layer >= 0; client_layer--) {

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

security/landlock/fs.c static int hook_path_truncate(const struct path *const path) · line 1611

Quoted 2 lines

security/landlock/fs.c +2 -0

+		if (masks->access[client_layer] & access &&
+		    client_walker == server_walker)

I'd prefer to first check client_walker == server_walker and then the access. My main concern is that only one bit of access matching masks->access[client_layer] clear all the access request bits. In practice there is only one, for now, but this code should be more strict by following a defensive approach.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

security/landlock/fs.c static int hook_path_truncate(const struct path *const path) · line 1612

Quoted 1 line

security/landlock/fs.c +1 -0

+			masks->access[client_layer] &= ~access;

Actually, why not removing the access argument and just reset masks->access[client_layer]? The doc would need some updates.

Quoted 255 lines · Mickaël Salaün

Quoted 254 lines · Mickaël Salaün

Quoted 109 lines · Günther Noack

Quoted 80 lines · Mickaël Salaün

Quoted 79 lines · Mickaël Salaün

Quoted 78 lines · Günther Noack

* Add a new access right LANDLOCK_ACCESS_FS_RESOLVE_UNIX, which controls the look up operations for named UNIX domain sockets. The resolution happens during connect() and sendmsg() (depending on socket type).
* Hook into the path lookup in unix_find_bsd() in af_unix.c, using a LSM hook. Make policy decisions based on the new access rights
* Increment the Landlock ABI version.
* Minor test adaptions to keep the tests working.

With this access right, access is granted if either of the following conditions is met:

* The target socket's filesystem path was allow-listed using a LANDLOCK_RULE_PATH_BENEATH rule, *or*:
* The target socket was created in the same Landlock domain in which LANDLOCK_ACCESS_FS_RESOLVE_UNIX was restricted.

In case of a denial, connect() and sendmsg() return EACCES, which is the same error as it is returned if the user does not have the write bit in the traditional Unix file system permissions of that file.

This feature was created with substantial discussion and input from Justin Suess, Tingmao Wang and Mickaël Salaün.

Cc Tingmao Wang <[email protected]>

Cc Justin Suess <[email protected]>

Cc Mickaël Salaün <[email protected]>

Suggested-by Jann Horn <[email protected]>

Link https://github.com/landlock-lsm/linux/issues/36

Signed-off-by Günther Noack <[email protected]>

---
 include/uapi/linux/landlock.h                |  10 ++
 security/landlock/access.h                   |  11 +-
 security/landlock/audit.c                    |   1 +
 security/landlock/fs.c                       | 102 ++++++++++++++++++-
 security/landlock/limits.h                   |   2 +-
 security/landlock/syscalls.c                 |   2 +-
 tools/testing/selftests/landlock/base_test.c |   2 +-
 tools/testing/selftests/landlock/fs_test.c   |   5 +-
 8 files changed, 128 insertions(+), 7 deletions(-)

index 60ff217ab95b..8d0edf94037d 100644

security/landlock/audit.c +1 -0

@@ -37,6 +37,7 @@ static const char *const fs_access_strings[] = {
 	[BIT_INDEX(LANDLOCK_ACCESS_FS_REFER)] = "fs.refer",
 	[BIT_INDEX(LANDLOCK_ACCESS_FS_TRUNCATE)] = "fs.truncate",
 	[BIT_INDEX(LANDLOCK_ACCESS_FS_IOCTL_DEV)] = "fs.ioctl_dev",
+	[BIT_INDEX(LANDLOCK_ACCESS_FS_RESOLVE_UNIX)] = "fs.resolve_unix",
 };
 
 static_assert(ARRAY_SIZE(fs_access_strings) == LANDLOCK_NUM_ACCESS_FS);

security/landlock/fs.c +3 -1

index e764470f588c..76035c6f2bf1 100644
@@ -27,6 +27,7 @@
 #include <linux/lsm_hooks.h>
 #include <linux/mount.h>
 #include <linux/namei.h>
+#include <linux/net.h>
 #include <linux/path.h>
 #include <linux/pid.h>
 #include <linux/rcupdate.h>

@@ -314,7 +315,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
 	LANDLOCK_ACCESS_FS_READ_FILE | \
 	LANDLOCK_ACCESS_FS_TRUNCATE | \
-	LANDLOCK_ACCESS_FS_IOCTL_DEV)
+	LANDLOCK_ACCESS_FS_IOCTL_DEV | \
+	LANDLOCK_ACCESS_FS_RESOLVE_UNIX)
 /* clang-format on */
 
 /*

@@ -1561,6 +1563,103 @@ static int hook_path_truncate(const struct path *const path)
 	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
 }
 

Quoted 8 lines

Quoted 6 lines

security/landlock/fs.c static int hook_path_truncate(const struct path *const path) · line 1571

Quoted 6 lines

security/landlock/fs.c +6 -0

+/**
+ * unmask_scoped_access - Remove access right bits in @masks in all layers
+ *                        where @client and @server have the same domain
+ *
+ * This does the same as domain_is_scoped(), but unmasks bits in @masks.
+ * It can not return early as domain_is_scoped() does.

Why can't we use the same logic as for other scopes?

The other scopes, for which this is implemented in domain_is_scoped(), do not need to do this layer-by-layer.

I have to admit, in my initial implementation, I was using domain_is_scoped() directly, and the logic at the end of the hook was roughly:

   --- BUGGY CODE START ---
       // ...
       
       if (!domain_is_scoped(..., ..., LANDLOCK_ACCESS_FS_RESOLVE_UNIX))
           return 0;  /* permitted */

       return current_check_access_path(path, LANDLOCK_ACCESS_FS_RESOLVE_UNIX)
   }
   --- BUGGY CODE END ---

Unfortunately, that is a logic error though -- it implements the formula

Quoted 7 lines

Access granted if:
(FOR-ALL l ∈ layers scoped-access-ok(l)) OR (FOR-ALL l ∈ layers path-access-ok(l)) (WRONG!)

but the formula we want is:

Access granted if:
FOR-ALL l ∈ layers (scoped-access-ok(l) OR path-access-ok(l)) (CORRECT!)

It is worth it to add this explanation to the unmask_scoped_access() description, also pointing to the test that check this case.

Quoted 70 lines

This makes a difference in the case where (pseudocode):

1. landlock_restrict_self(RESOLVE_UNIX) // d1
2. create_unix_server("./sock")
3. landlock_restrict_self(RESOLVE_UNIX, rule=Allow(".", RESOLVE_UNIX)) // d2
4. connect_unix("./sock")

,------------------------------------------------d1--,

   |                                                    |
   |    ./sock server                                   |
   |       ^                                            |
   |       |                                            |
   |  ,------------------------------------------d2--,  |
   |  |    |                                         |  |
   |  |  client                                      |  |
   |  |                                              |  |
   |  '----------------------------------------------'  |
   |                                                    |

'----------------------------------------------------'

(BTW, this scenario is covered in the selftests, that is why there is a variant of these selftests where instead of applying "no domain", we apply a domain with an exception rule like in step 3 in the pseudocode above. Applying that domain should behave the same as applying no domain at all.)

Intuitively, it is clear that the access should be granted:

- d1 does not restrict access to the server, because the socket was created within d1 itself.
- d2 does not restrict access to the server, because it has a rule to allow it

But the "buggy code" logic above comes to a different conclusion:

- the domain_is_scoped() check denies the access, because the server is in a more privileged domain relative to the client domain.
- the current_check_access_path() check denies the access as well, because the socket's path is not allow-listed in d1.

In the 'intuitive' reasoning above, we are checking d1 and d2 independently of each other. While Landlock is not implemented like that internally, we need to stay consistent with it so that domains compose correctly. The way to do that is to track is access check results on a per-layer basis again, and that is why unmask_scoped_access() uses a layer mask for tracking. The original domain_is_scoped() does not use a layer mask, but that also means that it can return early in some scenarios -- if for any of the relevant layer depths, the client and server domains are not the same, it exits early with failure because it's overall not fulfillable any more. In the RESOLVE_UNIX case though, we need to remember in which layers we failed (both high an low ones), because these layers can still be fulfilled with a PATH_BENEATH rule later.

Summary:

Option 1: We *can* unify this if you want. It just might come at a small performance penalty for domain_is_scoped(), which now uses the larger layer mask data structure and can't do the same early returns any more as before.

Option 2: Alternatively, if we move the two functions into the same module, we can keep them separate but still test them against each other to make sure they are in-line:

This invocation should return true...

domain_is_scoped(cli, srv, access)

Quoted 7 lines

...in the exactly the same situations where this invocation leaves any bits set in layer_masks:

  landlock_init_layer_masks(dom, access, &layer_masks, LL_KEY_INODE);
  unmask_scoped_access(cli, srv, &layer_masks, access);

What do you prefer?

I was thinking about factoring out domain_is_scoped() with unmask_scoped_access() but, after some tests, it is not worth it. Your approach is simple and good.

Quoted 20 lines

Quoted 20 lines

Quoted 15 lines

security/landlock/fs.c static int hook_path_truncate(const struct path *const path) · line 1581

Quoted 12 lines

security/landlock/fs.c +10 -0

+ *
+ * @client: Client domain
+ * @server: Server domain
+ * @masks: Layer access masks to unmask
+ * @access: Access bit that controls scoping
+ */
+static void unmask_scoped_access(const struct landlock_ruleset *const client,
+				 const struct landlock_ruleset *const server,
+				 struct layer_access_masks *const masks,
+				 const access_mask_t access)

This helper should be moved to task.c and factored out with domain_is_scoped(). This should be a dedicated patch.

Well, if domain_is_scoped() can be refactored and made generic, it would make more sense to move it to domain.c

Quoted 1 line

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

security/landlock/fs.c static int hook_path_truncate(const struct path *const path) · line 1587

Quoted 6 lines

security/landlock/fs.c +6 -0

+{
+	int client_layer, server_layer;
+	const struct landlock_hierarchy *client_walker, *server_walker;
+
+	if (WARN_ON_ONCE(!client))
+		return; /* should not happen */

Please no comment after ";"

Quoted 22 lines

Quoted 22 lines

Quoted 22 lines

Quoted 22 lines

security/landlock/fs.c +22 -0

+
+	if (!server)
+		return; /* server has no Landlock domain; nothing to clear */
+
+	client_layer = client->num_layers - 1;
+	client_walker = client->hierarchy;
+	server_layer = server->num_layers - 1;
+	server_walker = server->hierarchy;
+
+	/*
+	 * Clears the access bits at all layers where the client domain is the
+	 * same as the server domain.  We start the walk at min(client_layer,
+	 * server_layer).  The layer bits until there can not be cleared because
+	 * either the client or the server domain is missing.
+	 */
+	for (; client_layer > server_layer; client_layer--)
+		client_walker = client_walker->parent;
+
+	for (; server_layer > client_layer; server_layer--)
+		server_walker = server_walker->parent;
+
+	for (; client_layer >= 0; client_layer--) {

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

security/landlock/fs.c static int hook_path_truncate(const struct path *const path) · line 1611

Quoted 2 lines

security/landlock/fs.c +2 -0

+		if (masks->access[client_layer] & access &&
+		    client_walker == server_walker)

I'd prefer to first check client_walker == server_walker and then the access. My main concern is that only one bit of access matching masks->access[client_layer] clear all the access request bits. In practice there is only one, for now, but this code should be more strict by following a defensive approach.

This function works even if multiple access request bits with "scope-like" semantics were being checked in parallel; if you consider the logic in:

  if (masks->access[client_layer] & access &&
      client_walker == server_walker)
          masks->access[client_layer] &= ~access;

you'll realize that the check for "masks->access[client_layer] & access" is technically irrelevant - if that check fails, all the

affected bits are already zero, so clearing them is a no-op. This code is equivalent, but might perform slightly more writes (although it likely does not make a performance difference in practice):

  if (client_walker == server_walker)
          masks->access[client_layer] &= ~access;

With that code it's a bit easier to see that "access" is actually only used to decide which bits to clear. This works both with one and with multiple access rights.

This follows the same logic as outlined in the comment above in the code, where it says:

Clears the access bits at all layers where the client domain is the same as the server domain. We start the walk at min(client_layer, server_layer). The layer bits until there can not be cleared because either the client or the server domain is missing.

Clearing bits that aren't there is a no-op

<Optional Math>

I found it helpful to visualize the scoping logic, this is directly from my notes: (Web version is at https://wiki.gnoack.org/LandlockDomainIsScoped)

The domain_is_scoped() helper implements the following predicate:

∀ l ∈ (0,16): (hasbit(self, l) implies-that domain(self, l) == domain(other, l))

That is, we require for each layer l nesting depth that:

* **If** scoping is active at the layer,
* **Then** the domains of self and other are the same at the given nesting depth.

For example:

       [ ]
        |
       [x]     self and other have the same domain at this depth
        |
       [ ]
      /   \
    [x]   [ ]  self and other have differing domains at this depth
     |     |
    [ ]   [ ]
     |
    [ ]     "other"             "x" marks a domain where "self" has
                                    set the scoping bit

"self"

</Optional Math>

Quoted 4 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

security/landlock/fs.c static int hook_path_truncate(const struct path *const path) · line 1612

Quoted 1 line

security/landlock/fs.c +1 -0

+			masks->access[client_layer] &= ~access;

Actually, why not removing the access argument and just reset masks->access[client_layer]? The doc would need some updates.

It would feel brittle to me if this function were to clear out unrelated access rights. It receives a struct layer_access_masks after all, where it is normally expected that multiple kinds of access rights are set. In my understanding, the bit masking does not cost much extra performance compared to clearing it out entirely, so I'd prefer to have clearer semantics and only operate on the access rights that it's about, even when the other bits are all zero at the moment.

(For full disclosure, I have contemplated for a bit whether hook_unix_find() should take a layer_mask_t-like type where each bit indicates whether a given access right (LANDLOCK_ACCESS_FS_RESOLVE_UNIX, in this case) is set at a given layer, and then it would only clear out the bits there. That would be in some ways simpler, but then the caller would still need to convert back and forth to a layer mask anyway, because that's what the other functions there take. So it didn't seem like a good option in the bigger scheme (and I would also prefer to not re-introduce layer_mask_t after we just removed it).)

Maybe I did not understand your remark fully though; Does my argument sound reasonable?

Quoted 7 lines · Günther Noack

include/uapi/linux/landlock.h +0 -0

index f88fa1f68b77..3a8fc3af0d64 100644
@@ -248,6 +248,15 @@ struct landlock_net_port_attr {
  *
  *   This access right is available since the fifth version of the Landlock

include/uapi/linux/landlock.h struct landlock_net_port_attr { · line 259

Quoted 10 lines

include/uapi/linux/landlock.h +9 -0

  *   ABI.
+ * - %LANDLOCK_ACCESS_FS_RESOLVE_UNIX: Look up pathname UNIX domain sockets
+ *   (:manpage:`unix(7)`).  On UNIX domain sockets, this restricts both calls to
+ *   :manpage:`connect(2)` as well as calls to :manpage:`sendmsg(2)` with an
+ *   explicit recipient address.
+ *
+ *   This access right only applies to connections to UNIX server sockets which
+ *   were created outside of the newly created Landlock domain (e.g. from within
+ *   a parent domain or from an unrestricted process).  Newly created UNIX
+ *   servers within the same Landlock domain continue to be accessible.

It might help to add a reference to the explicit scope mechanism.

Please squash patch 9/9 into this one and also add a reference here to the rationale described in security/landlock.rst

include/uapi/linux/landlock.h struct landlock_net_port_attr {

Quoted 4 lines

include/uapi/linux/landlock.h +4 -0

+static void unmask_scoped_access(const struct landlock_ruleset *const client,
+				 const struct landlock_ruleset *const server,
+				 struct layer_access_masks *const masks,
+				 const access_mask_t access)

This helper should be moved to task.c and factored out with domain_is_scoped(). This should be a dedicated patch.

Quoted 18 lines

include/uapi/linux/landlock.h +18 -0

+static int hook_unix_find(const struct path *const path, struct sock *other,
+			  int flags)
+{
+	const struct landlock_ruleset *dom_other;
+	const struct landlock_cred_security *subject;
+	struct layer_access_masks layer_masks;
+	struct landlock_request request = {};
+	static const struct access_masks fs_resolve_unix = {
+		.fs = LANDLOCK_ACCESS_FS_RESOLVE_UNIX,
+	};
+
+	/* Lookup for the purpose of saving coredumps is OK. */
+	if (unlikely(flags & SOCK_COREDUMP))
+		return 0;
+
+	/* Access to the same (or a lower) domain is always allowed. */
+	subject = landlock_get_applicable_subject(current_cred(),
+						  fs_resolve_unix, NULL);

include/uapi/linux/landlock.h

Quoted 10 lines

include/uapi/linux/landlock.h +10 -0

+
+	if (!subject)
+		return 0;
+
+	if (!landlock_init_layer_masks(subject->domain, fs_resolve_unix.fs,
+				       &layer_masks, LANDLOCK_KEY_INODE))
+		return 0;
+
+	/* Checks the layers in which we are connecting within the same domain. */
+	dom_other = landlock_cred(other->sk_socket->file->f_cred)->domain;

We need to call unix_state_lock(other) before reading it, and check for SOCK_DEAD, and check sk_socket before dereferencing it. Indeed, the socket can be make orphan (see unix_dgram_sendmsg and unix_stream_connect). I *think* a socket cannot be "resurrected" or recycled once dead, so we may assume there is no race condition wrt dom_other, but please double check. This lockless call should be made clear in the LSM hook. It's OK to not lock the socket before security_unix_find() (1) because no LSM might implement and (2) they might not need to lock the socket (e.g. if the caller is not sandboxed).

The updated code should look something like this:

unix_state_unlock(other);

if (unlikely(sock_flag(other, SOCK_DEAD) || !other->sk_socket)) {
	unix_state_unlock(other);
	return 0;
}

dom_other = landlock_cred(other->sk_socket->file->f_cred)->domain; unix_state_unlock(other);

+netdev, we could use some advice on the locking approach in af_unix (see below)

Quoted 21 lines · Mickaël Salaün

Quoted 7 lines · Günther Noack

include/uapi/linux/landlock.h +0 -0

index f88fa1f68b77..3a8fc3af0d64 100644
@@ -248,6 +248,15 @@ struct landlock_net_port_attr {
  *
  *   This access right is available since the fifth version of the Landlock

include/uapi/linux/landlock.h struct landlock_net_port_attr { · line 259

Quoted 10 lines

include/uapi/linux/landlock.h +9 -0

  *   ABI.
+ * - %LANDLOCK_ACCESS_FS_RESOLVE_UNIX: Look up pathname UNIX domain sockets
+ *   (:manpage:`unix(7)`).  On UNIX domain sockets, this restricts both calls to
+ *   :manpage:`connect(2)` as well as calls to :manpage:`sendmsg(2)` with an
+ *   explicit recipient address.
+ *
+ *   This access right only applies to connections to UNIX server sockets which
+ *   were created outside of the newly created Landlock domain (e.g. from within
+ *   a parent domain or from an unrestricted process).  Newly created UNIX
+ *   servers within the same Landlock domain continue to be accessible.

It might help to add a reference to the explicit scope mechanism.

Quoted 2 lines

Please squash patch 9/9 into this one and also add a reference here to the rationale described in security/landlock.rst

Sounds good, will do.

Quoted 7 lines

include/uapi/linux/landlock.h struct landlock_net_port_attr {

Quoted 4 lines

include/uapi/linux/landlock.h +4 -0

+static void unmask_scoped_access(const struct landlock_ruleset *const client,
+				 const struct landlock_ruleset *const server,
+				 struct layer_access_masks *const masks,
+				 const access_mask_t access)

This helper should be moved to task.c and factored out with domain_is_scoped(). This should be a dedicated patch.

(already discussed in another follow-up mail)

Quoted 42 lines

Quoted 18 lines

include/uapi/linux/landlock.h +18 -0

+static int hook_unix_find(const struct path *const path, struct sock *other,
+			  int flags)
+{
+	const struct landlock_ruleset *dom_other;
+	const struct landlock_cred_security *subject;
+	struct layer_access_masks layer_masks;
+	struct landlock_request request = {};
+	static const struct access_masks fs_resolve_unix = {
+		.fs = LANDLOCK_ACCESS_FS_RESOLVE_UNIX,
+	};
+
+	/* Lookup for the purpose of saving coredumps is OK. */
+	if (unlikely(flags & SOCK_COREDUMP))
+		return 0;
+
+	/* Access to the same (or a lower) domain is always allowed. */
+	subject = landlock_get_applicable_subject(current_cred(),
+						  fs_resolve_unix, NULL);

include/uapi/linux/landlock.h

Quoted 10 lines

include/uapi/linux/landlock.h +10 -0

+
+	if (!subject)
+		return 0;
+
+	if (!landlock_init_layer_masks(subject->domain, fs_resolve_unix.fs,
+				       &layer_masks, LANDLOCK_KEY_INODE))
+		return 0;
+
+	/* Checks the layers in which we are connecting within the same domain. */
+	dom_other = landlock_cred(other->sk_socket->file->f_cred)->domain;

We need to call unix_state_lock(other) before reading it, and check for SOCK_DEAD, and check sk_socket before dereferencing it. Indeed, the socket can be make orphan (see unix_dgram_sendmsg and unix_stream_connect). I *think* a socket cannot be "resurrected" or recycled once dead, so we may assume there is no race condition wrt dom_other, but please double check. This lockless call should be made clear in the LSM hook. It's OK to not lock the socket before security_unix_find() (1) because no LSM might implement and (2) they might not need to lock the socket (e.g. if the caller is not sandboxed).

The updated code should look something like this:

unix_state_unlock(other);

Quoted 7 lines

if (unlikely(sock_flag(other, SOCK_DEAD) || !other->sk_socket)) {
	unix_state_unlock(other);
	return 0;
}

dom_other = landlock_cred(other->sk_socket->file->f_cred)->domain; unix_state_unlock(other);

Thank you for spotting the locking concern!

@anyone from netdev, could you please advise on the correct locking approach here?

* Do we need ot check SOCK_DEAD?

You are saying that we need to do that, but it's not clear to me why.

If you look at the places where unix_find_other() is called in af_unix.c, then you'll find that all of them check for SOCK_DEAD and then restart from unix_find_other() if they do actually discover that the socket is dead. I think that this is catching this race condition scenario:

* a server socket exists and is alive
* A client connects: af_unix.c's unix_stream_connect() calls unix_find_other() and finds the server socket...
* (Concurrently): The server closes the socket and enters unix_release_sock(). This function:
1. disassociates the server sock from the named socket inode number in the hash table (=> future unix_find_other() calls will fail).
2. calls sock_orphan(), which sets SOCK_DEAD.
* ...(client connection resuming): unix_stream_connect() continues, grabs the unix_state_lock(), which apparently protects everything here, checks that the socket is not dead - and discovers that it IS suddenly dead. This was not supposed to happen. The code recovers from that by retrying everything starting with the unix_find_other() call. From unix_release_sock(), we know that the inode is not associated with the sock any more -- so the unix_find_socket_by_inode() call should be failing on the next attempt.

(This works with unix_dgram_connect() and unix_dgram_sendmsg() as well.)

The comments next to the SOCK_DEAD checks are also suggesting this.

* What lock to use

I am having trouble reasoning about what lock is used for what in this code.

Is it possible that the lock protecting ->sk_socket is the ->sk_callback_lock, not the unix_state_lock()? The only callers to sk_set_socket are either sock_orphan/sock_graft (both grabbing sk_callback_lock), or they are creating new struct sock objects that they own exclusively, and don't need locks yet.

Admittedly, in af_unix.c, sock_orphan() and sock_graft() only get called in contexts where the unix_state_lock() is held, so it would probably work as well to lock that, but it is maybe a more fine-grained approach to use sk_callback_lock?

So... how about a scheme where we only check for ->sk_socket not being NULL:

read_lock_bh(&other->sk_callback_lock);
struct sock *other_sk = other->sk_socket;

if (!other_sk) {
	read_unlock_bh(&other->sk_callback_lock);
	return 0;
}
/* XXX double check whether we need a lock here too */
struct file *file = other_sk->file;
if (!other_file) {
	read_unlock_bh(&other->sk_callback_lock);
	return 0;
}

read_unlock_bh(&other->sk_callback_lock);

If this fails, that would in my understanding also be because the socket has died after the path lookup. We'd then return 0 (success), because we know that the surrounding SOCK_DEAD logic will repeat everything starting from the path lookup operation (this time likely failing with ECONNREFUSED, but maybe also with a success, if another server process was quick enough).

Does this sound reasonable?

–Günther

Quoted 5 lines

Quoted 5 lines

dom_other, but please double check. This lockless call should be made clear in the LSM hook. It's OK to not lock the socket before security_unix_find() (1) because no LSM might implement and (2) they might not need to lock the socket (e.g. if the caller is not sandboxed).

Quoted 3 lines

Quoted 3 lines

The updated code should look something like this:

unix_state_unlock(other);

unix_state_lock(other) of course...

Quoted 12 lines

Quoted 7 lines

if (unlikely(sock_flag(other, SOCK_DEAD) || !other->sk_socket)) {
	unix_state_unlock(other);
	return 0;
}

dom_other = landlock_cred(other->sk_socket->file->f_cred)->domain; unix_state_unlock(other);

Thank you for spotting the locking concern!

@anyone from netdev, could you please advise on the correct locking approach here?

Quoted 30 lines

* Do we need ot check SOCK_DEAD?

You are saying that we need to do that, but it's not clear to me why.

If you look at the places where unix_find_other() is called in af_unix.c, then you'll find that all of them check for SOCK_DEAD and then restart from unix_find_other() if they do actually discover that the socket is dead. I think that this is catching this race condition scenario:

* a server socket exists and is alive
* A client connects: af_unix.c's unix_stream_connect() calls unix_find_other() and finds the server socket...
* (Concurrently): The server closes the socket and enters unix_release_sock(). This function:
1. disassociates the server sock from the named socket inode number in the hash table (=> future unix_find_other() calls will fail).
2. calls sock_orphan(), which sets SOCK_DEAD.
* ...(client connection resuming): unix_stream_connect() continues, grabs the unix_state_lock(), which apparently protects everything here, checks that the socket is not dead - and discovers that it IS suddenly dead. This was not supposed to happen. The code recovers from that by retrying everything starting with the unix_find_other() call. From unix_release_sock(), we know that the inode is not associated with the sock any more -- so the unix_find_socket_by_inode() call should be failing on the next attempt.

Quoted 4 lines

(This works with unix_dgram_connect() and unix_dgram_sendmsg() as well.)

The comments next to the SOCK_DEAD checks are also suggesting this.

Quoted 5 lines

* What lock to use

I am having trouble reasoning about what lock is used for what in this code.

It's not clear to me neither, and it looks like it's not consistent across protocols.

Quoted 11 lines

Is it possible that the lock protecting ->sk_socket is the ->sk_callback_lock, not the unix_state_lock()? The only callers to sk_set_socket are either sock_orphan/sock_graft (both grabbing sk_callback_lock), or they are creating new struct sock objects that they own exclusively, and don't need locks yet.

Admittedly, in af_unix.c, sock_orphan() and sock_graft() only get called in contexts where the unix_state_lock() is held, so it would probably work as well to lock that, but it is maybe a more fine-grained approach to use sk_callback_lock?

Quoted 17 lines

So... how about a scheme where we only check for ->sk_socket not being NULL:

read_lock_bh(&other->sk_callback_lock);
struct sock *other_sk = other->sk_socket;

if (!other_sk) {
	read_unlock_bh(&other->sk_callback_lock);
	return 0;
}
/* XXX double check whether we need a lock here too */
struct file *file = other_sk->file;
if (!other_file) {
	read_unlock_bh(&other->sk_callback_lock);
	return 0;
}

read_unlock_bh(&other->sk_callback_lock);

Quoted 8 lines

If this fails, that would in my understanding also be because the socket has died after the path lookup. We'd then return 0 (success), because we know that the surrounding SOCK_DEAD logic will repeat everything starting from the path lookup operation (this time likely failing with ECONNREFUSED, but maybe also with a success, if another server process was quick enough).

Does this sound reasonable?

Actually, since commit 983512f3a87f ("net: Drop the lock in skb_may_tx_timestamp()"), we can just use RCU + READ_ONCE(sk_socket) + READ_ONCE(file). The socket and file should only be freed after the RCU grace periode. As a safeguard, this commit should be a Depends-on.

However, it is safer to return -ECONNREFULED when sk_socket or file are NULL.

I would be good to hear from netdev folks though.

TIL, there is an LSM hook for sock_graft().

Quoted 1 line

–Günther

Quoted 80 lines · Günther Noack

+netdev, we could use some advice on the locking approach in af_unix (see below)

Quoted 21 lines · Mickaël Salaün

Quoted 7 lines · Günther Noack

include/uapi/linux/landlock.h +0 -0

index f88fa1f68b77..3a8fc3af0d64 100644
@@ -248,6 +248,15 @@ struct landlock_net_port_attr {
  *
  *   This access right is available since the fifth version of the Landlock

include/uapi/linux/landlock.h struct landlock_net_port_attr { · line 259

Quoted 10 lines

include/uapi/linux/landlock.h +9 -0

  *   ABI.
+ * - %LANDLOCK_ACCESS_FS_RESOLVE_UNIX: Look up pathname UNIX domain sockets
+ *   (:manpage:`unix(7)`).  On UNIX domain sockets, this restricts both calls to
+ *   :manpage:`connect(2)` as well as calls to :manpage:`sendmsg(2)` with an
+ *   explicit recipient address.
+ *
+ *   This access right only applies to connections to UNIX server sockets which
+ *   were created outside of the newly created Landlock domain (e.g. from within
+ *   a parent domain or from an unrestricted process).  Newly created UNIX
+ *   servers within the same Landlock domain continue to be accessible.

It might help to add a reference to the explicit scope mechanism.

Quoted 2 lines

Please squash patch 9/9 into this one and also add a reference here to the rationale described in security/landlock.rst

Sounds good, will do.

Quoted 7 lines

include/uapi/linux/landlock.h struct landlock_net_port_attr {

Quoted 4 lines

include/uapi/linux/landlock.h +4 -0

+static void unmask_scoped_access(const struct landlock_ruleset *const client,
+                          const struct landlock_ruleset *const server,
+                          struct layer_access_masks *const masks,
+                          const access_mask_t access)

This helper should be moved to task.c and factored out with domain_is_scoped(). This should be a dedicated patch.

(already discussed in another follow-up mail)

Quoted 39 lines

Quoted 18 lines

include/uapi/linux/landlock.h +18 -0

+static int hook_unix_find(const struct path *const path, struct sock *other,
+                   int flags)
+{
+ const struct landlock_ruleset *dom_other;
+ const struct landlock_cred_security *subject;
+ struct layer_access_masks layer_masks;
+ struct landlock_request request = {};
+ static const struct access_masks fs_resolve_unix = {
+         .fs = LANDLOCK_ACCESS_FS_RESOLVE_UNIX,
+ };
+
+ /* Lookup for the purpose of saving coredumps is OK. */
+ if (unlikely(flags & SOCK_COREDUMP))
+         return 0;
+
+ /* Access to the same (or a lower) domain is always allowed. */
+ subject = landlock_get_applicable_subject(current_cred(),
+                                           fs_resolve_unix, NULL);

include/uapi/linux/landlock.h

Quoted 10 lines

include/uapi/linux/landlock.h +10 -0

+
+ if (!subject)
+         return 0;
+
+ if (!landlock_init_layer_masks(subject->domain, fs_resolve_unix.fs,
+                                &layer_masks, LANDLOCK_KEY_INODE))
+         return 0;
+
+ /* Checks the layers in which we are connecting within the same domain. */
+ dom_other = landlock_cred(other->sk_socket->file->f_cred)->domain;

We need to call unix_state_lock(other) before reading it, and check for SOCK_DEAD, and check sk_socket before dereferencing it. Indeed, the socket can be make orphan (see unix_dgram_sendmsg and unix_stream_connect). I *think* a socket cannot be "resurrected" or recycled once dead, so we may assume there is no race condition wrt dom_other, but please double check. This lockless call should be made clear in the LSM hook. It's OK to not lock the socket before security_unix_find() (1) because no LSM might implement and (2) they might not need to lock the socket (e.g. if the caller is not sandboxed).

Quoted 3 lines

Quoted 3 lines

The updated code should look something like this:

unix_state_unlock(other);

unix_state_lock(other) of course...

Quoted 14 lines

Quoted 7 lines

if (unlikely(sock_flag(other, SOCK_DEAD) || !other->sk_socket)) {
    unix_state_unlock(other);
    return 0;
}

dom_other = landlock_cred(other->sk_socket->file->f_cred)->domain; unix_state_unlock(other);

Thank you for spotting the locking concern!

@anyone from netdev, could you please advise on the correct locking approach here?

* Do we need ot check SOCK_DEAD?

Quoted 8 lines

You are saying that we need to do that, but it's not clear to me why.

If you look at the places where unix_find_other() is called in af_unix.c, then you'll find that all of them check for SOCK_DEAD and then restart from unix_find_other() if they do actually discover that the socket is dead.

Quoted 27 lines

I think that this is catching this race
condition scenario:

* a server socket exists and is alive
* A client connects: af_unix.c's unix_stream_connect() calls unix_find_other() and finds the server socket...
* (Concurrently): The server closes the socket and enters unix_release_sock(). This function:
1. disassociates the server sock from the named socket inode number in the hash table (=> future unix_find_other() calls will fail).
2. calls sock_orphan(), which sets SOCK_DEAD.
* ...(client connection resuming): unix_stream_connect() continues, grabs the unix_state_lock(), which apparently protects everything here, checks that the socket is not dead - and discovers that it IS suddenly dead. This was not supposed to happen. The code recovers from that by retrying everything starting with the unix_find_other() call. From unix_release_sock(), we know that the inode is not associated with the sock any more -- so the unix_find_socket_by_inode() call should be failing on the next attempt.

(This works with unix_dgram_connect() and unix_dgram_sendmsg() as well.)

The comments next to the SOCK_DEAD checks are also suggesting this.

Quoted 4 lines

* What lock to use

I am having trouble reasoning about what lock is used for what in this code.

It's not clear to me neither, and it looks like it's not consistent across protocols.

Quoted 3 lines

Is it possible that the lock protecting ->sk_socket is the ->sk_callback_lock, not the unix_state_lock()?

Quoted 35 lines

The only callers to
sk_set_socket are either sock_orphan/sock_graft (both grabbing sk_callback_lock), or they are creating new struct sock objects that they own exclusively, and don't need locks yet.

Admittedly, in af_unix.c, sock_orphan() and sock_graft() only get called in contexts where the unix_state_lock() is held, so it would probably work as well to lock that, but it is maybe a more fine-grained approach to use sk_callback_lock?

So... how about a scheme where we only check for ->sk_socket not being NULL:

read_lock_bh(&other->sk_callback_lock);
struct sock *other_sk = other->sk_socket;

if (!other_sk) {
      read_unlock_bh(&other->sk_callback_lock);
      return 0;
}
/* XXX double check whether we need a lock here too */
struct file *file = other_sk->file;
if (!other_file) {
      read_unlock_bh(&other->sk_callback_lock);
      return 0;
}

read_unlock_bh(&other->sk_callback_lock);

If this fails, that would in my understanding also be because the socket has died after the path lookup. We'd then return 0 (success), because we know that the surrounding SOCK_DEAD logic will repeat everything starting from the path lookup operation (this time likely failing with ECONNREFUSED, but maybe also with a success, if another server process was quick enough).

Quoted 1 line

Does this sound reasonable?

Actually, since commit 983512f3a87f ("net: Drop the lock in skb_may_tx_timestamp()"), we can just use RCU + READ_ONCE(sk_socket) + READ_ONCE(file). The socket and file should only be freed after the RCU grace periode. As a safeguard, this commit should be a Depends-on.

However, it is safer to return -ECONNREFULED when sk_socket or file are NULL.

I would be good to hear from netdev folks though.

TIL, there is an LSM hook for sock_graft().

Quoted 2 lines

–Günther

* Add a new access right LANDLOCK_ACCESS_FS_RESOLVE_UNIX, which controls the look up operations for named UNIX domain sockets. The resolution happens during connect() and sendmsg() (depending on socket type).
* Hook into the path lookup in unix_find_bsd() in af_unix.c, using a LSM hook. Make policy decisions based on the new access rights
* Increment the Landlock ABI version.
* Minor test adaptions to keep the tests working.

With this access right, access is granted if either of the following conditions is met:

* The target socket's filesystem path was allow-listed using a LANDLOCK_RULE_PATH_BENEATH rule, *or*:
* The target socket was created in the same Landlock domain in which LANDLOCK_ACCESS_FS_RESOLVE_UNIX was restricted.

In case of a denial, connect() and sendmsg() return EACCES, which is the same error as it is returned if the user does not have the write bit in the traditional Unix file system permissions of that file.

This feature was created with substantial discussion and input from Justin Suess, Tingmao Wang and Mickaël Salaün.

Cc Tingmao Wang <[email protected]>

Cc Justin Suess <[email protected]>

Cc Mickaël Salaün <[email protected]>

Suggested-by Jann Horn <[email protected]>

Link https://github.com/landlock-lsm/linux/issues/36

Signed-off-by Günther Noack <[email protected]>

---
 include/uapi/linux/landlock.h                |  10 ++
 security/landlock/access.h                   |  11 +-
 security/landlock/audit.c                    |   1 +
 security/landlock/fs.c                       | 102 ++++++++++++++++++-
 security/landlock/limits.h                   |   2 +-
 security/landlock/syscalls.c                 |   2 +-
 tools/testing/selftests/landlock/base_test.c |   2 +-
 tools/testing/selftests/landlock/fs_test.c   |   5 +-
 8 files changed, 128 insertions(+), 7 deletions(-)

security/landlock/fs.c +22 -0

+static int hook_unix_find(const struct path *const path, struct sock *other,
+			  int flags)
+{
+	const struct landlock_ruleset *dom_other;
+	const struct landlock_cred_security *subject;
+	struct layer_access_masks layer_masks;
+	struct landlock_request request = {};
+	static const struct access_masks fs_resolve_unix = {
+		.fs = LANDLOCK_ACCESS_FS_RESOLVE_UNIX,
+	};
+
+	/* Lookup for the purpose of saving coredumps is OK. */
+	if (unlikely(flags & SOCK_COREDUMP))
+		return 0;
+
+	/* Access to the same (or a lower) domain is always allowed. */
+	subject = landlock_get_applicable_subject(current_cred(),
+						  fs_resolve_unix, NULL);
+
+	if (!subject)
+		return 0;
+

security/landlock/fs.c +11 -0

+		return 0;
+
+	/* Checks the connections to allow-listed paths. */
+	if (is_access_to_paths_allowed(subject->domain, path,
+				       fs_resolve_unix.fs, &layer_masks,
+				       &request, NULL, 0, NULL, NULL, NULL))
+		return 0;
+
+	landlock_log_denial(subject, &request);
+	return -EACCES;
+}

Quoted 12 lines · Günther Noack

* Add a new access right LANDLOCK_ACCESS_FS_RESOLVE_UNIX, which controls the look up operations for named UNIX domain sockets. The resolution happens during connect() and sendmsg() (depending on socket type).
* Hook into the path lookup in unix_find_bsd() in af_unix.c, using a LSM hook. Make policy decisions based on the new access rights
* Increment the Landlock ABI version.
* Minor test adaptions to keep the tests working.

With this access right, access is granted if either of the following conditions is met:

Quoted 8 lines

* The target socket's filesystem path was allow-listed using a LANDLOCK_RULE_PATH_BENEATH rule, *or*:
* The target socket was created in the same Landlock domain in which LANDLOCK_ACCESS_FS_RESOLVE_UNIX was restricted.

In case of a denial, connect() and sendmsg() return EACCES, which is the same error as it is returned if the user does not have the write bit in the traditional Unix file system permissions of that file.

It is not the same error code as for scoped abstract unix socket (EPERM), but it makes sense because the scope restrictions are closer to ambient rights (i.e. similar to a network isolation), whereas here the final denial comes from a missing FS rule (and all FS access checks may return EACCES). It would be worth mentioning this difference in the user documentation.

Quoted 48 lines

This feature was created with substantial discussion and input from Justin Suess, Tingmao Wang and Mickaël Salaün.

Cc Tingmao Wang <[email protected]>

Cc Justin Suess <[email protected]>

Cc Mickaël Salaün <[email protected]>

Suggested-by Jann Horn <[email protected]>

Link https://github.com/landlock-lsm/linux/issues/36

Signed-off-by Günther Noack <[email protected]>

---
 include/uapi/linux/landlock.h                |  10 ++
 security/landlock/access.h                   |  11 +-
 security/landlock/audit.c                    |   1 +
 security/landlock/fs.c                       | 102 ++++++++++++++++++-
 security/landlock/limits.h                   |   2 +-
 security/landlock/syscalls.c                 |   2 +-
 tools/testing/selftests/landlock/base_test.c |   2 +-
 tools/testing/selftests/landlock/fs_test.c   |   5 +-
 8 files changed, 128 insertions(+), 7 deletions(-)

security/landlock/fs.c +28 -0

+static int hook_unix_find(const struct path *const path, struct sock *other,
+			  int flags)
+{
+	const struct landlock_ruleset *dom_other;
+	const struct landlock_cred_security *subject;
+	struct layer_access_masks layer_masks;
+	struct landlock_request request = {};
+	static const struct access_masks fs_resolve_unix = {
+		.fs = LANDLOCK_ACCESS_FS_RESOLVE_UNIX,
+	};
+
+	/* Lookup for the purpose of saving coredumps is OK. */
+	if (unlikely(flags & SOCK_COREDUMP))
+		return 0;
+
+	/* Access to the same (or a lower) domain is always allowed. */
+	subject = landlock_get_applicable_subject(current_cred(),
+						  fs_resolve_unix, NULL);
+
+	if (!subject)
+		return 0;
+
+	if (!landlock_init_layer_masks(subject->domain, fs_resolve_unix.fs,
+				       &layer_masks, LANDLOCK_KEY_INODE))
+		return 0;
+
+	/* Checks the layers in which we are connecting within the same domain. */
+	dom_other = landlock_cred(other->sk_socket->file->f_cred)->domain;

security/landlock/fs.c static int hook_path_truncate(const struct path *const path) · line 1650

Quoted 4 lines

security/landlock/fs.c +4 -0

+	unmask_scoped_access(subject->domain, dom_other, &layer_masks,
+			     fs_resolve_unix.fs);
+
+	if (layer_access_masks_empty(&layer_masks))

I don't see the point of this helper and this call wrt the following is_access_to_paths_allowed() call and the is_layer_masks_allowed() check.

Quoted 11 lines

security/landlock/fs.c +11 -0

+		return 0;
+
+	/* Checks the connections to allow-listed paths. */
+	if (is_access_to_paths_allowed(subject->domain, path,
+				       fs_resolve_unix.fs, &layer_masks,
+				       &request, NULL, 0, NULL, NULL, NULL))
+		return 0;
+
+	landlock_log_denial(subject, &request);
+	return -EACCES;
+}

The access right for UNIX domain socket lookups is grouped with the read-write rights in the sample tool. Rationale: In the general case, any operations are possible through a UNIX domain socket, including data-mutating operations.

Cc Justin Suess <[email protected]>

Cc Mickaël Salaün <[email protected]>

Signed-off-by Günther Noack <[email protected]>

---
 samples/landlock/sandboxer.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

samples/landlock/sandboxer.c +5 -3

index e7af02f98208..0bbbc5c9ead6 100644
@@ -111,7 +111,8 @@ static int parse_path(char *env_path, const char ***const path_list)
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
 	LANDLOCK_ACCESS_FS_READ_FILE | \
 	LANDLOCK_ACCESS_FS_TRUNCATE | \
-	LANDLOCK_ACCESS_FS_IOCTL_DEV)
+	LANDLOCK_ACCESS_FS_IOCTL_DEV | \
+	LANDLOCK_ACCESS_FS_RESOLVE_UNIX)
 
 /* clang-format on */
 

@@ -295,11 +296,12 @@ static bool check_ruleset_scope(const char *const env_var,
 	LANDLOCK_ACCESS_FS_MAKE_SYM | \
 	LANDLOCK_ACCESS_FS_REFER | \
 	LANDLOCK_ACCESS_FS_TRUNCATE | \
-	LANDLOCK_ACCESS_FS_IOCTL_DEV)
+	LANDLOCK_ACCESS_FS_IOCTL_DEV | \
+	LANDLOCK_ACCESS_FS_RESOLVE_UNIX)
 
 /* clang-format on */
 
-#define LANDLOCK_ABI_LAST 7
+#define LANDLOCK_ABI_LAST 9
 
 #define XSTR(s) #s
 #define STR(s) XSTR(s)

samples/landlock/sandboxer.c +5 -0

+	case 8:
+		/* Removes LANDLOCK_ACCESS_FS_RESOLVE_UNIX for ABI < 9 */
+		ruleset_attr.handled_access_fs &=
+			~LANDLOCK_ACCESS_FS_RESOLVE_UNIX;
+		__attribute__((fallthrough));
 	case LANDLOCK_ABI_LAST:
 		break;
 	default:

Signature · 1 line

2.52.0

samples/landlock/sandboxer.c int main(const int argc, char *const argv[], char *const *const envp) · line 450

Quoted 6 lines · Günther Noack

samples/landlock/sandboxer.c +2 -0

@@ -444,6 +446,13 @@ int main(const int argc, char *const argv[], char *const *const envp)
 			"provided by ABI version %d (instead of %d).\n",
 			LANDLOCK_ABI_LAST, abi);
 		__attribute__((fallthrough));
+	case 7:
+		__attribute__((fallthrough));

The current code should print the hint when ABI <= 7. Please send a dedicated patch to fix the TSYNC-related changes.