[PATCH v8 0/4] Landlock: Add abstract unix socket connect

[PATCH v8 0/4] Landlock: Add abstract unix socket connect

[Tahera Fahimi]

This patch series adds scoping mechanism for abstract unix sockets.
Closes: https://github.com/landlock-lsm/linux/issues/7

Problem
=======

Abstract unix sockets are used for local inter-process communications
independent of the filesystem. Currently, a sandboxed process can
connect to a socket outside of the sandboxed environment, since Landlock
has no restriction for connecting to an abstract socket address(see more
details in [1,2]). Access to such sockets for a sandboxed process should
be scoped the same way ptrace is limited.

[1] https://lore.kernel.org/all/20231023.ahphah4Wii4v@digikod.net/
[2] https://lore.kernel.org/all/20231102.MaeWaepav8nu@digikod.net/

Solution
========

To solve this issue, we extend the user space interface by adding a new
"scoped" field to Landlock ruleset attribute structure. This field can
contains different rights to restrict different functionalities. For
abstract unix sockets, we introduce
"LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" field to specify that a ruleset
will deny any connection from within the sandbox domain to its parent
(i.e. any parent sandbox or non-sandbox processes).

Example
=======

Starting a listening socket with socat(1):
	socat abstract-listen:mysocket -

Starting a sandboxed shell from $HOME with samples/landlock/sandboxer:
	LL_FS_RO=/ LL_FS_RW=. LL_SCOPED="a" ./sandboxer /bin/bash

If we try to connect to the listening socket, the connection would be
refused.
	socat - abstract-connect:mysocket --> fails


Notes of Implementation
=======================

* Using the "scoped" field provides enough compatibility and flexibility
  to extend the scoping mechanism for other IPCs(e.g. signals).

* To access the domain of a socket, we use its credentials of the file's FD
  which point to the credentials of the process that created the socket.
  (see more details in [3]). Cases where the process using the socket has
  a different domain than the process created it are covered in the 
  unix_sock_special_cases test.

[3]https://lore.kernel.org/all/20240611.Pi8Iph7ootae@digikod.net/


Previous Versions
=================
v6: https://lore.kernel.org/all/Zn32CYZiu7pY+rdI@tahera-OptiPlex-5000/
and https://lore.kernel.org/all/Zn32KKIJrY7Zi51K@tahera-OptiPlex-5000/
v5: https://lore.kernel.org/all/ZnSZnhGBiprI6FRk@tahera-OptiPlex-5000/
v4: https://lore.kernel.org/all/ZnNcE3ph2SWi1qmd@tahera-OptiPlex-5000/
v3: https://lore.kernel.org/all/ZmJJ7lZdQuQop7e5@tahera-OptiPlex-5000/
v2: https://lore.kernel.org/all/ZgX5TRTrSDPrJFfF@tahera-OptiPlex-5000/
v1: https://lore.kernel.org/all/ZgXN5fi6A1YQKiAQ@tahera-OptiPlex-5000/

Tahera Fahimi (4):
  Landlock: Add abstract unix socket connect restriction
  selftests/landlock: Abstract unix socket restriction tests
  sample/Landlock: Support abstract unix socket restriction
  Landlock: Document LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET and ABI
    versioning

 Documentation/userspace-api/landlock.rst      |   33 +-
 include/uapi/linux/landlock.h                 |   30 +
 samples/landlock/sandboxer.c                  |   56 +-
 security/landlock/limits.h                    |    3 +
 security/landlock/ruleset.c                   |    7 +-
 security/landlock/ruleset.h                   |   23 +-
 security/landlock/syscalls.c                  |   14 +-
 security/landlock/task.c                      |  155 +++
 tools/testing/selftests/landlock/base_test.c  |    2 +-
 tools/testing/selftests/landlock/common.h     |   72 ++
 tools/testing/selftests/landlock/fs_test.c    |   34 -
 tools/testing/selftests/landlock/net_test.c   |   31 +-
 .../landlock/scoped_abstract_unix_test.c      | 1136 +++++++++++++++++
 13 files changed, 1518 insertions(+), 78 deletions(-)
 create mode 100644 tools/testing/selftests/landlock/scoped_abstract_unix_test.c

-- 
2.34.1

[PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction

[Tahera Fahimi]

This patch introduces a new "scoped" attribute to the landlock_ruleset_attr
that can specify "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to scope
abstract Unix sockets from connecting to a process outside of
the same landlock domain. It implements two hooks, unix_stream_connect
and unix_may_send to enforce this restriction.

Closes: https://github.com/landlock-lsm/linux/issues/7
Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>

---
v8:
- Code refactoring (improve code readability, renaming variable, etc.) based
  on reviews by Mickaël Salaün on version 7.
- Adding warn_on_once to check (impossible) inconsistencies.
- Adding inline comments.
- Adding check_unix_address_format to check if the scoping socket is an abstract
  unix sockets.
v7:
 - Using socket's file credentials for both connected(STREAM) and
   non-connected(DGRAM) sockets.
 - Adding "domain_sock_scope" instead of the domain scoping mechanism used in
   ptrace ensures that if a server's domain is accessible from the client's
   domain (where the client is more privileged than the server), the client
   can connect to the server in all edge cases.
 - Removing debug codes.
v6:
 - Removing curr_ruleset from landlock_hierarchy, and switching back to use
   the same domain scoping as ptrace.
 - code clean up.
v5:
 - Renaming "LANDLOCK_*_ACCESS_SCOPE" to "LANDLOCK_*_SCOPE"
 - Adding curr_ruleset to hierarachy_ruleset structure to have access from
   landlock_hierarchy to its respective landlock_ruleset.
 - Using curr_ruleset to check if a domain is scoped while walking in the
   hierarchy of domains.
 - Modifying inline comments.
V4:
 - Rebased on Günther's Patch:
   https://lore.kernel.org/all/20240610082115.1693267-1-gnoack@google.com/
   so there is no need for "LANDLOCK_SHIFT_ACCESS_SCOPE", then it is removed.
 - Adding get_scope_accesses function to check all scoped access masks in a ruleset.
 - Using socket's file credentials instead of credentials stored in peer_cred
   for datagram sockets. (see discussion in [1])
 - Modifying inline comments.
V3:
 - Improving commit description.
 - Introducing "scoped" attribute to landlock_ruleset_attr for IPC scoping
   purpose, and adding related functions.
 - Changing structure of ruleset based on "scoped".
 - Removing rcu lock and using unix_sk lock instead.
 - Introducing scoping for datagram sockets in unix_may_send.
V2:
 - Removing wrapper functions

[1]https://lore.kernel.org/all/20240610.Aifee5ingugh@digikod.net/
----
---
 include/uapi/linux/landlock.h |  30 +++++++
 security/landlock/limits.h    |   3 +
 security/landlock/ruleset.c   |   7 +-
 security/landlock/ruleset.h   |  23 ++++-
 security/landlock/syscalls.c  |  14 ++-
 security/landlock/task.c      | 155 ++++++++++++++++++++++++++++++++++
 6 files changed, 225 insertions(+), 7 deletions(-)

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 68625e728f43..ab31e9f53e55 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -37,6 +37,12 @@ struct landlock_ruleset_attr {
 	 * rule explicitly allow them.
 	 */
 	__u64 handled_access_net;
+	/**
+	 * @scoped: Bitmask of scopes (cf. `Scope flags`_)
+	 * restricting a Landlock domain from accessing outside
+	 * resources(e.g. IPCs).
+	 */
+	__u64 scoped;
 };
 
 /*
@@ -266,4 +272,28 @@ struct landlock_net_port_attr {
 #define LANDLOCK_ACCESS_NET_BIND_TCP			(1ULL << 0)
 #define LANDLOCK_ACCESS_NET_CONNECT_TCP			(1ULL << 1)
 /* clang-format on */
+
+/**
+ * DOC: scope
+ *
+ * scoped attribute handles a set of restrictions on kernel IPCs through
+ * the following flags.
+ *
+ * Scope flags
+ * ~~~~~~~~~~~
+ *
+ * These flags enable to restrict a sandboxed process from a set of IPC
+ * actions. Setting a flag for a ruleset will isolate the Landlock domain
+ * to forbid connections to resources outside the domain.
+ *
+ * IPCs with scoped actions:
+ *
+ * - %LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET: Restrict a sandboxed process
+ *   from connecting to an abstract unix socket created by a process
+ *   outside the related Landlock domain (e.g. a parent domain or a
+ *   non-sandboxed process).
+ */
+/* clang-format off */
+#define LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET		(1ULL << 0)
+/* clang-format on*/
 #endif /* _UAPI_LINUX_LANDLOCK_H */
diff --git a/security/landlock/limits.h b/security/landlock/limits.h
index 4eb643077a2a..eb01d0fb2165 100644
--- a/security/landlock/limits.h
+++ b/security/landlock/limits.h
@@ -26,6 +26,9 @@
 #define LANDLOCK_MASK_ACCESS_NET	((LANDLOCK_LAST_ACCESS_NET << 1) - 1)
 #define LANDLOCK_NUM_ACCESS_NET		__const_hweight64(LANDLOCK_MASK_ACCESS_NET)
 
+#define LANDLOCK_LAST_SCOPE		LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET
+#define LANDLOCK_MASK_SCOPE		((LANDLOCK_LAST_SCOPE << 1) - 1)
+#define LANDLOCK_NUM_SCOPE		__const_hweight64(LANDLOCK_MASK_SCOPE)
 /* clang-format on */
 
 #endif /* _SECURITY_LANDLOCK_LIMITS_H */
diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index 6ff232f58618..a93bdbf52fff 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -52,12 +52,13 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
 
 struct landlock_ruleset *
 landlock_create_ruleset(const access_mask_t fs_access_mask,
-			const access_mask_t net_access_mask)
+			const access_mask_t net_access_mask,
+			const access_mask_t scope_mask)
 {
 	struct landlock_ruleset *new_ruleset;
 
 	/* Informs about useless ruleset. */
-	if (!fs_access_mask && !net_access_mask)
+	if (!fs_access_mask && !net_access_mask && !scope_mask)
 		return ERR_PTR(-ENOMSG);
 	new_ruleset = create_ruleset(1);
 	if (IS_ERR(new_ruleset))
@@ -66,6 +67,8 @@ landlock_create_ruleset(const access_mask_t fs_access_mask,
 		landlock_add_fs_access_mask(new_ruleset, fs_access_mask, 0);
 	if (net_access_mask)
 		landlock_add_net_access_mask(new_ruleset, net_access_mask, 0);
+	if (scope_mask)
+		landlock_add_scope_mask(new_ruleset, scope_mask, 0);
 	return new_ruleset;
 }
 
diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
index 0f1b5b4c8f6b..c749fa0b3ecd 100644
--- a/security/landlock/ruleset.h
+++ b/security/landlock/ruleset.h
@@ -35,6 +35,8 @@ typedef u16 access_mask_t;
 static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);
 /* Makes sure all network access rights can be stored. */
 static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET);
+/* Makes sure all scoped rights can be stored*/
+static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_SCOPE);
 /* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */
 static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
 
@@ -42,6 +44,7 @@ static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
 struct access_masks {
 	access_mask_t fs : LANDLOCK_NUM_ACCESS_FS;
 	access_mask_t net : LANDLOCK_NUM_ACCESS_NET;
+	access_mask_t scoped : LANDLOCK_NUM_SCOPE;
 };
 
 typedef u16 layer_mask_t;
@@ -233,7 +236,8 @@ struct landlock_ruleset {
 
 struct landlock_ruleset *
 landlock_create_ruleset(const access_mask_t access_mask_fs,
-			const access_mask_t access_mask_net);
+			const access_mask_t access_mask_net,
+			const access_mask_t scope_mask);
 
 void landlock_put_ruleset(struct landlock_ruleset *const ruleset);
 void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset);
@@ -280,6 +284,16 @@ landlock_add_net_access_mask(struct landlock_ruleset *const ruleset,
 	ruleset->access_masks[layer_level].net |= net_mask;
 }
 
+static inline void
+landlock_add_scope_mask(struct landlock_ruleset *const ruleset,
+			const access_mask_t scope_mask, const u16 layer_level)
+{
+	access_mask_t scoped_mask = scope_mask & LANDLOCK_MASK_SCOPE;
+
+	WARN_ON_ONCE(scope_mask != scoped_mask);
+	ruleset->access_masks[layer_level].scoped |= scoped_mask;
+}
+
 static inline access_mask_t
 landlock_get_raw_fs_access_mask(const struct landlock_ruleset *const ruleset,
 				const u16 layer_level)
@@ -303,6 +317,13 @@ landlock_get_net_access_mask(const struct landlock_ruleset *const ruleset,
 	return ruleset->access_masks[layer_level].net;
 }
 
+static inline access_mask_t
+landlock_get_scope_mask(const struct landlock_ruleset *const ruleset,
+			const u16 layer_level)
+{
+	return ruleset->access_masks[layer_level].scoped;
+}
+
 bool landlock_unmask_layers(const struct landlock_rule *const rule,
 			    const access_mask_t access_request,
 			    layer_mask_t (*const layer_masks)[],
diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index 03b470f5a85a..f51b29521d6b 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -97,8 +97,9 @@ static void build_check_abi(void)
 	 */
 	ruleset_size = sizeof(ruleset_attr.handled_access_fs);
 	ruleset_size += sizeof(ruleset_attr.handled_access_net);
+	ruleset_size += sizeof(ruleset_attr.scoped);
 	BUILD_BUG_ON(sizeof(ruleset_attr) != ruleset_size);
-	BUILD_BUG_ON(sizeof(ruleset_attr) != 16);
+	BUILD_BUG_ON(sizeof(ruleset_attr) != 24);
 
 	path_beneath_size = sizeof(path_beneath_attr.allowed_access);
 	path_beneath_size += sizeof(path_beneath_attr.parent_fd);
@@ -149,7 +150,7 @@ static const struct file_operations ruleset_fops = {
 	.write = fop_dummy_write,
 };
 
-#define LANDLOCK_ABI_VERSION 5
+#define LANDLOCK_ABI_VERSION 6
 
 /**
  * sys_landlock_create_ruleset - Create a new ruleset
@@ -170,7 +171,7 @@ static const struct file_operations ruleset_fops = {
  * Possible returned errors are:
  *
  * - %EOPNOTSUPP: Landlock is supported by the kernel but disabled at boot time;
- * - %EINVAL: unknown @flags, or unknown access, or too small @size;
+ * - %EINVAL: unknown @flags, or unknown access, or unknown scope, or too small @size;
  * - %E2BIG or %EFAULT: @attr or @size inconsistencies;
  * - %ENOMSG: empty &landlock_ruleset_attr.handled_access_fs.
  */
@@ -213,9 +214,14 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
 	    LANDLOCK_MASK_ACCESS_NET)
 		return -EINVAL;
 
+	/* Checks IPC scoping content (and 32-bits cast). */
+	if ((ruleset_attr.scoped | LANDLOCK_MASK_SCOPE) != LANDLOCK_MASK_SCOPE)
+		return -EINVAL;
+
 	/* Checks arguments and transforms to kernel struct. */
 	ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs,
-					  ruleset_attr.handled_access_net);
+					  ruleset_attr.handled_access_net,
+					  ruleset_attr.scoped);
 	if (IS_ERR(ruleset))
 		return PTR_ERR(ruleset);
 
diff --git a/security/landlock/task.c b/security/landlock/task.c
index 849f5123610b..7e8579ebae83 100644
--- a/security/landlock/task.c
+++ b/security/landlock/task.c
@@ -13,6 +13,8 @@
 #include <linux/lsm_hooks.h>
 #include <linux/rcupdate.h>
 #include <linux/sched.h>
+#include <net/sock.h>
+#include <net/af_unix.h>
 
 #include "common.h"
 #include "cred.h"
@@ -108,9 +110,162 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
 	return task_ptrace(parent, current);
 }
 
+static bool walk_and_check(const struct landlock_ruleset *const child,
+			   struct landlock_hierarchy **walker,
+			   size_t base_layer, size_t deep_layer,
+			   access_mask_t check_scoping)
+{
+	if (!child || base_layer < 0 || !(*walker))
+		return false;
+
+	for (deep_layer; base_layer < deep_layer; deep_layer--) {
+		if (check_scoping & landlock_get_scope_mask(child, deep_layer))
+			return false;
+		*walker = (*walker)->parent;
+		if (WARN_ON_ONCE(!*walker))
+			/* there is an inconsistency between num_layers
+			 * and landlock_hierarchy in the ruleset
+			 */
+			return false;
+	}
+	return true;
+}
+
+/**
+ * domain_IPC_scope - Checks if the client domain is scoped in the same
+ *		      domain as the server.
+ *
+ * @client: IPC sender domain.
+ * @server: IPC receiver domain.
+ *
+ * Check if the @client domain is scoped to access the @server; the @server
+ * must be scoped in the same domain.
+ */
+static bool domain_IPC_scope(const struct landlock_ruleset *const client,
+			     const struct landlock_ruleset *const server,
+			     access_mask_t ipc_type)
+{
+	size_t client_layer, server_layer = 0;
+	int base_layer;
+	struct landlock_hierarchy *client_walker, *server_walker;
+	bool is_scoped;
+
+	/* Quick return if client has no domain */
+	if (!client)
+		return true;
+
+	client_layer = client->num_layers - 1;
+	client_walker = client->hierarchy;
+	if (server) {
+		server_layer = server->num_layers - 1;
+		server_walker = server->hierarchy;
+	}
+	base_layer = (client_layer > server_layer) ? server_layer :
+						     client_layer;
+
+	/* For client domain, walk_and_check ensures the client domain is
+	 * not scoped until gets to base_layer.
+	 * For server_domain, it only ensures that the server domain exist.
+	 */
+	if (client_layer != server_layer) {
+		if (client_layer > server_layer)
+			is_scoped = walk_and_check(client, &client_walker,
+						   server_layer, client_layer,
+						   ipc_type);
+		else
+			is_scoped = walk_and_check(server, &server_walker,
+						   client_layer, server_layer,
+						   ipc_type & 0);
+		if (!is_scoped)
+			return false;
+	}
+	/* client and server are at the same level in hierarchy. If client is
+	 * scoped, the server must be scoped in the same domain
+	 */
+	for (base_layer; base_layer >= 0; base_layer--) {
+		if (landlock_get_scope_mask(client, base_layer) & ipc_type) {
+			/* This check must be here since access would be denied only if
+			 * the client is scoped and the server has no domain, so
+			 * if the client has a domain but is not scoped and the server
+			 * has no domain, access is guaranteed.
+			 */
+			if (!server)
+				return false;
+
+			if (server_walker == client_walker)
+				return true;
+
+			return false;
+		}
+		client_walker = client_walker->parent;
+		server_walker = server_walker->parent;
+		/* Warn if there is an incosistenncy between num_layers and
+		 * landlock_hierarchy in each of rulesets
+		 */
+		if (WARN_ON_ONCE(base_layer > 0 &&
+				 (!server_walker || !client_walker)))
+			return false;
+	}
+	return true;
+}
+
+static bool sock_is_scoped(struct sock *const other)
+{
+	const struct landlock_ruleset *dom_other;
+	const struct landlock_ruleset *const dom =
+		landlock_get_current_domain();
+
+	/* the credentials will not change */
+	lockdep_assert_held(&unix_sk(other)->lock);
+	dom_other = landlock_cred(other->sk_socket->file->f_cred)->domain;
+	return domain_IPC_scope(dom, dom_other,
+				LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET);
+}
+
+static bool check_unix_address_format(struct sock *const sock)
+{
+	struct unix_address *addr = unix_sk(sock)->addr;
+
+	if (!addr)
+		return true;
+
+	if (addr->len > sizeof(AF_UNIX)) {
+		/* handling unspec sockets */
+		if (!addr->name[0].sun_path)
+			return true;
+
+		if (addr->name[0].sun_path[0] == '\0')
+			if (!sock_is_scoped(sock))
+				return false;
+	}
+
+	return true;
+}
+
+static int hook_unix_stream_connect(struct sock *const sock,
+				    struct sock *const other,
+				    struct sock *const newsk)
+{
+	if (check_unix_address_format(other))
+		return 0;
+
+	return -EPERM;
+}
+
+static int hook_unix_may_send(struct socket *const sock,
+			      struct socket *const other)
+{
+	if (check_unix_address_format(other->sk))
+		return 0;
+
+	return -EPERM;
+}
+
 static struct security_hook_list landlock_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(ptrace_access_check, hook_ptrace_access_check),
 	LSM_HOOK_INIT(ptrace_traceme, hook_ptrace_traceme),
+	LSM_HOOK_INIT(unix_stream_connect, hook_unix_stream_connect),
+	LSM_HOOK_INIT(unix_may_send, hook_unix_may_send),
 };
 
 __init void landlock_add_task_hooks(void)
-- 
2.34.1

Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction

[Mickaël Salaün]

