[PATCH v3] landlock: Add abstract unix socket connect restriction

[PATCH v3] landlock: Add abstract unix socket connect restriction

[Tahera Fahimi]

Abstract unix sockets are used for local inter-process communications
without on a filesystem. Currently a sandboxed process can connect to a
socket outside of the sandboxed environment, since landlock has no
restriction for connecting to a unix socket in the abstract namespace.
Access to such sockets for a sandboxed process should be scoped the same
way ptrace is limited.

Because of compatibility reasons and since landlock should be flexible,
we extend the user space interface by adding a new "scoped" field. This
field optionally contains a "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to
specify that the ruleset will deny any connection from within the
sandbox to its parents(i.e. any parent sandbox or non-sandbox processes)

Closes: https://github.com/landlock-lsm/linux/issues/7

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

-------
V3: Added "scoped" field to landlock_ruleset_attr
V2: Remove wrapper functions

-------
---
 include/uapi/linux/landlock.h | 22 +++++++++++++++
 security/landlock/limits.h    |  5 ++++
 security/landlock/ruleset.c   | 16 +++++++----
 security/landlock/ruleset.h   | 31 ++++++++++++++++++--
 security/landlock/syscalls.c  |  9 ++++--
 security/landlock/task.c      | 53 ++++++++++++++++++-----------------
 6 files changed, 102 insertions(+), 34 deletions(-)

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 68625e728f43..1641aeb9eeaa 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 actions (cf. `Scope access flags`_)
+	 * that is handled by this ruleset and should be permitted
+	 * by default if no rule explicitly deny them.
+	 */
+	__u64 scoped;
 };
 
 /*
@@ -266,4 +272,20 @@ 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: scoped
+ *
+ * Scope access flags
+ * ~~~~~~~~~~~~~~~~~~~~
+ * These flags enable to restrict a sandboxed process to a set of
+ * inter-process communications actions. 
+ *
+ * IPCs with scoped actions:
+ * - %LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET: Restrict a sandbox process to
+ *   connect to another process through abstract unix sockets. 
+ */
+/* 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 20fdb5ff3514..d6fb82fd1e67 100644
--- a/security/landlock/limits.h
+++ b/security/landlock/limits.h
@@ -28,6 +28,11 @@
 #define LANDLOCK_NUM_ACCESS_NET		__const_hweight64(LANDLOCK_MASK_ACCESS_NET)
 #define LANDLOCK_SHIFT_ACCESS_NET	LANDLOCK_NUM_ACCESS_FS
 
+#define LANDLOCK_LAST_ACCESS_UNIX       LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET
+#define LANDLOCK_MASK_ACCESS_UNIX	((LANDLOCK_LAST_ACCESS_UNIX << 1) - 1)
+#define LANDLOCK_NUM_ACCESS_UNIX         __const_hweight64(LANDLOCK_MASK_ACCESS_UNIX)
+#define LANDLOCK_SHIFT_ACCESS_UNIX      LANDLOCK_SHIFT_ACCESS_NET
+
 /* clang-format on */
 
 #endif /* _SECURITY_LANDLOCK_LIMITS_H */
diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index e0a5fbf9201a..0592e53cdc9d 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 unix_access_mask)
 {
 	struct landlock_ruleset *new_ruleset;
 
 	/* Informs about useless ruleset. */
-	if (!fs_access_mask && !net_access_mask)
+	if (!fs_access_mask && !net_access_mask && !unix_access_mask)
 		return ERR_PTR(-ENOMSG);
 	new_ruleset = create_ruleset(1);
 	if (IS_ERR(new_ruleset))
@@ -66,6 +67,9 @@ 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 (unix_access_mask)
+		landlock_add_unix_socket_access_mask(new_ruleset,
+						     unix_access_mask, 0);
 	return new_ruleset;
 }
 
@@ -173,9 +177,11 @@ static void build_check_ruleset(void)
 
 	BUILD_BUG_ON(ruleset.num_rules < LANDLOCK_MAX_NUM_RULES);
 	BUILD_BUG_ON(ruleset.num_layers < LANDLOCK_MAX_NUM_LAYERS);
-	BUILD_BUG_ON(access_masks <
-		     ((LANDLOCK_MASK_ACCESS_FS << LANDLOCK_SHIFT_ACCESS_FS) |
-		      (LANDLOCK_MASK_ACCESS_NET << LANDLOCK_SHIFT_ACCESS_NET)));
+	BUILD_BUG_ON(
+		access_masks <
+		((LANDLOCK_MASK_ACCESS_FS << LANDLOCK_SHIFT_ACCESS_FS) |
+		 (LANDLOCK_MASK_ACCESS_NET << LANDLOCK_SHIFT_ACCESS_NET) |
+		 (LANDLOCK_MASK_ACCESS_UNIX << LANDLOCK_SHIFT_ACCESS_UNIX)));
 }
 
 /**
diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
index c7f1526784fd..6e755d924a5e 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 abstract Unix Socket access rights can be stored*/
+static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_UNIX);
 /* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */
 static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
 
@@ -42,7 +44,8 @@ static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
 typedef u32 access_masks_t;
 /* Makes sure all ruleset access rights can be stored. */
 static_assert(BITS_PER_TYPE(access_masks_t) >=
-	      LANDLOCK_NUM_ACCESS_FS + LANDLOCK_NUM_ACCESS_NET);
+	      LANDLOCK_NUM_ACCESS_FS + LANDLOCK_NUM_ACCESS_NET +
+		      LANDLOCK_NUM_ACCESS_UNIX);
 
 typedef u16 layer_mask_t;
 /* Makes sure all layers can be checked. */
@@ -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 access_mask_unix);
 
 void landlock_put_ruleset(struct landlock_ruleset *const ruleset);
 void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset);
@@ -282,6 +286,18 @@ landlock_add_net_access_mask(struct landlock_ruleset *const ruleset,
 		(net_mask << LANDLOCK_SHIFT_ACCESS_NET);
 }
 
+static inline void
+landlock_add_unix_socket_access_mask(struct landlock_ruleset *const ruleset,
+				     const access_mask_t unix_access_mask,
+				     const u16 layer_level)
+{
+	access_mask_t unix_mask = unix_access_mask & LANDLOCK_MASK_ACCESS_UNIX;
+
+	WARN_ON_ONCE(unix_access_mask != unix_mask);
+	ruleset->access_masks[layer_level] |=
+		(unix_mask << LANDLOCK_SHIFT_ACCESS_UNIX);
+}
+
 static inline access_mask_t
 landlock_get_raw_fs_access_mask(const struct landlock_ruleset *const ruleset,
 				const u16 layer_level)
@@ -309,6 +325,17 @@ landlock_get_net_access_mask(const struct landlock_ruleset *const ruleset,
 	       LANDLOCK_MASK_ACCESS_NET;
 }
 
+static inline access_mask_t
+landlock_get_unix_access_mask(const struct landlock_ruleset *const ruleset,
+			      const u16 layer_level)
+{
+	return landlock_get_raw_fs_access_mask(ruleset, layer_level) |
+	       LANDLOCK_ACCESS_FS_INITIALLY_DENIED;
+	return (ruleset->access_masks[layer_level] >>
+		LANDLOCK_SHIFT_ACCESS_UNIX) &
+	       LANDLOCK_MASK_ACCESS_UNIX;
+}
+
 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..955d3d028963 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);
@@ -212,10 +213,14 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
 	if ((ruleset_attr.handled_access_net | LANDLOCK_MASK_ACCESS_NET) !=
 	    LANDLOCK_MASK_ACCESS_NET)
 		return -EINVAL;
+	if ((ruleset_attr.scoped | LANDLOCK_MASK_ACCESS_UNIX) !=
+	    LANDLOCK_MASK_ACCESS_UNIX)
+		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 67528f87b7de..b42f31cca2ae 100644
--- a/security/landlock/task.c
+++ b/security/landlock/task.c
@@ -14,6 +14,7 @@
 #include <linux/rcupdate.h>
 #include <linux/sched.h>
 #include <net/sock.h>
+#include <net/af_unix.h>
 
 #include "common.h"
 #include "cred.h"
@@ -109,32 +110,25 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
 	return task_ptrace(parent, current);
 }
 
