diff mbox series

[RFC,4/9] User-space API for creating a supervisor-fd

Message ID 03d822634936f4c3ac8e4843f9913d1b1fa9d081.1741047969.git.m@maowtm.org (mailing list archive)
State New
Headers show
Series Landlock supervise: a mechanism for interactive permission requests | expand

Commit Message

Tingmao Wang March 4, 2025, 1:13 a.m. UTC
We allow the user to pass in an additional flag to landlock_create_ruleset
which will make the ruleset operate in "supervise" mode, with a supervisor
attached. We create additional space in the landlock_ruleset_attr
structure to pass the newly created supervisor fd back to user-space.

The intention, while not implemented yet, is that the user-space will read
events from this fd and write responses back to it.

Note: need to investigate if fd clone on fork() is handled correctly, but
should be fine if it shares the struct file. We might also want to let the
user customize the flags on this fd, so that they can request no
O_CLOEXEC.

NOTE: despite this patch having a new uapi, I'm still very open to e.g.
re-using fanotify stuff instead (if that makes sense in the end). This is
just a PoC.

Signed-off-by: Tingmao Wang <m@maowtm.org>
---
 include/uapi/linux/landlock.h |  10 ++++
 security/landlock/syscalls.c  | 102 +++++++++++++++++++++++++++++-----
 2 files changed, 98 insertions(+), 14 deletions(-)

Comments

Mickaël Salaün March 5, 2025, 4:09 p.m. UTC | #1
On Tue, Mar 04, 2025 at 01:13:00AM +0000, Tingmao Wang wrote:
> We allow the user to pass in an additional flag to landlock_create_ruleset
> which will make the ruleset operate in "supervise" mode, with a supervisor
> attached. We create additional space in the landlock_ruleset_attr
> structure to pass the newly created supervisor fd back to user-space.
> 
> The intention, while not implemented yet, is that the user-space will read
> events from this fd and write responses back to it.
> 
> Note: need to investigate if fd clone on fork() is handled correctly, but
> should be fine if it shares the struct file. We might also want to let the
> user customize the flags on this fd, so that they can request no
> O_CLOEXEC.
> 
> NOTE: despite this patch having a new uapi, I'm still very open to e.g.
> re-using fanotify stuff instead (if that makes sense in the end). This is
> just a PoC.

The main security risk of this feature is for this FD to leak and be
used by a sandboxed process to bypass all its restrictions.  This should
be highlighted in the UAPI documentation.

> 
> Signed-off-by: Tingmao Wang <m@maowtm.org>
> ---
>  include/uapi/linux/landlock.h |  10 ++++
>  security/landlock/syscalls.c  | 102 +++++++++++++++++++++++++++++-----
>  2 files changed, 98 insertions(+), 14 deletions(-)
> 
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index e1d2c27533b4..7bc1eb4859fb 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -50,6 +50,15 @@ struct landlock_ruleset_attr {
>  	 * resources (e.g. IPCs).
>  	 */
>  	__u64 scoped;
> +	/**
> +	 * @supervisor_fd: Placeholder to store the supervisor file
> +	 * descriptor when %LANDLOCK_CREATE_RULESET_SUPERVISE is set.
> +	 */
> +	__s32 supervisor_fd;

This interface would require the ruleset_attr becoming updatable by the
kernel, which might be OK in theory but requires current syscall wrapper
signature update, see sandboxer.c change.  It also creates a FD which
might not be useful (e.g. if an error occurs before the actual
enforcement).

I see a few alternatives.  We could just use/extend the ruleset FD
instead of creating a new one, but because leaking current rulesets is
not currently a security risk, we should be careful to not change that.

Another approach, similar to seccomp unotify, is to get a
"[landlock-domain]" FD returned by the landlock_restrict_self(2) when a
new LANDLOCK_RESTRICT_SELF_DOMAIN_FD flag is set.  This FD would be a
reference to the newly created domain, which is more specific than the
ruleset used to created this domain (and that can be used to create
other domains).  This domain FD could be used for introspection (i.e.
to get read-only properties such as domain ID), but being able to
directly supervise the referenced domain only with this FD would be a
risk that we should limit.

What we can do is to implement an IOCTL command for such domain FD that
would return a supervisor FD (if the LANDLOCK_RESTRICT_SELF_SUPERVISED
flag was also set).  The key point is to check (one time) that the
process calling this IOCTL is not restricted by the related domain (see
the scope helpers).

Relying on IOCTL commands (for all these FD types) instead of read/write
operations should also limit the risk of these FDs being misused through
a confused deputy attack (because such IOCTL command would convey an
explicit intent):
https://docs.kernel.org/security/credentials.html#open-file-credentials
https://lore.kernel.org/all/CAG48ez0HW-nScxn4G5p8UHtYy=T435ZkF3Tb1ARTyyijt_cNEg@mail.gmail.com/
We should get inspiration from seccomp unotify for this too:
https://lore.kernel.org/all/20181209182414.30862-1-tycho@tycho.ws/

> +	/**
> +	 * @pad: Unused, must be zero.
> +	 */
> +	__u32 pad;

In this case we should pack the struct instead.

>  };
>  
>  /*
> @@ -60,6 +69,7 @@ struct landlock_ruleset_attr {
>   */
>  /* clang-format off */
>  #define LANDLOCK_CREATE_RULESET_VERSION			(1U << 0)
> +#define LANDLOCK_CREATE_RULESET_SUPERVISE		(1U << 1)
>  /* clang-format on */
>  
>  /**

[...]
diff mbox series

Patch

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index e1d2c27533b4..7bc1eb4859fb 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -50,6 +50,15 @@  struct landlock_ruleset_attr {
 	 * resources (e.g. IPCs).
 	 */
 	__u64 scoped;