On Thu, Aug 01, 2024 at 10:02:33PM -0600, Tahera Fahimi wrote:
> This patch introduces a new "scoped" attribute to the landlock_ruleset_attr
> that can specify "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to scope
> abstract Unix sockets from connecting to a process outside of
> the same landlock domain. It implements two hooks, unix_stream_connect
> and unix_may_send to enforce this restriction.
> 
> Closes: https://github.com/landlock-lsm/linux/issues/7
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> 
> ---
> v8:
> - Code refactoring (improve code readability, renaming variable, etc.) based
>   on reviews by Mickaël Salaün on version 7.
> - Adding warn_on_once to check (impossible) inconsistencies.
> - Adding inline comments.
> - Adding check_unix_address_format to check if the scoping socket is an abstract
>   unix sockets.
> v7:
>  - Using socket's file credentials for both connected(STREAM) and
>    non-connected(DGRAM) sockets.
>  - Adding "domain_sock_scope" instead of the domain scoping mechanism used in
>    ptrace ensures that if a server's domain is accessible from the client's
>    domain (where the client is more privileged than the server), the client
>    can connect to the server in all edge cases.
>  - Removing debug codes.
> v6:
>  - Removing curr_ruleset from landlock_hierarchy, and switching back to use
>    the same domain scoping as ptrace.
>  - code clean up.
> v5:
>  - Renaming "LANDLOCK_*_ACCESS_SCOPE" to "LANDLOCK_*_SCOPE"
>  - Adding curr_ruleset to hierarachy_ruleset structure to have access from
>    landlock_hierarchy to its respective landlock_ruleset.
>  - Using curr_ruleset to check if a domain is scoped while walking in the
>    hierarchy of domains.
>  - Modifying inline comments.
> V4:
>  - Rebased on Günther's Patch:
>    https://lore.kernel.org/all/20240610082115.1693267-1-gnoack@google.com/
>    so there is no need for "LANDLOCK_SHIFT_ACCESS_SCOPE", then it is removed.
>  - Adding get_scope_accesses function to check all scoped access masks in a ruleset.
>  - Using socket's file credentials instead of credentials stored in peer_cred
>    for datagram sockets. (see discussion in [1])
>  - Modifying inline comments.
> V3:
>  - Improving commit description.
>  - Introducing "scoped" attribute to landlock_ruleset_attr for IPC scoping
>    purpose, and adding related functions.
>  - Changing structure of ruleset based on "scoped".
>  - Removing rcu lock and using unix_sk lock instead.
>  - Introducing scoping for datagram sockets in unix_may_send.
> V2:
>  - Removing wrapper functions
> 
> [1]https://lore.kernel.org/all/20240610.Aifee5ingugh@digikod.net/
> ----
> ---
>  include/uapi/linux/landlock.h |  30 +++++++
>  security/landlock/limits.h    |   3 +
>  security/landlock/ruleset.c   |   7 +-
>  security/landlock/ruleset.h   |  23 ++++-
>  security/landlock/syscalls.c  |  14 ++-
>  security/landlock/task.c      | 155 ++++++++++++++++++++++++++++++++++
>  6 files changed, 225 insertions(+), 7 deletions(-)
> 
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 68625e728f43..ab31e9f53e55 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -37,6 +37,12 @@ struct landlock_ruleset_attr {
>  	 * rule explicitly allow them.
>  	 */
>  	__u64 handled_access_net;
> +	/**
> +	 * @scoped: Bitmask of scopes (cf. `Scope flags`_)
> +	 * restricting a Landlock domain from accessing outside
> +	 * resources(e.g. IPCs).
> +	 */
> +	__u64 scoped;
>  };
>  
>  /*
> @@ -266,4 +272,28 @@ struct landlock_net_port_attr {
>  #define LANDLOCK_ACCESS_NET_BIND_TCP			(1ULL << 0)
>  #define LANDLOCK_ACCESS_NET_CONNECT_TCP			(1ULL << 1)
>  /* clang-format on */
> +
> +/**
> + * DOC: scope
> + *
> + * scoped attribute handles a set of restrictions on kernel IPCs through
> + * the following flags.

If present, this previous sentence should be after the "Scope flags"
section, otherwise it shows in the previous section.  I don't think this
sentence is useful because of the next one, so you can remove it.

> + *
> + * Scope flags
> + * ~~~~~~~~~~~
> + *
> + * These flags enable to restrict a sandboxed process from a set of IPC
> + * actions. Setting a flag for a ruleset will isolate the Landlock domain
> + * to forbid connections to resources outside the domain.
> + *
> + * IPCs with scoped actions:
> + *
> + * - %LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET: Restrict a sandboxed process
> + *   from connecting to an abstract unix socket created by a process
> + *   outside the related Landlock domain (e.g. a parent domain or a
> + *   non-sandboxed process).
> + */
> +/* clang-format off */
> +#define LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET		(1ULL << 0)
> +/* clang-format on*/
>  #endif /* _UAPI_LINUX_LANDLOCK_H */
> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> index 4eb643077a2a..eb01d0fb2165 100644
> --- a/security/landlock/limits.h
> +++ b/security/landlock/limits.h
> @@ -26,6 +26,9 @@
>  #define LANDLOCK_MASK_ACCESS_NET	((LANDLOCK_LAST_ACCESS_NET << 1) - 1)
>  #define LANDLOCK_NUM_ACCESS_NET		__const_hweight64(LANDLOCK_MASK_ACCESS_NET)
>  
> +#define LANDLOCK_LAST_SCOPE		LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET
> +#define LANDLOCK_MASK_SCOPE		((LANDLOCK_LAST_SCOPE << 1) - 1)
> +#define LANDLOCK_NUM_SCOPE		__const_hweight64(LANDLOCK_MASK_SCOPE)
>  /* clang-format on */
>  
>  #endif /* _SECURITY_LANDLOCK_LIMITS_H */
> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> index 6ff232f58618..a93bdbf52fff 100644
> --- a/security/landlock/ruleset.c
> +++ b/security/landlock/ruleset.c
> @@ -52,12 +52,13 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>  
>  struct landlock_ruleset *
>  landlock_create_ruleset(const access_mask_t fs_access_mask,
> -			const access_mask_t net_access_mask)
> +			const access_mask_t net_access_mask,
> +			const access_mask_t scope_mask)
>  {
>  	struct landlock_ruleset *new_ruleset;
>  
>  	/* Informs about useless ruleset. */
> -	if (!fs_access_mask && !net_access_mask)
> +	if (!fs_access_mask && !net_access_mask && !scope_mask)
>  		return ERR_PTR(-ENOMSG);
>  	new_ruleset = create_ruleset(1);
>  	if (IS_ERR(new_ruleset))
> @@ -66,6 +67,8 @@ landlock_create_ruleset(const access_mask_t fs_access_mask,
>  		landlock_add_fs_access_mask(new_ruleset, fs_access_mask, 0);
>  	if (net_access_mask)
>  		landlock_add_net_access_mask(new_ruleset, net_access_mask, 0);
> +	if (scope_mask)
> +		landlock_add_scope_mask(new_ruleset, scope_mask, 0);
>  	return new_ruleset;
>  }
>  
> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index 0f1b5b4c8f6b..c749fa0b3ecd 100644
> --- a/security/landlock/ruleset.h
> +++ b/security/landlock/ruleset.h
> @@ -35,6 +35,8 @@ typedef u16 access_mask_t;
>  static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);
>  /* Makes sure all network access rights can be stored. */
>  static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET);
> +/* Makes sure all scoped rights can be stored*/
> +static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_SCOPE);
>  /* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */
>  static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
>  
> @@ -42,6 +44,7 @@ static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
>  struct access_masks {
>  	access_mask_t fs : LANDLOCK_NUM_ACCESS_FS;
>  	access_mask_t net : LANDLOCK_NUM_ACCESS_NET;
> +	access_mask_t scoped : LANDLOCK_NUM_SCOPE;
>  };
>  
>  typedef u16 layer_mask_t;
> @@ -233,7 +236,8 @@ struct landlock_ruleset {
>  
>  struct landlock_ruleset *
>  landlock_create_ruleset(const access_mask_t access_mask_fs,
> -			const access_mask_t access_mask_net);
> +			const access_mask_t access_mask_net,
> +			const access_mask_t scope_mask);
>  
>  void landlock_put_ruleset(struct landlock_ruleset *const ruleset);
>  void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset);
> @@ -280,6 +284,16 @@ landlock_add_net_access_mask(struct landlock_ruleset *const ruleset,
>  	ruleset->access_masks[layer_level].net |= net_mask;
>  }
>  
> +static inline void
> +landlock_add_scope_mask(struct landlock_ruleset *const ruleset,
> +			const access_mask_t scope_mask, const u16 layer_level)
> +{
> +	access_mask_t scoped_mask = scope_mask & LANDLOCK_MASK_SCOPE;
> +
> +	WARN_ON_ONCE(scope_mask != scoped_mask);
> +	ruleset->access_masks[layer_level].scoped |= scoped_mask;
> +}
> +
>  static inline access_mask_t
>  landlock_get_raw_fs_access_mask(const struct landlock_ruleset *const ruleset,
>  				const u16 layer_level)
> @@ -303,6 +317,13 @@ landlock_get_net_access_mask(const struct landlock_ruleset *const ruleset,
>  	return ruleset->access_masks[layer_level].net;
>  }
>  
> +static inline access_mask_t
> +landlock_get_scope_mask(const struct landlock_ruleset *const ruleset,
> +			const u16 layer_level)
> +{
> +	return ruleset->access_masks[layer_level].scoped;
> +}
> +
>  bool landlock_unmask_layers(const struct landlock_rule *const rule,
>  			    const access_mask_t access_request,
>  			    layer_mask_t (*const layer_masks)[],
> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> index 03b470f5a85a..f51b29521d6b 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -97,8 +97,9 @@ static void build_check_abi(void)
>  	 */
>  	ruleset_size = sizeof(ruleset_attr.handled_access_fs);
>  	ruleset_size += sizeof(ruleset_attr.handled_access_net);
> +	ruleset_size += sizeof(ruleset_attr.scoped);
>  	BUILD_BUG_ON(sizeof(ruleset_attr) != ruleset_size);
> -	BUILD_BUG_ON(sizeof(ruleset_attr) != 16);
> +	BUILD_BUG_ON(sizeof(ruleset_attr) != 24);
>  
>  	path_beneath_size = sizeof(path_beneath_attr.allowed_access);
>  	path_beneath_size += sizeof(path_beneath_attr.parent_fd);
> @@ -149,7 +150,7 @@ static const struct file_operations ruleset_fops = {
>  	.write = fop_dummy_write,
>  };
>  
> -#define LANDLOCK_ABI_VERSION 5
> +#define LANDLOCK_ABI_VERSION 6
>  
>  /**
>   * sys_landlock_create_ruleset - Create a new ruleset
> @@ -170,7 +171,7 @@ static const struct file_operations ruleset_fops = {
>   * Possible returned errors are:
>   *
>   * - %EOPNOTSUPP: Landlock is supported by the kernel but disabled at boot time;
> - * - %EINVAL: unknown @flags, or unknown access, or too small @size;
> + * - %EINVAL: unknown @flags, or unknown access, or unknown scope, or too small @size;

I think you missed one of my previous review.

>   * - %E2BIG or %EFAULT: @attr or @size inconsistencies;
>   * - %ENOMSG: empty &landlock_ruleset_attr.handled_access_fs.
>   */
> @@ -213,9 +214,14 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
>  	    LANDLOCK_MASK_ACCESS_NET)
>  		return -EINVAL;
>  
> +	/* Checks IPC scoping content (and 32-bits cast). */
> +	if ((ruleset_attr.scoped | LANDLOCK_MASK_SCOPE) != LANDLOCK_MASK_SCOPE)
> +		return -EINVAL;
> +
>  	/* Checks arguments and transforms to kernel struct. */
>  	ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs,
> -					  ruleset_attr.handled_access_net);
> +					  ruleset_attr.handled_access_net,
> +					  ruleset_attr.scoped);
>  	if (IS_ERR(ruleset))
>  		return PTR_ERR(ruleset);
>  
> diff --git a/security/landlock/task.c b/security/landlock/task.c
> index 849f5123610b..7e8579ebae83 100644
> --- a/security/landlock/task.c
> +++ b/security/landlock/task.c
> @@ -13,6 +13,8 @@
>  #include <linux/lsm_hooks.h>
>  #include <linux/rcupdate.h>
>  #include <linux/sched.h>
> +#include <net/sock.h>
> +#include <net/af_unix.h>

Please sort the included files.

>  
>  #include "common.h"
>  #include "cred.h"
> @@ -108,9 +110,162 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
>  	return task_ptrace(parent, current);
>  }
>  
> +static bool walk_and_check(const struct landlock_ruleset *const child,
> +			   struct landlock_hierarchy **walker,
> +			   size_t base_layer, size_t deep_layer,
> +			   access_mask_t check_scoping)
> +{
> +	if (!child || base_layer < 0 || !(*walker))
> +		return false;
> +
> +	for (deep_layer; base_layer < deep_layer; deep_layer--) {
> +		if (check_scoping & landlock_get_scope_mask(child, deep_layer))
> +			return false;
> +		*walker = (*walker)->parent;
> +		if (WARN_ON_ONCE(!*walker))
> +			/* there is an inconsistency between num_layers
> +			 * and landlock_hierarchy in the ruleset
> +			 */
> +			return false;
> +	}
> +	return true;
> +}
> +
> +/**
> + * domain_IPC_scope - Checks if the client domain is scoped in the same

Please don't use uppercase (IPC) in function or variable's names, it is
reserved to "#define" statements only.

> + *		      domain as the server.
> + *
> + * @client: IPC sender domain.
> + * @server: IPC receiver domain.
> + *
> + * Check if the @client domain is scoped to access the @server; the @server
> + * must be scoped in the same domain.
> + */
> +static bool domain_IPC_scope(const struct landlock_ruleset *const client,
> +			     const struct landlock_ruleset *const server,
> +			     access_mask_t ipc_type)
> +{
> +	size_t client_layer, server_layer = 0;
> +	int base_layer;
> +	struct landlock_hierarchy *client_walker, *server_walker;
> +	bool is_scoped;
> +
> +	/* Quick return if client has no domain */
> +	if (!client)
> +		return true;
> +
> +	client_layer = client->num_layers - 1;
> +	client_walker = client->hierarchy;
> +	if (server) {
> +		server_layer = server->num_layers - 1;
> +		server_walker = server->hierarchy;
> +	}
> +	base_layer = (client_layer > server_layer) ? server_layer :
> +						     client_layer;
> +
> +	/* For client domain, walk_and_check ensures the client domain is
> +	 * not scoped until gets to base_layer.
> +	 * For server_domain, it only ensures that the server domain exist.
> +	 */
> +	if (client_layer != server_layer) {
> +		if (client_layer > server_layer)
> +			is_scoped = walk_and_check(client, &client_walker,
> +						   server_layer, client_layer,
> +						   ipc_type);
> +		else
> +			is_scoped = walk_and_check(server, &server_walker,
> +						   client_layer, server_layer,
> +						   ipc_type & 0);
> +		if (!is_scoped)
> +			return false;
> +	}
> +	/* client and server are at the same level in hierarchy. If client is
> +	 * scoped, the server must be scoped in the same domain
> +	 */
> +	for (base_layer; base_layer >= 0; base_layer--) {
> +		if (landlock_get_scope_mask(client, base_layer) & ipc_type) {
> +			/* This check must be here since access would be denied only if
> +			 * the client is scoped and the server has no domain, so
> +			 * if the client has a domain but is not scoped and the server
> +			 * has no domain, access is guaranteed.
> +			 */
> +			if (!server)
> +				return false;
> +
> +			if (server_walker == client_walker)
> +				return true;
> +
> +			return false;
> +		}
> +		client_walker = client_walker->parent;
> +		server_walker = server_walker->parent;
> +		/* Warn if there is an incosistenncy between num_layers and
> +		 * landlock_hierarchy in each of rulesets
> +		 */
> +		if (WARN_ON_ONCE(base_layer > 0 &&
> +				 (!server_walker || !client_walker)))
> +			return false;
> +	}
> +	return true;
> +}
> +
> +static bool sock_is_scoped(struct sock *const other)
> +{
> +	const struct landlock_ruleset *dom_other;
> +	const struct landlock_ruleset *const dom =
> +		landlock_get_current_domain();
> +
> +	/* the credentials will not change */
> +	lockdep_assert_held(&unix_sk(other)->lock);
> +	dom_other = landlock_cred(other->sk_socket->file->f_cred)->domain;
> +	return domain_IPC_scope(dom, dom_other,
> +				LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET);
> +}
> +
> +static bool check_unix_address_format(struct sock *const sock)
> +{
> +	struct unix_address *addr = unix_sk(sock)->addr;
> +
> +	if (!addr)
> +		return true;
> +
> +	if (addr->len > sizeof(AF_UNIX)) {

AF_UNIX is the family, not a length.

I'm wondering if we cannot also check the "path" field or something
else.

> +		/* handling unspec sockets */
> +		if (!addr->name[0].sun_path)
> +			return true;
> +
> +		if (addr->name[0].sun_path[0] == '\0')
> +			if (!sock_is_scoped(sock))

This function should only check the address format.  sock_is_scoped()
can be called in each LSM hook.

> +				return false;
> +	}
> +
> +	return true;
> +}
> +
> +static int hook_unix_stream_connect(struct sock *const sock,
> +				    struct sock *const other,
> +				    struct sock *const newsk)
> +{

To not impact unsandboxed tasks, we need to first check if current is
sandboxed with Landlock, and then pass this dom to the following
functions.

> +	if (check_unix_address_format(other))
> +		return 0;
> +
> +	return -EPERM;
> +}
> +
> +static int hook_unix_may_send(struct socket *const sock,
> +			      struct socket *const other)
> +{
> +	if (check_unix_address_format(other->sk))
> +		return 0;
> +
> +	return -EPERM;
> +}
> +
>  static struct security_hook_list landlock_hooks[] __ro_after_init = {
>  	LSM_HOOK_INIT(ptrace_access_check, hook_ptrace_access_check),
>  	LSM_HOOK_INIT(ptrace_traceme, hook_ptrace_traceme),
> +	LSM_HOOK_INIT(unix_stream_connect, hook_unix_stream_connect),
> +	LSM_HOOK_INIT(unix_may_send, hook_unix_may_send),
>  };
>  
>  __init void landlock_add_task_hooks(void)
> -- 
> 2.34.1
> 
> 

Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction

[Mickaël Salaün]

On Thu, Aug 01, 2024 at 10:02:33PM -0600, Tahera Fahimi wrote:
> This patch introduces a new "scoped" attribute to the landlock_ruleset_attr
> that can specify "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to scope
> abstract Unix sockets from connecting to a process outside of
> the same landlock domain. It implements two hooks, unix_stream_connect
> and unix_may_send to enforce this restriction.
> 
> Closes: https://github.com/landlock-lsm/linux/issues/7
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> 
> ---
> v8:
> - Code refactoring (improve code readability, renaming variable, etc.) based
>   on reviews by Mickaël Salaün on version 7.
> - Adding warn_on_once to check (impossible) inconsistencies.
> - Adding inline comments.
> - Adding check_unix_address_format to check if the scoping socket is an abstract
>   unix sockets.
> v7:
>  - Using socket's file credentials for both connected(STREAM) and
>    non-connected(DGRAM) sockets.
>  - Adding "domain_sock_scope" instead of the domain scoping mechanism used in
>    ptrace ensures that if a server's domain is accessible from the client's
>    domain (where the client is more privileged than the server), the client
>    can connect to the server in all edge cases.
>  - Removing debug codes.
> v6:
>  - Removing curr_ruleset from landlock_hierarchy, and switching back to use
>    the same domain scoping as ptrace.
>  - code clean up.
> v5:
>  - Renaming "LANDLOCK_*_ACCESS_SCOPE" to "LANDLOCK_*_SCOPE"
>  - Adding curr_ruleset to hierarachy_ruleset structure to have access from
>    landlock_hierarchy to its respective landlock_ruleset.
>  - Using curr_ruleset to check if a domain is scoped while walking in the
>    hierarchy of domains.
>  - Modifying inline comments.
> V4:
>  - Rebased on Günther's Patch:
>    https://lore.kernel.org/all/20240610082115.1693267-1-gnoack@google.com/
>    so there is no need for "LANDLOCK_SHIFT_ACCESS_SCOPE", then it is removed.
>  - Adding get_scope_accesses function to check all scoped access masks in a ruleset.
>  - Using socket's file credentials instead of credentials stored in peer_cred
>    for datagram sockets. (see discussion in [1])
>  - Modifying inline comments.
> V3:
>  - Improving commit description.
>  - Introducing "scoped" attribute to landlock_ruleset_attr for IPC scoping
>    purpose, and adding related functions.
>  - Changing structure of ruleset based on "scoped".
>  - Removing rcu lock and using unix_sk lock instead.
>  - Introducing scoping for datagram sockets in unix_may_send.
> V2:
>  - Removing wrapper functions
> 
> [1]https://lore.kernel.org/all/20240610.Aifee5ingugh@digikod.net/
> ----
> ---
>  include/uapi/linux/landlock.h |  30 +++++++
>  security/landlock/limits.h    |   3 +
>  security/landlock/ruleset.c   |   7 +-
>  security/landlock/ruleset.h   |  23 ++++-
>  security/landlock/syscalls.c  |  14 ++-
>  security/landlock/task.c      | 155 ++++++++++++++++++++++++++++++++++
>  6 files changed, 225 insertions(+), 7 deletions(-)

> diff --git a/security/landlock/task.c b/security/landlock/task.c
> index 849f5123610b..7e8579ebae83 100644
> --- a/security/landlock/task.c
> +++ b/security/landlock/task.c
> @@ -13,6 +13,8 @@
>  #include <linux/lsm_hooks.h>
>  #include <linux/rcupdate.h>
>  #include <linux/sched.h>
> +#include <net/sock.h>
> +#include <net/af_unix.h>
>  
>  #include "common.h"
>  #include "cred.h"
> @@ -108,9 +110,162 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
>  	return task_ptrace(parent, current);
>  }
>  
> +static bool walk_and_check(const struct landlock_ruleset *const child,
> +			   struct landlock_hierarchy **walker,
> +			   size_t base_layer, size_t deep_layer,
> +			   access_mask_t check_scoping)

s/check_scoping/scope/

> +{
> +	if (!child || base_layer < 0 || !(*walker))

I guess it should be:
WARN_ON_ONCE(!child || base_layer < 0 || !(*walker))

> +		return false;
> +
> +	for (deep_layer; base_layer < deep_layer; deep_layer--) {

No need to pass deep_layer as argument:
deep_layer = child->num_layers - 1

> +		if (check_scoping & landlock_get_scope_mask(child, deep_layer))
> +			return false;
> +		*walker = (*walker)->parent;
> +		if (WARN_ON_ONCE(!*walker))
> +			/* there is an inconsistency between num_layers

Please use full sentences starting with a capital letter and ending with
a dot, and in this case start with "/*"

> +			 * and landlock_hierarchy in the ruleset
> +			 */
> +			return false;
> +	}
> +	return true;
> +}
> +
> +/**
> + * domain_IPC_scope - Checks if the client domain is scoped in the same
> + *		      domain as the server.

Actually, you can remove IPC from the function name.

> + *
> + * @client: IPC sender domain.
> + * @server: IPC receiver domain.
> + *
> + * Check if the @client domain is scoped to access the @server; the @server
> + * must be scoped in the same domain.

Returns true if...

> + */
> +static bool domain_IPC_scope(const struct landlock_ruleset *const client,
> +			     const struct landlock_ruleset *const server,
> +			     access_mask_t ipc_type)
> +{
> +	size_t client_layer, server_layer = 0;
> +	int base_layer;
> +	struct landlock_hierarchy *client_walker, *server_walker;
> +	bool is_scoped;
> +
> +	/* Quick return if client has no domain */
> +	if (!client)
> +		return true;
> +
> +	client_layer = client->num_layers - 1;
> +	client_walker = client->hierarchy;
> +	if (server) {
> +		server_layer = server->num_layers - 1;
> +		server_walker = server->hierarchy;
> +	}

} else {
	server_layer = 0;
	server_walker = NULL;
}

> +	base_layer = (client_layer > server_layer) ? server_layer :
> +						     client_layer;
> +
> +	/* For client domain, walk_and_check ensures the client domain is
> +	 * not scoped until gets to base_layer.

until gets?

> +	 * For server_domain, it only ensures that the server domain exist.
> +	 */
> +	if (client_layer != server_layer) {

bool is_scoped;

> +		if (client_layer > server_layer)
> +			is_scoped = walk_and_check(client, &client_walker,
> +						   server_layer, client_layer,
> +						   ipc_type);
> +		else

server_walker may be uninitialized and still read here, and maybe later
in the for loop.  The whole code should maks sure this cannot happen,
and a test case should check this.

> +			is_scoped = walk_and_check(server, &server_walker,
> +						   client_layer, server_layer,
> +						   ipc_type & 0);

"ipc_type & 0" is the same as "0"

> +		if (!is_scoped)

The name doesn't reflect the semantic. walk_and_check() should return
the inverse.

> +			return false;
> +	}

This code would be simpler:

if (client_layer > server_layer) {
	base_layer = server_layer;
	// TODO: inverse boolean logic
	if (!walk_and_check(client, &client_walker,
				   base_layer, ipc_type))
		return false;
} else (client_layer < server_layer) {
	base_layer = client_layer;
	// TODO: inverse boolean logic
	if (!walk_and_check(server, &server_walker,
				   base_layer, 0))
		return false;
} else {
	base_layer = client_layer;
}


I think we can improve more to make sure there is no path/risk of
inconsistent pointers.


> +	/* client and server are at the same level in hierarchy. If client is
> +	 * scoped, the server must be scoped in the same domain
> +	 */
> +	for (base_layer; base_layer >= 0; base_layer--) {
> +		if (landlock_get_scope_mask(client, base_layer) & ipc_type) {

With each multi-line comment, the first line should be empty:
/*
 * This check must be here since access would be denied only if

> +			/* This check must be here since access would be denied only if
> +			 * the client is scoped and the server has no domain, so
> +			 * if the client has a domain but is not scoped and the server
> +			 * has no domain, access is guaranteed.
> +			 */
> +			if (!server)
> +				return false;
> +
> +			if (server_walker == client_walker)
> +				return true;
> +
> +			return false;
> +		}
> +		client_walker = client_walker->parent;
> +		server_walker = server_walker->parent;
> +		/* Warn if there is an incosistenncy between num_layers and

Makes sure there is no inconsistency between num_layers and


> +		 * landlock_hierarchy in each of rulesets
> +		 */
> +		if (WARN_ON_ONCE(base_layer > 0 &&
> +				 (!server_walker || !client_walker)))
> +			return false;
> +	}
> +	return true;
> +}

Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction

[Mickaël Salaün]

On Sat, Aug 03, 2024 at 01:29:09PM +0200, Mickaël Salaün wrote:
> On Thu, Aug 01, 2024 at 10:02:33PM -0600, Tahera Fahimi wrote:
> > This patch introduces a new "scoped" attribute to the landlock_ruleset_attr
> > that can specify "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to scope
> > abstract Unix sockets from connecting to a process outside of
> > the same landlock domain. It implements two hooks, unix_stream_connect
> > and unix_may_send to enforce this restriction.
> > 
> > Closes: https://github.com/landlock-lsm/linux/issues/7
> > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> > 
> > ---
> > v8:
> > - Code refactoring (improve code readability, renaming variable, etc.) based
> >   on reviews by Mickaël Salaün on version 7.
> > - Adding warn_on_once to check (impossible) inconsistencies.
> > - Adding inline comments.
> > - Adding check_unix_address_format to check if the scoping socket is an abstract
> >   unix sockets.
> > v7:
> >  - Using socket's file credentials for both connected(STREAM) and
> >    non-connected(DGRAM) sockets.
> >  - Adding "domain_sock_scope" instead of the domain scoping mechanism used in
> >    ptrace ensures that if a server's domain is accessible from the client's
> >    domain (where the client is more privileged than the server), the client
> >    can connect to the server in all edge cases.
> >  - Removing debug codes.
> > v6:
> >  - Removing curr_ruleset from landlock_hierarchy, and switching back to use
> >    the same domain scoping as ptrace.
> >  - code clean up.
> > v5:
> >  - Renaming "LANDLOCK_*_ACCESS_SCOPE" to "LANDLOCK_*_SCOPE"
> >  - Adding curr_ruleset to hierarachy_ruleset structure to have access from
> >    landlock_hierarchy to its respective landlock_ruleset.
> >  - Using curr_ruleset to check if a domain is scoped while walking in the
> >    hierarchy of domains.
> >  - Modifying inline comments.
> > V4:
> >  - Rebased on Günther's Patch:
> >    https://lore.kernel.org/all/20240610082115.1693267-1-gnoack@google.com/
> >    so there is no need for "LANDLOCK_SHIFT_ACCESS_SCOPE", then it is removed.
> >  - Adding get_scope_accesses function to check all scoped access masks in a ruleset.
> >  - Using socket's file credentials instead of credentials stored in peer_cred
> >    for datagram sockets. (see discussion in [1])
> >  - Modifying inline comments.
> > V3:
> >  - Improving commit description.
> >  - Introducing "scoped" attribute to landlock_ruleset_attr for IPC scoping
> >    purpose, and adding related functions.
> >  - Changing structure of ruleset based on "scoped".
> >  - Removing rcu lock and using unix_sk lock instead.
> >  - Introducing scoping for datagram sockets in unix_may_send.
> > V2:
> >  - Removing wrapper functions
> > 
> > [1]https://lore.kernel.org/all/20240610.Aifee5ingugh@digikod.net/
> > ----
> > ---
> >  include/uapi/linux/landlock.h |  30 +++++++
> >  security/landlock/limits.h    |   3 +
> >  security/landlock/ruleset.c   |   7 +-
> >  security/landlock/ruleset.h   |  23 ++++-
> >  security/landlock/syscalls.c  |  14 ++-
> >  security/landlock/task.c      | 155 ++++++++++++++++++++++++++++++++++
> >  6 files changed, 225 insertions(+), 7 deletions(-)
> 
> > diff --git a/security/landlock/task.c b/security/landlock/task.c
> > index 849f5123610b..7e8579ebae83 100644
> > --- a/security/landlock/task.c
> > +++ b/security/landlock/task.c
> > @@ -13,6 +13,8 @@
> >  #include <linux/lsm_hooks.h>
> >  #include <linux/rcupdate.h>
> >  #include <linux/sched.h>
> > +#include <net/sock.h>
> > +#include <net/af_unix.h>
> >  
> >  #include "common.h"
> >  #include "cred.h"
> > @@ -108,9 +110,162 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
> >  	return task_ptrace(parent, current);
> >  }
> >  
> > +static bool walk_and_check(const struct landlock_ruleset *const child,
> > +			   struct landlock_hierarchy **walker,
> > +			   size_t base_layer, size_t deep_layer,
> > +			   access_mask_t check_scoping)
> 
> s/check_scoping/scope/
> 
> > +{
> > +	if (!child || base_layer < 0 || !(*walker))
> 
> I guess it should be:
> WARN_ON_ONCE(!child || base_layer < 0 || !(*walker))
> 
> > +		return false;
> > +
> > +	for (deep_layer; base_layer < deep_layer; deep_layer--) {
> 
> No need to pass deep_layer as argument:
> deep_layer = child->num_layers - 1
> 
> > +		if (check_scoping & landlock_get_scope_mask(child, deep_layer))
> > +			return false;
> > +		*walker = (*walker)->parent;
> > +		if (WARN_ON_ONCE(!*walker))
> > +			/* there is an inconsistency between num_layers
> 
> Please use full sentences starting with a capital letter and ending with
> a dot, and in this case start with "/*"
> 
> > +			 * and landlock_hierarchy in the ruleset
> > +			 */
> > +			return false;
> > +	}
> > +	return true;
> > +}
> > +
> > +/**
> > + * domain_IPC_scope - Checks if the client domain is scoped in the same
> > + *		      domain as the server.
> 
> Actually, you can remove IPC from the function name.
> 
> > + *
> > + * @client: IPC sender domain.
> > + * @server: IPC receiver domain.
> > + *
> > + * Check if the @client domain is scoped to access the @server; the @server
> > + * must be scoped in the same domain.
> 
> Returns true if...
> 
> > + */
> > +static bool domain_IPC_scope(const struct landlock_ruleset *const client,
> > +			     const struct landlock_ruleset *const server,
> > +			     access_mask_t ipc_type)
> > +{
> > +	size_t client_layer, server_layer = 0;
> > +	int base_layer;
> > +	struct landlock_hierarchy *client_walker, *server_walker;
> > +	bool is_scoped;
> > +
> > +	/* Quick return if client has no domain */
> > +	if (!client)
> > +		return true;
> > +
> > +	client_layer = client->num_layers - 1;
> > +	client_walker = client->hierarchy;
> > +	if (server) {
> > +		server_layer = server->num_layers - 1;
> > +		server_walker = server->hierarchy;
> > +	}
> 
> } else {
> 	server_layer = 0;
> 	server_walker = NULL;
> }
> 
> > +	base_layer = (client_layer > server_layer) ? server_layer :
> > +						     client_layer;
> > +
> > +	/* For client domain, walk_and_check ensures the client domain is
> > +	 * not scoped until gets to base_layer.
> 
> until gets?
> 
> > +	 * For server_domain, it only ensures that the server domain exist.
> > +	 */
> > +	if (client_layer != server_layer) {
> 
> bool is_scoped;
> 
> > +		if (client_layer > server_layer)
> > +			is_scoped = walk_and_check(client, &client_walker,
> > +						   server_layer, client_layer,
> > +						   ipc_type);
> > +		else
> 
> server_walker may be uninitialized and still read here, and maybe later
> in the for loop.  The whole code should maks sure this cannot happen,
> and a test case should check this.
> 
> > +			is_scoped = walk_and_check(server, &server_walker,
> > +						   client_layer, server_layer,
> > +						   ipc_type & 0);
> 
> "ipc_type & 0" is the same as "0"
> 
> > +		if (!is_scoped)
> 
> The name doesn't reflect the semantic. walk_and_check() should return
> the inverse.
> 
> > +			return false;
> > +	}
> 
> This code would be simpler:
> 
> if (client_layer > server_layer) {
> 	base_layer = server_layer;
> 	// TODO: inverse boolean logic
> 	if (!walk_and_check(client, &client_walker,
> 				   base_layer, ipc_type))
> 		return false;
> } else (client_layer < server_layer) {
> 	base_layer = client_layer;
> 	// TODO: inverse boolean logic
> 	if (!walk_and_check(server, &server_walker,
> 				   base_layer, 0))
> 		return false;
> } else {
> 	base_layer = client_layer;
> }
> 
> 
> I think we can improve more to make sure there is no path/risk of
> inconsistent pointers.
> 
> 
> > +	/* client and server are at the same level in hierarchy. If client is
> > +	 * scoped, the server must be scoped in the same domain
> > +	 */
> > +	for (base_layer; base_layer >= 0; base_layer--) {
> > +		if (landlock_get_scope_mask(client, base_layer) & ipc_type) {
> 
> With each multi-line comment, the first line should be empty:
> /*
>  * This check must be here since access would be denied only if
> 
> > +			/* This check must be here since access would be denied only if
> > +			 * the client is scoped and the server has no domain, so
> > +			 * if the client has a domain but is not scoped and the server
> > +			 * has no domain, access is guaranteed.
> > +			 */
> > +			if (!server)
> > +				return false;
> > +
> > +			if (server_walker == client_walker)
> > +				return true;
> > +
> > +			return false;
> > +		}
> > +		client_walker = client_walker->parent;
> > +		server_walker = server_walker->parent;
> > +		/* Warn if there is an incosistenncy between num_layers and
> 
> Makes sure there is no inconsistency between num_layers and
> 
> 
> > +		 * landlock_hierarchy in each of rulesets
> > +		 */
> > +		if (WARN_ON_ONCE(base_layer > 0 &&
> > +				 (!server_walker || !client_walker)))
> > +			return false;
> > +	}
> > +	return true;
> > +}