-static bool unix_sock_is_scoped(struct sock *const sock,
-				struct sock *const other)
+static bool sock_is_scoped(struct sock *const sock, struct sock *const other)
 {
 	bool is_scoped = true;
-
-	/* get the ruleset of connecting sock*/
-	const struct landlock_ruleset *const dom_sock =
-		landlock_get_current_domain();
-
-	if (!dom_sock)
-		return true;
-
-	/* get credential of listening sock*/
-	const struct cred *cred_other = get_cred(other->sk_peer_cred);
-
-	if (!cred_other)
-		return true;
-
-	/* retrieve the landlock_rulesets */
-	const struct landlock_ruleset *dom_parent;
-
-	rcu_read_lock();
-	dom_parent = landlock_cred(cred_other)->domain;
-	is_scoped = domain_scope_le(dom_parent, dom_sock);
-	rcu_read_unlock();
-
+	const struct landlock_ruleset *dom_other;
+	const struct cred *cred_other;
+
+	const struct landlock_ruleset *const dom = landlock_get_current_domain();
+	if (!dom)
+		goto out_put_cred;
+
+	
+	lockdep_assert_held(&unix_sk(other)->lock);
+	/* the credentials will not change */
+	cred_other = get_cred(other->sk_peer_cred);
+	dom_other = landlock_cred(cred_other)->domain;
+	is_scoped = domain_scope_le(dom, dom_other);
+
+out_put_cred:
+	put_cred(cred_other);
 	return is_scoped;
 }
 
@@ -142,7 +136,15 @@ static int hook_unix_stream_connect(struct sock *const sock,
 				    struct sock *const other,
 				    struct sock *const newsk)
 {
-	if (unix_sock_is_scoped(sock, other))
+	if (sock_is_scoped(sock, other))
+		return 0;
+	return -EPERM;
+}
+
+static int hook_unix_may_send(struct socket *const sock,
+			      struct socket *const other)
+{
+	if (sock_is_scoped(sock->sk, other->sk))
 		return 0;
 	return -EPERM;
 }
@@ -151,6 +153,7 @@ 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 v3] landlock: Add abstract unix socket connect restriction

[Mickaël Salaün]

It looks like this patch only applies on top of the previous one, which should
be squashed here.

When running Landlock's tests, layout1.named_unix_domain_socket_ioctl fail.

The whole changes looks good!

On Wed, Jun 05, 2024 at 10:36:11PM -0600, Tahera Fahimi wrote:
> Abstract unix sockets are used for local inter-process communications
> without on a filesystem. Currently a sandboxed process can connect to a
> socket outside of the sandboxed environment, since landlock has no
> restriction for connecting to a unix socket in the abstract namespace.
> Access to such sockets for a sandboxed process should be scoped the same
> way ptrace is limited.
> 
> Because of compatibility reasons and since landlock should be flexible,
> we extend the user space interface by adding a new "scoped" field. This
> field optionally contains a "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to
> specify that the ruleset will deny any connection from within the
> sandbox to its parents(i.e. any parent sandbox or non-sandbox processes)
> 
> Closes: https://github.com/landlock-lsm/linux/issues/7
> 

No need for this new line, tags are grouped together.

> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> 
> -------
> V3: Added "scoped" field to landlock_ruleset_attr
> V2: Remove wrapper functions
> 
> -------
> ---
>  include/uapi/linux/landlock.h | 22 +++++++++++++++
>  security/landlock/limits.h    |  5 ++++
>  security/landlock/ruleset.c   | 16 +++++++----
>  security/landlock/ruleset.h   | 31 ++++++++++++++++++--
>  security/landlock/syscalls.c  |  9 ++++--
>  security/landlock/task.c      | 53 ++++++++++++++++++-----------------
>  6 files changed, 102 insertions(+), 34 deletions(-)
> 
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 68625e728f43..1641aeb9eeaa 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 actions (cf. `Scope access flags`_)
> +	 * that is handled by this ruleset and should be permitted
> +	 * by default if no rule explicitly deny them.
> +	 */
> +	__u64 scoped;
>  };
>  
>  /*
> @@ -266,4 +272,20 @@ 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: scoped
> + *
> + * Scope access flags
> + * ~~~~~~~~~~~~~~~~~~~~

Missing new line.

> + * These flags enable to restrict a sandboxed process to a set of
> + * inter-process communications actions. 

This needs to explain the concept of "scoped" restrictions, similar to
the ptrace restriction (i.e. isolate the Landlock domain to forbid
connections to resources outside the domain).

> + *
> + * IPCs with scoped actions:
> + * - %LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET: Restrict a sandbox process to
> + *   connect to another process through abstract unix sockets. 

"another process" is vague.

> + */
> +/* 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 20fdb5ff3514..d6fb82fd1e67 100644
> --- a/security/landlock/limits.h
> +++ b/security/landlock/limits.h
> @@ -28,6 +28,11 @@
>  #define LANDLOCK_NUM_ACCESS_NET		__const_hweight64(LANDLOCK_MASK_ACCESS_NET)
>  #define LANDLOCK_SHIFT_ACCESS_NET	LANDLOCK_NUM_ACCESS_FS
>  
> +#define LANDLOCK_LAST_ACCESS_UNIX       LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET
> +#define LANDLOCK_MASK_ACCESS_UNIX	((LANDLOCK_LAST_ACCESS_UNIX << 1) - 1)
> +#define LANDLOCK_NUM_ACCESS_UNIX         __const_hweight64(LANDLOCK_MASK_ACCESS_UNIX)
> +#define LANDLOCK_SHIFT_ACCESS_UNIX      LANDLOCK_SHIFT_ACCESS_NET

This is good but this is not specific to unix sockets.  Because this
"scope" will be useful for non-af-unix restrictions, you can rename
LANDLOCK_*_ACCESS_UNIX to LANDLOCK_*_SCOPE.  This should be fixed for
most new variable names with "unix".


> +
>  /* clang-format on */
>  
>  #endif /* _SECURITY_LANDLOCK_LIMITS_H */
> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> index e0a5fbf9201a..0592e53cdc9d 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 unix_access_mask)

Because this "scope" will be useful for non-af-unix restrictions, you
can rename unix_access_mask to 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 && !unix_access_mask)
>  		return ERR_PTR(-ENOMSG);
>  	new_ruleset = create_ruleset(1);
>  	if (IS_ERR(new_ruleset))
> @@ -66,6 +67,9 @@ 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 (unix_access_mask)
> +		landlock_add_unix_socket_access_mask(new_ruleset,
> +						     unix_access_mask, 0);
>  	return new_ruleset;
>  }
>  
> @@ -173,9 +177,11 @@ static void build_check_ruleset(void)
>  
>  	BUILD_BUG_ON(ruleset.num_rules < LANDLOCK_MAX_NUM_RULES);
>  	BUILD_BUG_ON(ruleset.num_layers < LANDLOCK_MAX_NUM_LAYERS);
> -	BUILD_BUG_ON(access_masks <
> -		     ((LANDLOCK_MASK_ACCESS_FS << LANDLOCK_SHIFT_ACCESS_FS) |
> -		      (LANDLOCK_MASK_ACCESS_NET << LANDLOCK_SHIFT_ACCESS_NET)));
> +	BUILD_BUG_ON(
> +		access_masks <
> +		((LANDLOCK_MASK_ACCESS_FS << LANDLOCK_SHIFT_ACCESS_FS) |
> +		 (LANDLOCK_MASK_ACCESS_NET << LANDLOCK_SHIFT_ACCESS_NET) |
> +		 (LANDLOCK_MASK_ACCESS_UNIX << LANDLOCK_SHIFT_ACCESS_UNIX)));
>  }
>  
>  /**
> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index c7f1526784fd..6e755d924a5e 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 abstract Unix Socket access rights can be stored*/

As explained above, it is not about unix nor access rights, but scope.