+	/**
+	 * @supervisor_fd: Placeholder to store the supervisor file
+	 * descriptor when %LANDLOCK_CREATE_RULESET_SUPERVISE is set.
+	 */
+	__s32 supervisor_fd;
+	/**
+	 * @pad: Unused, must be zero.
+	 */
+	__u32 pad;
 };
 
 /*
@@ -60,6 +69,7 @@  struct landlock_ruleset_attr {
  */
 /* clang-format off */
 #define LANDLOCK_CREATE_RULESET_VERSION			(1U << 0)
+#define LANDLOCK_CREATE_RULESET_SUPERVISE		(1U << 1)
 /* clang-format on */
 
 /**
diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index ead9b68168ad..adf7e77023b5 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -32,6 +32,7 @@ 
 #include "limits.h"
 #include "net.h"
 #include "ruleset.h"
+#include "supervise.h"
 #include "setup.h"
 
 static bool is_initialized(void)
@@ -99,8 +100,10 @@  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);
+	ruleset_size += sizeof(ruleset_attr.supervisor_fd);
+	ruleset_size += sizeof(ruleset_attr.pad);
 	BUILD_BUG_ON(sizeof(ruleset_attr) != ruleset_size);
-	BUILD_BUG_ON(sizeof(ruleset_attr) != 24);
+	BUILD_BUG_ON(sizeof(ruleset_attr) != 32);
 
 	path_beneath_size = sizeof(path_beneath_attr.allowed_access);
 	path_beneath_size += sizeof(path_beneath_attr.parent_fd);
@@ -151,16 +154,42 @@  static const struct file_operations ruleset_fops = {
 	.write = fop_dummy_write,
 };
 
-#define LANDLOCK_ABI_VERSION 6
+static int fop_supervisor_release(struct inode *const inode,
+				  struct file *const filp)
+{
+	struct landlock_supervisor *supervisor = filp->private_data;
+
+	landlock_put_supervisor(supervisor);
+	return 0;
+}
+
+static const struct file_operations supervisor_fops = {
+	.release = fop_supervisor_release,
+	/* TODO: read, write, poll, dup */
+	.read = fop_dummy_read,
+	.write = fop_dummy_write,
+};
+
+static int
+landlock_supervisor_open_fd(struct landlock_supervisor *const supervisor,
+			    const fmode_t mode)
+{
+	landlock_get_supervisor(supervisor);
+	return anon_inode_getfd("[landlock-supervisor]", &supervisor_fops,
+				supervisor, O_RDWR | O_CLOEXEC);
+}
+
+#define LANDLOCK_ABI_VERSION 7
 
 /**
  * sys_landlock_create_ruleset - Create a new ruleset
  *
- * @attr: Pointer to a &struct landlock_ruleset_attr identifying the scope of
- *        the new ruleset.
- * @size: Size of the pointed &struct landlock_ruleset_attr (needed for
- *        backward and forward compatibility).
- * @flags: Supported value: %LANDLOCK_CREATE_RULESET_VERSION.
+ * @attr:  Pointer to a &struct landlock_ruleset_attr identifying the scope of
+ *         the new ruleset.
+ * @size:  Size of the pointed &struct landlock_ruleset_attr (needed for
+ *         backward and forward compatibility).
+ * @flags: Supported value: %LANDLOCK_CREATE_RULESET_VERSION,
+ * 	       %LANDLOCK_CREATE_RULESET_SUPERVISE.
  *
  * This system call enables to create a new Landlock ruleset, and returns the
  * related file descriptor on success.
@@ -172,18 +201,21 @@  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 unknown scope, or too small @size;
+ * - %EINVAL: unknown @flags, or unknown access, or unknown
+ * 	          scope, or too small @size, or non-zero @pad;
  * - %E2BIG: @attr or @size inconsistencies;
  * - %EFAULT: @attr or @size inconsistencies;
  * - %ENOMSG: empty &landlock_ruleset_attr.handled_access_fs.
  */
 SYSCALL_DEFINE3(landlock_create_ruleset,
-		const struct landlock_ruleset_attr __user *const, attr,
-		const size_t, size, const __u32, flags)
+		struct landlock_ruleset_attr __user *const, attr, const size_t,
+		size, const __u32, flags)
 {
 	struct landlock_ruleset_attr ruleset_attr;
 	struct landlock_ruleset *ruleset;
+	struct landlock_supervisor *supervisor;
 	int err, ruleset_fd;
+	bool supervise = false;
 
 	/* Build-time checks. */
 	build_check_abi();
@@ -192,10 +224,16 @@  SYSCALL_DEFINE3(landlock_create_ruleset,
 		return -EOPNOTSUPP;
 
 	if (flags) {
-		if ((flags == LANDLOCK_CREATE_RULESET_VERSION) && !attr &&
-		    !size)
+		if (flags == LANDLOCK_CREATE_RULESET_VERSION) {
+			if (attr || size)
+				return -EINVAL;
 			return LANDLOCK_ABI_VERSION;
-		return -EINVAL;
+		}
+		if (flags == LANDLOCK_CREATE_RULESET_SUPERVISE) {
+			supervise = true;
+		} else {
+			return -EINVAL;
+		}
 	}
 
 	/* Copies raw user space buffer. */
@@ -206,6 +244,13 @@  SYSCALL_DEFINE3(landlock_create_ruleset,
 	if (err)
 		return err;
 
+	if (supervise && size < offsetofend(typeof(ruleset_attr), pad))
+		return -EINVAL;
+
+	if (size >= offsetofend(typeof(ruleset_attr), pad) &&
+	    ruleset_attr.pad != 0)
+		return -EINVAL;
+
 	/* Checks content (and 32-bits cast). */
 	if ((ruleset_attr.handled_access_fs | LANDLOCK_MASK_ACCESS_FS) !=
 	    LANDLOCK_MASK_ACCESS_FS)
@@ -227,11 +272,40 @@  SYSCALL_DEFINE3(landlock_create_ruleset,
 	if (IS_ERR(ruleset))
 		return PTR_ERR(ruleset);
 
+	if (supervise) {
+		supervisor = landlock_create_supervisor();
+		if (IS_ERR(supervisor)) {
+			landlock_put_ruleset(ruleset);
+			return -ENOMEM;
+		}
+		/* Pass ownership of supervisor to ruleset struct */
+		ruleset->layer_stack[0].supervisor = supervisor;
+	}
+
 	/* Creates anonymous FD referring to the ruleset. */
 	ruleset_fd = anon_inode_getfd("[landlock-ruleset]", &ruleset_fops,
 				      ruleset, O_RDWR | O_CLOEXEC);
-	if (ruleset_fd < 0)
+	if (ruleset_fd < 0) {
 		landlock_put_ruleset(ruleset);
+		return ruleset_fd;
+	}
+
+	if (supervise) {
+		int supervisor_fd;
+
+		supervisor_fd = landlock_supervisor_open_fd(
+			ruleset->layer_stack[0].supervisor, O_RDWR | O_CLOEXEC);
+		if (supervisor_fd < 0) {
+			landlock_put_ruleset(ruleset);
+			return supervisor_fd;
+		}
+		if (copy_to_user(&attr->supervisor_fd, &supervisor_fd,
+				 sizeof(supervisor_fd))) {
+			landlock_put_ruleset(ruleset);
+			return -EFAULT;
+		}
+	}
+
 	return ruleset_fd;
 }