Here is a refactoring that is easier to read and avoid potential pointer
misuse:

static bool domain_is_scoped(const struct landlock_ruleset *const client,
			     const struct landlock_ruleset *const server,
			     access_mask_t scope)
{
	int client_layer, server_layer;
	struct landlock_hierarchy *client_walker, *server_walker;

	if (WARN_ON_ONCE(!client))
		return false;

	client_layer = client->num_layers - 1;
	client_walker = client->hierarchy;

	/*
	 * client_layer must be a signed integer with greater capacity than
	 * client->num_layers to ensure the following loop stops.
	 */
	BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));

	if (!server) {
		/*
		 * Walks client's parent domains and checks that none of these
		 * domains are scoped.
		 */
		for (; client_layer >= 0; client_layer--) {
			if (landlock_get_scope_mask(client, client_layer) &
			    scope)
				return true;
		}
		return false;
	}

	server_layer = server->num_layers - 1;
	server_walker = server->hierarchy;

	/*
	 * Walks client's parent domains down to the same hierarchy level as
	 * the server's domain, and checks that none of these client's parent
	 * domains are scoped.
	 */
	for (; client_layer > server_layer; client_layer--) {
		if (landlock_get_scope_mask(client, client_layer) & scope)
			return true;

		client_walker = client_walker->parent;
	}

	/*
	 * Walks server's parent domains down to the same hierarchy level as
	 * the client's domain.
	 */
	for (; server_layer > client_layer; server_layer--)
		server_walker = server_walker->parent;

	for (; client_layer >= 0; client_layer--) {
		if (landlock_get_scope_mask(client, client_layer) & scope) {
			/*
			 * Client and server are at the same level in the
			 * hierarchy.  If the client is scoped, the request is
			 * only allowed if this domain is also a server's
			 * ancestor.
			 */
			if (server_walker == client_walker)
				return false;

			return true;
		}
		client_walker = client_walker->parent;
		server_walker = server_walker->parent;
	}
	return false;
}

Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction

[Jann Horn]

On Tue, Aug 6, 2024 at 9:36 PM Mickaël Salaün <mic@digikod.net> wrote:
> On Sat, Aug 03, 2024 at 01:29:09PM +0200, Mickaël Salaün wrote:
> > On Thu, Aug 01, 2024 at 10:02:33PM -0600, Tahera Fahimi wrote:
> > > This patch introduces a new "scoped" attribute to the landlock_ruleset_attr
> > > that can specify "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to scope
> > > abstract Unix sockets from connecting to a process outside of
> > > the same landlock domain. It implements two hooks, unix_stream_connect
> > > and unix_may_send to enforce this restriction.
[...]
> Here is a refactoring that is easier to read and avoid potential pointer
> misuse:
>
> static bool domain_is_scoped(const struct landlock_ruleset *const client,
>                              const struct landlock_ruleset *const server,
>                              access_mask_t scope)
> {
>         int client_layer, server_layer;
>         struct landlock_hierarchy *client_walker, *server_walker;
>
>         if (WARN_ON_ONCE(!client))
>                 return false;
>
>         client_layer = client->num_layers - 1;
>         client_walker = client->hierarchy;
>
>         /*
>          * client_layer must be a signed integer with greater capacity than
>          * client->num_layers to ensure the following loop stops.
>          */
>         BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
>
>         if (!server) {
>                 /*
>                  * Walks client's parent domains and checks that none of these
>                  * domains are scoped.
>                  */
>                 for (; client_layer >= 0; client_layer--) {
>                         if (landlock_get_scope_mask(client, client_layer) &
>                             scope)
>                                 return true;
>                 }
>                 return false;
>         }
>
>         server_layer = server->num_layers - 1;
>         server_walker = server->hierarchy;
>
>         /*
>          * Walks client's parent domains down to the same hierarchy level as
>          * the server's domain, and checks that none of these client's parent
>          * domains are scoped.
>          */
>         for (; client_layer > server_layer; client_layer--) {
>                 if (landlock_get_scope_mask(client, client_layer) & scope)
>                         return true;
>
>                 client_walker = client_walker->parent;
>         }
>
>         /*
>          * Walks server's parent domains down to the same hierarchy level as
>          * the client's domain.
>          */
>         for (; server_layer > client_layer; server_layer--)
>                 server_walker = server_walker->parent;
>
>         for (; client_layer >= 0; client_layer--) {
>                 if (landlock_get_scope_mask(client, client_layer) & scope) {
>                         /*
>                          * Client and server are at the same level in the
>                          * hierarchy.  If the client is scoped, the request is
>                          * only allowed if this domain is also a server's
>                          * ancestor.
>                          */
>                         if (server_walker == client_walker)
>                                 return false;
>
>                         return true;
>                 }
>                 client_walker = client_walker->parent;
>                 server_walker = server_walker->parent;
>         }
>         return false;
> }

I think adding something like this change on top of your code would
make it more concise (though this is entirely untested):

--- /tmp/a      2024-08-06 22:37:33.800158308 +0200
+++ /tmp/b      2024-08-06 22:44:49.539314039 +0200
@@ -15,25 +15,12 @@
          * client_layer must be a signed integer with greater capacity than
          * client->num_layers to ensure the following loop stops.
          */
         BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));

-        if (!server) {
-                /*
-                 * Walks client's parent domains and checks that none of these
-                 * domains are scoped.
-                 */
-                for (; client_layer >= 0; client_layer--) {
-                        if (landlock_get_scope_mask(client, client_layer) &
-                            scope)
-                                return true;
-                }
-                return false;
-        }
-
-        server_layer = server->num_layers - 1;
-        server_walker = server->hierarchy;
+        server_layer = server ? (server->num_layers - 1) : -1;
+        server_walker = server ? server->hierarchy : NULL;

         /*
          * Walks client's parent domains down to the same hierarchy level as
          * the server's domain, and checks that none of these client's parent
          * domains are scoped.

Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction

[Mickaël Salaün]

On Tue, Aug 06, 2024 at 10:46:43PM +0200, Jann Horn wrote:
> On Tue, Aug 6, 2024 at 9:36 PM Mickaël Salaün <mic@digikod.net> wrote:
> > On Sat, Aug 03, 2024 at 01:29:09PM +0200, Mickaël Salaün wrote:
> > > On Thu, Aug 01, 2024 at 10:02:33PM -0600, Tahera Fahimi wrote:
> > > > This patch introduces a new "scoped" attribute to the landlock_ruleset_attr
> > > > that can specify "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to scope
> > > > abstract Unix sockets from connecting to a process outside of
> > > > the same landlock domain. It implements two hooks, unix_stream_connect
> > > > and unix_may_send to enforce this restriction.
> [...]
> > Here is a refactoring that is easier to read and avoid potential pointer
> > misuse:
> >
> > static bool domain_is_scoped(const struct landlock_ruleset *const client,
> >                              const struct landlock_ruleset *const server,
> >                              access_mask_t scope)
> > {
> >         int client_layer, server_layer;
> >         struct landlock_hierarchy *client_walker, *server_walker;
> >
> >         if (WARN_ON_ONCE(!client))
> >                 return false;
> >
> >         client_layer = client->num_layers - 1;
> >         client_walker = client->hierarchy;
> >
> >         /*
> >          * client_layer must be a signed integer with greater capacity than
> >          * client->num_layers to ensure the following loop stops.
> >          */
> >         BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
> >
> >         if (!server) {
> >                 /*
> >                  * Walks client's parent domains and checks that none of these
> >                  * domains are scoped.
> >                  */
> >                 for (; client_layer >= 0; client_layer--) {
> >                         if (landlock_get_scope_mask(client, client_layer) &
> >                             scope)
> >                                 return true;
> >                 }
> >                 return false;
> >         }
> >
> >         server_layer = server->num_layers - 1;
> >         server_walker = server->hierarchy;
> >
> >         /*
> >          * Walks client's parent domains down to the same hierarchy level as
> >          * the server's domain, and checks that none of these client's parent
> >          * domains are scoped.
> >          */
> >         for (; client_layer > server_layer; client_layer--) {
> >                 if (landlock_get_scope_mask(client, client_layer) & scope)
> >                         return true;
> >
> >                 client_walker = client_walker->parent;
> >         }
> >
> >         /*
> >          * Walks server's parent domains down to the same hierarchy level as
> >          * the client's domain.
> >          */
> >         for (; server_layer > client_layer; server_layer--)
> >                 server_walker = server_walker->parent;
> >
> >         for (; client_layer >= 0; client_layer--) {
> >                 if (landlock_get_scope_mask(client, client_layer) & scope) {
> >                         /*
> >                          * Client and server are at the same level in the
> >                          * hierarchy.  If the client is scoped, the request is
> >                          * only allowed if this domain is also a server's
> >                          * ancestor.
> >                          */

We can move this comment before the if and...

> >                         if (server_walker == client_walker)
> >                                 return false;
> >
> >                         return true;

...add this without the curly braces:

return server_walker != client_walker;

> >                 }
> >                 client_walker = client_walker->parent;
> >                 server_walker = server_walker->parent;
> >         }
> >         return false;
> > }
> 
> I think adding something like this change on top of your code would
> make it more concise (though this is entirely untested):
> 
> --- /tmp/a      2024-08-06 22:37:33.800158308 +0200
> +++ /tmp/b      2024-08-06 22:44:49.539314039 +0200
> @@ -15,25 +15,12 @@
>           * client_layer must be a signed integer with greater capacity than
>           * client->num_layers to ensure the following loop stops.
>           */
>          BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
> 
> -        if (!server) {
> -                /*
> -                 * Walks client's parent domains and checks that none of these
> -                 * domains are scoped.
> -                 */
> -                for (; client_layer >= 0; client_layer--) {
> -                        if (landlock_get_scope_mask(client, client_layer) &
> -                            scope)
> -                                return true;
> -                }
> -                return false;
> -        }

This loop is redundant with the following one, but it makes sure there
is no issue nor inconsistencies with the server or server_walker
pointers.  That's the only approach I found to make sure we don't go
through a path that could use an incorrect pointer, and makes the code
easy to review.

> -
> -        server_layer = server->num_layers - 1;
> -        server_walker = server->hierarchy;
> +        server_layer = server ? (server->num_layers - 1) : -1;
> +        server_walker = server ? server->hierarchy : NULL;

We would need to change the last loop to avoid a null pointer deref.

> 
>          /*
>           * Walks client's parent domains down to the same hierarchy level as
>           * the server's domain, and checks that none of these client's parent
>           * domains are scoped.
> 

Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction

[Jann Horn]

On Wed, Aug 7, 2024 at 9:21 AM Mickaël Salaün <mic@digikod.net> wrote:
> On Tue, Aug 06, 2024 at 10:46:43PM +0200, Jann Horn wrote:
> > I think adding something like this change on top of your code would
> > make it more concise (though this is entirely untested):
> >
> > --- /tmp/a      2024-08-06 22:37:33.800158308 +0200
> > +++ /tmp/b      2024-08-06 22:44:49.539314039 +0200
> > @@ -15,25 +15,12 @@
> >           * client_layer must be a signed integer with greater capacity than
> >           * client->num_layers to ensure the following loop stops.
> >           */
> >          BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
> >
> > -        if (!server) {
> > -                /*
> > -                 * Walks client's parent domains and checks that none of these
> > -                 * domains are scoped.
> > -                 */
> > -                for (; client_layer >= 0; client_layer--) {
> > -                        if (landlock_get_scope_mask(client, client_layer) &
> > -                            scope)
> > -                                return true;
> > -                }
> > -                return false;
> > -        }
>
> This loop is redundant with the following one, but it makes sure there
> is no issue nor inconsistencies with the server or server_walker
> pointers.  That's the only approach I found to make sure we don't go
> through a path that could use an incorrect pointer, and makes the code
> easy to review.

My view is that this is a duplication of logic for one particular
special case - after all, you can also end up walking up to the same
state (client_layer==-1, server_layer==-1, client_walker==NULL,
server_walker==NULL) with the loop at the bottom.

But I guess my preference for more concise code is kinda subjective -
if you prefer the more verbose version, I'm fine with that too.

> > -
> > -        server_layer = server->num_layers - 1;
> > -        server_walker = server->hierarchy;
> > +        server_layer = server ? (server->num_layers - 1) : -1;
> > +        server_walker = server ? server->hierarchy : NULL;
>
> We would need to change the last loop to avoid a null pointer deref.

Why? The first loop would either exit or walk the client_walker up
until client_layer is -1 and client_walker is NULL; the second loop
wouldn't do anything because the walkers are at the same layer; the
third loop's body wouldn't be executed because client_layer is -1.

The case where the server is not in any Landlock domain is just one
subcase of the more general case "client and server do not have a
common ancestor domain".

> >
> >          /*
> >           * Walks client's parent domains down to the same hierarchy level as
> >           * the server's domain, and checks that none of these client's parent
> >           * domains are scoped.
> >

Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction

[Mickaël Salaün]

On Wed, Aug 07, 2024 at 03:45:18PM +0200, Jann Horn wrote:
> On Wed, Aug 7, 2024 at 9:21 AM Mickaël Salaün <mic@digikod.net> wrote:
> > On Tue, Aug 06, 2024 at 10:46:43PM +0200, Jann Horn wrote:
> > > I think adding something like this change on top of your code would
> > > make it more concise (though this is entirely untested):
> > >
> > > --- /tmp/a      2024-08-06 22:37:33.800158308 +0200
> > > +++ /tmp/b      2024-08-06 22:44:49.539314039 +0200
> > > @@ -15,25 +15,12 @@
> > >           * client_layer must be a signed integer with greater capacity than
> > >           * client->num_layers to ensure the following loop stops.
> > >           */
> > >          BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
> > >
> > > -        if (!server) {
> > > -                /*
> > > -                 * Walks client's parent domains and checks that none of these
> > > -                 * domains are scoped.
> > > -                 */
> > > -                for (; client_layer >= 0; client_layer--) {
> > > -                        if (landlock_get_scope_mask(client, client_layer) &
> > > -                            scope)
> > > -                                return true;
> > > -                }
> > > -                return false;
> > > -        }
> >
> > This loop is redundant with the following one, but it makes sure there
> > is no issue nor inconsistencies with the server or server_walker
> > pointers.  That's the only approach I found to make sure we don't go
> > through a path that could use an incorrect pointer, and makes the code
> > easy to review.
> 
> My view is that this is a duplication of logic for one particular
> special case - after all, you can also end up walking up to the same
> state (client_layer==-1, server_layer==-1, client_walker==NULL,
> server_walker==NULL) with the loop at the bottom.

Indeed

> 
> But I guess my preference for more concise code is kinda subjective -
> if you prefer the more verbose version, I'm fine with that too.
> 
> > > -
> > > -        server_layer = server->num_layers - 1;
> > > -        server_walker = server->hierarchy;
> > > +        server_layer = server ? (server->num_layers - 1) : -1;
> > > +        server_walker = server ? server->hierarchy : NULL;
> >
> > We would need to change the last loop to avoid a null pointer deref.
> 
> Why? The first loop would either exit or walk the client_walker up
> until client_layer is -1 and client_walker is NULL; the second loop
> wouldn't do anything because the walkers are at the same layer; the
> third loop's body wouldn't be executed because client_layer is -1.

Correct, I missed that client_layer would always be greater than
server_layer (-1).

Tahera, could you please take Jann's proposal?


> 
> The case where the server is not in any Landlock domain is just one
> subcase of the more general case "client and server do not have a
> common ancestor domain".
> 
> > >
> > >          /*
> > >           * Walks client's parent domains down to the same hierarchy level as
> > >           * the server's domain, and checks that none of these client's parent
> > >           * domains are scoped.
> > >
> 

Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction

[Tahera Fahimi]

On Wed, Aug 07, 2024 at 04:44:36PM +0200, Mickaël Salaün wrote:
> On Wed, Aug 07, 2024 at 03:45:18PM +0200, Jann Horn wrote:
> > On Wed, Aug 7, 2024 at 9:21 AM Mickaël Salaün <mic@digikod.net> wrote:
> > > On Tue, Aug 06, 2024 at 10:46:43PM +0200, Jann Horn wrote:
> > > > I think adding something like this change on top of your code would
> > > > make it more concise (though this is entirely untested):
> > > >
> > > > --- /tmp/a      2024-08-06 22:37:33.800158308 +0200
> > > > +++ /tmp/b      2024-08-06 22:44:49.539314039 +0200
> > > > @@ -15,25 +15,12 @@
> > > >           * client_layer must be a signed integer with greater capacity than
> > > >           * client->num_layers to ensure the following loop stops.
> > > >           */
> > > >          BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
> > > >
> > > > -        if (!server) {
> > > > -                /*
> > > > -                 * Walks client's parent domains and checks that none of these
> > > > -                 * domains are scoped.
> > > > -                 */
> > > > -                for (; client_layer >= 0; client_layer--) {
> > > > -                        if (landlock_get_scope_mask(client, client_layer) &
> > > > -                            scope)
> > > > -                                return true;
> > > > -                }
> > > > -                return false;
> > > > -        }
> > >
> > > This loop is redundant with the following one, but it makes sure there
> > > is no issue nor inconsistencies with the server or server_walker
> > > pointers.  That's the only approach I found to make sure we don't go
> > > through a path that could use an incorrect pointer, and makes the code
> > > easy to review.
> > 
> > My view is that this is a duplication of logic for one particular
> > special case - after all, you can also end up walking up to the same
> > state (client_layer==-1, server_layer==-1, client_walker==NULL,
> > server_walker==NULL) with the loop at the bottom.
> 
> Indeed
> 
> > 
> > But I guess my preference for more concise code is kinda subjective -
> > if you prefer the more verbose version, I'm fine with that too.
> > 
> > > > -
> > > > -        server_layer = server->num_layers - 1;
> > > > -        server_walker = server->hierarchy;
> > > > +        server_layer = server ? (server->num_layers - 1) : -1;
> > > > +        server_walker = server ? server->hierarchy : NULL;
> > >
> > > We would need to change the last loop to avoid a null pointer deref.
> > 
> > Why? The first loop would either exit or walk the client_walker up
> > until client_layer is -1 and client_walker is NULL; the second loop
> > wouldn't do anything because the walkers are at the same layer; the
> > third loop's body wouldn't be executed because client_layer is -1.
> 
> Correct, I missed that client_layer would always be greater than
> server_layer (-1).
> 
> Tahera, could you please take Jann's proposal?
Done.
We will have duplicate logic, but it would be easier to read and review.
> 
> > 
> > The case where the server is not in any Landlock domain is just one
> > subcase of the more general case "client and server do not have a
> > common ancestor domain".
> > 
> > > >
> > > >          /*
> > > >           * Walks client's parent domains down to the same hierarchy level as
> > > >           * the server's domain, and checks that none of these client's parent
> > > >           * domains are scoped.
> > > >
> > 

Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction

[Mickaël Salaün]

On Thu, Aug 08, 2024 at 05:17:10PM -0600, Tahera Fahimi wrote:
> On Wed, Aug 07, 2024 at 04:44:36PM +0200, Mickaël Salaün wrote:
> > On Wed, Aug 07, 2024 at 03:45:18PM +0200, Jann Horn wrote:
> > > On Wed, Aug 7, 2024 at 9:21 AM Mickaël Salaün <mic@digikod.net> wrote:
> > > > On Tue, Aug 06, 2024 at 10:46:43PM +0200, Jann Horn wrote:
> > > > > I think adding something like this change on top of your code would
> > > > > make it more concise (though this is entirely untested):
> > > > >
> > > > > --- /tmp/a      2024-08-06 22:37:33.800158308 +0200
> > > > > +++ /tmp/b      2024-08-06 22:44:49.539314039 +0200
> > > > > @@ -15,25 +15,12 @@
> > > > >           * client_layer must be a signed integer with greater capacity than
> > > > >           * client->num_layers to ensure the following loop stops.
> > > > >           */
> > > > >          BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
> > > > >
> > > > > -        if (!server) {
> > > > > -                /*
> > > > > -                 * Walks client's parent domains and checks that none of these
> > > > > -                 * domains are scoped.
> > > > > -                 */
> > > > > -                for (; client_layer >= 0; client_layer--) {
> > > > > -                        if (landlock_get_scope_mask(client, client_layer) &
> > > > > -                            scope)
> > > > > -                                return true;
> > > > > -                }
> > > > > -                return false;
> > > > > -        }
> > > >
> > > > This loop is redundant with the following one, but it makes sure there
> > > > is no issue nor inconsistencies with the server or server_walker
> > > > pointers.  That's the only approach I found to make sure we don't go
> > > > through a path that could use an incorrect pointer, and makes the code
> > > > easy to review.
> > > 
> > > My view is that this is a duplication of logic for one particular
> > > special case - after all, you can also end up walking up to the same
> > > state (client_layer==-1, server_layer==-1, client_walker==NULL,
> > > server_walker==NULL) with the loop at the bottom.
> > 
> > Indeed
> > 
> > > 
> > > But I guess my preference for more concise code is kinda subjective -
> > > if you prefer the more verbose version, I'm fine with that too.
> > > 
> > > > > -
> > > > > -        server_layer = server->num_layers - 1;
> > > > > -        server_walker = server->hierarchy;
> > > > > +        server_layer = server ? (server->num_layers - 1) : -1;
> > > > > +        server_walker = server ? server->hierarchy : NULL;
> > > >
> > > > We would need to change the last loop to avoid a null pointer deref.
> > > 
> > > Why? The first loop would either exit or walk the client_walker up
> > > until client_layer is -1 and client_walker is NULL; the second loop
> > > wouldn't do anything because the walkers are at the same layer; the
> > > third loop's body wouldn't be executed because client_layer is -1.
> > 
> > Correct, I missed that client_layer would always be greater than
> > server_layer (-1).
> > 
> > Tahera, could you please take Jann's proposal?
> Done.
> We will have duplicate logic, but it would be easier to read and review.

With Jann's proposal we don't have duplicate logic.