> +static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_UNIX);
>  /* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */
>  static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
>  
> @@ -42,7 +44,8 @@ static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
>  typedef u32 access_masks_t;
>  /* Makes sure all ruleset access rights can be stored. */
>  static_assert(BITS_PER_TYPE(access_masks_t) >=
> -	      LANDLOCK_NUM_ACCESS_FS + LANDLOCK_NUM_ACCESS_NET);
> +	      LANDLOCK_NUM_ACCESS_FS + LANDLOCK_NUM_ACCESS_NET +
> +		      LANDLOCK_NUM_ACCESS_UNIX);
>  
>  typedef u16 layer_mask_t;
>  /* Makes sure all layers can be checked. */
> @@ -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 access_mask_unix);
>  
>  void landlock_put_ruleset(struct landlock_ruleset *const ruleset);
>  void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset);
> @@ -282,6 +286,18 @@ landlock_add_net_access_mask(struct landlock_ruleset *const ruleset,
>  		(net_mask << LANDLOCK_SHIFT_ACCESS_NET);
>  }
>  
> +static inline void
> +landlock_add_unix_socket_access_mask(struct landlock_ruleset *const ruleset,
> +				     const access_mask_t unix_access_mask,
> +				     const u16 layer_level)
> +{
> +	access_mask_t unix_mask = unix_access_mask & LANDLOCK_MASK_ACCESS_UNIX;
> +
> +	WARN_ON_ONCE(unix_access_mask != unix_mask);
> +	ruleset->access_masks[layer_level] |=
> +		(unix_mask << LANDLOCK_SHIFT_ACCESS_UNIX);
> +}
> +
>  static inline access_mask_t
>  landlock_get_raw_fs_access_mask(const struct landlock_ruleset *const ruleset,
>  				const u16 layer_level)
> @@ -309,6 +325,17 @@ landlock_get_net_access_mask(const struct landlock_ruleset *const ruleset,
>  	       LANDLOCK_MASK_ACCESS_NET;
>  }
>  
> +static inline access_mask_t
> +landlock_get_unix_access_mask(const struct landlock_ruleset *const ruleset,
> +			      const u16 layer_level)
> +{
> +	return landlock_get_raw_fs_access_mask(ruleset, layer_level) |
> +	       LANDLOCK_ACCESS_FS_INITIALLY_DENIED;

This first return should not exist.

> +	return (ruleset->access_masks[layer_level] >>
> +		LANDLOCK_SHIFT_ACCESS_UNIX) &
> +	       LANDLOCK_MASK_ACCESS_UNIX;
> +}
> +
>  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..955d3d028963 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);
> @@ -212,10 +213,14 @@ SYSCALL_DEFINE3(landlock_create_ruleset,

The documentation of this function should be updated to reflect the
scope change:

 * - %EINVAL: unknown @flags, or unknown access, or unknown scope, or too small
 *   @size;

>  	if ((ruleset_attr.handled_access_net | LANDLOCK_MASK_ACCESS_NET) !=
>  	    LANDLOCK_MASK_ACCESS_NET)
>  		return -EINVAL;

A comment should explain what we check here, like for filesystem and
network.  A new line should help to differentiate network and scope
checks.

> +	if ((ruleset_attr.scoped | LANDLOCK_MASK_ACCESS_UNIX) !=
> +	    LANDLOCK_MASK_ACCESS_UNIX)
> +		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 67528f87b7de..b42f31cca2ae 100644
> --- a/security/landlock/task.c
> +++ b/security/landlock/task.c
> @@ -14,6 +14,7 @@
>  #include <linux/rcupdate.h>
>  #include <linux/sched.h>
>  #include <net/sock.h>
> +#include <net/af_unix.h>

You can sort all these headers with sort -u.

>  
>  #include "common.h"
>  #include "cred.h"
> @@ -109,32 +110,25 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
>  	return task_ptrace(parent, current);
>  }
>  

It's difficult to review such patch modifying another patch, but here are a
review of the whole change:

> -static bool unix_sock_is_scoped(struct sock *const sock,
> -				struct sock *const other)
> +static bool sock_is_scoped(struct sock *const sock, struct sock *const other)

If the "sock" argument is not used, it should either not exist or be used to
identify the creds instead of using the "current" creds.  This is not the same
thing because a newly created socket could be passed or inherited.  I'm not
sure what should be the best approach yet, but let's keep using the current
creds for now and remove the "sock" argument for now.

BTW, tests should check with sockets created before sandboxing.

>  {
>  	bool is_scoped = true;
> -
> -	/* get the ruleset of connecting sock*/
> -	const struct landlock_ruleset *const dom_sock =
> -		landlock_get_current_domain();
> -
> -	if (!dom_sock)
> -		return true;
> -
> -	/* get credential of listening sock*/
> -	const struct cred *cred_other = get_cred(other->sk_peer_cred);
> -
> -	if (!cred_other)
> -		return true;
> -
> -	/* retrieve the landlock_rulesets */
> -	const struct landlock_ruleset *dom_parent;
> -
> -	rcu_read_lock();
> -	dom_parent = landlock_cred(cred_other)->domain;
> -	is_scoped = domain_scope_le(dom_parent, dom_sock);
> -	rcu_read_unlock();
> -
> +	const struct landlock_ruleset *dom_other;
> +	const struct cred *cred_other;
> +
> +	const struct landlock_ruleset *const dom = landlock_get_current_domain();
> +	if (!dom)
> +		goto out_put_cred;

This case calls put_cred() on an uninitialized pointer.  It should just return.

> +
> +	
> +	lockdep_assert_held(&unix_sk(other)->lock);
> +	/* the credentials will not change */
> +	cred_other = get_cred(other->sk_peer_cred);
> +	dom_other = landlock_cred(cred_other)->domain;
> +	is_scoped = domain_scope_le(dom, dom_other);
> +
> +out_put_cred:
> +	put_cred(cred_other);
>  	return is_scoped;
>  }
>  
> @@ -142,7 +136,15 @@ static int hook_unix_stream_connect(struct sock *const sock,
>  				    struct sock *const other,
>  				    struct sock *const newsk)
>  {
> -	if (unix_sock_is_scoped(sock, other))
> +	if (sock_is_scoped(sock, other))
> +		return 0;

You can add a new line after a return like this.

> +	return -EPERM;
> +}
> +
> +static int hook_unix_may_send(struct socket *const sock,
> +			      struct socket *const other)
> +{
> +	if (sock_is_scoped(sock->sk, other->sk))
>  		return 0;
>  	return -EPERM;
>  }
> @@ -151,6 +153,7 @@ 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
> 
> 

[PATCH v3] landlock: Add abstract unix socket connect restriction

[Tahera Fahimi]

Abstract unix sockets are used for local inter-process communications
without on a filesystem. Currently a sandboxed process can connect to a
socket outside of the sandboxed environment, since landlock has no
restriction for connecting to a unix socket in the abstract namespace.
Access to such sockets for a sandboxed process should be scoped the same
way ptrace is limited.

Because of compatibility reasons and since landlock should be flexible,
we extend the user space interface by adding a new "scoped" field. This
field optionally contains a "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to
specify that the ruleset will deny any connection from within the
sandbox to its parents(i.e. any parent sandbox or non-sandbox processes)

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

-------
V3: Added "scoped" field to landlock_ruleset_attr
V2: Remove wrapper functions

-------
---
 include/uapi/linux/landlock.h | 28 +++++++++++++++++++++++
 security/landlock/limits.h    |  5 ++++
 security/landlock/ruleset.c   | 15 ++++++++----
 security/landlock/ruleset.h   | 28 +++++++++++++++++++++--
 security/landlock/syscalls.c  | 12 +++++++---
 security/landlock/task.c      | 43 +++++++++++++++++++++++++++++++++++
 6 files changed, 121 insertions(+), 10 deletions(-)

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 68625e728f43..d887e67dc0ed 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 actions (cf. `Scope access flags`_)
+	 * that is handled by this ruleset and should be permitted
+	 * by default if no rule explicitly deny them.
+	 */
+	__u64 scoped;
 };
 
 /*
@@ -266,4 +272,26 @@ 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: scoped
+ *
+ * Scoped handles a set of restrictions on kernel IPCs.
+ *
+ * Scope access flags
+ * ~~~~~~~~~~~~~~~~~~~~
+ * 
+ * These flags enable to restrict a sandboxed process from a set of
+ * inter-process communications actions. Setting a flag in a landlock
+ * domain will isolate the Landlock domain to forbid connections
+ * to resources outside the domain.
+ *
+ * IPCs with scoped actions:
+ * - %LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET: Restrict a sandbox process to
+ *   connect to a process outside of the sandbox domain through abstract
+ *   unix sockets. 
+ */
+/* 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 20fdb5ff3514..7b794b81ef05 100644
--- a/security/landlock/limits.h
+++ b/security/landlock/limits.h
@@ -28,6 +28,11 @@
 #define LANDLOCK_NUM_ACCESS_NET		__const_hweight64(LANDLOCK_MASK_ACCESS_NET)
 #define LANDLOCK_SHIFT_ACCESS_NET	LANDLOCK_NUM_ACCESS_FS
 
+#define LANDLOCK_LAST_ACCESS_SCOPE       LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET
+#define LANDLOCK_MASK_ACCESS_SCOPE	((LANDLOCK_LAST_ACCESS_SCOPE << 1) - 1)
+#define LANDLOCK_NUM_ACCESS_SCOPE         __const_hweight64(LANDLOCK_MASK_ACCESS_SCOPE)
+#define LANDLOCK_SHIFT_ACCESS_SCOPE      LANDLOCK_SHIFT_ACCESS_NET
+
 /* clang-format on */
 
 #endif /* _SECURITY_LANDLOCK_LIMITS_H */
diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index e0a5fbf9201a..635d0854be09 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;
 }
 
@@ -173,9 +176,11 @@ static void build_check_ruleset(void)
 
 	BUILD_BUG_ON(ruleset.num_rules < LANDLOCK_MAX_NUM_RULES);
 	BUILD_BUG_ON(ruleset.num_layers < LANDLOCK_MAX_NUM_LAYERS);
-	BUILD_BUG_ON(access_masks <
-		     ((LANDLOCK_MASK_ACCESS_FS << LANDLOCK_SHIFT_ACCESS_FS) |
-		      (LANDLOCK_MASK_ACCESS_NET << LANDLOCK_SHIFT_ACCESS_NET)));
+	BUILD_BUG_ON(
+		access_masks <
+		((LANDLOCK_MASK_ACCESS_FS << LANDLOCK_SHIFT_ACCESS_FS) |
+		 (LANDLOCK_MASK_ACCESS_NET << LANDLOCK_SHIFT_ACCESS_NET) |
+		 (LANDLOCK_MASK_ACCESS_SCOPE << LANDLOCK_SHIFT_ACCESS_SCOPE)));
 }
 
 /**
diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
index c7f1526784fd..b633d1b66452 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_ACCESS_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,7 +44,8 @@ static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
 typedef u32 access_masks_t;
 /* Makes sure all ruleset access rights can be stored. */
 static_assert(BITS_PER_TYPE(access_masks_t) >=
-	      LANDLOCK_NUM_ACCESS_FS + LANDLOCK_NUM_ACCESS_NET);
+	      LANDLOCK_NUM_ACCESS_FS + LANDLOCK_NUM_ACCESS_NET +
+		      LANDLOCK_NUM_ACCESS_SCOPE);
 
 typedef u16 layer_mask_t;
 /* Makes sure all layers can be checked. */
@@ -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);
@@ -282,6 +286,17 @@ landlock_add_net_access_mask(struct landlock_ruleset *const ruleset,
 		(net_mask << LANDLOCK_SHIFT_ACCESS_NET);
 }
 
+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_ACCESS_SCOPE;
+
+	WARN_ON_ONCE(scope_mask != scoped_mask);
+	ruleset->access_masks[layer_level] |=
+		(scoped_mask << LANDLOCK_SHIFT_ACCESS_SCOPE);
+}
+
 static inline access_mask_t
 landlock_get_raw_fs_access_mask(const struct landlock_ruleset *const ruleset,
 				const u16 layer_level)
@@ -309,6 +324,15 @@ landlock_get_net_access_mask(const struct landlock_ruleset *const ruleset,
 	       LANDLOCK_MASK_ACCESS_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] >>
+		LANDLOCK_SHIFT_ACCESS_SCOPE) &
+	       LANDLOCK_MASK_ACCESS_SCOPE;
+}
+
 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..e95e79752be0 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);
@@ -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 uknown scope, or too small @size;
  * - %E2BIG or %EFAULT: @attr or @size inconsistencies;
  * - %ENOMSG: empty &landlock_ruleset_attr.handled_access_fs.
  */
@@ -212,10 +213,15 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
 	if ((ruleset_attr.handled_access_net | LANDLOCK_MASK_ACCESS_NET) !=
 	    LANDLOCK_MASK_ACCESS_NET)
 		return -EINVAL;
+	/* Checks IPC scoping content (and 32-bits cast). */
+	if ((ruleset_attr.scoped | LANDLOCK_MASK_ACCESS_SCOPE) !=
+	    LANDLOCK_MASK_ACCESS_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..147c6545ef24 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,50 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
 	return task_ptrace(parent, current);
 }
 