> > 
> > > 
> > > The case where the server is not in any Landlock domain is just one
> > > subcase of the more general case "client and server do not have a
> > > common ancestor domain".
> > > 
> > > > >
> > > > >          /*
> > > > >           * Walks client's parent domains down to the same hierarchy level as
> > > > >           * the server's domain, and checks that none of these client's parent
> > > > >           * domains are scoped.
> > > > >
> > > 
> 

Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction

[Tahera Fahimi]

On Fri, Aug 09, 2024 at 10:49:17AM +0200, Mickaël Salaün wrote:
> On Thu, Aug 08, 2024 at 05:17:10PM -0600, Tahera Fahimi wrote:
> > On Wed, Aug 07, 2024 at 04:44:36PM +0200, Mickaël Salaün wrote:
> > > On Wed, Aug 07, 2024 at 03:45:18PM +0200, Jann Horn wrote:
> > > > On Wed, Aug 7, 2024 at 9:21 AM Mickaël Salaün <mic@digikod.net> wrote:
> > > > > On Tue, Aug 06, 2024 at 10:46:43PM +0200, Jann Horn wrote:
> > > > > > I think adding something like this change on top of your code would
> > > > > > make it more concise (though this is entirely untested):
> > > > > >
> > > > > > --- /tmp/a      2024-08-06 22:37:33.800158308 +0200
> > > > > > +++ /tmp/b      2024-08-06 22:44:49.539314039 +0200
> > > > > > @@ -15,25 +15,12 @@
> > > > > >           * client_layer must be a signed integer with greater capacity than
> > > > > >           * client->num_layers to ensure the following loop stops.
> > > > > >           */
> > > > > >          BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
> > > > > >
> > > > > > -        if (!server) {
> > > > > > -                /*
> > > > > > -                 * Walks client's parent domains and checks that none of these
> > > > > > -                 * domains are scoped.
> > > > > > -                 */
> > > > > > -                for (; client_layer >= 0; client_layer--) {
> > > > > > -                        if (landlock_get_scope_mask(client, client_layer) &
> > > > > > -                            scope)
> > > > > > -                                return true;
> > > > > > -                }
> > > > > > -                return false;
> > > > > > -        }
> > > > >
> > > > > This loop is redundant with the following one, but it makes sure there
> > > > > is no issue nor inconsistencies with the server or server_walker
> > > > > pointers.  That's the only approach I found to make sure we don't go
> > > > > through a path that could use an incorrect pointer, and makes the code
> > > > > easy to review.
> > > > 
> > > > My view is that this is a duplication of logic for one particular
> > > > special case - after all, you can also end up walking up to the same
> > > > state (client_layer==-1, server_layer==-1, client_walker==NULL,
> > > > server_walker==NULL) with the loop at the bottom.
> > > 
> > > Indeed
> > > 
> > > > 
> > > > But I guess my preference for more concise code is kinda subjective -
> > > > if you prefer the more verbose version, I'm fine with that too.
> > > > 
> > > > > > -
> > > > > > -        server_layer = server->num_layers - 1;
> > > > > > -        server_walker = server->hierarchy;
> > > > > > +        server_layer = server ? (server->num_layers - 1) : -1;
> > > > > > +        server_walker = server ? server->hierarchy : NULL;
> > > > >
> > > > > We would need to change the last loop to avoid a null pointer deref.
> > > > 
> > > > Why? The first loop would either exit or walk the client_walker up
> > > > until client_layer is -1 and client_walker is NULL; the second loop
> > > > wouldn't do anything because the walkers are at the same layer; the
> > > > third loop's body wouldn't be executed because client_layer is -1.
> > > 
> > > Correct, I missed that client_layer would always be greater than
> > > server_layer (-1).
> > > 
> > > Tahera, could you please take Jann's proposal?
> > Done.
> > We will have duplicate logic, but it would be easier to read and review.
> 
> With Jann's proposal we don't have duplicate logic.
Still the first two for loops apply the same logic for client and server
domains, but I totally understand that it is much easier to review and
understand.
> > > 
> > > > 
> > > > The case where the server is not in any Landlock domain is just one
> > > > subcase of the more general case "client and server do not have a
> > > > common ancestor domain".
> > > > 
> > > > > >
> > > > > >          /*
> > > > > >           * Walks client's parent domains down to the same hierarchy level as
> > > > > >           * the server's domain, and checks that none of these client's parent
> > > > > >           * domains are scoped.
> > > > > >
> > > > 
> > 

Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction

[Tahera Fahimi]

On Sat, Aug 03, 2024 at 01:29:04PM +0200, Mickaël Salaün wrote:
> On Thu, Aug 01, 2024 at 10:02:33PM -0600, Tahera Fahimi wrote:
> > This patch introduces a new "scoped" attribute to the landlock_ruleset_attr
> > that can specify "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to scope
> > abstract Unix sockets from connecting to a process outside of
> > the same landlock domain. It implements two hooks, unix_stream_connect
> > and unix_may_send to enforce this restriction.
> > 
> > Closes: https://github.com/landlock-lsm/linux/issues/7
> > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> > 
> > ---
> > v8:
> > - Code refactoring (improve code readability, renaming variable, etc.) based
> >   on reviews by Mickaël Salaün on version 7.
> > - Adding warn_on_once to check (impossible) inconsistencies.
> > - Adding inline comments.
> > - Adding check_unix_address_format to check if the scoping socket is an abstract
> >   unix sockets.
> > v7:
> >  - Using socket's file credentials for both connected(STREAM) and
> >    non-connected(DGRAM) sockets.
> >  - Adding "domain_sock_scope" instead of the domain scoping mechanism used in
> >    ptrace ensures that if a server's domain is accessible from the client's
> >    domain (where the client is more privileged than the server), the client
> >    can connect to the server in all edge cases.
> >  - Removing debug codes.
> > v6:
> >  - Removing curr_ruleset from landlock_hierarchy, and switching back to use
> >    the same domain scoping as ptrace.
> >  - code clean up.
> > v5:
> >  - Renaming "LANDLOCK_*_ACCESS_SCOPE" to "LANDLOCK_*_SCOPE"
> >  - Adding curr_ruleset to hierarachy_ruleset structure to have access from
> >    landlock_hierarchy to its respective landlock_ruleset.
> >  - Using curr_ruleset to check if a domain is scoped while walking in the
> >    hierarchy of domains.
> >  - Modifying inline comments.
> > V4:
> >  - Rebased on Günther's Patch:
> >    https://lore.kernel.org/all/20240610082115.1693267-1-gnoack@google.com/
> >    so there is no need for "LANDLOCK_SHIFT_ACCESS_SCOPE", then it is removed.
> >  - Adding get_scope_accesses function to check all scoped access masks in a ruleset.
> >  - Using socket's file credentials instead of credentials stored in peer_cred
> >    for datagram sockets. (see discussion in [1])
> >  - Modifying inline comments.
> > V3:
> >  - Improving commit description.
> >  - Introducing "scoped" attribute to landlock_ruleset_attr for IPC scoping
> >    purpose, and adding related functions.
> >  - Changing structure of ruleset based on "scoped".
> >  - Removing rcu lock and using unix_sk lock instead.
> >  - Introducing scoping for datagram sockets in unix_may_send.
> > V2:
> >  - Removing wrapper functions
> > 
> > [1]https://lore.kernel.org/all/20240610.Aifee5ingugh@digikod.net/
> > ----
> > ---
> >  include/uapi/linux/landlock.h |  30 +++++++
> >  security/landlock/limits.h    |   3 +
> >  security/landlock/ruleset.c   |   7 +-
> >  security/landlock/ruleset.h   |  23 ++++-
> >  security/landlock/syscalls.c  |  14 ++-
> >  security/landlock/task.c      | 155 ++++++++++++++++++++++++++++++++++
> >  6 files changed, 225 insertions(+), 7 deletions(-)
> 
> > diff --git a/security/landlock/task.c b/security/landlock/task.c
> > index 849f5123610b..7e8579ebae83 100644
> > --- a/security/landlock/task.c
> > +++ b/security/landlock/task.c
> > @@ -13,6 +13,8 @@
> >  #include <linux/lsm_hooks.h>
> >  #include <linux/rcupdate.h>
> >  #include <linux/sched.h>
> > +#include <net/sock.h>
> > +#include <net/af_unix.h>
> >  
> >  #include "common.h"
> >  #include "cred.h"
> > @@ -108,9 +110,162 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
> >  	return task_ptrace(parent, current);
> >  }
> >  
> > +static bool walk_and_check(const struct landlock_ruleset *const child,
> > +			   struct landlock_hierarchy **walker,
> > +			   size_t base_layer, size_t deep_layer,
> > +			   access_mask_t check_scoping)
> 
> s/check_scoping/scope/
> 
> > +{
> > +	if (!child || base_layer < 0 || !(*walker))
> 
> I guess it should be:
> WARN_ON_ONCE(!child || base_layer < 0 || !(*walker))
> 
> > +		return false;
> > +
> > +	for (deep_layer; base_layer < deep_layer; deep_layer--) {
> 
> No need to pass deep_layer as argument:
> deep_layer = child->num_layers - 1
> 
> > +		if (check_scoping & landlock_get_scope_mask(child, deep_layer))
> > +			return false;
> > +		*walker = (*walker)->parent;
> > +		if (WARN_ON_ONCE(!*walker))
> > +			/* there is an inconsistency between num_layers
> 
> Please use full sentences starting with a capital letter and ending with
> a dot, and in this case start with "/*"
> 
> > +			 * and landlock_hierarchy in the ruleset
> > +			 */
> > +			return false;
> > +	}
> > +	return true;
> > +}
> > +
> > +/**
> > + * domain_IPC_scope - Checks if the client domain is scoped in the same
> > + *		      domain as the server.
> 
> Actually, you can remove IPC from the function name.
> 
> > + *
> > + * @client: IPC sender domain.
> > + * @server: IPC receiver domain.
> > + *
> > + * Check if the @client domain is scoped to access the @server; the @server
> > + * must be scoped in the same domain.
> 
> Returns true if...
> 
> > + */
> > +static bool domain_IPC_scope(const struct landlock_ruleset *const client,
> > +			     const struct landlock_ruleset *const server,
> > +			     access_mask_t ipc_type)
> > +{
> > +	size_t client_layer, server_layer = 0;
> > +	int base_layer;
> > +	struct landlock_hierarchy *client_walker, *server_walker;
> > +	bool is_scoped;
> > +
> > +	/* Quick return if client has no domain */
> > +	if (!client)
> > +		return true;
> > +
> > +	client_layer = client->num_layers - 1;
> > +	client_walker = client->hierarchy;
> > +	if (server) {
> > +		server_layer = server->num_layers - 1;
> > +		server_walker = server->hierarchy;
> > +	}
> 
> } else {
> 	server_layer = 0;
> 	server_walker = NULL;
> }
> 
> > +	base_layer = (client_layer > server_layer) ? server_layer :
> > +						     client_layer;
> > +
> > +	/* For client domain, walk_and_check ensures the client domain is
> > +	 * not scoped until gets to base_layer.
> 
> until gets?
> 
> > +	 * For server_domain, it only ensures that the server domain exist.
> > +	 */
> > +	if (client_layer != server_layer) {
> 
> bool is_scoped;
It is defined above. 
> > +		if (client_layer > server_layer)
> > +			is_scoped = walk_and_check(client, &client_walker,
> > +						   server_layer, client_layer,
> > +						   ipc_type);
> > +		else
> 
> server_walker may be uninitialized and still read here, and maybe later
> in the for loop.  The whole code should maks sure this cannot happen,
> and a test case should check this.
I think this case never happens, since the server_walker can be read
here if there are more than one layers in server domain which means that
the server_walker is not unintialized.

> > +			is_scoped = walk_and_check(server, &server_walker,
> > +						   client_layer, server_layer,
> > +						   ipc_type & 0);
> 
> "ipc_type & 0" is the same as "0"
> 
> > +		if (!is_scoped)
> 
> The name doesn't reflect the semantic. walk_and_check() should return
> the inverse.
> 
> > +			return false;
> > +	}
> 
> This code would be simpler:
> 
> if (client_layer > server_layer) {
> 	base_layer = server_layer;
> 	// TODO: inverse boolean logic
> 	if (!walk_and_check(client, &client_walker,
> 				   base_layer, ipc_type))
> 		return false;
> } else (client_layer < server_layer) {
> 	base_layer = client_layer;
> 	// TODO: inverse boolean logic
> 	if (!walk_and_check(server, &server_walker,
> 				   base_layer, 0))
> 		return false;
> } else {
> 	base_layer = client_layer;
> }
> 
> 
> I think we can improve more to make sure there is no path/risk of
> inconsistent pointers.
> 
> 
> > +	/* client and server are at the same level in hierarchy. If client is
> > +	 * scoped, the server must be scoped in the same domain
> > +	 */
> > +	for (base_layer; base_layer >= 0; base_layer--) {
> > +		if (landlock_get_scope_mask(client, base_layer) & ipc_type) {
> 
> With each multi-line comment, the first line should be empty:
> /*
>  * This check must be here since access would be denied only if
> 
> > +			/* This check must be here since access would be denied only if
> > +			 * the client is scoped and the server has no domain, so
> > +			 * if the client has a domain but is not scoped and the server
> > +			 * has no domain, access is guaranteed.
> > +			 */
> > +			if (!server)
> > +				return false;
> > +
> > +			if (server_walker == client_walker)
> > +				return true;
> > +
> > +			return false;
> > +		}
> > +		client_walker = client_walker->parent;
> > +		server_walker = server_walker->parent;
> > +		/* Warn if there is an incosistenncy between num_layers and
> 
> Makes sure there is no inconsistency between num_layers and
> 
> 
> > +		 * landlock_hierarchy in each of rulesets
> > +		 */
> > +		if (WARN_ON_ONCE(base_layer > 0 &&
> > +				 (!server_walker || !client_walker)))
> > +			return false;
> > +	}
> > +	return true;
> > +}

Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction

[Mickaël Salaün]

On Wed, Aug 07, 2024 at 09:37:15AM -0600, Tahera Fahimi wrote:
> On Sat, Aug 03, 2024 at 01:29:04PM +0200, Mickaël Salaün wrote:
> > On Thu, Aug 01, 2024 at 10:02:33PM -0600, Tahera Fahimi wrote:
> > > This patch introduces a new "scoped" attribute to the landlock_ruleset_attr
> > > that can specify "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to scope
> > > abstract Unix sockets from connecting to a process outside of
> > > the same landlock domain. It implements two hooks, unix_stream_connect
> > > and unix_may_send to enforce this restriction.
> > > 
> > > Closes: https://github.com/landlock-lsm/linux/issues/7
> > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> > > 
> > > ---
> > > v8:
> > > - Code refactoring (improve code readability, renaming variable, etc.) based
> > >   on reviews by Mickaël Salaün on version 7.
> > > - Adding warn_on_once to check (impossible) inconsistencies.
> > > - Adding inline comments.
> > > - Adding check_unix_address_format to check if the scoping socket is an abstract
> > >   unix sockets.
> > > v7:
> > >  - Using socket's file credentials for both connected(STREAM) and
> > >    non-connected(DGRAM) sockets.
> > >  - Adding "domain_sock_scope" instead of the domain scoping mechanism used in
> > >    ptrace ensures that if a server's domain is accessible from the client's
> > >    domain (where the client is more privileged than the server), the client
> > >    can connect to the server in all edge cases.
> > >  - Removing debug codes.
> > > v6:
> > >  - Removing curr_ruleset from landlock_hierarchy, and switching back to use
> > >    the same domain scoping as ptrace.
> > >  - code clean up.
> > > v5:
> > >  - Renaming "LANDLOCK_*_ACCESS_SCOPE" to "LANDLOCK_*_SCOPE"
> > >  - Adding curr_ruleset to hierarachy_ruleset structure to have access from
> > >    landlock_hierarchy to its respective landlock_ruleset.
> > >  - Using curr_ruleset to check if a domain is scoped while walking in the
> > >    hierarchy of domains.
> > >  - Modifying inline comments.
> > > V4:
> > >  - Rebased on Günther's Patch:
> > >    https://lore.kernel.org/all/20240610082115.1693267-1-gnoack@google.com/
> > >    so there is no need for "LANDLOCK_SHIFT_ACCESS_SCOPE", then it is removed.
> > >  - Adding get_scope_accesses function to check all scoped access masks in a ruleset.
> > >  - Using socket's file credentials instead of credentials stored in peer_cred
> > >    for datagram sockets. (see discussion in [1])
> > >  - Modifying inline comments.
> > > V3:
> > >  - Improving commit description.
> > >  - Introducing "scoped" attribute to landlock_ruleset_attr for IPC scoping
> > >    purpose, and adding related functions.
> > >  - Changing structure of ruleset based on "scoped".
> > >  - Removing rcu lock and using unix_sk lock instead.
> > >  - Introducing scoping for datagram sockets in unix_may_send.
> > > V2:
> > >  - Removing wrapper functions
> > > 
> > > [1]https://lore.kernel.org/all/20240610.Aifee5ingugh@digikod.net/
> > > ----
> > > ---
> > >  include/uapi/linux/landlock.h |  30 +++++++
> > >  security/landlock/limits.h    |   3 +
> > >  security/landlock/ruleset.c   |   7 +-
> > >  security/landlock/ruleset.h   |  23 ++++-
> > >  security/landlock/syscalls.c  |  14 ++-
> > >  security/landlock/task.c      | 155 ++++++++++++++++++++++++++++++++++
> > >  6 files changed, 225 insertions(+), 7 deletions(-)
> > 
> > > diff --git a/security/landlock/task.c b/security/landlock/task.c
> > > index 849f5123610b..7e8579ebae83 100644
> > > --- a/security/landlock/task.c
> > > +++ b/security/landlock/task.c
> > > @@ -13,6 +13,8 @@
> > >  #include <linux/lsm_hooks.h>
> > >  #include <linux/rcupdate.h>
> > >  #include <linux/sched.h>
> > > +#include <net/sock.h>
> > > +#include <net/af_unix.h>
> > >  
> > >  #include "common.h"
> > >  #include "cred.h"
> > > @@ -108,9 +110,162 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
> > >  	return task_ptrace(parent, current);
> > >  }
> > >  
> > > +static bool walk_and_check(const struct landlock_ruleset *const child,
> > > +			   struct landlock_hierarchy **walker,
> > > +			   size_t base_layer, size_t deep_layer,
> > > +			   access_mask_t check_scoping)
> > 
> > s/check_scoping/scope/
> > 
> > > +{
> > > +	if (!child || base_layer < 0 || !(*walker))
> > 
> > I guess it should be:
> > WARN_ON_ONCE(!child || base_layer < 0 || !(*walker))
> > 
> > > +		return false;
> > > +
> > > +	for (deep_layer; base_layer < deep_layer; deep_layer--) {
> > 
> > No need to pass deep_layer as argument:
> > deep_layer = child->num_layers - 1
> > 
> > > +		if (check_scoping & landlock_get_scope_mask(child, deep_layer))
> > > +			return false;
> > > +		*walker = (*walker)->parent;
> > > +		if (WARN_ON_ONCE(!*walker))
> > > +			/* there is an inconsistency between num_layers
> > 
> > Please use full sentences starting with a capital letter and ending with
> > a dot, and in this case start with "/*"
> > 
> > > +			 * and landlock_hierarchy in the ruleset
> > > +			 */
> > > +			return false;
> > > +	}
> > > +	return true;
> > > +}
> > > +
> > > +/**
> > > + * domain_IPC_scope - Checks if the client domain is scoped in the same
> > > + *		      domain as the server.
> > 
> > Actually, you can remove IPC from the function name.
> > 
> > > + *
> > > + * @client: IPC sender domain.
> > > + * @server: IPC receiver domain.
> > > + *
> > > + * Check if the @client domain is scoped to access the @server; the @server
> > > + * must be scoped in the same domain.
> > 
> > Returns true if...
> > 
> > > + */
> > > +static bool domain_IPC_scope(const struct landlock_ruleset *const client,
> > > +			     const struct landlock_ruleset *const server,
> > > +			     access_mask_t ipc_type)
> > > +{
> > > +	size_t client_layer, server_layer = 0;
> > > +	int base_layer;
> > > +	struct landlock_hierarchy *client_walker, *server_walker;
> > > +	bool is_scoped;
> > > +
> > > +	/* Quick return if client has no domain */
> > > +	if (!client)
> > > +		return true;
> > > +
> > > +	client_layer = client->num_layers - 1;
> > > +	client_walker = client->hierarchy;
> > > +	if (server) {
> > > +		server_layer = server->num_layers - 1;
> > > +		server_walker = server->hierarchy;
> > > +	}
> > 
> > } else {
> > 	server_layer = 0;
> > 	server_walker = NULL;
> > }
> > 
> > > +	base_layer = (client_layer > server_layer) ? server_layer :
> > > +						     client_layer;
> > > +
> > > +	/* For client domain, walk_and_check ensures the client domain is
> > > +	 * not scoped until gets to base_layer.
> > 
> > until gets?
> > 
> > > +	 * For server_domain, it only ensures that the server domain exist.
> > > +	 */
> > > +	if (client_layer != server_layer) {
> > 
> > bool is_scoped;
> It is defined above. 

Yes, but it should be defined here because it is not used outside of
this block.

> > > +		if (client_layer > server_layer)
> > > +			is_scoped = walk_and_check(client, &client_walker,
> > > +						   server_layer, client_layer,
> > > +						   ipc_type);
> > > +		else
> > 
> > server_walker may be uninitialized and still read here, and maybe later
> > in the for loop.  The whole code should maks sure this cannot happen,
> > and a test case should check this.
> I think this case never happens, since the server_walker can be read
> here if there are more than one layers in server domain which means that
> the server_walker is not unintialized.

Yes, but this code makes it more difficult to convince yourself.  The
proposed refactoring should help.

> 
> > > +			is_scoped = walk_and_check(server, &server_walker,
> > > +						   client_layer, server_layer,
> > > +						   ipc_type & 0);
> > 
> > "ipc_type & 0" is the same as "0"
> > 
> > > +		if (!is_scoped)
> > 
> > The name doesn't reflect the semantic. walk_and_check() should return
> > the inverse.
> > 
> > > +			return false;
> > > +	}
> > 
> > This code would be simpler:
> > 
> > if (client_layer > server_layer) {
> > 	base_layer = server_layer;
> > 	// TODO: inverse boolean logic
> > 	if (!walk_and_check(client, &client_walker,
> > 				   base_layer, ipc_type))
> > 		return false;
> > } else (client_layer < server_layer) {
> > 	base_layer = client_layer;
> > 	// TODO: inverse boolean logic
> > 	if (!walk_and_check(server, &server_walker,
> > 				   base_layer, 0))
> > 		return false;
> > } else {
> > 	base_layer = client_layer;
> > }
> > 
> > 
> > I think we can improve more to make sure there is no path/risk of
> > inconsistent pointers.
> > 
> > 
> > > +	/* client and server are at the same level in hierarchy. If client is
> > > +	 * scoped, the server must be scoped in the same domain
> > > +	 */
> > > +	for (base_layer; base_layer >= 0; base_layer--) {
> > > +		if (landlock_get_scope_mask(client, base_layer) & ipc_type) {
> > 
> > With each multi-line comment, the first line should be empty:
> > /*
> >  * This check must be here since access would be denied only if
> > 
> > > +			/* This check must be here since access would be denied only if
> > > +			 * the client is scoped and the server has no domain, so
> > > +			 * if the client has a domain but is not scoped and the server
> > > +			 * has no domain, access is guaranteed.
> > > +			 */
> > > +			if (!server)
> > > +				return false;
> > > +
> > > +			if (server_walker == client_walker)
> > > +				return true;
> > > +
> > > +			return false;
> > > +		}
> > > +		client_walker = client_walker->parent;
> > > +		server_walker = server_walker->parent;
> > > +		/* Warn if there is an incosistenncy between num_layers and
> > 
> > Makes sure there is no inconsistency between num_layers and
> > 
> > 
> > > +		 * landlock_hierarchy in each of rulesets
> > > +		 */
> > > +		if (WARN_ON_ONCE(base_layer > 0 &&
> > > +				 (!server_walker || !client_walker)))
> > > +			return false;
> > > +	}
> > > +	return true;
> > > +}
> 

Re: [PATCH v8 1/4] Landlock: Add abstract unix socket connect restriction

[Jann Horn]

On Fri, Aug 2, 2024 at 6:03 AM Tahera Fahimi <fahimitahera@gmail.com> wrote:
> This patch introduces a new "scoped" attribute to the landlock_ruleset_attr
> that can specify "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to scope
> abstract Unix sockets from connecting to a process outside of
> the same landlock domain. It implements two hooks, unix_stream_connect
> and unix_may_send to enforce this restriction.
[...]
> +static bool check_unix_address_format(struct sock *const sock)
> +{
> +       struct unix_address *addr = unix_sk(sock)->addr;
> +
> +       if (!addr)
> +               return true;
> +
> +       if (addr->len > sizeof(AF_UNIX)) {
> +               /* handling unspec sockets */
> +               if (!addr->name[0].sun_path)
> +                       return true;

addr->name[0] is a "struct sockaddr_un", whose member "sun_path" is an
array member, not a pointer. If "addr" is a valid pointer,
"addr->name[0].sun_path" can't be NULL.


> +               if (addr->name[0].sun_path[0] == '\0')
> +                       if (!sock_is_scoped(sock))
> +                               return false;
> +       }
> +
> +       return true;
> +}

[PATCH v8 2/4] selftests/landlock: Abstract unix socket restriction tests

[Tahera Fahimi]

The patch introduces Landlock ABI version 6 and has four types of tests:
1) unix_socket: base tests of the abstract socket scoping mechanism for a
   landlocked process, same as the ptrace test.
2) optional_scoping: generates three processes with different domains and
   tests if a process with a non-scoped domain can connect to other
   processes.
3) unix_sock_special_cases: since the socket's creator credentials are used
   for scoping sockets, this test examines the cases where the socket's
   credentials are different from the process using it.
4) pathname_address_sockets: ensures that Unix sockets bound to a
   null-terminated filesystem can still connect to a socket outside of
   their scoped domain. This means that even if the domain is scoped with
   LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET, the socket can connect to a socket
   outside the scoped domain.

Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
---
Changes in versions:
V8:
- Move tests to scoped_abstract_unix_test.c file.
- To avoid potential conflicts among Unix socket names in different tests,
  set_unix_address is added to common.h to set different sun_path for Unix sockets.
- protocol_variant and service_fixture structures are also moved to common.h
- Adding pathname_address_sockets to cover all types of address formats
  for unix sockets, and moving remove_path() to common.h to reuse in this test.
V7:
- Introducing landlock ABI version 6.
- Adding some edge test cases to optional_scoping test.
- Using `enum` for different domains in optional_scoping tests.
- Extend unix_sock_special_cases test cases for connected(SOCK_STREAM) sockets.
- Modifying inline comments.
V6:
- Introducing optional_scoping test which ensures a sandboxed process with a
  non-scoped domain can still connect to another abstract unix socket(either
  sandboxed or non-sandboxed).
- Introducing unix_sock_special_cases test which tests examines scenarios where
  the connecting sockets have different domain than the process using them.
V4:
- Introducing unix_socket to evaluate the basic scoping mechanism for abstract
  unix sockets.
---
 tools/testing/selftests/landlock/base_test.c  |    2 +-
 tools/testing/selftests/landlock/common.h     |   72 ++
 tools/testing/selftests/landlock/fs_test.c    |   34 -
 tools/testing/selftests/landlock/net_test.c   |   31 +-
 .../landlock/scoped_abstract_unix_test.c      | 1136 +++++++++++++++++
 5 files changed, 1210 insertions(+), 65 deletions(-)
 create mode 100644 tools/testing/selftests/landlock/scoped_abstract_unix_test.c

diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
index 3c1e9f35b531..52b00472a487 100644
--- a/tools/testing/selftests/landlock/base_test.c
+++ b/tools/testing/selftests/landlock/base_test.c
@@ -75,7 +75,7 @@ TEST(abi_version)
 	const struct landlock_ruleset_attr ruleset_attr = {
 		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
 	};
-	ASSERT_EQ(5, landlock_create_ruleset(NULL, 0,
+	ASSERT_EQ(6, landlock_create_ruleset(NULL, 0,
 					     LANDLOCK_CREATE_RULESET_VERSION));
 
 	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h
index 7e2b431b9f90..a0c8126d94b4 100644
--- a/tools/testing/selftests/landlock/common.h
+++ b/tools/testing/selftests/landlock/common.h
@@ -16,8 +16,11 @@
 #include <sys/types.h>
 #include <sys/wait.h>
 #include <unistd.h>
+#include <sys/un.h>
+#include <arpa/inet.h>
 
 #include "../kselftest_harness.h"
+#define TMP_DIR "tmp"
 
 #ifndef __maybe_unused
 #define __maybe_unused __attribute__((__unused__))
@@ -226,3 +229,72 @@ enforce_ruleset(struct __test_metadata *const _metadata, const int ruleset_fd)
 		TH_LOG("Failed to enforce ruleset: %s", strerror(errno));
 	}
 }
+
+struct protocol_variant {
+	int domain;
+	int type;
+};
+
+struct service_fixture {
+	struct protocol_variant protocol;
+	/* port is also stored in ipv4_addr.sin_port or ipv6_addr.sin6_port */
+	unsigned short port;
+	union {
+		struct sockaddr_in ipv4_addr;
+		struct sockaddr_in6 ipv6_addr;
+		struct {
+			struct sockaddr_un unix_addr;
+			socklen_t unix_addr_len;
+		};
+	};
+};
+
+static pid_t __maybe_unused sys_gettid(void)
+{
+	return syscall(__NR_gettid);
+}
+
+static void __maybe_unused set_unix_address(struct service_fixture *const srv,
+					    const unsigned short index)
+{
+	srv->unix_addr.sun_family = AF_UNIX;
+	sprintf(srv->unix_addr.sun_path,
+		"_selftests-landlock-abstract-unix-tid%d-index%d", sys_gettid(),
+		index);
+	srv->unix_addr_len = SUN_LEN(&srv->unix_addr);
+	srv->unix_addr.sun_path[0] = '\0';
+}
+
+static int __maybe_unused remove_path(const char *const path)
+{
+	char *walker;
+	int i, ret, err = 0;
+
+	walker = strdup(path);
+	if (!walker) {
+		err = ENOMEM;
+		goto out;
+	}
+	if (unlink(path) && rmdir(path)) {
+		if (errno != ENOENT && errno != ENOTDIR)
+			err = errno;
+		goto out;
+	}
+	for (i = strlen(walker); i > 0; i--) {
+		if (walker[i] != '/')
+			continue;
+		walker[i] = '\0';
+		ret = rmdir(walker);
+		if (ret) {
+			if (errno != ENOTEMPTY && errno != EBUSY)
+				err = errno;
+			goto out;
+		}
+		if (strcmp(walker, TMP_DIR) == 0)
+			goto out;
+	}
+
+out:
+	free(walker);
+	return err;
+}
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 7d063c652be1..fc74cab6dfb8 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -221,40 +221,6 @@ static void create_file(struct __test_metadata *const _metadata,
 	}
 }
 
-static int remove_path(const char *const path)
-{
-	char *walker;
-	int i, ret, err = 0;
-
-	walker = strdup(path);
-	if (!walker) {
-		err = ENOMEM;
-		goto out;
-	}
-	if (unlink(path) && rmdir(path)) {
-		if (errno != ENOENT && errno != ENOTDIR)
-			err = errno;
-		goto out;
-	}
-	for (i = strlen(walker); i > 0; i--) {
-		if (walker[i] != '/')
-			continue;
-		walker[i] = '\0';
-		ret = rmdir(walker);
-		if (ret) {
-			if (errno != ENOTEMPTY && errno != EBUSY)
-				err = errno;
-			goto out;
-		}
-		if (strcmp(walker, TMP_DIR) == 0)
-			goto out;
-	}
-
-out:
-	free(walker);
-	return err;
-}
-
 struct mnt_opt {
 	const char *const source;
 	const char *const type;
diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
index f21cfbbc3638..4e0aeb53b225 100644
--- a/tools/testing/selftests/landlock/net_test.c
+++ b/tools/testing/selftests/landlock/net_test.c
@@ -36,30 +36,6 @@ enum sandbox_type {
 	TCP_SANDBOX,
 };
 
-struct protocol_variant {
-	int domain;
-	int type;
-};
-
-struct service_fixture {
-	struct protocol_variant protocol;
-	/* port is also stored in ipv4_addr.sin_port or ipv6_addr.sin6_port */
-	unsigned short port;
-	union {
-		struct sockaddr_in ipv4_addr;
-		struct sockaddr_in6 ipv6_addr;
-		struct {
-			struct sockaddr_un unix_addr;
-			socklen_t unix_addr_len;
-		};
-	};
-};
-
-static pid_t sys_gettid(void)
-{
-	return syscall(__NR_gettid);
-}
-
 static int set_service(struct service_fixture *const srv,
 		       const struct protocol_variant prot,
 		       const unsigned short index)
@@ -92,12 +68,7 @@ static int set_service(struct service_fixture *const srv,
 		return 0;
 
 	case AF_UNIX:
-		srv->unix_addr.sun_family = prot.domain;
-		sprintf(srv->unix_addr.sun_path,
-			"_selftests-landlock-net-tid%d-index%d", sys_gettid(),
-			index);
-		srv->unix_addr_len = SUN_LEN(&srv->unix_addr);
-		srv->unix_addr.sun_path[0] = '\0';
+		set_unix_address(srv, index);
 		return 0;
 	}
 	return 1;
diff --git a/tools/testing/selftests/landlock/scoped_abstract_unix_test.c b/tools/testing/selftests/landlock/scoped_abstract_unix_test.c
new file mode 100644
index 000000000000..ffa1f01dbbce
--- /dev/null
+++ b/tools/testing/selftests/landlock/scoped_abstract_unix_test.c
@@ -0,0 +1,1136 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Landlock tests - Abstract Unix Socket
+ *
+ * Copyright © 2017-2020 Mickaël Salaün <mic@digikod.net>
+ * Copyright © 2019-2020 ANSSI
+ */
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <linux/landlock.h>
+#include <signal.h>
+#include <sys/prctl.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <sys/socket.h>
+#include <sys/un.h>
+#include <stddef.h>
+#include <sys/stat.h>
+#include <sched.h>
+
+#include "common.h"
+
+static void create_fs_domain(struct __test_metadata *const _metadata)
+{
+	int ruleset_fd;
+	struct landlock_ruleset_attr ruleset_attr = {
+		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_DIR,
+	};
+
+	ruleset_fd =
+		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+	EXPECT_LE(0, ruleset_fd)
+	{
+		TH_LOG("Failed to create a ruleset: %s", strerror(errno));
+	}
+	EXPECT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
+	EXPECT_EQ(0, landlock_restrict_self(ruleset_fd, 0));
+	EXPECT_EQ(0, close(ruleset_fd));
+}
+
+static void create_unix_domain(struct __test_metadata *const _metadata)
+{
+	int ruleset_fd;
+	const struct landlock_ruleset_attr ruleset_attr = {
+		.scoped = LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET,
+	};
+
+	ruleset_fd =
+		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+	EXPECT_LE(0, ruleset_fd)
+	{
+		TH_LOG("Failed to create a ruleset: %s", strerror(errno));
+	}
+	enforce_ruleset(_metadata, ruleset_fd);
+	EXPECT_EQ(0, close(ruleset_fd));
+}
+
+/* clang-format off */
+FIXTURE(unix_socket)
+{
+	struct service_fixture stream_address, dgram_address;
+	int server, client, dgram_server, dgram_client;
+};
+
+/* clang-format on */
+FIXTURE_VARIANT(unix_socket)
+{
+	bool domain_both;
+	bool domain_parent;
+	bool domain_child;
+	bool connect_to_parent;
+};
+
+FIXTURE_SETUP(unix_socket)
+{
+	memset(&self->stream_address, 0, sizeof(self->stream_address));
+	memset(&self->dgram_address, 0, sizeof(self->dgram_address));
+
+	set_unix_address(&self->stream_address, 0);
+	set_unix_address(&self->dgram_address, 1);
+}
+
+FIXTURE_TEARDOWN(unix_socket)
+{
+	close(self->server);
+	close(self->client);
+	close(self->dgram_server);
+	close(self->dgram_client);
+}
+
+/*
+ *        No domain
+ *
+ *   P1-.               P1 -> P2 : allow
+ *       \              P2 -> P1 : allow
+ *        'P2
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, allow_without_domain_connect_to_parent) {
+	/* clang-format on */
+	.domain_both = false,
+	.domain_parent = false,
+	.domain_child = false,
+	.connect_to_parent = true,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, allow_without_domain_connect_to_child) {
+	/* clang-format on */
+	.domain_both = false,
+	.domain_parent = false,
+	.domain_child = false,
+	.connect_to_parent = false,
+};
+
+/*
+ *        Child domain
+ *
+ *   P1--.              P1 -> P2 : allow
+ *        \             P2 -> P1 : deny
+ *        .'-----.
+ *        |  P2  |
+ *        '------'
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, deny_with_one_domain_connect_to_parent) {
+	/* clang-format on */
+	.domain_both = false,
+	.domain_parent = false,
+	.domain_child = true,
+	.connect_to_parent = true,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, allow_with_one_domain_connect_to_child) {
+	/* clang-format on */
+	.domain_both = false,
+	.domain_parent = false,
+	.domain_child = true,
+	.connect_to_parent = false,
+};
+
+/*
+ *        Parent domain
+ * .------.
+ * |  P1  --.           P1 -> P2 : deny
+ * '------'  \          P2 -> P1 : allow
+ *            '
+ *            P2
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, allow_with_parent_domain_connect_to_parent) {
+	/* clang-format on */
+	.domain_both = false,
+	.domain_parent = true,
+	.domain_child = false,
+	.connect_to_parent = true,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, deny_with_parent_domain_connect_to_child) {
+	/* clang-format on */
+	.domain_both = false,
+	.domain_parent = true,
+	.domain_child = false,
+	.connect_to_parent = false,
+};
+
+/*
+ *        Parent + child domain (siblings)
+ * .------.
+ * |  P1  ---.          P1 -> P2 : deny
+ * '------'   \         P2 -> P1 : deny
+ *         .---'--.
+ *         |  P2  |
+ *         '------'
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, deny_with_sibling_domain_connect_to_parent) {
+	/* clang-format on */
+	.domain_both = false,
+	.domain_parent = true,
+	.domain_child = true,
+	.connect_to_parent = true,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, deny_with_sibling_domain_connect_to_child) {
+	/* clang-format on */
+	.domain_both = false,
+	.domain_parent = true,
+	.domain_child = true,
+	.connect_to_parent = false,
+};
+
+/*
+ *         Same domain (inherited)
+ * .-------------.
+ * | P1----.     |      P1 -> P2 : allow
+ * |        \    |      P2 -> P1 : allow
+ * |         '   |
+ * |         P2  |
+ * '-------------'
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, allow_inherited_domain_connect_to_parent) {
+	/* clang-format on */
+	.domain_both = true,
+	.domain_parent = false,
+	.domain_child = false,
+	.connect_to_parent = true,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, allow_inherited_domain_connect_to_child) {
+	/* clang-format on */
+	.domain_both = true,
+	.domain_parent = false,
+	.domain_child = false,
+	.connect_to_parent = false,
+};
+
+/*
+ *         Inherited + child domain
+ * .-----------------.
+ * |  P1----.        |  P1 -> P2 : allow
+ * |         \       |  P2 -> P1 : deny
+ * |        .-'----. |
+ * |        |  P2  | |
+ * |        '------' |
+ * '-----------------'
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, deny_nested_domain_connect_to_parent) {
+	/* clang-format on */
+	.domain_both = true,
+	.domain_parent = false,
+	.domain_child = true,
+	.connect_to_parent = true,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, allow_nested_domain_connect_to_child) {
+	/* clang-format on */
+	.domain_both = true,
+	.domain_parent = false,
+	.domain_child = true,
+	.connect_to_parent = false,
+};
+
+/*
+ *         Inherited + parent domain
+ * .-----------------.
+ * |.------.         |  P1 -> P2 : deny
+ * ||  P1  ----.     |  P2 -> P1 : allow
+ * |'------'    \    |
+ * |             '   |
+ * |             P2  |
+ * '-----------------'
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, allow_with_nested_and_parent_domain_connect_to_parent) {
+	/* clang-format on */
+	.domain_both = true,
+	.domain_parent = true,
+	.domain_child = false,
+	.connect_to_parent = true,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, deny_with_nested_and_parent_domain_connect_to_child) {
+	/* clang-format on */
+	.domain_both = true,
+	.domain_parent = true,
+	.domain_child = false,
+	.connect_to_parent = false,
+};
+
+/*
+ *         Inherited + parent and child domain (siblings)
+ * .-----------------.
+ * | .------.        |  P1 -> P2 : deny
+ * | |  P1  .        |  P2 -> P1 : deny
+ * | '------'\       |
+ * |          \      |
+ * |        .--'---. |
+ * |        |  P2  | |
+ * |        '------' |
+ * '-----------------'
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, deny_with_forked_domain_connect_to_parent) {
+	/* clang-format on */
+	.domain_both = true,
+	.domain_parent = true,
+	.domain_child = true,
+	.connect_to_parent = true,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_socket, deny_with_forked_domain_connect_to_child) {
+	/* clang-format on */
+	.domain_both = true,
+	.domain_parent = true,
+	.domain_child = true,
+	.connect_to_parent = false,
+};
+
+/* Test UNIX_STREAM_CONNECT and UNIX_MAY_SEND for parent and child,
+ * when they have scoped domain or no domain.
+ */
+TEST_F(unix_socket, abstract_unix_socket)
+{
+	int status;
+	pid_t child;
+	bool can_connect_to_parent, can_connect_to_child;
+	int err, err_dgram;
+	int pipe_child[2], pipe_parent[2];
+	char buf_parent;
+
+	/*
+	 * can_connect_to_child is true if a parent process can connect to its
+	 * child process. The parent process is not isolated from the child
+	 * with a dedicated Landlock domain.
+	 */
+	can_connect_to_child = !variant->domain_parent;
+	/*
+	 * can_connect_to_parent is true if a child process can connect to its
+	 * parent process. This depends on the child process is not isolated from
+	 * the parent with a dedicated Landlock domain.
+	 */
+	can_connect_to_parent = !variant->domain_child;
+
+	ASSERT_EQ(0, pipe2(pipe_child, O_CLOEXEC));
+	ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC));
+	if (variant->domain_both) {
+		create_unix_domain(_metadata);
+		if (!__test_passed(_metadata))
+			return;
+	}
+
+	child = fork();
+	ASSERT_LE(0, child);
+	if (child == 0) {
+		char buf_child;
+
+		ASSERT_EQ(0, close(pipe_parent[1]));
+		ASSERT_EQ(0, close(pipe_child[0]));
+		if (variant->domain_child)
+			create_unix_domain(_metadata);
+
+		/* Waits for the parent to be in a domain, if any. */
+		ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
+
+		if (variant->connect_to_parent) {
+			self->client = socket(AF_UNIX, SOCK_STREAM, 0);
+			self->dgram_client = socket(AF_UNIX, SOCK_DGRAM, 0);
+
+			ASSERT_NE(-1, self->client);
+			ASSERT_NE(-1, self->dgram_client);
+			ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
+
+			err = connect(self->client,
+				      &self->stream_address.unix_addr,
+				      (self->stream_address).unix_addr_len);
+			err_dgram =
+				connect(self->dgram_client,
+					&self->dgram_address.unix_addr,
+					(self->dgram_address).unix_addr_len);
+
+			if (can_connect_to_parent) {
+				EXPECT_EQ(0, err);
+				EXPECT_EQ(0, err_dgram);
+			} else {
+				EXPECT_EQ(-1, err);
+				EXPECT_EQ(-1, err_dgram);
+				EXPECT_EQ(EPERM, errno);
+			}
+		} else {
+			self->server = socket(AF_UNIX, SOCK_STREAM, 0);
+			self->dgram_server = socket(AF_UNIX, SOCK_DGRAM, 0);
+			ASSERT_NE(-1, self->server);
+			ASSERT_NE(-1, self->dgram_server);
+
+			ASSERT_EQ(0,
+				  bind(self->server,
+				       &self->stream_address.unix_addr,
+				       (self->stream_address).unix_addr_len));
+			ASSERT_EQ(0, bind(self->dgram_server,
+					  &self->dgram_address.unix_addr,
+					  (self->dgram_address).unix_addr_len));
+			ASSERT_EQ(0, listen(self->server, 32));
+
+			/* signal to parent that child is listening */
+			ASSERT_EQ(1, write(pipe_child[1], ".", 1));
+			/* wait to connect */
+			ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
+		}
+		_exit(_metadata->exit_code);
+		return;
+	}
+
+	ASSERT_EQ(0, close(pipe_child[1]));
+	ASSERT_EQ(0, close(pipe_parent[0]));
+
+	if (variant->domain_parent)
+		create_unix_domain(_metadata);
+
+	/* Signals that the parent is in a domain, if any. */
+	ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
+
+	if (!variant->connect_to_parent) {
+		self->client = socket(AF_UNIX, SOCK_STREAM, 0);
+		self->dgram_client = socket(AF_UNIX, SOCK_DGRAM, 0);
+
+		ASSERT_NE(-1, self->client);
+		ASSERT_NE(-1, self->dgram_client);
+
+		/* Waits for the child to listen */
+		ASSERT_EQ(1, read(pipe_child[0], &buf_parent, 1));
+		err = connect(self->client, &self->stream_address.unix_addr,
+			      (self->stream_address).unix_addr_len);
+		err_dgram = connect(self->dgram_client,
+				    &self->dgram_address.unix_addr,
+				    (self->dgram_address).unix_addr_len);
+
+		if (can_connect_to_child) {
+			EXPECT_EQ(0, err);
+			EXPECT_EQ(0, err_dgram);
+		} else {
+			EXPECT_EQ(-1, err);
+			EXPECT_EQ(-1, err_dgram);
+			EXPECT_EQ(EPERM, errno);
+		}
+		ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
+	} else {
+		self->server = socket(AF_UNIX, SOCK_STREAM, 0);
+		self->dgram_server = socket(AF_UNIX, SOCK_DGRAM, 0);
+		ASSERT_NE(-1, self->server);
+		ASSERT_NE(-1, self->dgram_server);
+		ASSERT_EQ(0, bind(self->server, &self->stream_address.unix_addr,
+				  (self->stream_address).unix_addr_len));
+		ASSERT_EQ(0, bind(self->dgram_server,
+				  &self->dgram_address.unix_addr,
+				  (self->dgram_address).unix_addr_len));
+		ASSERT_EQ(0, listen(self->server, 32));
+
+		/* signal to child that parent is listening */
+		ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
+	}
+
+	ASSERT_EQ(child, waitpid(child, &status, 0));
+
+	if (WIFSIGNALED(status) || !WIFEXITED(status) ||
+	    WEXITSTATUS(status) != EXIT_SUCCESS)
+		_metadata->exit_code = KSFT_FAIL;
+}
+
+enum sandbox_type {
+	NO_SANDBOX,
+	SCOPE_SANDBOX,
+	/* Any other type of sandboxing domain */
+	OTHER_SANDBOX,
+};
+
+/* clang-format off */
+FIXTURE(optional_scoping)
+{
+	struct service_fixture parent_address, child_address;
+	int parent_server, child_server, client;
+};
+
+/* clang-format on */
+FIXTURE_VARIANT(optional_scoping)
+{
+	const int domain_all;
+	const int domain_parent;
+	const int domain_children;
+	const int domain_child;
+	const int domain_grand_child;
+	const int type;
+};
+
+FIXTURE_SETUP(optional_scoping)
+{
+	memset(&self->parent_address, 0, sizeof(self->parent_address));
+	memset(&self->child_address, 0, sizeof(self->child_address));
+
+	set_unix_address(&self->parent_address, 0);
+	set_unix_address(&self->child_address, 1);
+}
+
+FIXTURE_TEARDOWN(optional_scoping)
+{
+	close(self->parent_server);
+	close(self->child_server);
+	close(self->client);
+}
+
+/*
+ * .-----------------.
+ * |         ####### |  P3 -> P2 : allow
+ * |   P1----# P2  # |  P3 -> P1 : deny
+ * |         #  |  # |
+ * |         # P3  # |
+ * |         ####### |
+ * '-----------------'
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(optional_scoping, deny_scoped) {
+	.domain_all = OTHER_SANDBOX,
+	.domain_parent = NO_SANDBOX,
+	.domain_children = SCOPE_SANDBOX,
+	.domain_child = NO_SANDBOX,
+	.domain_grand_child = NO_SANDBOX,
+	.type = SOCK_DGRAM,
+	/* clang-format on */
+};
+
+/*
+ * .-----------------.
+ * |         .-----. |  P3 -> P2 : allow
+ * |   P1----| P2  | |  P3 -> P1 : allow
+ * |         |     | |
+ * |         | P3  | |
+ * |         '-----' |
+ * '-----------------'
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(optional_scoping, allow_with_other_domain) {
+	.domain_all = OTHER_SANDBOX,
+	.domain_parent = NO_SANDBOX,
+	.domain_children = OTHER_SANDBOX,
+	.domain_child = NO_SANDBOX,
+	.domain_grand_child = NO_SANDBOX,
+	.type = SOCK_DGRAM,
+	/* clang-format on */
+};
+
+/*
+ *  .----.    ######   P3 -> P2 : allow
+ *  | P1 |----# P2 #   P3 -> P1 : allow
+ *  '----'    ######
+ *              |
+ *              P3
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(optional_scoping, allow_with_one_domain) {
+	.domain_all = NO_SANDBOX,
+	.domain_parent = OTHER_SANDBOX,
+	.domain_children = NO_SANDBOX,
+	.domain_child = SCOPE_SANDBOX,
+	.domain_grand_child = NO_SANDBOX,
+	.type = SOCK_DGRAM,
+	/* clang-format on */
+};
+
+/*
+ *  ######    .-----.   P3 -> P2 : allow
+ *  # P1 #----| P2  |   P3 -> P1 : allow
+ *  ######    '-----'
+ *              |
+ *              P3
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(optional_scoping, allow_with_grand_parent_scoped) {
+	.domain_all = NO_SANDBOX,
+	.domain_parent = SCOPE_SANDBOX,
+	.domain_children = NO_SANDBOX,
+	.domain_child = OTHER_SANDBOX,
+	.domain_grand_child = NO_SANDBOX,
+	.type = SOCK_STREAM,
+	/* clang-format on */
+};
+
+/*
+ *  ######    ######   P3 -> P2 : allow
+ *  # P1 #----# P2 #   P3 -> P1 : allow
+ *  ######    ######
+ *               |
+ *             .----.
+ *             | P3 |
+ *             '----'
+ */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(optional_scoping, allow_with_parents_domain) {
+	.domain_all = NO_SANDBOX,
+	.domain_parent = SCOPE_SANDBOX,
+	.domain_children = NO_SANDBOX,
+	.domain_child = SCOPE_SANDBOX,
+	.domain_grand_child = NO_SANDBOX,
+	.type = SOCK_STREAM,
+	/* clang-format on */
+};
+
+/* Test UNIX_STREAM_CONNECT and UNIX_MAY_SEND for parent, child
+ * and grand child processes when they can have scoped or non-scoped
+ * domains.
+ **/
+TEST_F(optional_scoping, unix_scoping)
+{
+	pid_t child;
+	int status;
+	bool can_connect_to_parent, can_connect_to_child;
+	int pipe_parent[2];
+
+	if (variant->domain_grand_child == SCOPE_SANDBOX)
+		can_connect_to_child = false;
+	else
+		can_connect_to_child = true;
+
+	if (!can_connect_to_child || variant->domain_children == SCOPE_SANDBOX)
+		can_connect_to_parent = false;
+	else
+		can_connect_to_parent = true;
+
+	ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC));
+
+	if (variant->domain_all == OTHER_SANDBOX)
+		create_fs_domain(_metadata);
+	else if (variant->domain_all == SCOPE_SANDBOX)
+		create_unix_domain(_metadata);
+
+	child = fork();
+	ASSERT_LE(0, child);
+	if (child == 0) {
+		int pipe_child[2];
+
+		ASSERT_EQ(0, pipe2(pipe_child, O_CLOEXEC));
+		pid_t grand_child;
+
+		if (variant->domain_children == OTHER_SANDBOX)
+			create_fs_domain(_metadata);
+		else if (variant->domain_children == SCOPE_SANDBOX)
+			create_unix_domain(_metadata);
+
+		grand_child = fork();
+		ASSERT_LE(0, grand_child);
+		if (grand_child == 0) {
+			ASSERT_EQ(0, close(pipe_parent[1]));
+			ASSERT_EQ(0, close(pipe_child[1]));
+
+			char buf1, buf2;
+			int err;
+
+			if (variant->domain_grand_child == OTHER_SANDBOX)
+				create_fs_domain(_metadata);
+			else if (variant->domain_grand_child == SCOPE_SANDBOX)
+				create_unix_domain(_metadata);
+
+			self->client = socket(AF_UNIX, variant->type, 0);
+			ASSERT_NE(-1, self->client);
+
+			ASSERT_EQ(1, read(pipe_child[0], &buf2, 1));
+			err = connect(self->client,
+				      &self->child_address.unix_addr,
+				      (self->child_address).unix_addr_len);
+			if (can_connect_to_child) {
+				EXPECT_EQ(0, err);
+			} else {
+				EXPECT_EQ(-1, err);
+				EXPECT_EQ(EPERM, errno);
+			}
+
+			if (variant->type == SOCK_STREAM) {
+				EXPECT_EQ(0, close(self->client));
+				self->client =
+					socket(AF_UNIX, variant->type, 0);
+				ASSERT_NE(-1, self->client);
+			}
+
+			ASSERT_EQ(1, read(pipe_parent[0], &buf1, 1));
+			err = connect(self->client,
+				      &self->parent_address.unix_addr,
+				      (self->parent_address).unix_addr_len);
+			if (can_connect_to_parent) {
+				EXPECT_EQ(0, err);
+			} else {
+				EXPECT_EQ(-1, err);
+				EXPECT_EQ(EPERM, errno);
+			}
+			EXPECT_EQ(0, close(self->client));
+
+			_exit(_metadata->exit_code);
+			return;
+		}
+
+		ASSERT_EQ(0, close(pipe_child[0]));
+		if (variant->domain_child == OTHER_SANDBOX)
+			create_fs_domain(_metadata);
+		else if (variant->domain_child == SCOPE_SANDBOX)
+			create_unix_domain(_metadata);
+
+		self->child_server = socket(AF_UNIX, variant->type, 0);
+		ASSERT_NE(-1, self->child_server);
+		ASSERT_EQ(0, bind(self->child_server,
+				  &self->child_address.unix_addr,
+				  (self->child_address).unix_addr_len));
+		if (variant->type == SOCK_STREAM)
+			ASSERT_EQ(0, listen(self->child_server, 32));
+
+		ASSERT_EQ(1, write(pipe_child[1], ".", 1));
+		ASSERT_EQ(grand_child, waitpid(grand_child, &status, 0));
+		return;
+	}
+	ASSERT_EQ(0, close(pipe_parent[0]));
+
+	if (variant->domain_parent == OTHER_SANDBOX)
+		create_fs_domain(_metadata);
+	else if (variant->domain_parent == SCOPE_SANDBOX)
+		create_unix_domain(_metadata);
+
+	self->parent_server = socket(AF_UNIX, variant->type, 0);
+	ASSERT_NE(-1, self->parent_server);
+	ASSERT_EQ(0, bind(self->parent_server, &self->parent_address.unix_addr,
+			  (self->parent_address).unix_addr_len));
+
+	if (variant->type == SOCK_STREAM)
+		ASSERT_EQ(0, listen(self->parent_server, 32));
+
+	ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
+	ASSERT_EQ(child, waitpid(child, &status, 0));
+	if (WIFSIGNALED(status) || !WIFEXITED(status) ||
+	    WEXITSTATUS(status) != EXIT_SUCCESS)
+		_metadata->exit_code = KSFT_FAIL;
+}
+
+/*
+ * Since the special case of scoping only happens when the connecting socket
+ * is scoped, the client's domain is true for all the following test cases.
+ */
+/* clang-format off */
+FIXTURE(unix_sock_special_cases) {
+	int server_socket, client;
+	int stream_server, stream_client;
+	struct service_fixture address, transit_address;
+};
+
+/* clang-format on */
+FIXTURE_VARIANT(unix_sock_special_cases)
+{
+	const bool domain_server;
+	const bool domain_server_socket;
+	const int type;
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_sock_special_cases, allow_dgram_server_sock_domain) {
+	/* clang-format on */
+	.domain_server = false,
+	.domain_server_socket = true,
+	.type = SOCK_DGRAM,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_sock_special_cases, deny_dgram_server_domain) {
+	/* clang-format on */
+	.domain_server = true,
+	.domain_server_socket = false,
+	.type = SOCK_DGRAM,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_sock_special_cases, allow_stream_server_sock_domain) {
+	/* clang-format on */
+	.domain_server = false,
+	.domain_server_socket = true,
+	.type = SOCK_STREAM,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(unix_sock_special_cases, deny_stream_server_domain) {
+	/* clang-format on */
+	.domain_server = true,
+	.domain_server_socket = false,
+	.type = SOCK_STREAM,
+};
+
+FIXTURE_SETUP(unix_sock_special_cases)
+{
+	memset(&self->transit_address, 0, sizeof(self->transit_address));
+	memset(&self->address, 0, sizeof(self->address));
+	set_unix_address(&self->transit_address, 0);
+	set_unix_address(&self->address, 1);
+}
+
+FIXTURE_TEARDOWN(unix_sock_special_cases)
+{
+	close(self->client);
+	close(self->server_socket);
+	close(self->stream_server);
+	close(self->stream_client);
+}
+
+/* Test UNIX_STREAM_CONNECT and UNIX_MAY_SEND for parent and
+ * child processes when connecting socket has different domain
+ * than the process using it.
+ **/
+TEST_F(unix_sock_special_cases, socket_with_different_domain)
+{
+	pid_t child;
+	int err, status;
+	int pipe_child[2], pipe_parent[2];
+	char buf_parent;
+
+	ASSERT_EQ(0, pipe2(pipe_child, O_CLOEXEC));
+	ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC));
+
+	child = fork();
+	ASSERT_LE(0, child);
+	if (child == 0) {
+		char buf_child;
+
+		ASSERT_EQ(0, close(pipe_parent[1]));
+		ASSERT_EQ(0, close(pipe_child[0]));
+
+		/* client always has domain */
+		create_unix_domain(_metadata);
+
+		if (variant->domain_server_socket) {
+			int data_socket;
+			int fd_sock = socket(AF_UNIX, variant->type, 0);
+
+			ASSERT_NE(-1, fd_sock);
+
+			self->stream_server = socket(AF_UNIX, SOCK_STREAM, 0);
+
+			ASSERT_NE(-1, self->stream_server);
+			ASSERT_EQ(0,
+				  bind(self->stream_server,
+				       &self->transit_address.unix_addr,
+				       (self->transit_address).unix_addr_len));
+			ASSERT_EQ(0, listen(self->stream_server, 32));
+
+			ASSERT_EQ(1, write(pipe_child[1], ".", 1));
+
+			data_socket = accept(self->stream_server, NULL, NULL);
+
+			ASSERT_EQ(0, send_fd(data_socket, fd_sock));
+			ASSERT_EQ(0, close(fd_sock));
+		}
+
+		self->client = socket(AF_UNIX, variant->type, 0);
+		ASSERT_NE(-1, self->client);
+		/* wait for parent signal for connection */
+		ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
+
+		err = connect(self->client, &self->address.unix_addr,
+			      (self->address).unix_addr_len);
+		if (!variant->domain_server_socket) {
+			EXPECT_EQ(-1, err);
+			EXPECT_EQ(EPERM, errno);
+		} else {
+			EXPECT_EQ(0, err);
+		}
+		_exit(_metadata->exit_code);
+		return;
+	}
+
+	ASSERT_EQ(0, close(pipe_child[1]));
+	ASSERT_EQ(0, close(pipe_parent[0]));
+
+	if (!variant->domain_server_socket) {
+		self->server_socket = socket(AF_UNIX, variant->type, 0);
+	} else {
+		int cli = socket(AF_UNIX, SOCK_STREAM, 0);
+
+		ASSERT_NE(-1, cli);
+		ASSERT_EQ(1, read(pipe_child[0], &buf_parent, 1));
+		ASSERT_EQ(0, connect(cli, &self->transit_address.unix_addr,
+				     (self->transit_address).unix_addr_len));
+
+		self->server_socket = recv_fd(cli);
+		ASSERT_LE(0, self->server_socket);
+		ASSERT_EQ(0, close(cli));
+	}
+
+	ASSERT_NE(-1, self->server_socket);
+
+	if (variant->domain_server)
+		create_unix_domain(_metadata);
+
+	ASSERT_EQ(0, bind(self->server_socket, &self->address.unix_addr,
+			  (self->address).unix_addr_len));
+	if (variant->type == SOCK_STREAM)
+		ASSERT_EQ(0, listen(self->server_socket, 32));
+	/* signal to child that parent is listening */
+	ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
+
+	ASSERT_EQ(child, waitpid(child, &status, 0));
+
+	if (WIFSIGNALED(status) || !WIFEXITED(status) ||
+	    WEXITSTATUS(status) != EXIT_SUCCESS)
+		_metadata->exit_code = KSFT_FAIL;
+}
+
+static const char path1_variant1[] = TMP_DIR "/s1_variant1";
+static const char path2_variant1[] = TMP_DIR "/s2_variant1";
+
+/* clang-format off */
+FIXTURE(pathname_address_sockets) {
+	struct service_fixture stream_address, dgram_address;
+};
+
+/* clang-format on */
+FIXTURE_VARIANT(pathname_address_sockets)
+{
+	const int domain;
+	const char *stream_path;
+	const char *dgram_path;
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(pathname_address_sockets, pathname_socket_scoped_domain) {
+	/* clang-format on */
+	.domain = SCOPE_SANDBOX,
+	.stream_path = path1_variant1,
+	.dgram_path = path2_variant1,
+};
+
+FIXTURE_SETUP(pathname_address_sockets)
+{
+	/* setup abstract addresses */
+	memset(&self->stream_address, 0, sizeof(self->stream_address));
+	set_unix_address(&self->stream_address, 0);
+
+	memset(&self->dgram_address, 0, sizeof(self->dgram_address));
+	set_unix_address(&self->dgram_address, 0);
+
+	const char *s_path = variant->stream_path;
+	const char *dg_path = variant->dgram_path;
+
+	disable_caps(_metadata);
+	umask(0077);
+	ASSERT_EQ(0, mkdir(TMP_DIR, 0700));
+
+	ASSERT_EQ(0, mknod(s_path, S_IFREG | 0700, 0))
+	{
+		TH_LOG("Failed to create file \"%s\": %s", s_path,
+		       strerror(errno));
+		remove_path(TMP_DIR);
+	}
+	ASSERT_EQ(0, mknod(dg_path, S_IFREG | 0700, 0))
+	{
+		TH_LOG("Failed to create file \"%s\": %s", dg_path,
+		       strerror(errno));
+		remove_path(TMP_DIR);
+	}
+}
+
+FIXTURE_TEARDOWN(pathname_address_sockets)
+{
+	const char *s_path = variant->stream_path;
+	const char *dg_path = variant->dgram_path;
+
+	ASSERT_EQ(0, remove_path(dg_path));
+	ASSERT_EQ(0, remove_path(s_path));
+	ASSERT_EQ(0, remove_path(TMP_DIR));
+}
+
+TEST_F(pathname_address_sockets, scoped_pathname_sockets)
+{
+	const char *const stream_path = variant->stream_path;
+	const char *const dgram_path = variant->dgram_path;
+	int srv_fd, srv_fd_dg;
+	socklen_t size, size_dg;
+	struct sockaddr_un srv_un, srv_un_dg;
+	int pipe_parent[2];
+	pid_t child;
+	int status;
+	char buf_child;
+	int socket_fds_stream[2];
+	int server, client, dgram_server, dgram_client;
+	int recv_data;
+
+	ASSERT_EQ(0, socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0,
+				socket_fds_stream));
+
+	ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC));
+
+	child = fork();
+	ASSERT_LE(0, child);
+	if (child == 0) {
+		struct sockaddr_un cli_un, cli_un_dg;
+		int cli_fd, cli_fd_dg;
+		socklen_t size, size_dg;
+		int sample = socket(AF_UNIX, SOCK_STREAM, 0);
+		int err, err_dg;
+
+		ASSERT_LE(0, sample);
+
+		ASSERT_EQ(0, close(pipe_parent[1]));
+
+		/* scope the domain */
+		if (variant->domain == SCOPE_SANDBOX)
+			create_unix_domain(_metadata);
+		else if (variant->domain == OTHER_SANDBOX)
+			create_fs_domain(_metadata);
+		ASSERT_EQ(0, close(socket_fds_stream[1]));
+		ASSERT_EQ(0, send_fd(socket_fds_stream[0], sample));
+		ASSERT_EQ(0, close(sample));
+		ASSERT_EQ(0, close(socket_fds_stream[0]));
+
+		/* wait for server to listen */
+		ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
+
+		cli_un.sun_family = AF_UNIX;
+		cli_fd = socket(AF_UNIX, SOCK_STREAM, 0);
+		ASSERT_LE(0, cli_fd);
+
+		size = offsetof(struct sockaddr_un, sun_path) +
+		       strlen(cli_un.sun_path);
+		ASSERT_EQ(0, bind(cli_fd, (struct sockaddr *)&cli_un, size));
+
+		bzero(&cli_un, sizeof(cli_un));
+		cli_un.sun_family = AF_UNIX;
+		snprintf(cli_un.sun_path, sizeof(cli_un.sun_path), "%s",
+			 stream_path);
+		size = offsetof(struct sockaddr_un, sun_path) +
+		       strlen(cli_un.sun_path);
+
+		ASSERT_EQ(0, connect(cli_fd, (struct sockaddr *)&cli_un, size));
+
+		ASSERT_EQ(0, close(cli_fd));
+
+		cli_un_dg.sun_family = AF_UNIX;
+		cli_fd_dg = socket(AF_UNIX, SOCK_DGRAM, 0);
+		ASSERT_LE(0, cli_fd_dg);
+
+		size_dg = offsetof(struct sockaddr_un, sun_path) +
+			  strlen(cli_un_dg.sun_path);
+		ASSERT_EQ(0, bind(cli_fd_dg, (struct sockaddr *)&cli_un_dg,
+				  size_dg));
+
+		bzero(&cli_un_dg, sizeof(cli_un_dg));
+		cli_un_dg.sun_family = AF_UNIX;
+		snprintf(cli_un_dg.sun_path, sizeof(cli_un_dg.sun_path), "%s",
+			 dgram_path);
+		size_dg = offsetof(struct sockaddr_un, sun_path) +
+			  strlen(cli_un_dg.sun_path);
+
+		ASSERT_EQ(0, connect(cli_fd_dg, (struct sockaddr *)&cli_un_dg,
+				     size_dg));
+
+		ASSERT_EQ(0, close(cli_fd_dg));
+
+		/* check connection with abstract sockets */
+
+		client = socket(AF_UNIX, SOCK_STREAM, 0);
+		dgram_client = socket(AF_UNIX, SOCK_DGRAM, 0);
+
+		ASSERT_NE(-1, client);
+		ASSERT_NE(-1, dgram_client);
+
+		err = connect(client, &(self->stream_address).unix_addr,
+			      (self->stream_address).unix_addr_len);
+		err_dg = connect(dgram_client, &(self->dgram_address).unix_addr,
+				 (self->dgram_address).unix_addr_len);
+		if (variant->domain == SCOPE_SANDBOX) {
+			EXPECT_EQ(-1, err);
+			EXPECT_EQ(-1, err_dg);
+			EXPECT_EQ(EPERM, errno);
+		} else {
+			EXPECT_EQ(0, err);
+			EXPECT_EQ(0, err_dg);
+		}
+		ASSERT_EQ(0, close(client));
+		ASSERT_EQ(0, close(dgram_client));
+
+		_exit(_metadata->exit_code);
+		return;
+	}
+
+	ASSERT_EQ(0, close(pipe_parent[0]));
+
+	recv_data = recv_fd(socket_fds_stream[1]);
+	ASSERT_LE(0, recv_data);
+	ASSERT_LE(0, close(socket_fds_stream[1]));
+
+	/* Sets up a server */
+	srv_un.sun_family = AF_UNIX;
+	snprintf(srv_un.sun_path, sizeof(srv_un.sun_path), "%s", stream_path);
+
+	ASSERT_EQ(0, unlink(stream_path));
+	srv_fd = socket(AF_UNIX, SOCK_STREAM, 0);
+	ASSERT_LE(0, srv_fd);
+
+	size = offsetof(struct sockaddr_un, sun_path) + strlen(srv_un.sun_path);
+	ASSERT_EQ(0, bind(srv_fd, (struct sockaddr *)&srv_un, size));
+	ASSERT_EQ(0, listen(srv_fd, 10));
+
+	/* set up a datagram server */
+	srv_un_dg.sun_family = AF_UNIX;
+	snprintf(srv_un_dg.sun_path, sizeof(srv_un_dg.sun_path), "%s",
+		 dgram_path);
+
+	ASSERT_EQ(0, unlink(dgram_path));
+	srv_fd_dg = socket(AF_UNIX, SOCK_DGRAM, 0);
+	ASSERT_LE(0, srv_fd_dg);
+
+	size_dg = offsetof(struct sockaddr_un, sun_path) +
+		  strlen(srv_un_dg.sun_path);
+	ASSERT_EQ(0, bind(srv_fd_dg, (struct sockaddr *)&srv_un_dg, size_dg));
+
+	/*set up abstract servers */
+
+	server = socket(AF_UNIX, SOCK_STREAM, 0);
+	dgram_server = socket(AF_UNIX, SOCK_DGRAM, 0);
+	ASSERT_NE(-1, server);
+	ASSERT_NE(-1, dgram_server);
+
+	ASSERT_EQ(0, bind(server, &(self->stream_address).unix_addr,
+			  self->stream_address.unix_addr_len));
+	ASSERT_EQ(0, bind(dgram_server, &(self->dgram_address).unix_addr,
+			  self->dgram_address.unix_addr_len));
+	ASSERT_EQ(0, listen(server, 32));
+
+	/* signal to child! */
+	ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
+
+	ASSERT_EQ(child, waitpid(child, &status, 0));
+	ASSERT_EQ(0, close(srv_fd));
+	ASSERT_EQ(0, close(srv_fd_dg));
+	ASSERT_EQ(0, close(server));
+	ASSERT_EQ(0, close(dgram_server));
+
+	if (WIFSIGNALED(status) || !WIFEXITED(status) ||
+	    WEXITSTATUS(status) != EXIT_SUCCESS)
+		_metadata->exit_code = KSFT_FAIL;
+}
+
+TEST_HARNESS_MAIN
-- 
2.34.1