+static bool sock_is_scoped(struct sock *const other)
+{
+	bool is_scoped = true;
+	const struct landlock_ruleset *dom_other;
+	const struct cred *cred_other;
+
+	const struct landlock_ruleset *const dom =
+		landlock_get_current_domain();
+	if (!dom)
+		return true;
+
+	lockdep_assert_held(&unix_sk(other)->lock);
+	/* the credentials will not change */
+	cred_other = get_cred(other->sk_peer_cred);
+	dom_other = landlock_cred(cred_other)->domain;
+	is_scoped = domain_scope_le(dom, dom_other);
+	put_cred(cred_other);
+	return is_scoped;
+}
+
+static int hook_unix_stream_connect(struct sock *const sock,
+				    struct sock *const other,
+				    struct sock *const newsk)
+{
+	if (sock_is_scoped(other))
+		return 0;
+
+	return -EPERM;
+}
+
+static int hook_unix_may_send(struct socket *const sock,
+			      struct socket *const other)
+{
+	if (sock_is_scoped(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 v3] landlock: Add abstract unix socket connect restriction

[Günther Noack]

Hello Tahera!

Thanks for sending another revision of your patch set!

On Thu, Jun 06, 2024 at 05:44:46PM -0600, Tahera Fahimi wrote:
> Abstract unix sockets are used for local inter-process communications
> without on a filesystem. Currently a sandboxed process can connect to a
> socket outside of the sandboxed environment, since landlock has no
> restriction for connecting to a unix socket in the abstract namespace.
> Access to such sockets for a sandboxed process should be scoped the same
> way ptrace is limited.
> 
> Because of compatibility reasons and since landlock should be flexible,
> we extend the user space interface by adding a new "scoped" field. This
> field optionally contains a "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to
> specify that the ruleset will deny any connection from within the
> sandbox to its parents(i.e. any parent sandbox or non-sandbox processes)
> 
> Closes: https://github.com/landlock-lsm/linux/issues/7
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> 
> -------
> V3: Added "scoped" field to landlock_ruleset_attr
> V2: Remove wrapper functions
> 
> -------
> ---
>  include/uapi/linux/landlock.h | 28 +++++++++++++++++++++++
>  security/landlock/limits.h    |  5 ++++
>  security/landlock/ruleset.c   | 15 ++++++++----
>  security/landlock/ruleset.h   | 28 +++++++++++++++++++++--
>  security/landlock/syscalls.c  | 12 +++++++---
>  security/landlock/task.c      | 43 +++++++++++++++++++++++++++++++++++
>  6 files changed, 121 insertions(+), 10 deletions(-)
> 
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 68625e728f43..d887e67dc0ed 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 actions (cf. `Scope access flags`_)
> +	 * that is handled by this ruleset and should be permitted
> +	 * by default if no rule explicitly deny them.
> +	 */
> +	__u64 scoped;

I have trouble understanding what this docstring means.

If those are "handled" things, shouldn't the name also start with "handled_", in
line with the other fields?  Also, I don't see any way to manipulate these
rights with a Landlock rule in this ?

How about:

/**
 * handled_scoped: Bitmask of IPC actions (cf. `Scoped access flags`_)
 * which are confined to only affect the current Landlock domain.
 */
__u64 handled_scoped;

>  };
>  
>  /*
> @@ -266,4 +272,26 @@ 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: scoped
> + *
> + * Scoped handles a set of restrictions on kernel IPCs.
> + *
> + * Scope access flags

Scoped with a "d"?

> + * ~~~~~~~~~~~~~~~~~~~~
> + * 
> + * These flags enable to restrict a sandboxed process from a set of
> + * inter-process communications actions. Setting a flag in a landlock
> + * domain will isolate the Landlock domain to forbid connections
> + * to resources outside the domain.
> + *
> + * IPCs with scoped actions:
> + * - %LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET: Restrict a sandbox process to
> + *   connect to a process outside of the sandbox domain through abstract
> + *   unix sockets.
> + */
> +/* clang-format off */
> +#define LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET		(1ULL << 0)

Should the name of this #define indicate the direction that we are restricting?
If I understand your documentation correctly, this is about *connecting out* of
the current Landlock domain, but incoming connections from more privileged
domains are OK, right?


Also:

Is it intentional that you are both restricting the connection and the sending
with the same flag (security_unix_may_send)?  If an existing Unix Domain Socket
gets passed in to a program from the outside (e.g. as stdout), shouldn't it
still be possible that the program enables a Landlock policy and then still
writes to it?  (Does that work?  Am I mis-reading the patch?)

The way that write access is normally checked for other files is at the time
when you open the file, not during write(), and I believe it would be more in
line with that normal "check at open" behaviour if we did the same here?


> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> index 20fdb5ff3514..7b794b81ef05 100644
> --- a/security/landlock/limits.h
> +++ b/security/landlock/limits.h
> @@ -28,6 +28,11 @@
>  #define LANDLOCK_NUM_ACCESS_NET		__const_hweight64(LANDLOCK_MASK_ACCESS_NET)
>  #define LANDLOCK_SHIFT_ACCESS_NET	LANDLOCK_NUM_ACCESS_FS
>  
> +#define LANDLOCK_LAST_ACCESS_SCOPE       LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET
> +#define LANDLOCK_MASK_ACCESS_SCOPE	((LANDLOCK_LAST_ACCESS_SCOPE << 1) - 1)
> +#define LANDLOCK_NUM_ACCESS_SCOPE         __const_hweight64(LANDLOCK_MASK_ACCESS_SCOPE)
> +#define LANDLOCK_SHIFT_ACCESS_SCOPE      LANDLOCK_SHIFT_ACCESS_NET
                                            ^^^^^^^^^^^^^^^^^^^^^^^^^

I believe this #define has the wrong value, and as a consequence, the code
suffers from the same problem as we already had on the other patch set from
Mikhail Ivanov -- see [1] for that discussion.

The LANDLOCK_SHIFT_ACCESS_FOO variable is used for determining the position of
your flag in the access_masks_t type, where all access masks are combined
together in one big bit vector.  If you are defining this the same for _SCOPE as
for _NET, I believe that we will start using the same bits in that vector for
both the _NET flags and the _SCOPE flags, and that will manifest in unwanted
interactions between the different types of restrictions.  (e.g. you will create
a policy to restrict _SCOPE, and you will find yourself unable to do some things
with TCP ports)

Please also see the other thread for more discussions about how we can avoid
such problems in the future.  (This code is easy to get wrong,
apparently... When we don't test what happens across multiple types of
restrictions, everything looks fine.)

[1] https://lore.kernel.org/all/ebd680cc-25d6-ee14-4856-310f5e5e28e4@huawei-partners.com/

—Günther

Re: [PATCH v3] landlock: Add abstract unix socket connect restriction

[Simon Horman]

On Wed, Jun 05, 2024 at 10:36:11PM -0600, Tahera Fahimi wrote:
> Abstract unix sockets are used for local inter-process communications
> without on a filesystem. Currently a sandboxed process can connect to a
> socket outside of the sandboxed environment, since landlock has no
> restriction for connecting to a unix socket in the abstract namespace.
> Access to such sockets for a sandboxed process should be scoped the same
> way ptrace is limited.
> 
> Because of compatibility reasons and since landlock should be flexible,
> we extend the user space interface by adding a new "scoped" field. This
> field optionally contains a "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to
> specify that the ruleset will deny any connection from within the
> sandbox to its parents(i.e. any parent sandbox or non-sandbox processes)
> 
> Closes: https://github.com/landlock-lsm/linux/issues/7
> 
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>

...

> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 68625e728f43..1641aeb9eeaa 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 actions (cf. `Scope access flags`_)

nit: s/scoped: /@scoped: / 

     Flagged by ./scripts/kernel-doc -none

> +	 * that is handled by this ruleset and should be permitted
> +	 * by default if no rule explicitly deny them.
> +	 */
> +	__u64 scoped;
>  };
>  
>  /*

...

Re: [PATCH v3] landlock: Add abstract unix socket connect restriction

[Tahera Fahimi]

On Fri, Jun 07, 2024 at 10:28:35AM +0200, Günther Noack wrote:
> Hello Tahera!
> 
> Thanks for sending another revision of your patch set!
Hello Günther, 
Thanks for your feedback.

> On Thu, Jun 06, 2024 at 05:44:46PM -0600, Tahera Fahimi wrote:
> > Abstract unix sockets are used for local inter-process communications
> > without on a filesystem. Currently a sandboxed process can connect to a
> > socket outside of the sandboxed environment, since landlock has no
> > restriction for connecting to a unix socket in the abstract namespace.
> > Access to such sockets for a sandboxed process should be scoped the same
> > way ptrace is limited.
> > 
> > Because of compatibility reasons and since landlock should be flexible,
> > we extend the user space interface by adding a new "scoped" field. This
> > field optionally contains a "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to
> > specify that the ruleset will deny any connection from within the
> > sandbox to its parents(i.e. any parent sandbox or non-sandbox processes)
> > 
> > Closes: https://github.com/landlock-lsm/linux/issues/7
> > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> > 
> > -------
> > V3: Added "scoped" field to landlock_ruleset_attr
> > V2: Remove wrapper functions
> > 
> > -------
> > ---
> >  include/uapi/linux/landlock.h | 28 +++++++++++++++++++++++
> >  security/landlock/limits.h    |  5 ++++
> >  security/landlock/ruleset.c   | 15 ++++++++----
> >  security/landlock/ruleset.h   | 28 +++++++++++++++++++++--
> >  security/landlock/syscalls.c  | 12 +++++++---
> >  security/landlock/task.c      | 43 +++++++++++++++++++++++++++++++++++
> >  6 files changed, 121 insertions(+), 10 deletions(-)
> > 
> > diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> > index 68625e728f43..d887e67dc0ed 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 actions (cf. `Scope access flags`_)
> > +	 * that is handled by this ruleset and should be permitted
> > +	 * by default if no rule explicitly deny them.
> > +	 */
> > +	__u64 scoped;
> 
> I have trouble understanding what this docstring means.
> 
> If those are "handled" things, shouldn't the name also start with "handled_", in
> line with the other fields?  Also, I don't see any way to manipulate these
> rights with a Landlock rule in this ?

.scoped attribute is not defined as .handled_scope since there is no
rule to handle/manipulate it, simply because this attribute shows either
action is permitted or denied. 

> How about:
> 
> /**
>  * handled_scoped: Bitmask of IPC actions (cf. `Scoped access flags`_)
>  * which are confined to only affect the current Landlock domain.
>  */

This is a good docstring. I will use it. 

> __u64 handled_scoped;
> 
> >  };
> >  
> >  /*
> > @@ -266,4 +272,26 @@ 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: scoped
> > + *
> > + * Scoped handles a set of restrictions on kernel IPCs.
> > + *
> > + * Scope access flags
> 
> Scoped with a "d"?
Scoped meant to point to .scoped attribute.  
> > + * ~~~~~~~~~~~~~~~~~~~~
> > + * 
> > + * These flags enable to restrict a sandboxed process from a set of
> > + * inter-process communications actions. Setting a flag in a landlock
> > + * domain will isolate the Landlock domain to forbid connections
> > + * to resources outside the domain.
> > + *
> > + * IPCs with scoped actions:
> > + * - %LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET: Restrict a sandbox process to
> > + *   connect to a process outside of the sandbox domain through abstract
> > + *   unix sockets.
> > + */
> > +/* clang-format off */
> > +#define LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET		(1ULL << 0)
> 
> Should the name of this #define indicate the direction that we are restricting?

Since the domain of a process specifies if a process can connect or not,
the direction of the connection does not matter. This restriction is the
same as ptrace.

> If I understand your documentation correctly, this is about *connecting out* of
> the current Landlock domain, but incoming connections from more privileged
> domains are OK, right?

Yes, Incoming connections are allowed if they are from a higher
privileged domain (or no domain). Consider two process P1 and P2 where
P1 wants to connect to P2. If P1 is not landlocked, it can connect to P2
regardless of whether P2 has a domain. If P1 is landlocked, it must have
an equal or less domain than P2 to connect to P2. We disscussed about
direction in [2]
https://lore.kernel.org/outreachy/20240603.Quaes2eich5f@digikod.net/T/#m6d5c5e65e43eaa1c8c38309f1225d169be3d6f87

> 
> Also:
> 
> Is it intentional that you are both restricting the connection and the sending
> with the same flag (security_unix_may_send)?  If an existing Unix Domain Socket
> gets passed in to a program from the outside (e.g. as stdout), shouldn't it
> still be possible that the program enables a Landlock policy and then still
> writes to it?  (Does that work?  Am I mis-reading the patch?)

security_unix_may_send checks if AF_UNIX socket can send datagrams, so
connecting and sending datagrams happens at the same state. I am not
sure if I understand your example correctly. Can you please explain a
bit more?

> The way that write access is normally checked for other files is at the time
> when you open the file, not during write(), and I believe it would be more in
> line with that normal "check at open" behaviour if we did the same here?

It checks the ability to connect to a unix socket at the point of
connecting, so I think it is aligned with the "check at point"
behaviour. This security check is called right before finalizing the
connection. 