Re: [PATCH v8 2/4] selftests/landlock: Abstract unix socket restriction tests

[Mickaël Salaün]

On Thu, Aug 01, 2024 at 10:02:34PM -0600, Tahera Fahimi wrote:
> The patch introduces Landlock ABI version 6 and has four types of tests:
> 1) unix_socket: base tests of the abstract socket scoping mechanism for a
>    landlocked process, same as the ptrace test.
> 2) optional_scoping: generates three processes with different domains and
>    tests if a process with a non-scoped domain can connect to other
>    processes.
> 3) unix_sock_special_cases: since the socket's creator credentials are used
>    for scoping sockets, this test examines the cases where the socket's
>    credentials are different from the process using it.
> 4) pathname_address_sockets: ensures that Unix sockets bound to a
>    null-terminated filesystem can still connect to a socket outside of
>    their scoped domain. This means that even if the domain is scoped with
>    LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET, the socket can connect to a socket
>    outside the scoped domain.
> 
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> ---
> Changes in versions:
> V8:
> - Move tests to scoped_abstract_unix_test.c file.
> - To avoid potential conflicts among Unix socket names in different tests,
>   set_unix_address is added to common.h to set different sun_path for Unix sockets.
> - protocol_variant and service_fixture structures are also moved to common.h
> - Adding pathname_address_sockets to cover all types of address formats
>   for unix sockets, and moving remove_path() to common.h to reuse in this test.
> V7:
> - Introducing landlock ABI version 6.
> - Adding some edge test cases to optional_scoping test.
> - Using `enum` for different domains in optional_scoping tests.
> - Extend unix_sock_special_cases test cases for connected(SOCK_STREAM) sockets.
> - Modifying inline comments.
> V6:
> - Introducing optional_scoping test which ensures a sandboxed process with a
>   non-scoped domain can still connect to another abstract unix socket(either
>   sandboxed or non-sandboxed).
> - Introducing unix_sock_special_cases test which tests examines scenarios where
>   the connecting sockets have different domain than the process using them.
> V4:
> - Introducing unix_socket to evaluate the basic scoping mechanism for abstract
>   unix sockets.
> ---
>  tools/testing/selftests/landlock/base_test.c  |    2 +-
>  tools/testing/selftests/landlock/common.h     |   72 ++
>  tools/testing/selftests/landlock/fs_test.c    |   34 -
>  tools/testing/selftests/landlock/net_test.c   |   31 +-
>  .../landlock/scoped_abstract_unix_test.c      | 1136 +++++++++++++++++
>  5 files changed, 1210 insertions(+), 65 deletions(-)
>  create mode 100644 tools/testing/selftests/landlock/scoped_abstract_unix_test.c

> diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
> index 3c1e9f35b531..52b00472a487 100644
> --- a/tools/testing/selftests/landlock/base_test.c
> +++ b/tools/testing/selftests/landlock/base_test.c
> @@ -75,7 +75,7 @@ TEST(abi_version)
>  	const struct landlock_ruleset_attr ruleset_attr = {
>  		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
>  	};
> -	ASSERT_EQ(5, landlock_create_ruleset(NULL, 0,
> +	ASSERT_EQ(6, landlock_create_ruleset(NULL, 0,
>  					     LANDLOCK_CREATE_RULESET_VERSION));
>  
>  	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
> diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h
> index 7e2b431b9f90..a0c8126d94b4 100644
> --- a/tools/testing/selftests/landlock/common.h
> +++ b/tools/testing/selftests/landlock/common.h
> @@ -16,8 +16,11 @@
>  #include <sys/types.h>
>  #include <sys/wait.h>
>  #include <unistd.h>
> +#include <sys/un.h>
> +#include <arpa/inet.h>
>  
>  #include "../kselftest_harness.h"
> +#define TMP_DIR "tmp"
>  
>  #ifndef __maybe_unused
>  #define __maybe_unused __attribute__((__unused__))
> @@ -226,3 +229,72 @@ enforce_ruleset(struct __test_metadata *const _metadata, const int ruleset_fd)
>  		TH_LOG("Failed to enforce ruleset: %s", strerror(errno));
>  	}
>  }
> +
> +struct protocol_variant {
> +	int domain;
> +	int type;
> +};
> +
> +struct service_fixture {
> +	struct protocol_variant protocol;
> +	/* port is also stored in ipv4_addr.sin_port or ipv6_addr.sin6_port */
> +	unsigned short port;
> +	union {
> +		struct sockaddr_in ipv4_addr;
> +		struct sockaddr_in6 ipv6_addr;
> +		struct {
> +			struct sockaddr_un unix_addr;
> +			socklen_t unix_addr_len;
> +		};
> +	};
> +};
> +
> +static pid_t __maybe_unused sys_gettid(void)
> +{
> +	return syscall(__NR_gettid);
> +}
> +
> +static void __maybe_unused set_unix_address(struct service_fixture *const srv,
> +					    const unsigned short index)
> +{
> +	srv->unix_addr.sun_family = AF_UNIX;
> +	sprintf(srv->unix_addr.sun_path,
> +		"_selftests-landlock-abstract-unix-tid%d-index%d", sys_gettid(),
> +		index);
> +	srv->unix_addr_len = SUN_LEN(&srv->unix_addr);
> +	srv->unix_addr.sun_path[0] = '\0';
> +}
> +
> +static int __maybe_unused remove_path(const char *const path)

We don't need to import remove_path() for these simple filesystem
actions.

> +{
> +	char *walker;
> +	int i, ret, err = 0;
> +
> +	walker = strdup(path);
> +	if (!walker) {
> +		err = ENOMEM;
> +		goto out;
> +	}
> +	if (unlink(path) && rmdir(path)) {
> +		if (errno != ENOENT && errno != ENOTDIR)
> +			err = errno;
> +		goto out;
> +	}
> +	for (i = strlen(walker); i > 0; i--) {
> +		if (walker[i] != '/')
> +			continue;
> +		walker[i] = '\0';
> +		ret = rmdir(walker);
> +		if (ret) {
> +			if (errno != ENOTEMPTY && errno != EBUSY)
> +				err = errno;
> +			goto out;
> +		}
> +		if (strcmp(walker, TMP_DIR) == 0)
> +			goto out;
> +	}
> +
> +out:
> +	free(walker);
> +	return err;
> +}
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 7d063c652be1..fc74cab6dfb8 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -221,40 +221,6 @@ static void create_file(struct __test_metadata *const _metadata,
>  	}
>  }
>  
> -static int remove_path(const char *const path)
> -{
> -	char *walker;
> -	int i, ret, err = 0;
> -
> -	walker = strdup(path);
> -	if (!walker) {
> -		err = ENOMEM;
> -		goto out;
> -	}
> -	if (unlink(path) && rmdir(path)) {
> -		if (errno != ENOENT && errno != ENOTDIR)
> -			err = errno;
> -		goto out;
> -	}
> -	for (i = strlen(walker); i > 0; i--) {
> -		if (walker[i] != '/')
> -			continue;
> -		walker[i] = '\0';
> -		ret = rmdir(walker);
> -		if (ret) {
> -			if (errno != ENOTEMPTY && errno != EBUSY)
> -				err = errno;
> -			goto out;
> -		}
> -		if (strcmp(walker, TMP_DIR) == 0)
> -			goto out;
> -	}
> -
> -out:
> -	free(walker);
> -	return err;
> -}
> -
>  struct mnt_opt {
>  	const char *const source;
>  	const char *const type;
> diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
> index f21cfbbc3638..4e0aeb53b225 100644
> --- a/tools/testing/selftests/landlock/net_test.c
> +++ b/tools/testing/selftests/landlock/net_test.c
> @@ -36,30 +36,6 @@ enum sandbox_type {
>  	TCP_SANDBOX,
>  };
>  
> -struct protocol_variant {
> -	int domain;
> -	int type;
> -};
> -
> -struct service_fixture {
> -	struct protocol_variant protocol;
> -	/* port is also stored in ipv4_addr.sin_port or ipv6_addr.sin6_port */
> -	unsigned short port;
> -	union {
> -		struct sockaddr_in ipv4_addr;
> -		struct sockaddr_in6 ipv6_addr;
> -		struct {
> -			struct sockaddr_un unix_addr;
> -			socklen_t unix_addr_len;
> -		};
> -	};
> -};
> -
> -static pid_t sys_gettid(void)
> -{
> -	return syscall(__NR_gettid);
> -}
> -
>  static int set_service(struct service_fixture *const srv,
>  		       const struct protocol_variant prot,
>  		       const unsigned short index)
> @@ -92,12 +68,7 @@ static int set_service(struct service_fixture *const srv,
>  		return 0;
>  
>  	case AF_UNIX:
> -		srv->unix_addr.sun_family = prot.domain;
> -		sprintf(srv->unix_addr.sun_path,
> -			"_selftests-landlock-net-tid%d-index%d", sys_gettid(),
> -			index);
> -		srv->unix_addr_len = SUN_LEN(&srv->unix_addr);
> -		srv->unix_addr.sun_path[0] = '\0';
> +		set_unix_address(srv, index);

Please create a standalone patch for existing code refactoring, and then
another patch for the new tests.

>  		return 0;
>  	}
>  	return 1;
> diff --git a/tools/testing/selftests/landlock/scoped_abstract_unix_test.c b/tools/testing/selftests/landlock/scoped_abstract_unix_test.c
> new file mode 100644
> index 000000000000..ffa1f01dbbce
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/scoped_abstract_unix_test.c

> +TEST_F(unix_socket, abstract_unix_socket)
> +{
> +	int status;
> +	pid_t child;
> +	bool can_connect_to_parent, can_connect_to_child;
> +	int err, err_dgram;
> +	int pipe_child[2], pipe_parent[2];
> +	char buf_parent;
> +
> +	/*
> +	 * can_connect_to_child is true if a parent process can connect to its
> +	 * child process. The parent process is not isolated from the child
> +	 * with a dedicated Landlock domain.
> +	 */
> +	can_connect_to_child = !variant->domain_parent;
> +	/*
> +	 * can_connect_to_parent is true if a child process can connect to its
> +	 * parent process. This depends on the child process is not isolated from
> +	 * the parent with a dedicated Landlock domain.
> +	 */
> +	can_connect_to_parent = !variant->domain_child;
> +
> +	ASSERT_EQ(0, pipe2(pipe_child, O_CLOEXEC));
> +	ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC));
> +	if (variant->domain_both) {
> +		create_unix_domain(_metadata);
> +		if (!__test_passed(_metadata))
> +			return;
> +	}
> +
> +	child = fork();
> +	ASSERT_LE(0, child);
> +	if (child == 0) {
> +		char buf_child;
> +
> +		ASSERT_EQ(0, close(pipe_parent[1]));
> +		ASSERT_EQ(0, close(pipe_child[0]));
> +		if (variant->domain_child)
> +			create_unix_domain(_metadata);
> +
> +		/* Waits for the parent to be in a domain, if any. */
> +		ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
> +
> +		if (variant->connect_to_parent) {
> +			self->client = socket(AF_UNIX, SOCK_STREAM, 0);
> +			self->dgram_client = socket(AF_UNIX, SOCK_DGRAM, 0);
> +
> +			ASSERT_NE(-1, self->client);
> +			ASSERT_NE(-1, self->dgram_client);
> +			ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
> +
> +			err = connect(self->client,
> +				      &self->stream_address.unix_addr,
> +				      (self->stream_address).unix_addr_len);
> +			err_dgram =
> +				connect(self->dgram_client,
> +					&self->dgram_address.unix_addr,
> +					(self->dgram_address).unix_addr_len);
> +
> +			if (can_connect_to_parent) {
> +				EXPECT_EQ(0, err);
> +				EXPECT_EQ(0, err_dgram);
> +			} else {
> +				EXPECT_EQ(-1, err);
> +				EXPECT_EQ(-1, err_dgram);
> +				EXPECT_EQ(EPERM, errno);
> +			}
> +		} else {
> +			self->server = socket(AF_UNIX, SOCK_STREAM, 0);
> +			self->dgram_server = socket(AF_UNIX, SOCK_DGRAM, 0);
> +			ASSERT_NE(-1, self->server);
> +			ASSERT_NE(-1, self->dgram_server);
> +
> +			ASSERT_EQ(0,
> +				  bind(self->server,
> +				       &self->stream_address.unix_addr,
> +				       (self->stream_address).unix_addr_len));
> +			ASSERT_EQ(0, bind(self->dgram_server,
> +					  &self->dgram_address.unix_addr,
> +					  (self->dgram_address).unix_addr_len));
> +			ASSERT_EQ(0, listen(self->server, 32));

Please, avoid the use of hardcoded values (e.g. in all listen calls).
You can create a global backlog variable like in net_test.c (just copy
it, no need to include it in common.h).

> +
> +			/* signal to parent that child is listening */
> +			ASSERT_EQ(1, write(pipe_child[1], ".", 1));
> +			/* wait to connect */
> +			ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
> +		}
> +		_exit(_metadata->exit_code);
> +		return;
> +	}
> +
> +	ASSERT_EQ(0, close(pipe_child[1]));
> +	ASSERT_EQ(0, close(pipe_parent[0]));
> +
> +	if (variant->domain_parent)
> +		create_unix_domain(_metadata);
> +
> +	/* Signals that the parent is in a domain, if any. */
> +	ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
> +
> +	if (!variant->connect_to_parent) {
> +		self->client = socket(AF_UNIX, SOCK_STREAM, 0);
> +		self->dgram_client = socket(AF_UNIX, SOCK_DGRAM, 0);
> +
> +		ASSERT_NE(-1, self->client);
> +		ASSERT_NE(-1, self->dgram_client);
> +
> +		/* Waits for the child to listen */
> +		ASSERT_EQ(1, read(pipe_child[0], &buf_parent, 1));
> +		err = connect(self->client, &self->stream_address.unix_addr,
> +			      (self->stream_address).unix_addr_len);
> +		err_dgram = connect(self->dgram_client,
> +				    &self->dgram_address.unix_addr,
> +				    (self->dgram_address).unix_addr_len);
> +
> +		if (can_connect_to_child) {
> +			EXPECT_EQ(0, err);
> +			EXPECT_EQ(0, err_dgram);
> +		} else {
> +			EXPECT_EQ(-1, err);
> +			EXPECT_EQ(-1, err_dgram);
> +			EXPECT_EQ(EPERM, errno);
> +		}
> +		ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
> +	} else {
> +		self->server = socket(AF_UNIX, SOCK_STREAM, 0);
> +		self->dgram_server = socket(AF_UNIX, SOCK_DGRAM, 0);
> +		ASSERT_NE(-1, self->server);
> +		ASSERT_NE(-1, self->dgram_server);
> +		ASSERT_EQ(0, bind(self->server, &self->stream_address.unix_addr,
> +				  (self->stream_address).unix_addr_len));
> +		ASSERT_EQ(0, bind(self->dgram_server,
> +				  &self->dgram_address.unix_addr,
> +				  (self->dgram_address).unix_addr_len));
> +		ASSERT_EQ(0, listen(self->server, 32));
> +
> +		/* signal to child that parent is listening */
> +		ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
> +	}
> +
> +	ASSERT_EQ(child, waitpid(child, &status, 0));
> +
> +	if (WIFSIGNALED(status) || !WIFEXITED(status) ||
> +	    WEXITSTATUS(status) != EXIT_SUCCESS)
> +		_metadata->exit_code = KSFT_FAIL;
> +}
> +
> +enum sandbox_type {
> +	NO_SANDBOX,
> +	SCOPE_SANDBOX,
> +	/* Any other type of sandboxing domain */
> +	OTHER_SANDBOX,
> +};
> +
> +/* clang-format off */
> +FIXTURE(optional_scoping)
> +{
> +	struct service_fixture parent_address, child_address;

> +	int parent_server, child_server, client;

The variables should be local ones.

> +};
> +
> +/* clang-format on */
> +FIXTURE_VARIANT(optional_scoping)
> +{
> +	const int domain_all;
> +	const int domain_parent;
> +	const int domain_children;
> +	const int domain_child;

> +	const int domain_grand_child;

domain_grand_child is always NO_SANDBOX in the variants.


> +	const int type;
> +};
> +
> +FIXTURE_SETUP(optional_scoping)

s/optional_scoping/scoping/ ?

> +{
> +	memset(&self->parent_address, 0, sizeof(self->parent_address));
> +	memset(&self->child_address, 0, sizeof(self->child_address));
> +
> +	set_unix_address(&self->parent_address, 0);
> +	set_unix_address(&self->child_address, 1);
> +}

> +TEST_F(optional_scoping, unix_scoping)
> +{
> +	pid_t child;
> +	int status;
> +	bool can_connect_to_parent, can_connect_to_child;
> +	int pipe_parent[2];
> +
> +	if (variant->domain_grand_child == SCOPE_SANDBOX)
> +		can_connect_to_child = false;

We never go through this case.

> +	else
> +		can_connect_to_child = true;
> +
> +	if (!can_connect_to_child || variant->domain_children == SCOPE_SANDBOX)
> +		can_connect_to_parent = false;
> +	else
> +		can_connect_to_parent = true;

We never go through this case.

> +
> +	ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC));
> +
> +	if (variant->domain_all == OTHER_SANDBOX)
> +		create_fs_domain(_metadata);
> +	else if (variant->domain_all == SCOPE_SANDBOX)
> +		create_unix_domain(_metadata);
> +
> +	child = fork();
> +	ASSERT_LE(0, child);
> +	if (child == 0) {
> +		int pipe_child[2];
> +
> +		ASSERT_EQ(0, pipe2(pipe_child, O_CLOEXEC));
> +		pid_t grand_child;
> +
> +		if (variant->domain_children == OTHER_SANDBOX)
> +			create_fs_domain(_metadata);
> +		else if (variant->domain_children == SCOPE_SANDBOX)
> +			create_unix_domain(_metadata);
> +
> +		grand_child = fork();
> +		ASSERT_LE(0, grand_child);
> +		if (grand_child == 0) {
> +			ASSERT_EQ(0, close(pipe_parent[1]));
> +			ASSERT_EQ(0, close(pipe_child[1]));
> +
> +			char buf1, buf2;
> +			int err;
> +
> +			if (variant->domain_grand_child == OTHER_SANDBOX)
> +				create_fs_domain(_metadata);
> +			else if (variant->domain_grand_child == SCOPE_SANDBOX)
> +				create_unix_domain(_metadata);
> +
> +			self->client = socket(AF_UNIX, variant->type, 0);
> +			ASSERT_NE(-1, self->client);
> +
> +			ASSERT_EQ(1, read(pipe_child[0], &buf2, 1));
> +			err = connect(self->client,
> +				      &self->child_address.unix_addr,

> +				      (self->child_address).unix_addr_len);

No need for these extra parenthesis.

> +			if (can_connect_to_child) {
> +				EXPECT_EQ(0, err);
> +			} else {
> +				EXPECT_EQ(-1, err);
> +				EXPECT_EQ(EPERM, errno);
> +			}
> +
> +			if (variant->type == SOCK_STREAM) {
> +				EXPECT_EQ(0, close(self->client));
> +				self->client =
> +					socket(AF_UNIX, variant->type, 0);
> +				ASSERT_NE(-1, self->client);
> +			}
> +
> +			ASSERT_EQ(1, read(pipe_parent[0], &buf1, 1));
> +			err = connect(self->client,
> +				      &self->parent_address.unix_addr,
> +				      (self->parent_address).unix_addr_len);
> +			if (can_connect_to_parent) {
> +				EXPECT_EQ(0, err);
> +			} else {
> +				EXPECT_EQ(-1, err);
> +				EXPECT_EQ(EPERM, errno);
> +			}
> +			EXPECT_EQ(0, close(self->client));
> +
> +			_exit(_metadata->exit_code);
> +			return;
> +		}
> +
> +		ASSERT_EQ(0, close(pipe_child[0]));
> +		if (variant->domain_child == OTHER_SANDBOX)
> +			create_fs_domain(_metadata);
> +		else if (variant->domain_child == SCOPE_SANDBOX)
> +			create_unix_domain(_metadata);
> +
> +		self->child_server = socket(AF_UNIX, variant->type, 0);
> +		ASSERT_NE(-1, self->child_server);
> +		ASSERT_EQ(0, bind(self->child_server,
> +				  &self->child_address.unix_addr,
> +				  (self->child_address).unix_addr_len));
> +		if (variant->type == SOCK_STREAM)
> +			ASSERT_EQ(0, listen(self->child_server, 32));
> +
> +		ASSERT_EQ(1, write(pipe_child[1], ".", 1));
> +		ASSERT_EQ(grand_child, waitpid(grand_child, &status, 0));
> +		return;
> +	}
> +	ASSERT_EQ(0, close(pipe_parent[0]));
> +
> +	if (variant->domain_parent == OTHER_SANDBOX)
> +		create_fs_domain(_metadata);
> +	else if (variant->domain_parent == SCOPE_SANDBOX)
> +		create_unix_domain(_metadata);
> +
> +	self->parent_server = socket(AF_UNIX, variant->type, 0);
> +	ASSERT_NE(-1, self->parent_server);
> +	ASSERT_EQ(0, bind(self->parent_server, &self->parent_address.unix_addr,
> +			  (self->parent_address).unix_addr_len));
> +
> +	if (variant->type == SOCK_STREAM)
> +		ASSERT_EQ(0, listen(self->parent_server, 32));
> +
> +	ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
> +	ASSERT_EQ(child, waitpid(child, &status, 0));
> +	if (WIFSIGNALED(status) || !WIFEXITED(status) ||
> +	    WEXITSTATUS(status) != EXIT_SUCCESS)
> +		_metadata->exit_code = KSFT_FAIL;
> +}

> +FIXTURE_SETUP(pathname_address_sockets)
> +{
> +	/* setup abstract addresses */
> +	memset(&self->stream_address, 0, sizeof(self->stream_address));
> +	set_unix_address(&self->stream_address, 0);
> +
> +	memset(&self->dgram_address, 0, sizeof(self->dgram_address));
> +	set_unix_address(&self->dgram_address, 0);
> +
> +	const char *s_path = variant->stream_path;
> +	const char *dg_path = variant->dgram_path;
> +
> +	disable_caps(_metadata);
> +	umask(0077);
> +	ASSERT_EQ(0, mkdir(TMP_DIR, 0700));
> +
> +	ASSERT_EQ(0, mknod(s_path, S_IFREG | 0700, 0))
> +	{
> +		TH_LOG("Failed to create file \"%s\": %s", s_path,
> +		       strerror(errno));
> +		remove_path(TMP_DIR);

Just use rmdir() instead of remove_path() everywhere.

> +	}
> +	ASSERT_EQ(0, mknod(dg_path, S_IFREG | 0700, 0))
> +	{
> +		TH_LOG("Failed to create file \"%s\": %s", dg_path,
> +		       strerror(errno));
> +		remove_path(TMP_DIR);
> +	}
> +}
> +
> +FIXTURE_TEARDOWN(pathname_address_sockets)
> +{
> +	const char *s_path = variant->stream_path;
> +	const char *dg_path = variant->dgram_path;
> +
> +	ASSERT_EQ(0, remove_path(dg_path));

Just use unlink() and rmdir().

> +	ASSERT_EQ(0, remove_path(s_path));
> +	ASSERT_EQ(0, remove_path(TMP_DIR));
> +}

[PATCH v8 3/4] sample/Landlock: Support abstract unix socket restriction

[Tahera Fahimi]

A sandboxer can receive the character "a" as input from the environment
variable LL_SCOPE to restrict the abstract unix sockets from connecting
to a process outside its scoped domain.

Example
=======
Create an abstract unix socket to listen with socat(1):
socat abstract-listen:mysocket -
Create a sandboxed shell and pass the character "a" to LL_SCOPED:
LL_FS_RO=/ LL_FS_RW=. LL_SCOPED="a" ./sandboxer /bin/bash
If the sandboxed process tries to connect to the listening socket
with command "socat - abstract-connect:mysocket", the connection
will fail.

Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>

---
v8:
- Adding check_ruleset_scope function to parse the scope environment
  variable and update the landlock attribute based on the restriction
  provided by the user.
- Adding Mickaël Salaün reviews on version 7.

v7:
- Adding IPC scoping to the sandbox demo by defining a new "LL_SCOPED"
  environment variable. "LL_SCOPED" gets value "a" to restrict abstract
  unix sockets.
- Change LANDLOCK_ABI_LAST to 6.
---
 samples/landlock/sandboxer.c | 56 +++++++++++++++++++++++++++++++++---
 1 file changed, 52 insertions(+), 4 deletions(-)

diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index e8223c3e781a..98132fd823ad 100644
--- a/samples/landlock/sandboxer.c
+++ b/samples/landlock/sandboxer.c
@@ -14,6 +14,7 @@
 #include <fcntl.h>
 #include <linux/landlock.h>
 #include <linux/prctl.h>
+#include <linux/socket.h>
 #include <stddef.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -22,6 +23,7 @@
 #include <sys/stat.h>
 #include <sys/syscall.h>
 #include <unistd.h>
+#include <stdbool.h>
 
 #ifndef landlock_create_ruleset
 static inline int
@@ -55,6 +57,7 @@ static inline int landlock_restrict_self(const int ruleset_fd,
 #define ENV_FS_RW_NAME "LL_FS_RW"
 #define ENV_TCP_BIND_NAME "LL_TCP_BIND"
 #define ENV_TCP_CONNECT_NAME "LL_TCP_CONNECT"
+#define ENV_SCOPED_NAME "LL_SCOPED"
 #define ENV_DELIMITER ":"
 
 static int parse_path(char *env_path, const char ***const path_list)
@@ -184,6 +187,38 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
 	return ret;
 }
 
+static bool check_ruleset_scope(const char *const env_var,
+				struct landlock_ruleset_attr *ruleset_attr)
+{
+	bool ret = true;
+	char *env_type_scope, *env_type_scope_next, *ipc_scoping_name;
+
+	ruleset_attr->scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
+	env_type_scope = getenv(env_var);
+	/* scoping is not supported by the user */
+	if (!env_type_scope)
+		return true;
+	env_type_scope = strdup(env_type_scope);
+	unsetenv(env_var);
+
+	env_type_scope_next = env_type_scope;
+	while ((ipc_scoping_name =
+			strsep(&env_type_scope_next, ENV_DELIMITER))) {
+		if (strcmp("a", ipc_scoping_name) == 0) {
+			ruleset_attr->scoped |=
+				LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
+		} else {
+			fprintf(stderr, "Unsupported scoping \"%s\"\n",
+				ipc_scoping_name);
+			ret = false;
+			goto out_free_name;
+		}
+	}
+out_free_name:
+	free(env_type_scope);
+	return ret;
+}
+
 /* clang-format off */
 
 #define ACCESS_FS_ROUGHLY_READ ( \
@@ -208,7 +243,7 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
 
 /* clang-format on */
 