> 
> > diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> > index 20fdb5ff3514..7b794b81ef05 100644
> > --- a/security/landlock/limits.h
> > +++ b/security/landlock/limits.h
> > @@ -28,6 +28,11 @@
> >  #define LANDLOCK_NUM_ACCESS_NET		__const_hweight64(LANDLOCK_MASK_ACCESS_NET)
> >  #define LANDLOCK_SHIFT_ACCESS_NET	LANDLOCK_NUM_ACCESS_FS
> >  
> > +#define LANDLOCK_LAST_ACCESS_SCOPE       LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET
> > +#define LANDLOCK_MASK_ACCESS_SCOPE	((LANDLOCK_LAST_ACCESS_SCOPE << 1) - 1)
> > +#define LANDLOCK_NUM_ACCESS_SCOPE         __const_hweight64(LANDLOCK_MASK_ACCESS_SCOPE)
> > +#define LANDLOCK_SHIFT_ACCESS_SCOPE      LANDLOCK_SHIFT_ACCESS_NET
>                                             ^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> I believe this #define has the wrong value, and as a consequence, the code
> suffers from the same problem as we already had on the other patch set from
> Mikhail Ivanov -- see [1] for that discussion.

Thanks for the hint. I will definitly check this. 

> The LANDLOCK_SHIFT_ACCESS_FOO variable is used for determining the position of
> your flag in the access_masks_t type, where all access masks are combined
> together in one big bit vector.  If you are defining this the same for _SCOPE as
> for _NET, I believe that we will start using the same bits in that vector for
> both the _NET flags and the _SCOPE flags, and that will manifest in unwanted
> interactions between the different types of restrictions.  (e.g. you will create
> a policy to restrict _SCOPE, and you will find yourself unable to do some things
> with TCP ports)
> 
> Please also see the other thread for more discussions about how we can avoid
> such problems in the future.  (This code is easy to get wrong,
> apparently... When we don't test what happens across multiple types of
> restrictions, everything looks fine.)
> 
> [1] https://lore.kernel.org/all/ebd680cc-25d6-ee14-4856-310f5e5e28e4@huawei-partners.com/
> 
> —Günther
> 

Re: [PATCH v3] landlock: Add abstract unix socket connect restriction

[Mickaël Salaün]

On Fri, Jun 07, 2024 at 01:41:39PM -0600, Tahera Fahimi wrote:
> On Fri, Jun 07, 2024 at 10:28:35AM +0200, Günther Noack wrote:
> > Hello Tahera!
> > 
> > Thanks for sending another revision of your patch set!
> Hello Günther, 
> Thanks for your feedback.
> 
> > On Thu, Jun 06, 2024 at 05:44:46PM -0600, Tahera Fahimi wrote:
> > > Abstract unix sockets are used for local inter-process communications
> > > without on a filesystem. Currently a sandboxed process can connect to a
> > > socket outside of the sandboxed environment, since landlock has no
> > > restriction for connecting to a unix socket in the abstract namespace.
> > > Access to such sockets for a sandboxed process should be scoped the same
> > > way ptrace is limited.
> > > 
> > > Because of compatibility reasons and since landlock should be flexible,
> > > we extend the user space interface by adding a new "scoped" field. This
> > > field optionally contains a "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to
> > > specify that the ruleset will deny any connection from within the
> > > sandbox to its parents(i.e. any parent sandbox or non-sandbox processes)
> > > 
> > > Closes: https://github.com/landlock-lsm/linux/issues/7
> > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> > > 
> > > -------
> > > V3: Added "scoped" field to landlock_ruleset_attr
> > > V2: Remove wrapper functions
> > > 
> > > -------
> > > ---
> > >  include/uapi/linux/landlock.h | 28 +++++++++++++++++++++++
> > >  security/landlock/limits.h    |  5 ++++
> > >  security/landlock/ruleset.c   | 15 ++++++++----
> > >  security/landlock/ruleset.h   | 28 +++++++++++++++++++++--
> > >  security/landlock/syscalls.c  | 12 +++++++---
> > >  security/landlock/task.c      | 43 +++++++++++++++++++++++++++++++++++
> > >  6 files changed, 121 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> > > index 68625e728f43..d887e67dc0ed 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 actions (cf. `Scope access flags`_)
> > > +	 * that is handled by this ruleset and should be permitted
> > > +	 * by default if no rule explicitly deny them.
> > > +	 */
> > > +	__u64 scoped;
> > 
> > I have trouble understanding what this docstring means.
> > 
> > If those are "handled" things, shouldn't the name also start with "handled_", in
> > line with the other fields?  Also, I don't see any way to manipulate these
> > rights with a Landlock rule in this ?
> 
> .scoped attribute is not defined as .handled_scope since there is no
> rule to handle/manipulate it, simply because this attribute shows either
> action is permitted or denied. 

Correct.  Günther, what do you think about the naming?

> 
> > How about:
> > 
> > /**
> >  * handled_scoped: Bitmask of IPC actions (cf. `Scoped access flags`_)
> >  * which are confined to only affect the current Landlock domain.
> >  */
> 
> This is a good docstring. I will use it. 
> 
> > __u64 handled_scoped;
> > 
> > >  };
> > >  
> > >  /*
> > > @@ -266,4 +272,26 @@ 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: scoped
> > > + *
> > > + * Scoped handles a set of restrictions on kernel IPCs.
> > > + *
> > > + * Scope access flags
> > 
> > Scoped with a "d"?
> Scoped meant to point to .scoped attribute.  

Right, but a "d" was missing.

> > > + * ~~~~~~~~~~~~~~~~~~~~
> > > + * 
> > > + * These flags enable to restrict a sandboxed process from a set of
> > > + * inter-process communications actions. Setting a flag in a landlock
> > > + * domain will isolate the Landlock domain to forbid connections
> > > + * to resources outside the domain.
> > > + *
> > > + * IPCs with scoped actions:
> > > + * - %LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET: Restrict a sandbox process to
> > > + *   connect to a process outside of the sandbox domain through abstract
> > > + *   unix sockets.
> > > + */
> > > +/* clang-format off */
> > > +#define LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET		(1ULL << 0)
> > 
> > Should the name of this #define indicate the direction that we are restricting?
> 
> Since the domain of a process specifies if a process can connect or not,
> the direction of the connection does not matter. This restriction is the
> same as ptrace.
> 
> > If I understand your documentation correctly, this is about *connecting out* of
> > the current Landlock domain, but incoming connections from more privileged
> > domains are OK, right?
> 
> Yes, Incoming connections are allowed if they are from a higher
> privileged domain (or no domain). Consider two process P1 and P2 where
> P1 wants to connect to P2. If P1 is not landlocked, it can connect to P2
> regardless of whether P2 has a domain. If P1 is landlocked, it must have
> an equal or less domain than P2 to connect to P2. We disscussed about
> direction in [2]
> https://lore.kernel.org/outreachy/20240603.Quaes2eich5f@digikod.net/T/#m6d5c5e65e43eaa1c8c38309f1225d169be3d6f87

Correct, this is what Günther highlighted: restriction on direction.

About the name "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET", I think it should
be OK if the "scoped" docstring section clearly explains this direction.

> 
> > 
> > Also:
> > 
> > Is it intentional that you are both restricting the connection and the sending
> > with the same flag (security_unix_may_send)?  If an existing Unix Domain Socket
> > gets passed in to a program from the outside (e.g. as stdout), shouldn't it
> > still be possible that the program enables a Landlock policy and then still
> > writes to it?  (Does that work?  Am I mis-reading the patch?)

If a passed socket is already connected, then a write/send should work.

> 
> security_unix_may_send checks if AF_UNIX socket can send datagrams, so
> connecting and sending datagrams happens at the same state. I am not
> sure if I understand your example correctly. Can you please explain a
> bit more?

The concern is about using the current's or the socket's credential.

> 
> > The way that write access is normally checked for other files is at the time
> > when you open the file, not during write(), and I believe it would be more in
> > line with that normal "check at open" behaviour if we did the same here?
> 
> It checks the ability to connect to a unix socket at the point of
> connecting, so I think it is aligned with the "check at point"
> behaviour. This security check is called right before finalizing the
> connection. 

From my point of view, the main difference between a file's FD and a
socket's FD is that a file's FD allows actions on only the referenced
file, whereas the socket's FD may not only reference a connection
because it can be disconnected (see net_test.c:protocol.connect_unspec)
and reconnected.  Well, as we can see in the test, this doesn't work
with AF_UNIX but I'm wondering if there is no other way to do it.
Anyway, this seems closer to the use of a directory's FD to open another
file: the access check is done when accessing a "new" resource.  To say
it another way, a socket may be seen as a builder object to exchange
data with a peer, and this object can be reconfigured/recycled to
exchange data with another peer.  This rationale also applies to TCP
connect and bind control.