-#define LANDLOCK_ABI_LAST 5
+#define LANDLOCK_ABI_LAST 6
 
 int main(const int argc, char *const argv[], char *const *const envp)
 {
@@ -223,14 +258,15 @@ int main(const int argc, char *const argv[], char *const *const envp)
 		.handled_access_fs = access_fs_rw,
 		.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
 				      LANDLOCK_ACCESS_NET_CONNECT_TCP,
+		.scoped = LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET,
 	};
 
 	if (argc < 2) {
 		fprintf(stderr,
-			"usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\"%s "
+			"usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s "
 			"<cmd> [args]...\n\n",
 			ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
-			ENV_TCP_CONNECT_NAME, argv[0]);
+			ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv[0]);
 		fprintf(stderr,
 			"Execute a command in a restricted environment.\n\n");
 		fprintf(stderr,
@@ -251,15 +287,18 @@ int main(const int argc, char *const argv[], char *const *const envp)
 		fprintf(stderr,
 			"* %s: list of ports allowed to connect (client).\n",
 			ENV_TCP_CONNECT_NAME);
+		fprintf(stderr, "* %s: list of restrictions on IPCs.\n",
+			ENV_SCOPED_NAME);
 		fprintf(stderr,
 			"\nexample:\n"
 			"%s=\"${PATH}:/lib:/usr:/proc:/etc:/dev/urandom\" "
 			"%s=\"/dev/null:/dev/full:/dev/zero:/dev/pts:/tmp\" "
 			"%s=\"9418\" "
 			"%s=\"80:443\" "
+			"%s=\"a\" "
 			"%s bash -i\n\n",
 			ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
-			ENV_TCP_CONNECT_NAME, argv[0]);
+			ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv[0]);
 		fprintf(stderr,
 			"This sandboxer can use Landlock features "
 			"up to ABI version %d.\n",
@@ -327,6 +366,10 @@ int main(const int argc, char *const argv[], char *const *const envp)
 		/* Removes LANDLOCK_ACCESS_FS_IOCTL_DEV for ABI < 5 */
 		ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL_DEV;
 
+		__attribute__((fallthrough));
+	case 5:
+		/* Removes LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET for ABI < 6 */
+		ruleset_attr.scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
 		fprintf(stderr,
 			"Hint: You should update the running kernel "
 			"to leverage Landlock features "
@@ -358,6 +401,11 @@ int main(const int argc, char *const argv[], char *const *const envp)
 			~LANDLOCK_ACCESS_NET_CONNECT_TCP;
 	}
 
+	if (!check_ruleset_scope(ENV_SCOPED_NAME, &ruleset_attr)) {
+		perror("Unsupported IPC scoping requested");
+		return 1;
+	}
+
 	ruleset_fd =
 		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
 	if (ruleset_fd < 0) {
-- 
2.34.1

Re: [PATCH v8 3/4] sample/Landlock: Support abstract unix socket restriction

[Mickaël Salaün]

On Thu, Aug 01, 2024 at 10:02:35PM -0600, Tahera Fahimi wrote:
> A sandboxer can receive the character "a" as input from the environment
> variable LL_SCOPE to restrict the abstract unix sockets from connecting
> to a process outside its scoped domain.
> 
> Example
> =======
> Create an abstract unix socket to listen with socat(1):
> socat abstract-listen:mysocket -
> Create a sandboxed shell and pass the character "a" to LL_SCOPED:
> LL_FS_RO=/ LL_FS_RW=. LL_SCOPED="a" ./sandboxer /bin/bash
> If the sandboxed process tries to connect to the listening socket
> with command "socat - abstract-connect:mysocket", the connection
> will fail.
> 
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> 
> ---
> v8:
> - Adding check_ruleset_scope function to parse the scope environment
>   variable and update the landlock attribute based on the restriction
>   provided by the user.
> - Adding Mickaël Salaün reviews on version 7.
> 
> v7:
> - Adding IPC scoping to the sandbox demo by defining a new "LL_SCOPED"
>   environment variable. "LL_SCOPED" gets value "a" to restrict abstract
>   unix sockets.
> - Change LANDLOCK_ABI_LAST to 6.
> ---
>  samples/landlock/sandboxer.c | 56 +++++++++++++++++++++++++++++++++---
>  1 file changed, 52 insertions(+), 4 deletions(-)
> 
> diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
> index e8223c3e781a..98132fd823ad 100644
> --- a/samples/landlock/sandboxer.c
> +++ b/samples/landlock/sandboxer.c
> @@ -14,6 +14,7 @@
>  #include <fcntl.h>
>  #include <linux/landlock.h>
>  #include <linux/prctl.h>
> +#include <linux/socket.h>
>  #include <stddef.h>
>  #include <stdio.h>
>  #include <stdlib.h>
> @@ -22,6 +23,7 @@
>  #include <sys/stat.h>
>  #include <sys/syscall.h>
>  #include <unistd.h>
> +#include <stdbool.h>
>  
>  #ifndef landlock_create_ruleset
>  static inline int
> @@ -55,6 +57,7 @@ static inline int landlock_restrict_self(const int ruleset_fd,
>  #define ENV_FS_RW_NAME "LL_FS_RW"
>  #define ENV_TCP_BIND_NAME "LL_TCP_BIND"
>  #define ENV_TCP_CONNECT_NAME "LL_TCP_CONNECT"
> +#define ENV_SCOPED_NAME "LL_SCOPED"
>  #define ENV_DELIMITER ":"
>  
>  static int parse_path(char *env_path, const char ***const path_list)
> @@ -184,6 +187,38 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
>  	return ret;
>  }
>  
> +static bool check_ruleset_scope(const char *const env_var,
> +				struct landlock_ruleset_attr *ruleset_attr)
> +{
> +	bool ret = true;
> +	char *env_type_scope, *env_type_scope_next, *ipc_scoping_name;
> +
> +	ruleset_attr->scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;

Why always removing the suported scope?
What happen if ABI < 6 ?

> +	env_type_scope = getenv(env_var);
> +	/* scoping is not supported by the user */
> +	if (!env_type_scope)
> +		return true;
> +	env_type_scope = strdup(env_type_scope);
> +	unsetenv(env_var);
> +
> +	env_type_scope_next = env_type_scope;
> +	while ((ipc_scoping_name =
> +			strsep(&env_type_scope_next, ENV_DELIMITER))) {
> +		if (strcmp("a", ipc_scoping_name) == 0) {
> +			ruleset_attr->scoped |=
> +				LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;

There are two issues here:
1. this would not work for ABI < 6
2. "a" can be repeated several times, which should probably not be
   allowed because we don't want to support this
   unspecified/undocumented behavior.


> +		} else {
> +			fprintf(stderr, "Unsupported scoping \"%s\"\n",
> +				ipc_scoping_name);
> +			ret = false;
> +			goto out_free_name;
> +		}
> +	}
> +out_free_name:
> +	free(env_type_scope);
> +	return ret;
> +}
> +
>  /* clang-format off */
>  
>  #define ACCESS_FS_ROUGHLY_READ ( \
> @@ -208,7 +243,7 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
>  
>  /* clang-format on */
>  
> -#define LANDLOCK_ABI_LAST 5
> +#define LANDLOCK_ABI_LAST 6
>  
>  int main(const int argc, char *const argv[], char *const *const envp)
>  {
> @@ -223,14 +258,15 @@ int main(const int argc, char *const argv[], char *const *const envp)
>  		.handled_access_fs = access_fs_rw,
>  		.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
>  				      LANDLOCK_ACCESS_NET_CONNECT_TCP,
> +		.scoped = LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET,
>  	};
>  
>  	if (argc < 2) {
>  		fprintf(stderr,
> -			"usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\"%s "
> +			"usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s "
>  			"<cmd> [args]...\n\n",
>  			ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
> -			ENV_TCP_CONNECT_NAME, argv[0]);
> +			ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv[0]);
>  		fprintf(stderr,
>  			"Execute a command in a restricted environment.\n\n");
>  		fprintf(stderr,
> @@ -251,15 +287,18 @@ int main(const int argc, char *const argv[], char *const *const envp)
>  		fprintf(stderr,
>  			"* %s: list of ports allowed to connect (client).\n",
>  			ENV_TCP_CONNECT_NAME);
> +		fprintf(stderr, "* %s: list of restrictions on IPCs.\n",
> +			ENV_SCOPED_NAME);
>  		fprintf(stderr,
>  			"\nexample:\n"
>  			"%s=\"${PATH}:/lib:/usr:/proc:/etc:/dev/urandom\" "
>  			"%s=\"/dev/null:/dev/full:/dev/zero:/dev/pts:/tmp\" "
>  			"%s=\"9418\" "
>  			"%s=\"80:443\" "
> +			"%s=\"a\" "
>  			"%s bash -i\n\n",
>  			ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
> -			ENV_TCP_CONNECT_NAME, argv[0]);
> +			ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv[0]);
>  		fprintf(stderr,
>  			"This sandboxer can use Landlock features "
>  			"up to ABI version %d.\n",
> @@ -327,6 +366,10 @@ int main(const int argc, char *const argv[], char *const *const envp)
>  		/* Removes LANDLOCK_ACCESS_FS_IOCTL_DEV for ABI < 5 */
>  		ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL_DEV;
>  
> +		__attribute__((fallthrough));
> +	case 5:
> +		/* Removes LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET for ABI < 6 */
> +		ruleset_attr.scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
>  		fprintf(stderr,
>  			"Hint: You should update the running kernel "
>  			"to leverage Landlock features "
> @@ -358,6 +401,11 @@ int main(const int argc, char *const argv[], char *const *const envp)
>  			~LANDLOCK_ACCESS_NET_CONNECT_TCP;
>  	}
>  
> +	if (!check_ruleset_scope(ENV_SCOPED_NAME, &ruleset_attr)) {

You should use the same pattern as for TCP access rigths: if the
environment variable is not set then remove the ruleset's scopes.

> +		perror("Unsupported IPC scoping requested");
> +		return 1;
> +	}
> +
>  	ruleset_fd =
>  		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
>  	if (ruleset_fd < 0) {
> -- 
> 2.34.1
> 
> 

Re: [PATCH v8 3/4] sample/Landlock: Support abstract unix socket restriction

[Tahera Fahimi]

On Fri, Aug 09, 2024 at 04:11:47PM +0200, Mickaël Salaün wrote:
> On Thu, Aug 01, 2024 at 10:02:35PM -0600, Tahera Fahimi wrote:
> > A sandboxer can receive the character "a" as input from the environment
> > variable LL_SCOPE to restrict the abstract unix sockets from connecting
> > to a process outside its scoped domain.
> > 
> > Example
> > =======
> > Create an abstract unix socket to listen with socat(1):
> > socat abstract-listen:mysocket -
> > Create a sandboxed shell and pass the character "a" to LL_SCOPED:
> > LL_FS_RO=/ LL_FS_RW=. LL_SCOPED="a" ./sandboxer /bin/bash
> > If the sandboxed process tries to connect to the listening socket
> > with command "socat - abstract-connect:mysocket", the connection
> > will fail.
> > 
> > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> > 
> > ---
> > v8:
> > - Adding check_ruleset_scope function to parse the scope environment
> >   variable and update the landlock attribute based on the restriction
> >   provided by the user.
> > - Adding Mickaël Salaün reviews on version 7.
> > 
> > v7:
> > - Adding IPC scoping to the sandbox demo by defining a new "LL_SCOPED"
> >   environment variable. "LL_SCOPED" gets value "a" to restrict abstract
> >   unix sockets.
> > - Change LANDLOCK_ABI_LAST to 6.
> > ---
> >  samples/landlock/sandboxer.c | 56 +++++++++++++++++++++++++++++++++---
> >  1 file changed, 52 insertions(+), 4 deletions(-)
> > 
> > diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
> > index e8223c3e781a..98132fd823ad 100644
> > --- a/samples/landlock/sandboxer.c
> > +++ b/samples/landlock/sandboxer.c
> > @@ -14,6 +14,7 @@
> >  #include <fcntl.h>
> >  #include <linux/landlock.h>
> >  #include <linux/prctl.h>
> > +#include <linux/socket.h>
> >  #include <stddef.h>
> >  #include <stdio.h>
> >  #include <stdlib.h>
> > @@ -22,6 +23,7 @@
> >  #include <sys/stat.h>
> >  #include <sys/syscall.h>
> >  #include <unistd.h>
> > +#include <stdbool.h>
> >  
> >  #ifndef landlock_create_ruleset
> >  static inline int
> > @@ -55,6 +57,7 @@ static inline int landlock_restrict_self(const int ruleset_fd,
> >  #define ENV_FS_RW_NAME "LL_FS_RW"
> >  #define ENV_TCP_BIND_NAME "LL_TCP_BIND"
> >  #define ENV_TCP_CONNECT_NAME "LL_TCP_CONNECT"
> > +#define ENV_SCOPED_NAME "LL_SCOPED"
> >  #define ENV_DELIMITER ":"
> >  
> >  static int parse_path(char *env_path, const char ***const path_list)
> > @@ -184,6 +187,38 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
> >  	return ret;
> >  }
> >  
> > +static bool check_ruleset_scope(const char *const env_var,
> > +				struct landlock_ruleset_attr *ruleset_attr)
> > +{
> > +	bool ret = true;
> > +	char *env_type_scope, *env_type_scope_next, *ipc_scoping_name;
> > +
> > +	ruleset_attr->scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
> 
> Why always removing the suported scope?
> What happen if ABI < 6 ?
Right, I will add this check before calling chek_ruleset_scope function.

> > +	env_type_scope = getenv(env_var);
> > +	/* scoping is not supported by the user */
> > +	if (!env_type_scope)
> > +		return true;
> > +	env_type_scope = strdup(env_type_scope);
> > +	unsetenv(env_var);
> > +
> > +	env_type_scope_next = env_type_scope;
> > +	while ((ipc_scoping_name =
> > +			strsep(&env_type_scope_next, ENV_DELIMITER))) {
> > +		if (strcmp("a", ipc_scoping_name) == 0) {
> > +			ruleset_attr->scoped |=
> > +				LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
> 
> There are two issues here:
> 1. this would not work for ABI < 6
> 2. "a" can be repeated several times, which should probably not be
>    allowed because we don't want to support this
>    unspecified/undocumented behavior.
For the second note, I think even if the user provides multiple "a"
(something like "a:a"), It would not have a different effect (for now).
Do you suggest that I change this way of handeling this environment
variable or add documents that mention this note?
> 
> > +		} else {
> > +			fprintf(stderr, "Unsupported scoping \"%s\"\n",
> > +				ipc_scoping_name);
> > +			ret = false;
> > +			goto out_free_name;
> > +		}
> > +	}
> > +out_free_name:
> > +	free(env_type_scope);
> > +	return ret;
> > +}
> > +
> >  /* clang-format off */
> >  
> >  #define ACCESS_FS_ROUGHLY_READ ( \
> > @@ -208,7 +243,7 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
> >  
> >  /* clang-format on */
> >  
> > -#define LANDLOCK_ABI_LAST 5
> > +#define LANDLOCK_ABI_LAST 6
> >  
> >  int main(const int argc, char *const argv[], char *const *const envp)
> >  {
> > @@ -223,14 +258,15 @@ int main(const int argc, char *const argv[], char *const *const envp)
> >  		.handled_access_fs = access_fs_rw,
> >  		.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
> >  				      LANDLOCK_ACCESS_NET_CONNECT_TCP,
> > +		.scoped = LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET,
> >  	};
> >  
> >  	if (argc < 2) {
> >  		fprintf(stderr,
> > -			"usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\"%s "
> > +			"usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s "
> >  			"<cmd> [args]...\n\n",
> >  			ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
> > -			ENV_TCP_CONNECT_NAME, argv[0]);
> > +			ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv[0]);
> >  		fprintf(stderr,
> >  			"Execute a command in a restricted environment.\n\n");
> >  		fprintf(stderr,
> > @@ -251,15 +287,18 @@ int main(const int argc, char *const argv[], char *const *const envp)
> >  		fprintf(stderr,
> >  			"* %s: list of ports allowed to connect (client).\n",
> >  			ENV_TCP_CONNECT_NAME);
> > +		fprintf(stderr, "* %s: list of restrictions on IPCs.\n",
> > +			ENV_SCOPED_NAME);
> >  		fprintf(stderr,
> >  			"\nexample:\n"
> >  			"%s=\"${PATH}:/lib:/usr:/proc:/etc:/dev/urandom\" "
> >  			"%s=\"/dev/null:/dev/full:/dev/zero:/dev/pts:/tmp\" "
> >  			"%s=\"9418\" "
> >  			"%s=\"80:443\" "
> > +			"%s=\"a\" "
> >  			"%s bash -i\n\n",
> >  			ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
> > -			ENV_TCP_CONNECT_NAME, argv[0]);
> > +			ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv[0]);
> >  		fprintf(stderr,
> >  			"This sandboxer can use Landlock features "
> >  			"up to ABI version %d.\n",
> > @@ -327,6 +366,10 @@ int main(const int argc, char *const argv[], char *const *const envp)
> >  		/* Removes LANDLOCK_ACCESS_FS_IOCTL_DEV for ABI < 5 */
> >  		ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL_DEV;
> >  
> > +		__attribute__((fallthrough));
> > +	case 5:
> > +		/* Removes LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET for ABI < 6 */
> > +		ruleset_attr.scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
> >  		fprintf(stderr,
> >  			"Hint: You should update the running kernel "
> >  			"to leverage Landlock features "
> > @@ -358,6 +401,11 @@ int main(const int argc, char *const argv[], char *const *const envp)
> >  			~LANDLOCK_ACCESS_NET_CONNECT_TCP;
> >  	}
> >  
> > +	if (!check_ruleset_scope(ENV_SCOPED_NAME, &ruleset_attr)) {
> 
> You should use the same pattern as for TCP access rigths: if the
> environment variable is not set then remove the ruleset's scopes.
I think this happens in check_ruleset_scope function. However, I will
add a condition (abi >=6) to this "if" statement.

> > +		perror("Unsupported IPC scoping requested");
> > +		return 1;
> > +	}
> > +
> >  	ruleset_fd =
> >  		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> >  	if (ruleset_fd < 0) {
> > -- 
> > 2.34.1
> > 
> > 

Re: [PATCH v8 3/4] sample/Landlock: Support abstract unix socket restriction

[Mickaël Salaün]

On Fri, Aug 09, 2024 at 12:16:37PM -0600, Tahera Fahimi wrote:
> On Fri, Aug 09, 2024 at 04:11:47PM +0200, Mickaël Salaün wrote:
> > On Thu, Aug 01, 2024 at 10:02:35PM -0600, Tahera Fahimi wrote:
> > > A sandboxer can receive the character "a" as input from the environment
> > > variable LL_SCOPE to restrict the abstract unix sockets from connecting
> > > to a process outside its scoped domain.
> > > 
> > > Example
> > > =======
> > > Create an abstract unix socket to listen with socat(1):
> > > socat abstract-listen:mysocket -
> > > Create a sandboxed shell and pass the character "a" to LL_SCOPED:
> > > LL_FS_RO=/ LL_FS_RW=. LL_SCOPED="a" ./sandboxer /bin/bash
> > > If the sandboxed process tries to connect to the listening socket
> > > with command "socat - abstract-connect:mysocket", the connection
> > > will fail.
> > > 
> > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> > > 
> > > ---
> > > v8:
> > > - Adding check_ruleset_scope function to parse the scope environment
> > >   variable and update the landlock attribute based on the restriction
> > >   provided by the user.
> > > - Adding Mickaël Salaün reviews on version 7.
> > > 
> > > v7:
> > > - Adding IPC scoping to the sandbox demo by defining a new "LL_SCOPED"
> > >   environment variable. "LL_SCOPED" gets value "a" to restrict abstract
> > >   unix sockets.
> > > - Change LANDLOCK_ABI_LAST to 6.
> > > ---
> > >  samples/landlock/sandboxer.c | 56 +++++++++++++++++++++++++++++++++---
> > >  1 file changed, 52 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
> > > index e8223c3e781a..98132fd823ad 100644
> > > --- a/samples/landlock/sandboxer.c
> > > +++ b/samples/landlock/sandboxer.c
> > > @@ -14,6 +14,7 @@
> > >  #include <fcntl.h>
> > >  #include <linux/landlock.h>
> > >  #include <linux/prctl.h>
> > > +#include <linux/socket.h>
> > >  #include <stddef.h>
> > >  #include <stdio.h>
> > >  #include <stdlib.h>
> > > @@ -22,6 +23,7 @@
> > >  #include <sys/stat.h>
> > >  #include <sys/syscall.h>
> > >  #include <unistd.h>
> > > +#include <stdbool.h>
> > >  
> > >  #ifndef landlock_create_ruleset
> > >  static inline int
> > > @@ -55,6 +57,7 @@ static inline int landlock_restrict_self(const int ruleset_fd,
> > >  #define ENV_FS_RW_NAME "LL_FS_RW"
> > >  #define ENV_TCP_BIND_NAME "LL_TCP_BIND"
> > >  #define ENV_TCP_CONNECT_NAME "LL_TCP_CONNECT"
> > > +#define ENV_SCOPED_NAME "LL_SCOPED"
> > >  #define ENV_DELIMITER ":"
> > >  
> > >  static int parse_path(char *env_path, const char ***const path_list)
> > > @@ -184,6 +187,38 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
> > >  	return ret;
> > >  }
> > >  
> > > +static bool check_ruleset_scope(const char *const env_var,
> > > +				struct landlock_ruleset_attr *ruleset_attr)
> > > +{
> > > +	bool ret = true;
> > > +	char *env_type_scope, *env_type_scope_next, *ipc_scoping_name;
> > > +
> > > +	ruleset_attr->scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
> > 
> > Why always removing the suported scope?
> > What happen if ABI < 6 ?
> Right, I will add this check before calling chek_ruleset_scope function.
> 
> > > +	env_type_scope = getenv(env_var);
> > > +	/* scoping is not supported by the user */
> > > +	if (!env_type_scope)
> > > +		return true;
> > > +	env_type_scope = strdup(env_type_scope);
> > > +	unsetenv(env_var);
> > > +
> > > +	env_type_scope_next = env_type_scope;
> > > +	while ((ipc_scoping_name =
> > > +			strsep(&env_type_scope_next, ENV_DELIMITER))) {
> > > +		if (strcmp("a", ipc_scoping_name) == 0) {
> > > +			ruleset_attr->scoped |=
> > > +				LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
> > 
> > There are two issues here:
> > 1. this would not work for ABI < 6
> > 2. "a" can be repeated several times, which should probably not be
> >    allowed because we don't want to support this
> >    unspecified/undocumented behavior.
> For the second note, I think even if the user provides multiple "a"
> (something like "a:a"), It would not have a different effect (for now).
> Do you suggest that I change this way of handeling this environment
> variable or add documents that mention this note?

We should have a stricter approach to only allow zero or one "a" letter.


> > 
> > > +		} else {
> > > +			fprintf(stderr, "Unsupported scoping \"%s\"\n",
> > > +				ipc_scoping_name);
> > > +			ret = false;
> > > +			goto out_free_name;
> > > +		}
> > > +	}
> > > +out_free_name:
> > > +	free(env_type_scope);
> > > +	return ret;
> > > +}
> > > +
> > >  /* clang-format off */
> > >  
> > >  #define ACCESS_FS_ROUGHLY_READ ( \
> > > @@ -208,7 +243,7 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
> > >  
> > >  /* clang-format on */
> > >  
> > > -#define LANDLOCK_ABI_LAST 5
> > > +#define LANDLOCK_ABI_LAST 6
> > >  
> > >  int main(const int argc, char *const argv[], char *const *const envp)
> > >  {
> > > @@ -223,14 +258,15 @@ int main(const int argc, char *const argv[], char *const *const envp)
> > >  		.handled_access_fs = access_fs_rw,
> > >  		.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
> > >  				      LANDLOCK_ACCESS_NET_CONNECT_TCP,
> > > +		.scoped = LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET,
> > >  	};
> > >  
> > >  	if (argc < 2) {
> > >  		fprintf(stderr,
> > > -			"usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\"%s "
> > > +			"usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s "
> > >  			"<cmd> [args]...\n\n",
> > >  			ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
> > > -			ENV_TCP_CONNECT_NAME, argv[0]);
> > > +			ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv[0]);
> > >  		fprintf(stderr,
> > >  			"Execute a command in a restricted environment.\n\n");
> > >  		fprintf(stderr,
> > > @@ -251,15 +287,18 @@ int main(const int argc, char *const argv[], char *const *const envp)
> > >  		fprintf(stderr,
> > >  			"* %s: list of ports allowed to connect (client).\n",
> > >  			ENV_TCP_CONNECT_NAME);
> > > +		fprintf(stderr, "* %s: list of restrictions on IPCs.\n",
> > > +			ENV_SCOPED_NAME);
> > >  		fprintf(stderr,
> > >  			"\nexample:\n"
> > >  			"%s=\"${PATH}:/lib:/usr:/proc:/etc:/dev/urandom\" "
> > >  			"%s=\"/dev/null:/dev/full:/dev/zero:/dev/pts:/tmp\" "
> > >  			"%s=\"9418\" "
> > >  			"%s=\"80:443\" "
> > > +			"%s=\"a\" "
> > >  			"%s bash -i\n\n",
> > >  			ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
> > > -			ENV_TCP_CONNECT_NAME, argv[0]);
> > > +			ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv[0]);
> > >  		fprintf(stderr,
> > >  			"This sandboxer can use Landlock features "
> > >  			"up to ABI version %d.\n",
> > > @@ -327,6 +366,10 @@ int main(const int argc, char *const argv[], char *const *const envp)
> > >  		/* Removes LANDLOCK_ACCESS_FS_IOCTL_DEV for ABI < 5 */
> > >  		ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL_DEV;
> > >  
> > > +		__attribute__((fallthrough));
> > > +	case 5:
> > > +		/* Removes LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET for ABI < 6 */
> > > +		ruleset_attr.scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
> > >  		fprintf(stderr,
> > >  			"Hint: You should update the running kernel "
> > >  			"to leverage Landlock features "
> > > @@ -358,6 +401,11 @@ int main(const int argc, char *const argv[], char *const *const envp)
> > >  			~LANDLOCK_ACCESS_NET_CONNECT_TCP;
> > >  	}
> > >  
> > > +	if (!check_ruleset_scope(ENV_SCOPED_NAME, &ruleset_attr)) {
> > 
> > You should use the same pattern as for TCP access rigths: if the
> > environment variable is not set then remove the ruleset's scopes.
> I think this happens in check_ruleset_scope function. However, I will
> add a condition (abi >=6) to this "if" statement.
> 
> > > +		perror("Unsupported IPC scoping requested");
> > > +		return 1;
> > > +	}
> > > +
> > >  	ruleset_fd =
> > >  		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> > >  	if (ruleset_fd < 0) {
> > > -- 
> > > 2.34.1
> > > 
> > > 
> 

[PATCH v8 4/4] Landlock: Document LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET and ABI versioning

[Tahera Fahimi]

Introducing LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET as an IPC scoping
mechanism in Landlock ABI version 6, and updating ruleset_attr,
Landlock ABI version, and access rights code blocks based on that.

Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
---
v8:
- Improving documentation by specifying differences between scoped and
  non-scoped domains.
- Adding review notes of version 7.
- Update date
v7:
- Add "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" explanation to IPC scoping
  section and updating ABI to version 6.
- Adding "scoped" attribute to the Access rights section.
- In current limitation, unnamed sockets are specified as sockets that
  are not restricted.
- Update date
---
 Documentation/userspace-api/landlock.rst | 33 ++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
index 07b63aec56fa..d602567b5139 100644
--- a/Documentation/userspace-api/landlock.rst
+++ b/Documentation/userspace-api/landlock.rst
@@ -8,7 +8,7 @@ Landlock: unprivileged access control
 =====================================
 
 :Author: Mickaël Salaün
-:Date: April 2024
+:Date: August 2024
 
 The goal of Landlock is to enable to restrict ambient rights (e.g. global
 filesystem or network access) for a set of processes.  Because Landlock
@@ -81,6 +81,8 @@ to be explicit about the denied-by-default access rights.
         .handled_access_net =
             LANDLOCK_ACCESS_NET_BIND_TCP |
             LANDLOCK_ACCESS_NET_CONNECT_TCP,
+        .scoped =
+            LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET,
     };
 
 Because we may not know on which kernel version an application will be
@@ -119,6 +121,9 @@ version, and only use the available subset of access rights:
     case 4:
         /* Removes LANDLOCK_ACCESS_FS_IOCTL_DEV for ABI < 5 */
         ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL_DEV;
+    case 5:
+        /* Removes LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET for ABI < 6 */
+        ruleset_attr.scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
     }
 
 This enables to create an inclusive ruleset that will contain our rules.
@@ -306,6 +311,23 @@ To be allowed to use :manpage:`ptrace(2)` and related syscalls on a target
 process, a sandboxed process should have a subset of the target process rules,
 which means the tracee must be in a sub-domain of the tracer.
 
+IPC Scoping
+-----------
+
+Similar to the implicit `Ptrace restrictions`_, we may want to further restrict
+interactions between sandboxes. Each Landlock domain can be explicitly scoped
+for a set of actions by specifying it on a ruleset. For example, if a sandboxed
+process should not be able to :manpage:`connect(2)` to a non-sandboxed process
+through abstract :manpage:`unix(7)` sockets, we can specify such restriction
+with ``LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET``.
+
+A sandboxed process can connect to a non-sandboxed process when its domain is
+not scoped. If a process's domain is scoped, it can only connect to processes in
+the same scoped domain.
+
+IPC scoping does not support Landlock rules, so if a domain is scoped, no rules
+can be added to allow accessing to a resource outside of the scoped domain.
+
 Truncating files
 ----------------
 
@@ -404,7 +426,7 @@ Access rights
 -------------
 
 .. kernel-doc:: include/uapi/linux/landlock.h
-    :identifiers: fs_access net_access
+    :identifiers: fs_access net_access scope
 
 Creating a new ruleset
 ----------------------
@@ -541,6 +563,13 @@ earlier ABI.
 Starting with the Landlock ABI version 5, it is possible to restrict the use of
 :manpage:`ioctl(2)` using the new ``LANDLOCK_ACCESS_FS_IOCTL_DEV`` right.
 
+Abstract Unix sockets Restriction  (ABI < 6)
+--------------------------------------------
+
+With ABI version 6, it is possible to restrict connection to an abstract Unix socket
+through ``LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET``, thanks to the ``scoped`` ruleset
+attribute.
+
 .. _kernel_support:
 
 Kernel support
-- 
2.34.1

Re: [PATCH v8 4/4] Landlock: Document LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET and ABI versioning

[Mickaël Salaün]

On Thu, Aug 01, 2024 at 10:02:36PM -0600, Tahera Fahimi wrote:
> Introducing LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET as an IPC scoping
> mechanism in Landlock ABI version 6, and updating ruleset_attr,
> Landlock ABI version, and access rights code blocks based on that.
> 
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> ---
> v8:
> - Improving documentation by specifying differences between scoped and
>   non-scoped domains.
> - Adding review notes of version 7.
> - Update date
> v7:
> - Add "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" explanation to IPC scoping
>   section and updating ABI to version 6.
> - Adding "scoped" attribute to the Access rights section.
> - In current limitation, unnamed sockets are specified as sockets that
>   are not restricted.
> - Update date
> ---
>  Documentation/userspace-api/landlock.rst | 33 ++++++++++++++++++++++--
>  1 file changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> index 07b63aec56fa..d602567b5139 100644
> --- a/Documentation/userspace-api/landlock.rst
> +++ b/Documentation/userspace-api/landlock.rst
> @@ -8,7 +8,7 @@ Landlock: unprivileged access control
>  =====================================
>  
>  :Author: Mickaël Salaün
> -:Date: April 2024
> +:Date: August 2024
>  
>  The goal of Landlock is to enable to restrict ambient rights (e.g. global
>  filesystem or network access) for a set of processes.  Because Landlock
> @@ -81,6 +81,8 @@ to be explicit about the denied-by-default access rights.
>          .handled_access_net =
>              LANDLOCK_ACCESS_NET_BIND_TCP |
>              LANDLOCK_ACCESS_NET_CONNECT_TCP,
> +        .scoped =
> +            LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET,
>      };
>  
>  Because we may not know on which kernel version an application will be
> @@ -119,6 +121,9 @@ version, and only use the available subset of access rights:
>      case 4:
>          /* Removes LANDLOCK_ACCESS_FS_IOCTL_DEV for ABI < 5 */
>          ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL_DEV;
> +    case 5:
> +        /* Removes LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET for ABI < 6 */
> +        ruleset_attr.scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
>      }
>  
>  This enables to create an inclusive ruleset that will contain our rules.
> @@ -306,6 +311,23 @@ To be allowed to use :manpage:`ptrace(2)` and related syscalls on a target
>  process, a sandboxed process should have a subset of the target process rules,
>  which means the tracee must be in a sub-domain of the tracer.
>  
> +IPC Scoping
> +-----------
> +
> +Similar to the implicit `Ptrace restrictions`_, we may want to further restrict
> +interactions between sandboxes. Each Landlock domain can be explicitly scoped
> +for a set of actions by specifying it on a ruleset. For example, if a sandboxed
> +process should not be able to :manpage:`connect(2)` to a non-sandboxed process
> +through abstract :manpage:`unix(7)` sockets, we can specify such restriction
> +with ``LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET``.
> +
> +A sandboxed process can connect to a non-sandboxed process when its domain is
> +not scoped. If a process's domain is scoped, it can only connect to processes in

...it can only connect to sockets created by proccesses in the same
scoped domain.

> +the same scoped domain.
> +
> +IPC scoping does not support Landlock rules, so if a domain is scoped, no rules
> +can be added to allow accessing to a resource outside of the scoped domain.
> +
>  Truncating files
>  ----------------
>  
> @@ -404,7 +426,7 @@ Access rights
>  -------------
>  
>  .. kernel-doc:: include/uapi/linux/landlock.h
> -    :identifiers: fs_access net_access
> +    :identifiers: fs_access net_access scope
>  
>  Creating a new ruleset
>  ----------------------
> @@ -541,6 +563,13 @@ earlier ABI.
>  Starting with the Landlock ABI version 5, it is possible to restrict the use of
>  :manpage:`ioctl(2)` using the new ``LANDLOCK_ACCESS_FS_IOCTL_DEV`` right.
>  
> +Abstract Unix sockets Restriction  (ABI < 6)

Let's follow the capitalization used by man pages: "UNIX" instead of
"Unix".

> +--------------------------------------------
> +
> +With ABI version 6, it is possible to restrict connection to an abstract Unix socket
> +through ``LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET``, thanks to the ``scoped`` ruleset
> +attribute.
> +
>  .. _kernel_support:
>  
>  Kernel support
> -- 
> 2.34.1
>