> 
> > 
> > > diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> > > index 20fdb5ff3514..7b794b81ef05 100644
> > > --- a/security/landlock/limits.h
> > > +++ b/security/landlock/limits.h
> > > @@ -28,6 +28,11 @@
> > >  #define LANDLOCK_NUM_ACCESS_NET		__const_hweight64(LANDLOCK_MASK_ACCESS_NET)
> > >  #define LANDLOCK_SHIFT_ACCESS_NET	LANDLOCK_NUM_ACCESS_FS
> > >  
> > > +#define LANDLOCK_LAST_ACCESS_SCOPE       LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET
> > > +#define LANDLOCK_MASK_ACCESS_SCOPE	((LANDLOCK_LAST_ACCESS_SCOPE << 1) - 1)
> > > +#define LANDLOCK_NUM_ACCESS_SCOPE         __const_hweight64(LANDLOCK_MASK_ACCESS_SCOPE)
> > > +#define LANDLOCK_SHIFT_ACCESS_SCOPE      LANDLOCK_SHIFT_ACCESS_NET
> >                                             ^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > I believe this #define has the wrong value, and as a consequence, the code
> > suffers from the same problem as we already had on the other patch set from
> > Mikhail Ivanov -- see [1] for that discussion.
> 
> Thanks for the hint. I will definitly check this. 
> 
> > The LANDLOCK_SHIFT_ACCESS_FOO variable is used for determining the position of
> > your flag in the access_masks_t type, where all access masks are combined
> > together in one big bit vector.  If you are defining this the same for _SCOPE as
> > for _NET, I believe that we will start using the same bits in that vector for
> > both the _NET flags and the _SCOPE flags, and that will manifest in unwanted
> > interactions between the different types of restrictions.  (e.g. you will create
> > a policy to restrict _SCOPE, and you will find yourself unable to do some things
> > with TCP ports)
> > 
> > Please also see the other thread for more discussions about how we can avoid
> > such problems in the future.  (This code is easy to get wrong,
> > apparently... When we don't test what happens across multiple types of
> > restrictions, everything looks fine.)
> > 
> > [1] https://lore.kernel.org/all/ebd680cc-25d6-ee14-4856-310f5e5e28e4@huawei-partners.com/

Yep, good catch.

A test should be able to catch this issue.

> > 
> > —Günther
> > 
> 

Re: [PATCH v3] landlock: Add abstract unix socket connect restriction

[Jann Horn]

On Mon, Jun 10, 2024 at 6:36 PM Mickaël Salaün <mic@digikod.net> wrote:
> On Fri, Jun 07, 2024 at 01:41:39PM -0600, Tahera Fahimi wrote:
> > On Fri, Jun 07, 2024 at 10:28:35AM +0200, Günther Noack wrote:
> > > Is it intentional that you are both restricting the connection and the sending
> > > with the same flag (security_unix_may_send)?  If an existing Unix Domain Socket
> > > gets passed in to a program from the outside (e.g. as stdout), shouldn't it
> > > still be possible that the program enables a Landlock policy and then still
> > > writes to it?  (Does that work?  Am I mis-reading the patch?)
>
> If a passed socket is already connected, then a write/send should work.

If I'm reading unix_dgram_sendmsg() correctly, we'll always hit
security_unix_may_send() for any UNIX socket type other than
SOCK_SEQPACKET (meaning SOCK_STREAM and SOCK_DGRAM), even if the
socket is already connected, and then we'll do the landlock check.
That's probably not the intended behavior for Landlock, unless I'm
misreading the code?

Maybe to get nice semantics it's necessary to add a parameter to
security_unix_may_send() that says whether the destination address
came from the caller or from the socket?

Re: [PATCH v3] landlock: Add abstract unix socket connect restriction

[Jann Horn]

Hi!

Thanks for helping with making Landlock more comprehensive!

On Fri, Jun 7, 2024 at 1:44 AM Tahera Fahimi <fahimitahera@gmail.com> wrote:
> Abstract unix sockets are used for local inter-process communications
> without on a filesystem. Currently a sandboxed process can connect to a
> socket outside of the sandboxed environment, since landlock has no
> restriction for connecting to a unix socket in the abstract namespace.
> Access to such sockets for a sandboxed process should be scoped the same
> way ptrace is limited.

This reminds me - from what I remember, Landlock also doesn't restrict
access to filesystem-based unix sockets yet... I'm I'm right about
that, we should probably at some point add code at some point to
restrict that as part of the path-based filesystem access rules? (But
to be clear, I'm not saying I expect you to do that as part of your
patch, just commenting for context.)

> Because of compatibility reasons and since landlock should be flexible,
> we extend the user space interface by adding a new "scoped" field. This
> field optionally contains a "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to
> specify that the ruleset will deny any connection from within the
> sandbox to its parents(i.e. any parent sandbox or non-sandbox processes)

You call the feature "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET", but I
don't see anything in this code that actually restricts it to abstract
unix sockets (as opposed to path-based ones and unnamed ones, see the
"Three types of address are distinguished" paragraph of
https://man7.org/linux/man-pages/man7/unix.7.html). If the feature is
supposed to be limited to abstract unix sockets, I guess you'd maybe
have to inspect the unix_sk(other)->addr, check that it's non-NULL,
and then check that `unix_sk(other)->addr->name->sun_path[0] == 0`,
similar to what unix_seq_show() does? (unix_seq_show() shows abstract
sockets with an "@".)

Separately, I wonder if it would be useful to have another mode for
forbidding access to abstract unix sockets entirely; or alternatively
to change the semantics of LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET so
that it also forbids access from outside the landlocked domain as was
discussed elsewhere in the thread. If a landlocked process starts
listening on something like "@/tmp/.X11-unix/X0", maybe X11 clients
elsewhere on my system shouldn't be confused into connecting to that
landlocked socket...

[...]
> +static bool sock_is_scoped(struct sock *const other)
> +{
> +       bool is_scoped = true;
> +       const struct landlock_ruleset *dom_other;
> +       const struct cred *cred_other;
> +
> +       const struct landlock_ruleset *const dom =
> +               landlock_get_current_domain();
> +       if (!dom)
> +               return true;
> +
> +       lockdep_assert_held(&unix_sk(other)->lock);
> +       /* the credentials will not change */
> +       cred_other = get_cred(other->sk_peer_cred);
> +       dom_other = landlock_cred(cred_other)->domain;
> +       is_scoped = domain_scope_le(dom, dom_other);
> +       put_cred(cred_other);

You don't have to use get_cred()/put_cred() here; as the comment says,
the credentials will not change, so we don't need to take another
reference to them.

> +       return is_scoped;
> +}

Re: [PATCH v3] landlock: Add abstract unix socket connect restriction

[Mickaël Salaün]

On Mon, Jun 10, 2024 at 11:49:21PM +0200, Jann Horn wrote:
> On Mon, Jun 10, 2024 at 6:36 PM Mickaël Salaün <mic@digikod.net> wrote:
> > On Fri, Jun 07, 2024 at 01:41:39PM -0600, Tahera Fahimi wrote:
> > > On Fri, Jun 07, 2024 at 10:28:35AM +0200, Günther Noack wrote:
> > > > Is it intentional that you are both restricting the connection and the sending
> > > > with the same flag (security_unix_may_send)?  If an existing Unix Domain Socket
> > > > gets passed in to a program from the outside (e.g. as stdout), shouldn't it
> > > > still be possible that the program enables a Landlock policy and then still
> > > > writes to it?  (Does that work?  Am I mis-reading the patch?)
> >
> > If a passed socket is already connected, then a write/send should work.
> 
> If I'm reading unix_dgram_sendmsg() correctly, we'll always hit
> security_unix_may_send() for any UNIX socket type other than
> SOCK_SEQPACKET (meaning SOCK_STREAM and SOCK_DGRAM), even if the
> socket is already connected, and then we'll do the landlock check.
> That's probably not the intended behavior for Landlock, unless I'm
> misreading the code?
> 
> Maybe to get nice semantics it's necessary to add a parameter to
> security_unix_may_send() that says whether the destination address
> came from the caller or from the socket?

I think it would make sense to ignore connected sockets with
security_unix_may_send() because it should already be controlled by
security_unix_stream_connect().  This would still allow passed/inherited
connected sockets to be used, which is an important feature and would
be consistent with read/write on other passed FDs.  This would not work
with dgram sockets though.

We need tests for this case and with different socket modes (inspired by
the net_test.c:protocol variants).

Re: [PATCH v3] landlock: Add abstract unix socket connect restriction

[Mickaël Salaün]

On Tue, Jun 11, 2024 at 12:27:58AM +0200, Jann Horn wrote:
> Hi!
> 
> Thanks for helping with making Landlock more comprehensive!
> 
> On Fri, Jun 7, 2024 at 1:44 AM Tahera Fahimi <fahimitahera@gmail.com> wrote:
> > Abstract unix sockets are used for local inter-process communications
> > without on a filesystem. Currently a sandboxed process can connect to a
> > socket outside of the sandboxed environment, since landlock has no
> > restriction for connecting to a unix socket in the abstract namespace.
> > Access to such sockets for a sandboxed process should be scoped the same
> > way ptrace is limited.
> 
> This reminds me - from what I remember, Landlock also doesn't restrict
> access to filesystem-based unix sockets yet... I'm I'm right about
> that, we should probably at some point add code at some point to
> restrict that as part of the path-based filesystem access rules? (But
> to be clear, I'm not saying I expect you to do that as part of your
> patch, just commenting for context.)

Yes, I totally agree.  For now, unix socket binding requires to create
the LANDLOCK_ACCESS_FS_MAKE_SOCK right, but connecting to an existing
socket is not controlled.  The abstract unix socket scoping is
orthogonal and extends Landlock with unix socket LSM hooks, which are
required to extend the "filesystem" access rights to control path-based
unix socket.

> 
> > Because of compatibility reasons and since landlock should be flexible,
> > we extend the user space interface by adding a new "scoped" field. This
> > field optionally contains a "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to
> > specify that the ruleset will deny any connection from within the
> > sandbox to its parents(i.e. any parent sandbox or non-sandbox processes)
> 
> You call the feature "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET", but I
> don't see anything in this code that actually restricts it to abstract
> unix sockets (as opposed to path-based ones and unnamed ones, see the
> "Three types of address are distinguished" paragraph of
> https://man7.org/linux/man-pages/man7/unix.7.html). If the feature is
> supposed to be limited to abstract unix sockets, I guess you'd maybe
> have to inspect the unix_sk(other)->addr, check that it's non-NULL,
> and then check that `unix_sk(other)->addr->name->sun_path[0] == 0`,
> similar to what unix_seq_show() does? (unix_seq_show() shows abstract
> sockets with an "@".)

Right, that should be part of the next series.  Tests should check that
too.

> 
> Separately, I wonder if it would be useful to have another mode for
> forbidding access to abstract unix sockets entirely; or alternatively
> to change the semantics of LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET so
> that it also forbids access from outside the landlocked domain as was
> discussed elsewhere in the thread. If a landlocked process starts
> listening on something like "@/tmp/.X11-unix/X0", maybe X11 clients
> elsewhere on my system shouldn't be confused into connecting to that
> landlocked socket...

In this case, I think we should have a (light) Landlock domain for a
user session to make sure apps only connect to the legitimate X11 socket
(either in the same domain, or through a path-based socket).

There is also ongoing work to restrict socket creation according to their
type:
https://lore.kernel.org/all/20240524093015.2402952-1-ivanov.mikhail1@huawei-partners.com/
This will make possible to control abstract unix socket creation and
avoid this kind of issue too.

> 
> [...]
> > +static bool sock_is_scoped(struct sock *const other)
> > +{
> > +       bool is_scoped = true;
> > +       const struct landlock_ruleset *dom_other;
> > +       const struct cred *cred_other;
> > +
> > +       const struct landlock_ruleset *const dom =
> > +               landlock_get_current_domain();
> > +       if (!dom)
> > +               return true;
> > +
> > +       lockdep_assert_held(&unix_sk(other)->lock);
> > +       /* the credentials will not change */
> > +       cred_other = get_cred(other->sk_peer_cred);
> > +       dom_other = landlock_cred(cred_other)->domain;
> > +       is_scoped = domain_scope_le(dom, dom_other);
> > +       put_cred(cred_other);
> 
> You don't have to use get_cred()/put_cred() here; as the comment says,
> the credentials will not change, so we don't need to take another
> reference to them.
> 
> > +       return is_scoped;
> > +}
> 

Re: [PATCH v3] landlock: Add abstract unix socket connect restriction

[Tahera Fahimi]

On Tue, Jun 11, 2024 at 12:27:58AM +0200, Jann Horn wrote:
> Hi!
> 
> Thanks for helping with making Landlock more comprehensive!
Thanks for your feedback!

> On Fri, Jun 7, 2024 at 1:44 AM Tahera Fahimi <fahimitahera@gmail.com> wrote:
> > Abstract unix sockets are used for local inter-process communications
> > without on a filesystem. Currently a sandboxed process can connect to a
> > socket outside of the sandboxed environment, since landlock has no
> > restriction for connecting to a unix socket in the abstract namespace.
> > Access to such sockets for a sandboxed process should be scoped the same
> > way ptrace is limited.
> 
> This reminds me - from what I remember, Landlock also doesn't restrict
> access to filesystem-based unix sockets yet... I'm I'm right about
> that, we should probably at some point add code at some point to
> restrict that as part of the path-based filesystem access rules? (But
> to be clear, I'm not saying I expect you to do that as part of your
> patch, just commenting for context.)
> 
> > Because of compatibility reasons and since landlock should be flexible,
> > we extend the user space interface by adding a new "scoped" field. This
> > field optionally contains a "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to
> > specify that the ruleset will deny any connection from within the
> > sandbox to its parents(i.e. any parent sandbox or non-sandbox processes)
> 
> You call the feature "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET", but I
> don't see anything in this code that actually restricts it to abstract
> unix sockets (as opposed to path-based ones and unnamed ones, see the
> "Three types of address are distinguished" paragraph of
> https://man7.org/linux/man-pages/man7/unix.7.html). If the feature is
> supposed to be limited to abstract unix sockets, I guess you'd maybe
> have to inspect the unix_sk(other)->addr, check that it's non-NULL,
> and then check that `unix_sk(other)->addr->name->sun_path[0] == 0`,
> similar to what unix_seq_show() does? (unix_seq_show() shows abstract
> sockets with an "@".)
Correct, I will break it into another function that checks if it is an
abstract unix socket. In this case, we can add other restrictions on
connection for pathname and unname sockets later. 

> Separately, I wonder if it would be useful to have another mode for
> forbidding access to abstract unix sockets entirely; or alternatively
> to change the semantics of LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET so
> that it also forbids access from outside the landlocked domain as was
> discussed elsewhere in the thread. If a landlocked process starts
> listening on something like "@/tmp/.X11-unix/X0", maybe X11 clients
> elsewhere on my system shouldn't be confused into connecting to that
> landlocked socket...
> 
> [...]
> > +static bool sock_is_scoped(struct sock *const other)
> > +{
> > +       bool is_scoped = true;
> > +       const struct landlock_ruleset *dom_other;
> > +       const struct cred *cred_other;
> > +
> > +       const struct landlock_ruleset *const dom =
> > +               landlock_get_current_domain();
> > +       if (!dom)
> > +               return true;
> > +
> > +       lockdep_assert_held(&unix_sk(other)->lock);
> > +       /* the credentials will not change */
> > +       cred_other = get_cred(other->sk_peer_cred);
> > +       dom_other = landlock_cred(cred_other)->domain;
> > +       is_scoped = domain_scope_le(dom, dom_other);
> > +       put_cred(cred_other);
> 
> You don't have to use get_cred()/put_cred() here; as the comment says,
> the credentials will not change, so we don't need to take another
> reference to them.
Noted. Thanks.

> > +       return is_scoped;
> > +}

Re: [PATCH v3] landlock: Add abstract unix socket connect restriction

[Günther Noack]

On Tue, Jun 11, 2024 at 10:19:20AM +0200, Mickaël Salaün wrote:
> On Tue, Jun 11, 2024 at 12:27:58AM +0200, Jann Horn wrote:
> > This reminds me - from what I remember, Landlock also doesn't restrict
> > access to filesystem-based unix sockets yet... I'm I'm right about
> > that, we should probably at some point add code at some point to
> > restrict that as part of the path-based filesystem access rules? (But
> > to be clear, I'm not saying I expect you to do that as part of your
> > patch, just commenting for context.)
> 
> Yes, I totally agree.  For now, unix socket binding requires to create
> the LANDLOCK_ACCESS_FS_MAKE_SOCK right, but connecting to an existing
> socket is not controlled.  The abstract unix socket scoping is
> orthogonal and extends Landlock with unix socket LSM hooks, which are
> required to extend the "filesystem" access rights to control path-based
> unix socket.

Thanks for the reminder, Jann!  I filed it as
https://github.com/landlock-lsm/linux/issues/36.

–Günther