[RFC 0/4] Landlock: ioctl support

[RFC 0/4] Landlock: ioctl support

[Günther Noack]

Hello!

These patches add ioctl support to Landlock.

It's an early version - it potentially needs more tests and
documentation.  I'd like to circulate the patches early to discuss
whether this approach is feasible.

Objective
~~~~~~~~~

Make ioctl(2) requests restrictable with Landlock,
in a way that is useful for real-world applications.

Proposed approach
~~~~~~~~~~~~~~~~~

Introduce the LANDLOCK_ACCESS_FS_IOCTL right, which restricts the use
of ioctl(2) on file descriptors.

We attach the LANDLOCK_ACCESS_FS_IOCTL right to opened file
descriptors, as we already do for LANDLOCK_ACCESS_FS_TRUNCATE.

I believe that this approach works for the majority of use cases, and
offers a good trade-off between Landlock API and implementation
complexity and flexibility when the feature is used.

Notable implications of this approach
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

* Existing inherited file descriptors stay unaffected
  when a program enables Landlock.

  This means in particular that in common scenarios,
  the terminal's ioctls (ioctl_tty(2)) continue to work.

* ioctl(2) continues to be available for file descriptors acquired
  through means other than open(2).  Example: Network sockets.

Examples
~~~~~~~~

Starting a sandboxed shell from $HOME with samples/landlock/sandboxer:

  LL_FS_RO=/ LL_FS_RW=. ./sandboxer /bin/bash

The LANDLOCK_ACCESS_FS_IOCTL right is part of the "read-write" rights
here, so we expect that newly opened files outside of $HOME don't work
with ioctl(2).

  * "stty" works: It probes terminal properties

  * "stty </dev/tty" fails: /dev/tty can be reopened, but the ioctl is
    denied.

  * "eject" fails: ioctls to use CD-ROM drive are denied.

  * "ls /dev" works: It uses ioctl to get the terminal size for
    columnar layout

  * The text editors "vim" and "mg" work.  (GNU Emacs fails because it
    attempts to reopen /dev/tty.)

Alternatives considered: Allow-listing specific ioctl requests
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

It would be technically possible to keep an allow-list of ioctl
requests and thereby control in more detail which ioctls should work.

I believe that this is not needed for the majority of use cases and
that it is a reasonable trade-off to make here (but I'm happy to hear
about counterexamples).  The reasoning is:

* Many programs do not need ioctl at all,
  and denying all ioctl(2) requests OK for these.

* Other programs need ioctl, but only for the terminal FDs.
  This is supported because these file descriptors are usually
  inherited from the parent process - so the parent process gets to
  control the ioctl(2) policy for them.

* Some programs need ioctl on specific files that they are opening
  themselves.  They can allow-list these file paths for ioctl(2).
  This makes the programs work, but it restricts a variety of other
  ioctl requests which are otherwise possible through opening other
  files.

Because the LANDLOCK_ACCESS_FS_IOCTL right is attached to the file
descriptor, programs have flexible options to control which ioctl
operations should work, without the implementation complexity of
additional ioctl allow-lists in the kernel.

Finally, the proposed approach is simpler in implementation and has
lower API complexity, but it does *not* preclude us from implementing
per-ioctl-request allow lists later, if that turns out to be necessary
at a later point.

Related Work
~~~~~~~~~~~~

OpenBSD's pledge(2) [1] restricts ioctl(2) independent of the file
descriptor which is used.  The implementers maintain multiple
allow-lists of predefined ioctl(2) operations required for different
application domains such as "audio", "bpf", "tty" and "inet".

OpenBSD does not guarantee ABI backwards compatibility to the same
extent as Linux does, so it's easier for them to update these lists in
later versions.  It might not be a feasible approach for Linux though.

[1] https://man.openbsd.org/OpenBSD-7.3/pledge.2

Feedback I'm looking for
~~~~~~~~~~~~~~~~~~~~~~~~

Some specific points I would like to get your opinion on:

* Is this the right general approach to restricting ioctl(2)?

  It will probably be possible to find counter-examples where the
  alternative (see below) is better.  I'd be interested in these, and
  in how common they are, to understand whether we have picked the
  right trade-off here.

* Should we introduce a "landlock_fd_rights_limit()" syscall?

  We could potentially implement a system call for dropping the
  LANDLOCK_ACCESS_FS_IOCTL and LANDLOCK_ACCESS_FS_TRUNCATE rights from
  existing file descriptors (independent of the type of file
  descriptor, even).

  Possible use cases would be to (a) restrict the rights on inherited
  file descriptors like std{in,out,err} and to (b) restrict ioctl and
  truncate operations on file descriptors that are not acquired
  through open(2), such as network sockets.

  This would be similar to the cap_rights_limit(2) system call in
  FreeBSD's Capsicum.

  This idea feels somewhat orthogonal to the ioctl patch, but it would
  start to be more useful if the ioctl right exists.


Günther Noack (4):
  landlock: Increment Landlock ABI version to 4
  landlock: Add LANDLOCK_ACCESS_FS_IOCTL access right
  selftests/landlock: Test ioctl support
  samples/landlock: Add support for LANDLOCK_ACCESS_FS_IOCTL

 include/uapi/linux/landlock.h                | 19 ++++--
 samples/landlock/sandboxer.c                 | 12 +++-
 security/landlock/fs.c                       | 20 +++++-
 security/landlock/limits.h                   |  2 +-
 security/landlock/syscalls.c                 |  2 +-
 tools/testing/selftests/landlock/base_test.c |  2 +-
 tools/testing/selftests/landlock/fs_test.c   | 67 +++++++++++++++++++-
 7 files changed, 107 insertions(+), 17 deletions(-)


base-commit: 457391b0380335d5e9a5babdec90ac53928b23b4
-- 
2.40.1

[RFC 1/4] landlock: Increment Landlock ABI version to 4

[Günther Noack]

Increment the Landlock ABI version in preparation for the ioctl
feature.

Signed-off-by: Günther Noack <gnoack3000@gmail.com>
---
 security/landlock/syscalls.c                 | 2 +-
 tools/testing/selftests/landlock/base_test.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index 245cc650a4d..c70fc9e6fe9 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -129,7 +129,7 @@ static const struct file_operations ruleset_fops = {
 	.write = fop_dummy_write,
 };
 
-#define LANDLOCK_ABI_VERSION 3
+#define LANDLOCK_ABI_VERSION 4
 
 /**
  * sys_landlock_create_ruleset - Create a new ruleset
diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
index 792c3f0a59b..646f778dfb1 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(3, landlock_create_ruleset(NULL, 0,
+	ASSERT_EQ(4, landlock_create_ruleset(NULL, 0,
 					     LANDLOCK_CREATE_RULESET_VERSION));
 
 	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
-- 
2.40.1

Re: [RFC 1/4] landlock: Increment Landlock ABI version to 4

[Mickaël Salaün]

We should increment the version with the (next) commit that enables the 
feature. This helps avoid a backport with a wrong version.


On 02/05/2023 19:17, Günther Noack wrote:
> Increment the Landlock ABI version in preparation for the ioctl
> feature.
> 
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> ---
>   security/landlock/syscalls.c                 | 2 +-
>   tools/testing/selftests/landlock/base_test.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> index 245cc650a4d..c70fc9e6fe9 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -129,7 +129,7 @@ static const struct file_operations ruleset_fops = {
>   	.write = fop_dummy_write,
>   };
>   
> -#define LANDLOCK_ABI_VERSION 3
> +#define LANDLOCK_ABI_VERSION 4
>   
>   /**
>    * sys_landlock_create_ruleset - Create a new ruleset
> diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
> index 792c3f0a59b..646f778dfb1 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(3, landlock_create_ruleset(NULL, 0,
> +	ASSERT_EQ(4, landlock_create_ruleset(NULL, 0,
>   					     LANDLOCK_CREATE_RULESET_VERSION));
>   
>   	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,

[RFC 2/4] landlock: Add LANDLOCK_ACCESS_FS_IOCTL access right

[Günther Noack]

Like the truncate right, this right is associated with a file
descriptor at the time of open(2), and gets respected even when the
file descriptor is used outside of the thread which it was originally
created in.

In particular, this happens for the commonly inherited file
descriptors stdin, stdout and stderr, if these are bound to a tty.
This means that programs using tty ioctls can drop the ioctl access
right, but continue using these ioctls on the already opened input and
output file descriptors.

Signed-off-by: Günther Noack <gnoack3000@gmail.com>
---
 include/uapi/linux/landlock.h              | 19 ++++++++++++-------
 security/landlock/fs.c                     | 20 ++++++++++++++++++--
 security/landlock/limits.h                 |  2 +-
 tools/testing/selftests/landlock/fs_test.c |  5 +++--
 4 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index f3223f96469..d87457a1c22 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -102,12 +102,16 @@ struct landlock_path_beneath_attr {
  * - %LANDLOCK_ACCESS_FS_READ_FILE: Open a file with read access.
  * - %LANDLOCK_ACCESS_FS_TRUNCATE: Truncate a file with :manpage:`truncate(2)`,
  *   :manpage:`ftruncate(2)`, :manpage:`creat(2)`, or :manpage:`open(2)` with
- *   ``O_TRUNC``. Whether an opened file can be truncated with
- *   :manpage:`ftruncate(2)` is determined during :manpage:`open(2)`, in the
- *   same way as read and write permissions are checked during
- *   :manpage:`open(2)` using %LANDLOCK_ACCESS_FS_READ_FILE and
- *   %LANDLOCK_ACCESS_FS_WRITE_FILE. This access right is available since the
- *   third version of the Landlock ABI.
+ *   ``O_TRUNC``.  This access right is available since the third version of the
+ *   Landlock ABI.
+ * - %LANDLOCK_ACCESS_FS_IOCTL: Invoke :manpage:`ioctl(2)` on the opened file.
+ *   This access right is available since the fourth version of the Landlock
+ *   ABI.
+ *
+ * Whether an opened file can be truncated with :manpage:`ftruncate(2)` or used
+ * with `ioctl(2)` is determined during :manpage:`open(2)`, in the same way as
+ * read and write permissions are checked during :manpage:`open(2)` using
+ * %LANDLOCK_ACCESS_FS_READ_FILE and %LANDLOCK_ACCESS_FS_WRITE_FILE.
  *
  * A directory can receive access rights related to files or directories.  The
  * following access right is applied to the directory itself, and the
@@ -152,7 +156,7 @@ struct landlock_path_beneath_attr {
  *   accessible through these syscall families: :manpage:`chdir(2)`,
  *   :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`chmod(2)`,
  *   :manpage:`chown(2)`, :manpage:`setxattr(2)`, :manpage:`utime(2)`,
- *   :manpage:`ioctl(2)`, :manpage:`fcntl(2)`, :manpage:`access(2)`.
+ *   :manpage:`fcntl(2)`, :manpage:`access(2)`.
  *   Future Landlock evolutions will enable to restrict them.
  */
 /* clang-format off */
@@ -171,6 +175,7 @@ struct landlock_path_beneath_attr {
 #define LANDLOCK_ACCESS_FS_MAKE_SYM			(1ULL << 12)
 #define LANDLOCK_ACCESS_FS_REFER			(1ULL << 13)
 #define LANDLOCK_ACCESS_FS_TRUNCATE			(1ULL << 14)
+#define LANDLOCK_ACCESS_FS_IOCTL			(1ULL << 15)
 /* clang-format on */
 
 #endif /* _UAPI_LINUX_LANDLOCK_H */
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index adcea0fe7e6..b13c765733c 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -147,7 +147,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
 	LANDLOCK_ACCESS_FS_EXECUTE | \
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
 	LANDLOCK_ACCESS_FS_READ_FILE | \
-	LANDLOCK_ACCESS_FS_TRUNCATE)
+	LANDLOCK_ACCESS_FS_TRUNCATE | \
+	LANDLOCK_ACCESS_FS_IOCTL)
 /* clang-format on */
 
 /*
@@ -1207,7 +1208,8 @@ static int hook_file_open(struct file *const file)
 {
 	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
 	access_mask_t open_access_request, full_access_request, allowed_access;
-	const access_mask_t optional_access = LANDLOCK_ACCESS_FS_TRUNCATE;
+	const access_mask_t optional_access = LANDLOCK_ACCESS_FS_TRUNCATE |
+					      LANDLOCK_ACCESS_FS_IOCTL;
 	const struct landlock_ruleset *const dom =
 		landlock_get_current_domain();
 
@@ -1280,6 +1282,19 @@ static int hook_file_truncate(struct file *const file)
 	return -EACCES;
 }
 
+static int hook_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	/*
+	 * It is the access rights at the time of opening the file which
+	 * determine whether ioctl can be used on the opened file later.
+	 *
+	 * The access right is attached to the opened file in hook_file_open().
+	 */
+	if (landlock_file(file)->allowed_access & LANDLOCK_ACCESS_FS_IOCTL)
+		return 0;
+	return -EACCES;
+}
+
 static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(inode_free_security, hook_inode_free_security),
 
@@ -1302,6 +1317,7 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(file_alloc_security, hook_file_alloc_security),
 	LSM_HOOK_INIT(file_open, hook_file_open),
 	LSM_HOOK_INIT(file_truncate, hook_file_truncate),
+	LSM_HOOK_INIT(file_ioctl, hook_file_ioctl),
 };
 
 __init void landlock_add_fs_hooks(void)
diff --git a/security/landlock/limits.h b/security/landlock/limits.h
index 82288f0e9e5..40d8f17698b 100644
--- a/security/landlock/limits.h
+++ b/security/landlock/limits.h
@@ -18,7 +18,7 @@
 #define LANDLOCK_MAX_NUM_LAYERS		16
 #define LANDLOCK_MAX_NUM_RULES		U32_MAX
 
-#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_TRUNCATE
+#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_IOCTL
 #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
 #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
 
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index b6c4be3faf7..fdd7d439ce4 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -446,9 +446,10 @@ TEST_F_FORK(layout1, inval)
 	LANDLOCK_ACCESS_FS_EXECUTE | \
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
 	LANDLOCK_ACCESS_FS_READ_FILE | \
-	LANDLOCK_ACCESS_FS_TRUNCATE)
+	LANDLOCK_ACCESS_FS_TRUNCATE | \
+	LANDLOCK_ACCESS_FS_IOCTL)
 
-#define ACCESS_LAST LANDLOCK_ACCESS_FS_TRUNCATE
+#define ACCESS_LAST LANDLOCK_ACCESS_FS_IOCTL
 
 #define ACCESS_ALL ( \
 	ACCESS_FILE | \
-- 
2.40.1

Re: [RFC 2/4] landlock: Add LANDLOCK_ACCESS_FS_IOCTL access right

[Mickaël Salaün]

On 02/05/2023 19:17, Günther Noack wrote:
> Like the truncate right, this right is associated with a file
> descriptor at the time of open(2), and gets respected even when the
> file descriptor is used outside of the thread which it was originally
> created in.
> 
> In particular, this happens for the commonly inherited file
> descriptors stdin, stdout and stderr, if these are bound to a tty.

s/tty/TTY/

> This means that programs using tty ioctls can drop the ioctl access

s/ioctl/IOCTLs/

> right, but continue using these ioctls on the already opened input and
> output file descriptors.

I'd like a new documentation paragraph explaining the limitation of 
LANDLOCK_ACCESS_FS_IOCTL (not fine-grained; should be careful about 
fscrypt-like features for regular files; compatibility with TTY and 
other common IOCTLs), a way to get more guarantees (e.g. using nodev 
mount points when possible), and a sentence explaining that future work 
will enable a more fine-grained access control.


> 
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> ---
>   include/uapi/linux/landlock.h              | 19 ++++++++++++-------
>   security/landlock/fs.c                     | 20 ++++++++++++++++++--
>   security/landlock/limits.h                 |  2 +-
>   tools/testing/selftests/landlock/fs_test.c |  5 +++--
>   4 files changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index f3223f96469..d87457a1c22 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -102,12 +102,16 @@ struct landlock_path_beneath_attr {
>    * - %LANDLOCK_ACCESS_FS_READ_FILE: Open a file with read access.
>    * - %LANDLOCK_ACCESS_FS_TRUNCATE: Truncate a file with :manpage:`truncate(2)`,
>    *   :manpage:`ftruncate(2)`, :manpage:`creat(2)`, or :manpage:`open(2)` with
> - *   ``O_TRUNC``. Whether an opened file can be truncated with
> - *   :manpage:`ftruncate(2)` is determined during :manpage:`open(2)`, in the
> - *   same way as read and write permissions are checked during
> - *   :manpage:`open(2)` using %LANDLOCK_ACCESS_FS_READ_FILE and
> - *   %LANDLOCK_ACCESS_FS_WRITE_FILE. This access right is available since the
> - *   third version of the Landlock ABI.
> + *   ``O_TRUNC``.  This access right is available since the third version of the
> + *   Landlock ABI.
> + * - %LANDLOCK_ACCESS_FS_IOCTL: Invoke :manpage:`ioctl(2)` on the opened file.
> + *   This access right is available since the fourth version of the Landlock
> + *   ABI.
> + *
> + * Whether an opened file can be truncated with :manpage:`ftruncate(2)` or used
> + * with `ioctl(2)` is determined during :manpage:`open(2)`, in the same way as
> + * read and write permissions are checked during :manpage:`open(2)` using
> + * %LANDLOCK_ACCESS_FS_READ_FILE and %LANDLOCK_ACCESS_FS_WRITE_FILE.
>    *
>    * A directory can receive access rights related to files or directories.  The
>    * following access right is applied to the directory itself, and the
> @@ -152,7 +156,7 @@ struct landlock_path_beneath_attr {
>    *   accessible through these syscall families: :manpage:`chdir(2)`,
>    *   :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`chmod(2)`,
>    *   :manpage:`chown(2)`, :manpage:`setxattr(2)`, :manpage:`utime(2)`,
> - *   :manpage:`ioctl(2)`, :manpage:`fcntl(2)`, :manpage:`access(2)`.
> + *   :manpage:`fcntl(2)`, :manpage:`access(2)`.
>    *   Future Landlock evolutions will enable to restrict them.
>    */
>   /* clang-format off */
> @@ -171,6 +175,7 @@ struct landlock_path_beneath_attr {
>   #define LANDLOCK_ACCESS_FS_MAKE_SYM			(1ULL << 12)
>   #define LANDLOCK_ACCESS_FS_REFER			(1ULL << 13)
>   #define LANDLOCK_ACCESS_FS_TRUNCATE			(1ULL << 14)
> +#define LANDLOCK_ACCESS_FS_IOCTL			(1ULL << 15)
>   /* clang-format on */
>   
>   #endif /* _UAPI_LINUX_LANDLOCK_H */
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index adcea0fe7e6..b13c765733c 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -147,7 +147,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
>   	LANDLOCK_ACCESS_FS_EXECUTE | \
>   	LANDLOCK_ACCESS_FS_WRITE_FILE | \
>   	LANDLOCK_ACCESS_FS_READ_FILE | \
> -	LANDLOCK_ACCESS_FS_TRUNCATE)
> +	LANDLOCK_ACCESS_FS_TRUNCATE | \
> +	LANDLOCK_ACCESS_FS_IOCTL)
>   /* clang-format on */
>   
>   /*
> @@ -1207,7 +1208,8 @@ static int hook_file_open(struct file *const file)
>   {
>   	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
>   	access_mask_t open_access_request, full_access_request, allowed_access;
> -	const access_mask_t optional_access = LANDLOCK_ACCESS_FS_TRUNCATE;
> +	const access_mask_t optional_access = LANDLOCK_ACCESS_FS_TRUNCATE |
> +					      LANDLOCK_ACCESS_FS_IOCTL;
>   	const struct landlock_ruleset *const dom =
>   		landlock_get_current_domain();
>   
> @@ -1280,6 +1282,19 @@ static int hook_file_truncate(struct file *const file)
>   	return -EACCES;
>   }
>   
> +static int hook_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +	/*
> +	 * It is the access rights at the time of opening the file which
> +	 * determine whether ioctl can be used on the opened file later.
> +	 *
> +	 * The access right is attached to the opened file in hook_file_open().
> +	 */
> +	if (landlock_file(file)->allowed_access & LANDLOCK_ACCESS_FS_IOCTL)
> +		return 0;

Please add a new line here.

> +	return -EACCES;
> +}
> +
>   static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
>   	LSM_HOOK_INIT(inode_free_security, hook_inode_free_security),
>   
> @@ -1302,6 +1317,7 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
>   	LSM_HOOK_INIT(file_alloc_security, hook_file_alloc_security),
>   	LSM_HOOK_INIT(file_open, hook_file_open),
>   	LSM_HOOK_INIT(file_truncate, hook_file_truncate),
> +	LSM_HOOK_INIT(file_ioctl, hook_file_ioctl),
>   };
>   
>   __init void landlock_add_fs_hooks(void)
> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> index 82288f0e9e5..40d8f17698b 100644
> --- a/security/landlock/limits.h
> +++ b/security/landlock/limits.h
> @@ -18,7 +18,7 @@
>   #define LANDLOCK_MAX_NUM_LAYERS		16
>   #define LANDLOCK_MAX_NUM_RULES		U32_MAX
>   
> -#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_TRUNCATE
> +#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_IOCTL
>   #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
>   #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
>   
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index b6c4be3faf7..fdd7d439ce4 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -446,9 +446,10 @@ TEST_F_FORK(layout1, inval)
>   	LANDLOCK_ACCESS_FS_EXECUTE | \
>   	LANDLOCK_ACCESS_FS_WRITE_FILE | \
>   	LANDLOCK_ACCESS_FS_READ_FILE | \
> -	LANDLOCK_ACCESS_FS_TRUNCATE)
> +	LANDLOCK_ACCESS_FS_TRUNCATE | \
> +	LANDLOCK_ACCESS_FS_IOCTL)
>   
> -#define ACCESS_LAST LANDLOCK_ACCESS_FS_TRUNCATE
> +#define ACCESS_LAST LANDLOCK_ACCESS_FS_IOCTL
>   
>   #define ACCESS_ALL ( \
>   	ACCESS_FILE | \

Re: [RFC 2/4] landlock: Add LANDLOCK_ACCESS_FS_IOCTL access right

[Günther Noack]

Hi!

On Mon, Jun 19, 2023 at 04:42:07PM +0200, Mickaël Salaün wrote:
> I'd like a new documentation paragraph explaining the limitation of
> LANDLOCK_ACCESS_FS_IOCTL (not fine-grained; should be careful about
> fscrypt-like features for regular files; compatibility with TTY and other
> common IOCTLs), a way to get more guarantees (e.g. using nodev mount points
> when possible), and a sentence explaining that future work will enable a
> more fine-grained access control.

I tried to add this comment but realized that I don't understand it well enough -

Regarding fscrypt:

  If a process is not the fscrypt user space tool itself, in which ways do the
  fscrypt ioctls matter for that process?

  I dug up a list of ioctls in tools/include/uapi/linux/fscrypt.h which look
  related, but these look like they are only needed for the set up of encrypted
  files and directories, but not for using these files later from other
  processes?

  Am I misunderstanding that?

  The one thing that seems to stand out with the fscrypt ioctls is that the same
  ioctl numbers are implemented by multiple different file systems.

Regarding nodev mount points:

  I guess this is not relevant any more if we split the IOCTL right into a
  device-only and non-device-only flag?

  (I prefer that solution over nodev mounts as well, because that solution works
  unprivileged from the perspective of the process that defines the Landlock
  policy. Re-mounting with different options requires more rights and can often
  not be influenced by small utilities.)

Thanks,
—Günther

-- 
Sent using Mutt 🐕 Woof Woof

Re: [RFC 2/4] landlock: Add LANDLOCK_ACCESS_FS_IOCTL access right

[Mickaël Salaün]

On Fri, Jul 14, 2023 at 02:46:09PM +0200, Günther Noack wrote:
> Hi!
> 
> On Mon, Jun 19, 2023 at 04:42:07PM +0200, Mickaël Salaün wrote:
> > I'd like a new documentation paragraph explaining the limitation of
> > LANDLOCK_ACCESS_FS_IOCTL (not fine-grained; should be careful about
> > fscrypt-like features for regular files; compatibility with TTY and other
> > common IOCTLs), a way to get more guarantees (e.g. using nodev mount points
> > when possible), and a sentence explaining that future work will enable a
> > more fine-grained access control.
> 
> I tried to add this comment but realized that I don't understand it well enough -
> 
> Regarding fscrypt:
> 
>   If a process is not the fscrypt user space tool itself, in which ways do the
>   fscrypt ioctls matter for that process?

These IOCTLs may be only used by the fscrypt tool on most distros but
there are of course no guarantee any application could use it to encrypt
its own files according to various use cases.

> 
>   I dug up a list of ioctls in tools/include/uapi/linux/fscrypt.h which look
>   related, but these look like they are only needed for the set up of encrypted
>   files and directories, but not for using these files later from other
>   processes?

Correct

> 
>   Am I misunderstanding that?
> 
>   The one thing that seems to stand out with the fscrypt ioctls is that the same
>   ioctl numbers are implemented by multiple different file systems.

My point was to highlight that nodev IOCTLs might be used by user space
on regular files and directories, not only /dev files, and we should be
careful to not inadvertently deny legitimate IOCTLs.  I think
nodev-IOCTLs are legitimate for unprivileged users, taking into account
capability checks and file mode checks, which might not be the case for
IOCTLs implemented by drivers.

You can take a look ad do_vfs_ioctl() to get a hint about other
FS-related IOCTLs (i.e. FIGETBSZ, FIONREAD), and all other nodev-files
(e.g. pipe, sockets) IOCTLs that may be required to properly use them.

We should be careful to find the good balance between restricting as
much as possible but without breaking user space and bother users that
would then disable security features. ;)

> 
> Regarding nodev mount points:
> 
>   I guess this is not relevant any more if we split the IOCTL right into a
>   device-only and non-device-only flag?
> 
>   (I prefer that solution over nodev mounts as well, because that solution works
>   unprivileged from the perspective of the process that defines the Landlock
>   policy. Re-mounting with different options requires more rights and can often
>   not be influenced by small utilities.)

We're now going withouth the split, but this nodev property could in fact
be part of step 2 (or 3).  Indeed, being able to differenciate file type
would enable to enforce a nodev-like control over a file hierarchy.

> 
> Thanks,
> —Günther
> 
> -- 
> Sent using Mutt 🐕 Woof Woof
> 

[RFC 3/4] selftests/landlock: Test ioctl support

[Günther Noack]

Exercise the use of Landlock's ioctl restriction: If ioctl is
restricted, the use of ioctl fails with a freshly opened /dev/tty
file.

Signed-off-by: Günther Noack <gnoack3000@gmail.com>
---
 tools/testing/selftests/landlock/fs_test.c | 62 ++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index fdd7d439ce4..1f827604374 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -3655,6 +3655,68 @@ TEST(memfd_ftruncate)
 	ASSERT_EQ(0, close(fd));
 }
 
+/*
+ * Invokes ioctl(2) and returns its errno or 0.
+ * The provided fd needs to be a tty for this to work.
+ */
+static int test_tty_ioctl(int fd)
+{
+	struct winsize ws;
+
+	if (ioctl(fd, TIOCGWINSZ, &ws) < 0)
+		return errno;
+	return 0;
+}
+
+/*
+ * Attempt ioctl on /dev/tty0 and /dev/tty1,
+ * with file descriptors opened before and after landlocking.
+ */
+TEST_F_FORK(layout1, ioctl)
+{
+	const struct rule rules[] = {
+		{
+			.path = "/dev/tty1",
+			.access = LANDLOCK_ACCESS_FS_IOCTL,
+		},
+		/* Implicitly: No ioctl access on /dev/tty0. */
+		{},
+	};
+	const __u64 handled = LANDLOCK_ACCESS_FS_IOCTL;
+	int ruleset_fd;
+	int old_tty0_fd, tty0_fd, tty1_fd;
+
+	old_tty0_fd = open("/dev/tty0", O_RDWR);
+	ASSERT_LE(0, old_tty0_fd);
+
+	/* Checks that ioctl works before landlocking. */
+	EXPECT_EQ(0, test_tty_ioctl(old_tty0_fd));
+
+	/* Enable Landlock. */
+	ruleset_fd = create_ruleset(_metadata, handled, rules);
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	/* Checks that ioctl with existing FD works after landlocking. */
+	EXPECT_EQ(0, test_tty_ioctl(old_tty0_fd));
+
+	/* Checks that same ioctl fails when file is opened after landlocking. */
+	tty0_fd = open("/dev/tty0", O_RDWR);
+	ASSERT_LE(0, tty0_fd);
+	EXPECT_EQ(EACCES, test_tty_ioctl(tty0_fd));
+
+	/* Checks that same ioctl fails when file is opened after landlocking. */
+	tty1_fd = open("/dev/tty1", O_RDWR);
+	ASSERT_LE(0, tty1_fd);
+	EXPECT_EQ(0, test_tty_ioctl(tty1_fd));
+
+	/* Close all TTY file descriptors. */
+	ASSERT_EQ(0, close(old_tty0_fd));
+	ASSERT_EQ(0, close(tty0_fd));
+	ASSERT_EQ(0, close(tty1_fd));
+}
+
 /* clang-format off */
 FIXTURE(layout1_bind) {};
 /* clang-format on */
-- 
2.40.1

Re: [RFC 3/4] selftests/landlock: Test ioctl support

[Mickaël Salaün]

On 02/05/2023 19:17, Günther Noack wrote:
> Exercise the use of Landlock's ioctl restriction: If ioctl is
> restricted, the use of ioctl fails with a freshly opened /dev/tty
> file.
> 
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> ---
>   tools/testing/selftests/landlock/fs_test.c | 62 ++++++++++++++++++++++
>   1 file changed, 62 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index fdd7d439ce4..1f827604374 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -3655,6 +3655,68 @@ TEST(memfd_ftruncate)
>   	ASSERT_EQ(0, close(fd));
>   }
>   
> +/*
> + * Invokes ioctl(2) and returns its errno or 0.
> + * The provided fd needs to be a tty for this to work.
> + */
> +static int test_tty_ioctl(int fd)
> +{
> +	struct winsize ws;
> +
> +	if (ioctl(fd, TIOCGWINSZ, &ws) < 0)
> +		return errno;
> +	return 0;
> +}
> +
> +/*
> + * Attempt ioctl on /dev/tty0 and /dev/tty1,
> + * with file descriptors opened before and after landlocking.
> + */
> +TEST_F_FORK(layout1, ioctl)
> +{
> +	const struct rule rules[] = {
> +		{
> +			.path = "/dev/tty1",
> +			.access = LANDLOCK_ACCESS_FS_IOCTL,
> +		},
> +		/* Implicitly: No ioctl access on /dev/tty0. */

We should create a new PTS mount point, create a new session, and use 
that for tests to limit the dependency on the test environment and not 
mess with it.


> +		{},
> +	};
> +	const __u64 handled = LANDLOCK_ACCESS_FS_IOCTL;
> +	int ruleset_fd;
> +	int old_tty0_fd, tty0_fd, tty1_fd;
> +
> +	old_tty0_fd = open("/dev/tty0", O_RDWR);
> +	ASSERT_LE(0, old_tty0_fd);
> +
> +	/* Checks that ioctl works before landlocking. */
> +	EXPECT_EQ(0, test_tty_ioctl(old_tty0_fd));
> +
> +	/* Enable Landlock. */

Enable*s*

> +	ruleset_fd = create_ruleset(_metadata, handled, rules);
> +	ASSERT_LE(0, ruleset_fd);
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	/* Checks that ioctl with existing FD works after landlocking. */
> +	EXPECT_EQ(0, test_tty_ioctl(old_tty0_fd));
> +
> +	/* Checks that same ioctl fails when file is opened after landlocking. */
> +	tty0_fd = open("/dev/tty0", O_RDWR);
> +	ASSERT_LE(0, tty0_fd);
> +	EXPECT_EQ(EACCES, test_tty_ioctl(tty0_fd));
> +
> +	/* Checks that same ioctl fails when file is opened after landlocking. */
> +	tty1_fd = open("/dev/tty1", O_RDWR);
> +	ASSERT_LE(0, tty1_fd);
> +	EXPECT_EQ(0, test_tty_ioctl(tty1_fd));

/dev, or rather the test PTS mount point, and its parent, should also be 
tested. We can use three layers in the same test for that.


> +
> +	/* Close all TTY file descriptors. */
> +	ASSERT_EQ(0, close(old_tty0_fd));
> +	ASSERT_EQ(0, close(tty0_fd));
> +	ASSERT_EQ(0, close(tty1_fd));
> +}
> +
>   /* clang-format off */
>   FIXTURE(layout1_bind) {};
>   /* clang-format on */

Re: [RFC 3/4] selftests/landlock: Test ioctl support

[Günther Noack]

Hello!

Thanks for the review!

On Mon, Jun 19, 2023 at 04:42:17PM +0200, Mickaël Salaün wrote:
> On 02/05/2023 19:17, Günther Noack wrote:
> > Exercise the use of Landlock's ioctl restriction: If ioctl is
> > restricted, the use of ioctl fails with a freshly opened /dev/tty
> > file.
> > 
> > Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> > ---
> >   tools/testing/selftests/landlock/fs_test.c | 62 ++++++++++++++++++++++
> >   1 file changed, 62 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> > index fdd7d439ce4..1f827604374 100644
> > --- a/tools/testing/selftests/landlock/fs_test.c
> > +++ b/tools/testing/selftests/landlock/fs_test.c
> > @@ -3655,6 +3655,68 @@ TEST(memfd_ftruncate)
> >   	ASSERT_EQ(0, close(fd));
> >   }
> > +/*
> > + * Invokes ioctl(2) and returns its errno or 0.
> > + * The provided fd needs to be a tty for this to work.
> > + */
> > +static int test_tty_ioctl(int fd)
> > +{
> > +	struct winsize ws;
> > +
> > +	if (ioctl(fd, TIOCGWINSZ, &ws) < 0)
> > +		return errno;
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Attempt ioctl on /dev/tty0 and /dev/tty1,
> > + * with file descriptors opened before and after landlocking.
> > + */
> > +TEST_F_FORK(layout1, ioctl)
> > +{
> > +	const struct rule rules[] = {
> > +		{
> > +			.path = "/dev/tty1",
> > +			.access = LANDLOCK_ACCESS_FS_IOCTL,
> > +		},
> > +		/* Implicitly: No ioctl access on /dev/tty0. */
> 
> We should create a new PTS mount point, create a new session, and use that
> for tests to limit the dependency on the test environment and not mess with
> it.

I have pondered this, and I feel that this is unnecessarily complicating the
test.  The mechanism that I intend to test here is just the general filtering of
IOCTL commands, but not TTYs specifically.  TTYs are a common use case for
IOCTLs, but they are not the only one.

If you are not strongly opposed to it, I would rather look for a different IOCTL
command that works on a different file, where we don't need any special set up?
That would simplify the test and exercise the same functionality in the end.
Does that sounds reasonable?


> > +		{},
> > +	};
> > +	const __u64 handled = LANDLOCK_ACCESS_FS_IOCTL;
> > +	int ruleset_fd;
> > +	int old_tty0_fd, tty0_fd, tty1_fd;
> > +
> > +	old_tty0_fd = open("/dev/tty0", O_RDWR);
> > +	ASSERT_LE(0, old_tty0_fd);
> > +
> > +	/* Checks that ioctl works before landlocking. */
> > +	EXPECT_EQ(0, test_tty_ioctl(old_tty0_fd));
> > +
> > +	/* Enable Landlock. */
> 
> Enable*s*

Done.


> > +	ruleset_fd = create_ruleset(_metadata, handled, rules);
> > +	ASSERT_LE(0, ruleset_fd);
> > +	enforce_ruleset(_metadata, ruleset_fd);
> > +	ASSERT_EQ(0, close(ruleset_fd));
> > +
> > +	/* Checks that ioctl with existing FD works after landlocking. */
> > +	EXPECT_EQ(0, test_tty_ioctl(old_tty0_fd));
> > +
> > +	/* Checks that same ioctl fails when file is opened after landlocking. */
> > +	tty0_fd = open("/dev/tty0", O_RDWR);
> > +	ASSERT_LE(0, tty0_fd);
> > +	EXPECT_EQ(EACCES, test_tty_ioctl(tty0_fd));
> > +
> > +	/* Checks that same ioctl fails when file is opened after landlocking. */
> > +	tty1_fd = open("/dev/tty1", O_RDWR);
> > +	ASSERT_LE(0, tty1_fd);
> > +	EXPECT_EQ(0, test_tty_ioctl(tty1_fd));
> 
> /dev, or rather the test PTS mount point, and its parent, should also be
> tested. We can use three layers in the same test for that.

We've already tested the inheritance of access rights across different
directories and mount points in other tests.  I feel that exercising it in all
combinations of access rights and inheritance mechanisms makes the tests harder
to understand and maintain, and does not give us much additional confidence on
top of what we already have.  What balance do you want to find there?

Thanks,
—Günther

-- 
Sent using Mutt 🐕 Woof Woof

Re: [RFC 3/4] selftests/landlock: Test ioctl support

[Mickaël Salaün]

On Mon, Aug 07, 2023 at 09:39:50AM +0200, Günther Noack wrote:
> Hello!
> 
> Thanks for the review!
> 
> On Mon, Jun 19, 2023 at 04:42:17PM +0200, Mickaël Salaün wrote:
> > On 02/05/2023 19:17, Günther Noack wrote:
> > > Exercise the use of Landlock's ioctl restriction: If ioctl is
> > > restricted, the use of ioctl fails with a freshly opened /dev/tty
> > > file.
> > > 
> > > Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> > > ---
> > >   tools/testing/selftests/landlock/fs_test.c | 62 ++++++++++++++++++++++
> > >   1 file changed, 62 insertions(+)
> > > 
> > > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> > > index fdd7d439ce4..1f827604374 100644
> > > --- a/tools/testing/selftests/landlock/fs_test.c
> > > +++ b/tools/testing/selftests/landlock/fs_test.c
> > > @@ -3655,6 +3655,68 @@ TEST(memfd_ftruncate)
> > >   	ASSERT_EQ(0, close(fd));
> > >   }
> > > +/*
> > > + * Invokes ioctl(2) and returns its errno or 0.
> > > + * The provided fd needs to be a tty for this to work.
> > > + */
> > > +static int test_tty_ioctl(int fd)
> > > +{
> > > +	struct winsize ws;
> > > +
> > > +	if (ioctl(fd, TIOCGWINSZ, &ws) < 0)
> > > +		return errno;
> > > +	return 0;
> > > +}
> > > +
> > > +/*
> > > + * Attempt ioctl on /dev/tty0 and /dev/tty1,
> > > + * with file descriptors opened before and after landlocking.
> > > + */
> > > +TEST_F_FORK(layout1, ioctl)
> > > +{
> > > +	const struct rule rules[] = {
> > > +		{
> > > +			.path = "/dev/tty1",
> > > +			.access = LANDLOCK_ACCESS_FS_IOCTL,
> > > +		},
> > > +		/* Implicitly: No ioctl access on /dev/tty0. */
> > 
> > We should create a new PTS mount point, create a new session, and use that
> > for tests to limit the dependency on the test environment and not mess with
> > it.
> 
> I have pondered this, and I feel that this is unnecessarily complicating the
> test.  The mechanism that I intend to test here is just the general filtering of
> IOCTL commands, but not TTYs specifically.  TTYs are a common use case for
> IOCTLs, but they are not the only one.
> 
> If you are not strongly opposed to it, I would rather look for a different IOCTL
> command that works on a different file, where we don't need any special set up?
> That would simplify the test and exercise the same functionality in the end.
> Does that sounds reasonable?

Yes, simpler is better.

> 
> 
> > > +		{},
> > > +	};
> > > +	const __u64 handled = LANDLOCK_ACCESS_FS_IOCTL;
> > > +	int ruleset_fd;
> > > +	int old_tty0_fd, tty0_fd, tty1_fd;
> > > +
> > > +	old_tty0_fd = open("/dev/tty0", O_RDWR);
> > > +	ASSERT_LE(0, old_tty0_fd);
> > > +
> > > +	/* Checks that ioctl works before landlocking. */
> > > +	EXPECT_EQ(0, test_tty_ioctl(old_tty0_fd));
> > > +
> > > +	/* Enable Landlock. */
> > 
> > Enable*s*
> 
> Done.
> 
> 
> > > +	ruleset_fd = create_ruleset(_metadata, handled, rules);
> > > +	ASSERT_LE(0, ruleset_fd);
> > > +	enforce_ruleset(_metadata, ruleset_fd);
> > > +	ASSERT_EQ(0, close(ruleset_fd));
> > > +
> > > +	/* Checks that ioctl with existing FD works after landlocking. */
> > > +	EXPECT_EQ(0, test_tty_ioctl(old_tty0_fd));
> > > +
> > > +	/* Checks that same ioctl fails when file is opened after landlocking. */
> > > +	tty0_fd = open("/dev/tty0", O_RDWR);
> > > +	ASSERT_LE(0, tty0_fd);
> > > +	EXPECT_EQ(EACCES, test_tty_ioctl(tty0_fd));
> > > +
> > > +	/* Checks that same ioctl fails when file is opened after landlocking. */
> > > +	tty1_fd = open("/dev/tty1", O_RDWR);
> > > +	ASSERT_LE(0, tty1_fd);
> > > +	EXPECT_EQ(0, test_tty_ioctl(tty1_fd));
> > 
> > /dev, or rather the test PTS mount point, and its parent, should also be
> > tested. We can use three layers in the same test for that.
> 
> We've already tested the inheritance of access rights across different
> directories and mount points in other tests.  I feel that exercising it in all
> combinations of access rights and inheritance mechanisms makes the tests harder
> to understand and maintain, and does not give us much additional confidence on
> top of what we already have.  What balance do you want to find there?

Indeed. It should be notted that this new IOCTL access right will be the
first one to directly apply to both files and directories.  It should
then have the same scope as LANDLOCK_ACCESS_FS_READ i.e., apply to the
target directory itself and files/directories beneath it.

We then need to test a directory's IOCTL, for instance using FIOQSIZE.

What about this two rules and related access checks, combined with
already-opened FD?
- dir_s1d1: always denied (negative test)
- file1_s1d1: allowed with a rule (checks ACCESS_FILE)
- dir_s2d1: allowed with a rule (checks directory right)

Re: [RFC 3/4] selftests/landlock: Test ioctl support

[Günther Noack]

Hi!

On Mon, Aug 07, 2023 at 11:41:48AM +0200, Mickaël Salaün wrote:
> On Mon, Aug 07, 2023 at 09:39:50AM +0200, Günther Noack wrote:
> > We've already tested the inheritance of access rights across different
> > directories and mount points in other tests.  I feel that exercising it in all
> > combinations of access rights and inheritance mechanisms makes the tests harder
> > to understand and maintain, and does not give us much additional confidence on
> > top of what we already have.  What balance do you want to find there?
> 
> Indeed. It should be notted that this new IOCTL access right will be the
> first one to directly apply to both files and directories.  It should
> then have the same scope as LANDLOCK_ACCESS_FS_READ i.e., apply to the
> target directory itself and files/directories beneath it.
> 
> We then need to test a directory's IOCTL, for instance using FIOQSIZE.
> 
> What about this two rules and related access checks, combined with
> already-opened FD?
> - dir_s1d1: always denied (negative test)
> - file1_s1d1: allowed with a rule (checks ACCESS_FILE)
> - dir_s2d1: allowed with a rule (checks directory right)

Ah, that's an excellent point - I had not realized yet that it is different to
the other access rights in that way, and it makes a lot of sense to test that. 👍

I'll dig up one IOCTL command for regular files and one IOCTL command for
directories like FIOQSIZE, which are both not blanket-permitted, and I'll test
it with that, and will make sure to cover the combinations you listed above.

Thanks!
—Günther

-- 
Sent using Mutt 🐕 Woof Woof

[RFC 4/4] samples/landlock: Add support for LANDLOCK_ACCESS_FS_IOCTL

[Günther Noack]

Add ioctl support to the Landlock sample tool.

The ioctl right is grouped with the read-write rights in the sample
tool, as some ioctl requests provide features that mutate state.

Signed-off-by: Günther Noack <gnoack3000@gmail.com>
---
 samples/landlock/sandboxer.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index e2056c8b902..c70d96d15c7 100644
--- a/samples/landlock/sandboxer.c
+++ b/samples/landlock/sandboxer.c
@@ -77,7 +77,8 @@ static int parse_path(char *env_path, const char ***const path_list)
 	LANDLOCK_ACCESS_FS_EXECUTE | \
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
 	LANDLOCK_ACCESS_FS_READ_FILE | \
-	LANDLOCK_ACCESS_FS_TRUNCATE)
+	LANDLOCK_ACCESS_FS_TRUNCATE | \
+	LANDLOCK_ACCESS_FS_IOCTL)
 
 /* clang-format on */
 
@@ -162,11 +163,12 @@ static int populate_ruleset(const char *const env_var, const int ruleset_fd,
 	LANDLOCK_ACCESS_FS_MAKE_BLOCK | \
 	LANDLOCK_ACCESS_FS_MAKE_SYM | \
 	LANDLOCK_ACCESS_FS_REFER | \
-	LANDLOCK_ACCESS_FS_TRUNCATE)
+	LANDLOCK_ACCESS_FS_TRUNCATE | \
+	LANDLOCK_ACCESS_FS_IOCTL)
 
 /* clang-format on */
 
-#define LANDLOCK_ABI_LAST 3
+#define LANDLOCK_ABI_LAST 4
 
 int main(const int argc, char *const argv[], char *const *const envp)
 {
@@ -255,6 +257,10 @@ int main(const int argc, char *const argv[], char *const *const envp)
 	case 2:
 		/* Removes LANDLOCK_ACCESS_FS_TRUNCATE for ABI < 3 */
 		ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
+		__attribute__((fallthrough));
+	case 3:
+		/* Removes LANDLOCK_ACCESS_FS_IOCTL for ABI < 4 */
+		ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL;
 
 		fprintf(stderr,
 			"Hint: You should update the running kernel "
-- 
2.40.1

Re: [RFC 0/4] Landlock: ioctl support

[Mickaël Salaün]

Thanks for this RFC, this is interesting!

I previously thought a lot about IOCTLs restrictions and here are some 
notes:

IOCTLs are everywhere, from devices to filesystems (see fscrypt). Each 
different file type may behave differently for the same IOCTL 
command/ID. It is also worth noting that there are a lot of different 
IOCTLs, they are growing over time, some might be dedicated to get some 
data (i.e. read) and others to change the state of a device (i.e. 
write), some might be innocuous (e.g. FIOCLEX, FIONCLEX) and others 
potentially harmful. _IOC_READ and _IOC_WRITE can be useful but they are 
not mandatory, there are exceptions, and it may be difficult to identify 
if a command pertains to one, the other, or both kind of actions.

I then think it would be very useful to be able to tie file/device types 
to a set of IOCTLs, letting user space libraries define and classify the 
IOCTL semantic.

Instead of relying on a LANDLOCK_ACCESS_FS_IOCTL which would allow or 
deny all IOCTLs, we can extend the path_beneath struct to manage IOCTLs 
in addition to regular file accesses. Because dealing with a set of 
IOCTLs would imply to deal with a lot of data and combinations, I 
thought about creating groups of IOCTLs (defining access semantic) that 
could be matched against file hierarchies. The composability nature of 
Landlock domains is also another constraint to keep in mind.

// New rule type dedicated to define groups of IOCTLs.
struct landlock_ioctl_attr {
   __u32 command; // IOCTL number/ID
   dev_t device; // must be 0 for regular file and directory
   __u8 file_type;
   __u8 id_mask; // if 0, then applied globally
};

We could use landlock_add_rule(2) to fill a set of landlock_ioctl_attr 
into a ruleset and use them with landlock_path_beneath_attr entries:

// LANDLOCK_RULE_PATH_BENEATH, leveraging the extensible design of
// landlock_path_beneath_attr, hence the same first fields.
struct landlock_path_beneath_attr {
   __u64 allowed_access;
   __s32 parent_fd;
   __u16 allowed_ioctl_id_mask;
};

landlock_ioctl_attr includes a 8-bit mask for which each bit identifies 
a set of allowed IOCTLs per device/file type. This mask is then tied to 
a path_beneath_attr. We cannot use number IDs because of dev_t+IOCTL->ID 
intersection conflicts. Using an id_mask enables to group (specific) 
IOCTLs together, then creating synthetic access rights.

When merging a ruleset with a domain, each IOCTL ID mask is shifted and 
ORed with the other layer ones to get a (8*16) 128-bit mask, stored in 
an IOCTL/dev_t table and in the related landlock_object. When looking 
for an IOCTL request, Landlock first looks into the IOCTL set ID table 
and get the global set ID mask, which kind of translates to a 
composition of synthetic access rights (stored with the 
landlock_layer.ioctl_access bitmask). We then walk through all the 
inodes to match the whole mask.

I realize that this is complex and this explanation might be confusing 
though. What do you think?



On 02/05/2023 19:17, Günther Noack wrote:
> Hello!
> 
> These patches add ioctl support to Landlock.
> 
> It's an early version - it potentially needs more tests and
> documentation.  I'd like to circulate the patches early to discuss
> whether this approach is feasible.
> 
> Objective
> ~~~~~~~~~
> 
> Make ioctl(2) requests restrictable with Landlock,
> in a way that is useful for real-world applications.
> 
> Proposed approach
> ~~~~~~~~~~~~~~~~~
> 
> Introduce the LANDLOCK_ACCESS_FS_IOCTL right, which restricts the use
> of ioctl(2) on file descriptors.
> 
> We attach the LANDLOCK_ACCESS_FS_IOCTL right to opened file
> descriptors, as we already do for LANDLOCK_ACCESS_FS_TRUNCATE.
> 
> I believe that this approach works for the majority of use cases, and
> offers a good trade-off between Landlock API and implementation
> complexity and flexibility when the feature is used.
> 
> Notable implications of this approach
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> * Existing inherited file descriptors stay unaffected
>    when a program enables Landlock.
> 
>    This means in particular that in common scenarios,
>    the terminal's ioctls (ioctl_tty(2)) continue to work.
> 
> * ioctl(2) continues to be available for file descriptors acquired
>    through means other than open(2).  Example: Network sockets.
> 
> Examples
> ~~~~~~~~
> 
> Starting a sandboxed shell from $HOME with samples/landlock/sandboxer:
> 
>    LL_FS_RO=/ LL_FS_RW=. ./sandboxer /bin/bash
> 
> The LANDLOCK_ACCESS_FS_IOCTL right is part of the "read-write" rights
> here, so we expect that newly opened files outside of $HOME don't work
> with ioctl(2).
> 
>    * "stty" works: It probes terminal properties
> 
>    * "stty </dev/tty" fails: /dev/tty can be reopened, but the ioctl is
>      denied.
> 
>    * "eject" fails: ioctls to use CD-ROM drive are denied.
> 
>    * "ls /dev" works: It uses ioctl to get the terminal size for
>      columnar layout
> 
>    * The text editors "vim" and "mg" work.  (GNU Emacs fails because it
>      attempts to reopen /dev/tty.)
> 
> Alternatives considered: Allow-listing specific ioctl requests
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> It would be technically possible to keep an allow-list of ioctl
> requests and thereby control in more detail which ioctls should work.
> 
> I believe that this is not needed for the majority of use cases and
> that it is a reasonable trade-off to make here (but I'm happy to hear
> about counterexamples).  The reasoning is:
> 
> * Many programs do not need ioctl at all,
>    and denying all ioctl(2) requests OK for these.
> 
> * Other programs need ioctl, but only for the terminal FDs.
>    This is supported because these file descriptors are usually
>    inherited from the parent process - so the parent process gets to
>    control the ioctl(2) policy for them.
> 
> * Some programs need ioctl on specific files that they are opening
>    themselves.  They can allow-list these file paths for ioctl(2).
>    This makes the programs work, but it restricts a variety of other
>    ioctl requests which are otherwise possible through opening other
>    files.
> 
> Because the LANDLOCK_ACCESS_FS_IOCTL right is attached to the file
> descriptor, programs have flexible options to control which ioctl
> operations should work, without the implementation complexity of
> additional ioctl allow-lists in the kernel.
> 
> Finally, the proposed approach is simpler in implementation and has
> lower API complexity, but it does *not* preclude us from implementing
> per-ioctl-request allow lists later, if that turns out to be necessary
> at a later point.

I value this simplicity, but I'm also wondering about how much this 
allow/deny all IOCTLs approach would be useful in real case scenarios. ;)


> 
> Related Work
> ~~~~~~~~~~~~
> 
> OpenBSD's pledge(2) [1] restricts ioctl(2) independent of the file
> descriptor which is used.  The implementers maintain multiple
> allow-lists of predefined ioctl(2) operations required for different
> application domains such as "audio", "bpf", "tty" and "inet".
> 
> OpenBSD does not guarantee ABI backwards compatibility to the same
> extent as Linux does, so it's easier for them to update these lists in
> later versions.  It might not be a feasible approach for Linux though.
> 
> [1] https://man.openbsd.org/OpenBSD-7.3/pledge.2
> 
> Feedback I'm looking for
> ~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Some specific points I would like to get your opinion on:
> 
> * Is this the right general approach to restricting ioctl(2)?
> 
>    It will probably be possible to find counter-examples where the
>    alternative (see below) is better.  I'd be interested in these, and
>    in how common they are, to understand whether we have picked the
>    right trade-off here.

In the long term, I'd like Landlock to be able to restrict a set of 
IOCTLs, taking into account the type of file/device. Being able to deny 
all IOCTLs might be useful and is much easier to implement though.


> 
> * Should we introduce a "landlock_fd_rights_limit()" syscall?
> 
>    We could potentially implement a system call for dropping the
>    LANDLOCK_ACCESS_FS_IOCTL and LANDLOCK_ACCESS_FS_TRUNCATE rights from
>    existing file descriptors (independent of the type of file
>    descriptor, even).
> 
>    Possible use cases would be to (a) restrict the rights on inherited
>    file descriptors like std{in,out,err} and to (b) restrict ioctl and
>    truncate operations on file descriptors that are not acquired
>    through open(2), such as network sockets.
> 
>    This would be similar to the cap_rights_limit(2) system call in
>    FreeBSD's Capsicum.
> 
>    This idea feels somewhat orthogonal to the ioctl patch, but it would
>    start to be more useful if the ioctl right exists.

This is indeed interesting, and that should be useful for the cases you 
explained. I think supporting IOCTLs is more important for now, but a 
new syscall to restrict FDs could be useful in the future. We should 
also think about batch operations on FD (see the close_range syscall), 
for instance to deny all IOCTLs on inherited or received FDs.


> 
> 
> Günther Noack (4):
>    landlock: Increment Landlock ABI version to 4
>    landlock: Add LANDLOCK_ACCESS_FS_IOCTL access right
>    selftests/landlock: Test ioctl support
>    samples/landlock: Add support for LANDLOCK_ACCESS_FS_IOCTL
> 
>   include/uapi/linux/landlock.h                | 19 ++++--
>   samples/landlock/sandboxer.c                 | 12 +++-
>   security/landlock/fs.c                       | 20 +++++-
>   security/landlock/limits.h                   |  2 +-
>   security/landlock/syscalls.c                 |  2 +-
>   tools/testing/selftests/landlock/base_test.c |  2 +-
>   tools/testing/selftests/landlock/fs_test.c   | 67 +++++++++++++++++++-
>   7 files changed, 107 insertions(+), 17 deletions(-)
> 
> 
> base-commit: 457391b0380335d5e9a5babdec90ac53928b23b4

Re: [RFC 0/4] Landlock: ioctl support

[Günther Noack]

Hello Mickaël!

Sorry for the late reply.  This was indeed a bit difficult for me to
understand; maybe it just needs more clarification.

Let me try to paraphrase your proposal (inline below) so we can
resolve the misunderstandings.


On Thu, May 04, 2023 at 11:12:00PM +0200, Mickaël Salaün wrote:
> Thanks for this RFC, this is interesting!
> 
> I previously thought a lot about IOCTLs restrictions and here are some
> notes:
> 
> IOCTLs are everywhere, from devices to filesystems (see fscrypt). Each
> different file type may behave differently for the same IOCTL command/ID. It
> is also worth noting that there are a lot of different IOCTLs, they are
> growing over time, some might be dedicated to get some data (i.e. read) and
> others to change the state of a device (i.e. write), some might be innocuous
> (e.g. FIOCLEX, FIONCLEX) and others potentially harmful. _IOC_READ and
> _IOC_WRITE can be useful but they are not mandatory, there are exceptions,
> and it may be difficult to identify if a command pertains to one, the other,
> or both kind of actions.
> 
> I then think it would be very useful to be able to tie file/device types to
> a set of IOCTLs, letting user space libraries define and classify the IOCTL
> semantic.
> 
> Instead of relying on a LANDLOCK_ACCESS_FS_IOCTL which would allow or deny
> all IOCTLs, we can extend the path_beneath struct to manage IOCTLs in
> addition to regular file accesses. Because dealing with a set of IOCTLs
> would imply to deal with a lot of data and combinations, I thought about
> creating groups of IOCTLs (defining access semantic) that could be matched
> against file hierarchies. The composability nature of Landlock domains is
> also another constraint to keep in mind.
> 
> // New rule type dedicated to define groups of IOCTLs.
> struct landlock_ioctl_attr {
>   __u32 command; // IOCTL number/ID
>   dev_t device; // must be 0 for regular file and directory
>   __u8 file_type;
>   __u8 id_mask; // if 0, then applied globally
> };
>
> We could use landlock_add_rule(2) to fill a set of landlock_ioctl_attr into
> a ruleset and use them with landlock_path_beneath_attr entries:
> 
> // LANDLOCK_RULE_PATH_BENEATH, leveraging the extensible design of
> // landlock_path_beneath_attr, hence the same first fields.
> struct landlock_path_beneath_attr {
>   __u64 allowed_access;
>   __s32 parent_fd;
>   __u16 allowed_ioctl_id_mask;
> };
> 
> landlock_ioctl_attr includes a 8-bit mask for which each bit identifies a
> set of allowed IOCTLs per device/file type. This mask is then tied to a
> path_beneath_attr. We cannot use number IDs because of dev_t+IOCTL->ID
> intersection conflicts. Using an id_mask enables to group (specific) IOCTLs
> together, then creating synthetic access rights.
> 
> When merging a ruleset with a domain, each IOCTL ID mask is shifted and ORed
> with the other layer ones to get a (8*16) 128-bit mask, stored in an
> IOCTL/dev_t table and in the related landlock_object. When looking for an
> IOCTL request, Landlock first looks into the IOCTL set ID table and get the
> global set ID mask, which kind of translates to a composition of synthetic
> access rights (stored with the landlock_layer.ioctl_access bitmask). We then
> walk through all the inodes to match the whole mask.
> 
> I realize that this is complex and this explanation might be confusing
> though. What do you think?

To be honest, I am not fully sure I understand the landlock_ioctl_attr
struct correctly.

My current guess is:

  command:   a single ioctl request number that should be permitted

  device:    if device != 0; require the dev_t (major+minor number)
             of the file to match, before permitting the ioctl

  file_type: if set (!= 0?), require the file type to match,
             before permitting the ioctl

  id_mask:   Indicates the IDs of the groups where this ioctl
             should be permitted(?)

So -- if we were to implement this without any optimizations -- the
logic of this is presumably something like this?:

  If Landlock checks an ioctl with a request_id on a file f:

  We look up the allowed_ioctl_id_mask and loop over the bits set in
  that mask.

  For every bit set in that mask, we look up the matching ioctl group.
  Within the group, we look whether we can find a landlock_ioctl_attr
  rule that belongs to the group which permits that ioctl request.

  The request is granted by a landlock_ioctl_attr if:

    attr.command == request_id
    && (attr.device == 0 || file_inode(file).i_rdev == attr.device)
    && (attr.file_type == 0 || landlock_get_file_type?(file))

Is this roughly what you imagined?


Some specific things I don't understand well are:

* How does id_mask identify the ioctl group?  Do you envision an
  interface where you can add a landlock_ioctl_attr to multiple groups
  at once?

  When the added landlock_ioctl_attr has id_mask=3, does it add that
  attr to group 2 and 1?

* How does allowed_ioctl_id_mask match against the ioctl group IDs
  (id_mask)?  I'm particularly confused because the
  allowed_ioctl_id_mask is __u16, whereas id_mask is __u8?  Is this
  intentional?

* What is file_type?  Are those the file types as used in mknod(2),
  so that you can distinguish between regular files, directories,
  named pipes and the like?

* If I understand the proposal right, the .device and .file_type in
  landlock_ioctl_attr are narrowing down the set of files that the
  ioctl policy applies to.  In all rules up until Landlock v3, which
  we already have, the selection of the files which the rule applies
  to is purely done based on file hierarchy.

  Would it not be a more orthogonal API if the "file selection" part
  of the Landlock API and the "policy adding" part for these selected
  files were independent of each other?  Then the .device and
  .file_type selection scheme could be used for the existing policies
  as well?

* When restricting by dev_t (major and minor number), aren't there use
  cases where a system might have 256 CDROM drives, and you'd need to
  allow-list all of these minor number combinations?

* Aren't many ioctl use cases already usable with just the proposal I
  made?  If you add a rule to permit IOCTL for /dev/cdrom0, that
  opened file will anyway only expose a small subset of the ioctls
  that the kernel as a whole offers, no?  Are there ioctls which are
  offered independent of the file type which I'm overlooking? (Sorry,
  this might be a naive question. :))


> On 02/05/2023 19:17, Günther Noack wrote:
> > Alternatives considered: Allow-listing specific ioctl requests
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > It would be technically possible to keep an allow-list of ioctl
> > requests and thereby control in more detail which ioctls should work.
> > 
> > I believe that this is not needed for the majority of use cases and
> > that it is a reasonable trade-off to make here (but I'm happy to hear
> > about counterexamples).  The reasoning is:
> > 
> > * Many programs do not need ioctl at all,
> >    and denying all ioctl(2) requests OK for these.
> > 
> > * Other programs need ioctl, but only for the terminal FDs.
> >    This is supported because these file descriptors are usually
> >    inherited from the parent process - so the parent process gets to
> >    control the ioctl(2) policy for them.
> > 
> > * Some programs need ioctl on specific files that they are opening
> >    themselves.  They can allow-list these file paths for ioctl(2).
> >    This makes the programs work, but it restricts a variety of other
> >    ioctl requests which are otherwise possible through opening other
> >    files.
> > 
> > Because the LANDLOCK_ACCESS_FS_IOCTL right is attached to the file
> > descriptor, programs have flexible options to control which ioctl
> > operations should work, without the implementation complexity of
> > additional ioctl allow-lists in the kernel.
> > 
> > Finally, the proposed approach is simpler in implementation and has
> > lower API complexity, but it does *not* preclude us from implementing
> > per-ioctl-request allow lists later, if that turns out to be necessary
> > at a later point.
> 
> I value this simplicity, but I'm also wondering about how much this
> allow/deny all IOCTLs approach would be useful in real case scenarios. ;)

Mickaël, would you be open to gather some more data for this from
existing users, to understand better which use cases they have?

(Looking in the direction of Jeff Xu, who has inquired about Landlock
for Chromium in the past -- do you happen to know in which ways you'd
want to restrict ioctls, if you have that need? :))


> > Related Work
> > ~~~~~~~~~~~~
> > 
> > OpenBSD's pledge(2) [1] restricts ioctl(2) independent of the file
> > descriptor which is used.  The implementers maintain multiple
> > allow-lists of predefined ioctl(2) operations required for different
> > application domains such as "audio", "bpf", "tty" and "inet".
> > 
> > OpenBSD does not guarantee ABI backwards compatibility to the same
> > extent as Linux does, so it's easier for them to update these lists in
> > later versions.  It might not be a feasible approach for Linux though.
> > 
> > [1] https://man.openbsd.org/OpenBSD-7.3/pledge.2
> > 
> > Feedback I'm looking for
> > ~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > Some specific points I would like to get your opinion on:
> > 
> > * Is this the right general approach to restricting ioctl(2)?
> > 
> >    It will probably be possible to find counter-examples where the
> >    alternative (see below) is better.  I'd be interested in these, and
> >    in how common they are, to understand whether we have picked the
> >    right trade-off here.
> 
> In the long term, I'd like Landlock to be able to restrict a set of IOCTLs,
> taking into account the type of file/device. Being able to deny all IOCTLs
> might be useful and is much easier to implement though.

In my mind, it's a trade-off between implementation complexity and
flexibility.

My proposal is more simple-minded than what you explained, but might
solve the bulk of real-life use cases for a lower implementation
complexity. (With the caveat that I don't understand the real life use
cases well enough to know how far it really gets us, that's why this
is just an RFC.)

I'm not *just* saying this because I'm lazy ;), but I also feel that
we should be careful with the amount of complexity that we take on,
especially if there is a chance that it won't be needed in practice.

I think that it might be a feasible approach to start with the
LANDLOCK_ACCESS_FS_IOCTL approach and then look at its usage to
understand whether we see a significant number of programs whose
sandboxes are too coarse because of this.

If more fine-granular control is needed, we can still put the other
approach on top, and the additional complexity from
LANDLOCK_ACCESS_FS_IOCTL that we have to support is not that
dramatically high.

If more fine-granular control is not needed, we can skip the
implementation of the other approach and Landlock is simpler.

Then again, I'm somewhat new to kernel development still, I'm not sure
whether this is an approach that is deemed acceptable in this setting?


> > * Should we introduce a "landlock_fd_rights_limit()" syscall?
> > 
> >    We could potentially implement a system call for dropping the
> >    LANDLOCK_ACCESS_FS_IOCTL and LANDLOCK_ACCESS_FS_TRUNCATE rights from
> >    existing file descriptors (independent of the type of file
> >    descriptor, even).
> > 
> >    Possible use cases would be to (a) restrict the rights on inherited
> >    file descriptors like std{in,out,err} and to (b) restrict ioctl and
> >    truncate operations on file descriptors that are not acquired
> >    through open(2), such as network sockets.
> > 
> >    This would be similar to the cap_rights_limit(2) system call in
> >    FreeBSD's Capsicum.
> > 
> >    This idea feels somewhat orthogonal to the ioctl patch, but it would
> >    start to be more useful if the ioctl right exists.
> 
> This is indeed interesting, and that should be useful for the cases you
> explained. I think supporting IOCTLs is more important for now, but a new
> syscall to restrict FDs could be useful in the future.

Ack, OK.  I agree, it's not that urgent yet.


> We should also think about batch operations on FD (see the
> close_range syscall), for instance to deny all IOCTLs on inherited
> or received FDs.

Hm, you mean a landlock_fd_rights_limit_range() syscall to limit the
rights for an entire range of FDs?

I have to admit, I'm not familiar with the real-life use cases of
close_range().  In most programs I work with, it's difficult to reason
about their ordering once the program has really started to run. So I
imagine that close_range() is mostly used to "sanitize" the open file
descriptors at the start of main(), and you have a similar use case in
mind for this one as well?

If it's just about closing the range from 0 to 2, I'm not sure it's
worth adding a custom syscall. :)

Thanks for the review!
–Günther

Re: [RFC 0/4] Landlock: ioctl support

[Jeff Xu]

Sorry for the late reply.
>
> (Looking in the direction of Jeff Xu, who has inquired about Landlock
> for Chromium in the past -- do you happen to know in which ways you'd
> want to restrict ioctls, if you have that need? :))
>

Regarding this patch, here is some feedback from ChromeOS:
 - In the short term: we are looking to integrate Landlock into our
sandboxer, so the ability to restrict to a specific device is huge.
- Fundamentally though, in the effort of bringing process expected
behaviour closest to allowed behaviour, the ability to speak of
ioctl() path access in Landlock would be huge -- at least we can
continue to enumerate in policy what processes are allowed to do, even
if we still lack the ability to restrict individual ioctl commands for
a specific device node.

Regarding medium term:
My thoughts are, from software architecture point of view, it would be
nice to think in planes: i.e. Data plane / Control plane/ Signaling
Plane/Normal User Plane/Privileged User Plane. Let the application
define its planes, and assign operations to them. Landlock provides
data structure and syscall to construct the planes.

However, one thing I'm not sure is the third arg from ioctl:
int ioctl(int fd, unsigned long request, ...);
Is it possible for the driver to use the same request id, then put
whatever into the third arg ? how to deal with that effectively ?

For real world user cases, Dmitry Torokhov (added to list) can help.

PS: There is also lwn article about SELinux implementation of ioctl: [1]
[1] https://lwn.net/Articles/428140/

Thanks!
-Jeff Xu

Re: [RFC 0/4] Landlock: ioctl support

[Mickaël Salaün]

On 24/05/2023 23:43, Jeff Xu wrote:
> Sorry for the late reply.
>>
>> (Looking in the direction of Jeff Xu, who has inquired about Landlock
>> for Chromium in the past -- do you happen to know in which ways you'd
>> want to restrict ioctls, if you have that need? :))
>>
> 
> Regarding this patch, here is some feedback from ChromeOS:
>   - In the short term: we are looking to integrate Landlock into our
> sandboxer, so the ability to restrict to a specific device is huge.
> - Fundamentally though, in the effort of bringing process expected
> behaviour closest to allowed behaviour, the ability to speak of
> ioctl() path access in Landlock would be huge -- at least we can
> continue to enumerate in policy what processes are allowed to do, even
> if we still lack the ability to restrict individual ioctl commands for
> a specific device node.

Thanks for the feedback!

> 
> Regarding medium term:
> My thoughts are, from software architecture point of view, it would be
> nice to think in planes: i.e. Data plane / Control plane/ Signaling
> Plane/Normal User Plane/Privileged User Plane. Let the application
> define its planes, and assign operations to them. Landlock provides
> data structure and syscall to construct the planes.

I'm not sure to follow this plane thing. Could you give examples for 
these planes applied to Landlock?


> 
> However, one thing I'm not sure is the third arg from ioctl:
> int ioctl(int fd, unsigned long request, ...);
> Is it possible for the driver to use the same request id, then put
> whatever into the third arg ? how to deal with that effectively ?

I'm not sure about the value of all the arguments (except the command 
one) vs. the complexity to filter them, but we could discuss that when 
we'll reach this step.

> 
> For real world user cases, Dmitry Torokhov (added to list) can help.

Yes please!

> 
> PS: There is also lwn article about SELinux implementation of ioctl: [1]
> [1] https://lwn.net/Articles/428140/

Thanks for the pointer, this shows how complex this IOCTL access control 
is. For Landlock, I'd like to provide the minimal required features to 
enable user space to define their own rules, which means to let user 
space (and especially libraries) to identify useful or potentially 
harmful IOCTLs.

> 
> Thanks!
> -Jeff Xu

Re: [RFC 0/4] Landlock: ioctl support

[Jeff Xu]

On Sat, Jun 17, 2023 at 2:49 AM Mickaël Salaün <mic@digikod.net> wrote:
>
>
> On 24/05/2023 23:43, Jeff Xu wrote:
> > Sorry for the late reply.
> >>
> >> (Looking in the direction of Jeff Xu, who has inquired about Landlock
> >> for Chromium in the past -- do you happen to know in which ways you'd
> >> want to restrict ioctls, if you have that need? :))
> >>
> >
> > Regarding this patch, here is some feedback from ChromeOS:
> >   - In the short term: we are looking to integrate Landlock into our
> > sandboxer, so the ability to restrict to a specific device is huge.
> > - Fundamentally though, in the effort of bringing process expected
> > behaviour closest to allowed behaviour, the ability to speak of
> > ioctl() path access in Landlock would be huge -- at least we can
> > continue to enumerate in policy what processes are allowed to do, even
> > if we still lack the ability to restrict individual ioctl commands for
> > a specific device node.
>
> Thanks for the feedback!
>
> >
> > Regarding medium term:
> > My thoughts are, from software architecture point of view, it would be
> > nice to think in planes: i.e. Data plane / Control plane/ Signaling
> > Plane/Normal User Plane/Privileged User Plane. Let the application
> > define its planes, and assign operations to them. Landlock provides
> > data structure and syscall to construct the planes.
>
> I'm not sure to follow this plane thing. Could you give examples for
> these planes applied to Landlock?
>
The idea is probably along the same lines as yours: let user space
define/categorize ioctls.  For example, for a camera driver, users can
define two planes - control plane: setup parameters of lens, data
plane: setup data buffers for data transfer and do start/stop (I'm
just making up the example since I don't really know the camera
driver).

The idea is for Landlock to provide a mechanism to let user space to
divide/assign ioctls to different planes, such that the user space
processes can set/define security boundaries according to the plane it
is on.

>
> >
> > However, one thing I'm not sure is the third arg from ioctl:
> > int ioctl(int fd, unsigned long request, ...);
> > Is it possible for the driver to use the same request id, then put
> > whatever into the third arg ? how to deal with that effectively ?
>
> I'm not sure about the value of all the arguments (except the command
> one) vs. the complexity to filter them, but we could discuss that when
> we'll reach this step.
>
> >
> > For real world user cases, Dmitry Torokhov (added to list) can help.
>
> Yes please!
>
ya,  it will help with the design if there is a real world scenario to study.

> >
> > PS: There is also lwn article about SELinux implementation of ioctl: [1]
> > [1] https://lwn.net/Articles/428140/
>
> Thanks for the pointer, this shows how complex this IOCTL access control
> is. For Landlock, I'd like to provide the minimal required features to
> enable user space to define their own rules, which means to let user
> space (and especially libraries) to identify useful or potentially
> harmful IOCTLs.
>
Yes. That makes sense.

> >
> > Thanks!
> > -Jeff Xu

Re: [RFC 0/4] Landlock: ioctl support

[Mickaël Salaün]

On 21/06/2023 01:44, Jeff Xu wrote:
> On Sat, Jun 17, 2023 at 2:49 AM Mickaël Salaün <mic@digikod.net> wrote:
>>
>>
>> On 24/05/2023 23:43, Jeff Xu wrote:
>>> Sorry for the late reply.
>>>>
>>>> (Looking in the direction of Jeff Xu, who has inquired about Landlock
>>>> for Chromium in the past -- do you happen to know in which ways you'd
>>>> want to restrict ioctls, if you have that need? :))
>>>>
>>>
>>> Regarding this patch, here is some feedback from ChromeOS:
>>>    - In the short term: we are looking to integrate Landlock into our
>>> sandboxer, so the ability to restrict to a specific device is huge.
>>> - Fundamentally though, in the effort of bringing process expected
>>> behaviour closest to allowed behaviour, the ability to speak of
>>> ioctl() path access in Landlock would be huge -- at least we can
>>> continue to enumerate in policy what processes are allowed to do, even
>>> if we still lack the ability to restrict individual ioctl commands for
>>> a specific device node.
>>
>> Thanks for the feedback!
>>
>>>
>>> Regarding medium term:
>>> My thoughts are, from software architecture point of view, it would be
>>> nice to think in planes: i.e. Data plane / Control plane/ Signaling
>>> Plane/Normal User Plane/Privileged User Plane. Let the application
>>> define its planes, and assign operations to them. Landlock provides
>>> data structure and syscall to construct the planes.
>>
>> I'm not sure to follow this plane thing. Could you give examples for
>> these planes applied to Landlock?
>>
> The idea is probably along the same lines as yours: let user space
> define/categorize ioctls.  For example, for a camera driver, users can
> define two planes - control plane: setup parameters of lens, data
> plane: setup data buffers for data transfer and do start/stop (I'm
> just making up the example since I don't really know the camera
> driver).
> 
> The idea is for Landlock to provide a mechanism to let user space to
> divide/assign ioctls to different planes, such that the user space
> processes can set/define security boundaries according to the plane it
> is on.

Right, we're on the same track. :)


> 
>>
>>>
>>> However, one thing I'm not sure is the third arg from ioctl:
>>> int ioctl(int fd, unsigned long request, ...);
>>> Is it possible for the driver to use the same request id, then put
>>> whatever into the third arg ? how to deal with that effectively ?
>>
>> I'm not sure about the value of all the arguments (except the command
>> one) vs. the complexity to filter them, but we could discuss that when
>> we'll reach this step.
>>
>>>
>>> For real world user cases, Dmitry Torokhov (added to list) can help.
>>
>> Yes please!
>>
> ya,  it will help with the design if there is a real world scenario to study.

I'll get internal requirements too.


> 
>>>
>>> PS: There is also lwn article about SELinux implementation of ioctl: [1]
>>> [1] https://lwn.net/Articles/428140/
>>
>> Thanks for the pointer, this shows how complex this IOCTL access control
>> is. For Landlock, I'd like to provide the minimal required features to
>> enable user space to define their own rules, which means to let user
>> space (and especially libraries) to identify useful or potentially
>> harmful IOCTLs.
>>
> Yes. That makes sense.
> 
>>>
>>> Thanks!
>>> -Jeff Xu

Re: [RFC 0/4] Landlock: ioctl support

[Mickaël Salaün]

This subject is not easy, but I think we're reaching a consensus (see my 
3-steps proposal plan below). I answered your questions about the 
(complex) interface I proposed, but we should focus on the first step 
now (your initial proposal) and get back to the other steps later in 
another email thread.


On 10/05/2023 21:21, Günther Noack wrote:
> Hello Mickaël!
> 
> Sorry for the late reply.  This was indeed a bit difficult for me to
> understand; maybe it just needs more clarification.
> 
> Let me try to paraphrase your proposal (inline below) so we can
> resolve the misunderstandings.
> 
> 
> On Thu, May 04, 2023 at 11:12:00PM +0200, Mickaël Salaün wrote:
>> Thanks for this RFC, this is interesting!
>>
>> I previously thought a lot about IOCTLs restrictions and here are some
>> notes:
>>
>> IOCTLs are everywhere, from devices to filesystems (see fscrypt). Each
>> different file type may behave differently for the same IOCTL command/ID. It
>> is also worth noting that there are a lot of different IOCTLs, they are
>> growing over time, some might be dedicated to get some data (i.e. read) and
>> others to change the state of a device (i.e. write), some might be innocuous
>> (e.g. FIOCLEX, FIONCLEX) and others potentially harmful. _IOC_READ and
>> _IOC_WRITE can be useful but they are not mandatory, there are exceptions,
>> and it may be difficult to identify if a command pertains to one, the other,
>> or both kind of actions.
>>
>> I then think it would be very useful to be able to tie file/device types to
>> a set of IOCTLs, letting user space libraries define and classify the IOCTL
>> semantic.
>>
>> Instead of relying on a LANDLOCK_ACCESS_FS_IOCTL which would allow or deny
>> all IOCTLs, we can extend the path_beneath struct to manage IOCTLs in
>> addition to regular file accesses. Because dealing with a set of IOCTLs
>> would imply to deal with a lot of data and combinations, I thought about
>> creating groups of IOCTLs (defining access semantic) that could be matched
>> against file hierarchies. The composability nature of Landlock domains is
>> also another constraint to keep in mind.
>>
>> // New rule type dedicated to define groups of IOCTLs.
>> struct landlock_ioctl_attr {
>>    __u32 command; // IOCTL number/ID
>>    dev_t device; // must be 0 for regular file and directory
>>    __u8 file_type;
>>    __u8 id_mask; // if 0, then applied globally
>> };
>>
>> We could use landlock_add_rule(2) to fill a set of landlock_ioctl_attr into
>> a ruleset and use them with landlock_path_beneath_attr entries:
>>
>> // LANDLOCK_RULE_PATH_BENEATH, leveraging the extensible design of
>> // landlock_path_beneath_attr, hence the same first fields.
>> struct landlock_path_beneath_attr {
>>    __u64 allowed_access;
>>    __s32 parent_fd;
>>    __u16 allowed_ioctl_id_mask;
>> };
>>
>> landlock_ioctl_attr includes a 8-bit mask for which each bit identifies a
>> set of allowed IOCTLs per device/file type. This mask is then tied to a
>> path_beneath_attr. We cannot use number IDs because of dev_t+IOCTL->ID
>> intersection conflicts. Using an id_mask enables to group (specific) IOCTLs
>> together, then creating synthetic access rights.
>>
>> When merging a ruleset with a domain, each IOCTL ID mask is shifted and ORed
>> with the other layer ones to get a (8*16) 128-bit mask, stored in an
>> IOCTL/dev_t table and in the related landlock_object. When looking for an
>> IOCTL request, Landlock first looks into the IOCTL set ID table and get the
>> global set ID mask, which kind of translates to a composition of synthetic
>> access rights (stored with the landlock_layer.ioctl_access bitmask). We then
>> walk through all the inodes to match the whole mask.
>>
>> I realize that this is complex and this explanation might be confusing
>> though. What do you think?
> 
> To be honest, I am not fully sure I understand the landlock_ioctl_attr
> struct correctly.
> 
> My current guess is:
> 
>    command:   a single ioctl request number that should be permitted
> 
>    device:    if device != 0; require the dev_t (major+minor number)
>               of the file to match, before permitting the ioctl
> 
>    file_type: if set (!= 0?), require the file type to match,
>               before permitting the ioctl
> 
>    id_mask:   Indicates the IDs of the groups where this ioctl
>               should be permitted(?)
> 
> So -- if we were to implement this without any optimizations -- the
> logic of this is presumably something like this?:
> 
>    If Landlock checks an ioctl with a request_id on a file f:
> 
>    We look up the allowed_ioctl_id_mask and loop over the bits set in
>    that mask.
> 
>    For every bit set in that mask, we look up the matching ioctl group.
>    Within the group, we look whether we can find a landlock_ioctl_attr
>    rule that belongs to the group which permits that ioctl request.
> 
>    The request is granted by a landlock_ioctl_attr if:
> 
>      attr.command == request_id
>      && (attr.device == 0 || file_inode(file).i_rdev == attr.device)
>      && (attr.file_type == 0 || landlock_get_file_type?(file))
> 
> Is this roughly what you imagined?

Yes :)

> 
> 
> Some specific things I don't understand well are:
> 
> * How does id_mask identify the ioctl group?  Do you envision an
>    interface where you can add a landlock_ioctl_attr to multiple groups
>    at once?

id_mask would be an arbitrary value picked by the user (library). I was 
thinking about a bitmask (for allowed_ioctl_id_mask) because different 
ioctl_attr could overlap (e.g., for different file hierarchy).

> 
>    When the added landlock_ioctl_attr has id_mask=3, does it add that
>    attr to group 2 and 1?

No, it assigns this IOCTL attribute to the group 3, which can then be 
matched against with allowed_ioctl_id_mask & (1 << 2).

> 
> * How does allowed_ioctl_id_mask match against the ioctl group IDs
>    (id_mask)?  I'm particularly confused because the
>    allowed_ioctl_id_mask is __u16, whereas id_mask is __u8?  Is this
>    intentional?

The idea was to assign each ioctl_attr to one group (ID) but enable to 
match several groups with path_beneath_attr (allowed_ioctl_id_mask being 
the only bitmask, whereas id_mask is a number). Yes, the naming is 
confusing…

> 
> * What is file_type?  Are those the file types as used in mknod(2),
>    so that you can distinguish between regular files, directories,
>    named pipes and the like?

Yes

> 
> * If I understand the proposal right, the .device and .file_type in
>    landlock_ioctl_attr are narrowing down the set of files that the
>    ioctl policy applies to.  In all rules up until Landlock v3, which
>    we already have, the selection of the files which the rule applies
>    to is purely done based on file hierarchy.
> 
>    Would it not be a more orthogonal API if the "file selection" part
>    of the Landlock API and the "policy adding" part for these selected
>    files were independent of each other?  Then the .device and
>    .file_type selection scheme could be used for the existing policies
>    as well?

Both approaches have pros and cons. I propose a new incremental approach 
below that starts with the simple case where there is no direct links 
between different rule types (only the third step add that).

> 
> * When restricting by dev_t (major and minor number), aren't there use
>    cases where a system might have 256 CDROM drives, and you'd need to
>    allow-list all of these minor number combinations?

Indeed, we should be able to just ignore device minors.

> 
> * Aren't many ioctl use cases already usable with just the proposal I
>    made?  If you add a rule to permit IOCTL for /dev/cdrom0, that
>    opened file will anyway only expose a small subset of the ioctls
>    that the kernel as a whole offers, no?  Are there ioctls which are
>    offered independent of the file type which I'm overlooking? (Sorry,
>    this might be a naive question. :))

This is correct until users allow IOCTL for a directory (e.g. /etc). It 
depends on use cases though.

> 
> 
>> On 02/05/2023 19:17, Günther Noack wrote:
>>> Alternatives considered: Allow-listing specific ioctl requests
>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> It would be technically possible to keep an allow-list of ioctl
>>> requests and thereby control in more detail which ioctls should work.
>>>
>>> I believe that this is not needed for the majority of use cases and
>>> that it is a reasonable trade-off to make here (but I'm happy to hear
>>> about counterexamples).  The reasoning is:
>>>
>>> * Many programs do not need ioctl at all,
>>>     and denying all ioctl(2) requests OK for these.
>>>
>>> * Other programs need ioctl, but only for the terminal FDs.
>>>     This is supported because these file descriptors are usually
>>>     inherited from the parent process - so the parent process gets to
>>>     control the ioctl(2) policy for them.
>>>
>>> * Some programs need ioctl on specific files that they are opening
>>>     themselves.  They can allow-list these file paths for ioctl(2).
>>>     This makes the programs work, but it restricts a variety of other
>>>     ioctl requests which are otherwise possible through opening other
>>>     files.
>>>
>>> Because the LANDLOCK_ACCESS_FS_IOCTL right is attached to the file
>>> descriptor, programs have flexible options to control which ioctl
>>> operations should work, without the implementation complexity of
>>> additional ioctl allow-lists in the kernel.
>>>
>>> Finally, the proposed approach is simpler in implementation and has
>>> lower API complexity, but it does *not* preclude us from implementing
>>> per-ioctl-request allow lists later, if that turns out to be necessary
>>> at a later point.
>>
>> I value this simplicity, but I'm also wondering about how much this
>> allow/deny all IOCTLs approach would be useful in real case scenarios. ;)
> 
> Mickaël, would you be open to gather some more data for this from
> existing users, to understand better which use cases they have?

Of course, thanks!

I'm trying to project myself as an app developer, a sysadmin and a 
desktop user. I know some sandboxed softwares, I sandboxed some, and I'm 
looking into new ones. The main issue I see is (from the sysadmin and 
user POV) when we want to manage a whole file hierarchy, which may 
contain different file/device types.


> 
> (Looking in the direction of Jeff Xu, who has inquired about Landlock
> for Chromium in the past -- do you happen to know in which ways you'd
> want to restrict ioctls, if you have that need? :))
> 
> 
>>> Related Work
>>> ~~~~~~~~~~~~
>>>
>>> OpenBSD's pledge(2) [1] restricts ioctl(2) independent of the file
>>> descriptor which is used.  The implementers maintain multiple
>>> allow-lists of predefined ioctl(2) operations required for different
>>> application domains such as "audio", "bpf", "tty" and "inet".
>>>
>>> OpenBSD does not guarantee ABI backwards compatibility to the same
>>> extent as Linux does, so it's easier for them to update these lists in
>>> later versions.  It might not be a feasible approach for Linux though.
>>>
>>> [1] https://man.openbsd.org/OpenBSD-7.3/pledge.2
>>>
>>> Feedback I'm looking for
>>> ~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> Some specific points I would like to get your opinion on:
>>>
>>> * Is this the right general approach to restricting ioctl(2)?
>>>
>>>     It will probably be possible to find counter-examples where the
>>>     alternative (see below) is better.  I'd be interested in these, and
>>>     in how common they are, to understand whether we have picked the
>>>     right trade-off here.
>>
>> In the long term, I'd like Landlock to be able to restrict a set of IOCTLs,
>> taking into account the type of file/device. Being able to deny all IOCTLs
>> might be useful and is much easier to implement though.
> 
> In my mind, it's a trade-off between implementation complexity and
> flexibility.
> 
> My proposal is more simple-minded than what you explained, but might
> solve the bulk of real-life use cases for a lower implementation
> complexity. (With the caveat that I don't understand the real life use
> cases well enough to know how far it really gets us, that's why this
> is just an RFC.)
> 
> I'm not *just* saying this because I'm lazy ;), but I also feel that
> we should be careful with the amount of complexity that we take on,
> especially if there is a chance that it won't be needed in practice.

I agree :)

> 
> I think that it might be a feasible approach to start with the
> LANDLOCK_ACCESS_FS_IOCTL approach and then look at its usage to
> understand whether we see a significant number of programs whose
> sandboxes are too coarse because of this.
> 
> If more fine-granular control is needed, we can still put the other
> approach on top, and the additional complexity from
> LANDLOCK_ACCESS_FS_IOCTL that we have to support is not that
> dramatically high.
> 
> If more fine-granular control is not needed, we can skip the
> implementation of the other approach and Landlock is simpler.
> 
> Then again, I'm somewhat new to kernel development still, I'm not sure
> whether this is an approach that is deemed acceptable in this setting?

I agree that IOCTLs are a security risk and that we should propose a 
simple solution short-term, and maybe a more complete one long-term.

The main issue with a unique IOCTL access right for a file hierarchy is 
that we may not know which device/driver will be the target/server, and 
hence if we need to allow some IOCTL for regular files (e.g., fscrypt), 
we might end up allowing all IOCTLs.

Here is a plan to incrementally develop a fine-grained IOCTL access 
control in 3 steps:

1/ Add a simple IOCTL access right for path_beneath: what you initially 
proposed. For systems that already configure nodev mount points, it 
could be even more useful (e.g., safely allow IOCTL on /home for 
fscrypt, and specific /dev files otherwise).


2/ Create a new type of rule to identify file/device type:
struct landlock_inode_type_attr {
     __u64 allowed_access; /* same as for path_beneath */
     __u64 flags; /* bit 0: ignores device minor */
     dev_t device; /* same as stat's st_rdev */
     __u16 file_type; /* same as stat's st_mode & S_IFMT */
};

We'll probably need to differentiate the handled accesses for 
path_beneath rules and those for inode_type rules to be more useful.

One issue with this type of rule is that it could be used as an oracle 
to bypass stat restrictions. We could check if such (virtual) action is 
allowed without the current domain though.


3/ Add a new type of rule to match IOCTL commands, with a mechanism to 
tie this to inode_type rules (because a command ID is relative to a file 
type/device), and potentially the same mechanism to tie inode_type rules 
to path_beneath rules.


Each of this step can be implemented one after the other, and each one 
is valuable. What do you think?


> 
> 
>>> * Should we introduce a "landlock_fd_rights_limit()" syscall?
>>>
>>>     We could potentially implement a system call for dropping the
>>>     LANDLOCK_ACCESS_FS_IOCTL and LANDLOCK_ACCESS_FS_TRUNCATE rights from
>>>     existing file descriptors (independent of the type of file
>>>     descriptor, even).
>>>
>>>     Possible use cases would be to (a) restrict the rights on inherited
>>>     file descriptors like std{in,out,err} and to (b) restrict ioctl and
>>>     truncate operations on file descriptors that are not acquired
>>>     through open(2), such as network sockets.
>>>
>>>     This would be similar to the cap_rights_limit(2) system call in
>>>     FreeBSD's Capsicum.
>>>
>>>     This idea feels somewhat orthogonal to the ioctl patch, but it would
>>>     start to be more useful if the ioctl right exists.
>>
>> This is indeed interesting, and that should be useful for the cases you
>> explained. I think supporting IOCTLs is more important for now, but a new
>> syscall to restrict FDs could be useful in the future.
> 
> Ack, OK.  I agree, it's not that urgent yet.
> 
> 
>> We should also think about batch operations on FD (see the
>> close_range syscall), for instance to deny all IOCTLs on inherited
>> or received FDs.
> 
> Hm, you mean a landlock_fd_rights_limit_range() syscall to limit the
> rights for an entire range of FDs?
> 
> I have to admit, I'm not familiar with the real-life use cases of
> close_range().  In most programs I work with, it's difficult to reason
> about their ordering once the program has really started to run. So I
> imagine that close_range() is mostly used to "sanitize" the open file
> descriptors at the start of main(), and you have a similar use case in
> mind for this one as well?
> 
> If it's just about closing the range from 0 to 2, I'm not sure it's
> worth adding a custom syscall. :)

The advantage of this kind of range is to efficiently manage all 
potential FDs, and the main use case is to close (or change, see the 
flags) everything *except" 0-2 (i.e. 3-~0), and then avoid a lot of 
(potentially useless) syscalls.

The Landlock interface doesn't need to be a syscall. We could just add a 
new rule type which could take a FD range and restrict them when calling 
landlock_restrict_self(). Something like this:
struct landlock_fd_attr {
     __u64 allowed_access;
     __u32 first;
     __u32 last;
}

> 
> Thanks for the review!
> –Günther

Re: [RFC 0/4] Landlock: ioctl support

[Günther Noack]

Hello Mickaël!

On Sat, Jun 17, 2023 at 11:47:55AM +0200, Mickaël Salaün wrote:
> This subject is not easy, but I think we're reaching a consensus (see my
> 3-steps proposal plan below). I answered your questions about the (complex)
> interface I proposed, but we should focus on the first step now (your
> initial proposal) and get back to the other steps later in another email
> thread.

Thanks for the review!


> On 10/05/2023 21:21, Günther Noack wrote:
> > [...]
> > Some specific things I don't understand well are:
> > [...]

Thanks, this all make sense now. 👍


> >    Would it not be a more orthogonal API if the "file selection" part
> >    of the Landlock API and the "policy adding" part for these selected
> >    files were independent of each other?  Then the .device and
> >    .file_type selection scheme could be used for the existing policies
> >    as well?
> 
> Both approaches have pros and cons. I propose a new incremental approach
> below that starts with the simple case where there is no direct links
> between different rule types (only the third step add that).
> 
> > 
> > * When restricting by dev_t (major and minor number), aren't there use
> >    cases where a system might have 256 CDROM drives, and you'd need to
> >    allow-list all of these minor number combinations?
> 
> Indeed, we should be able to just ignore device minors.

They device numbers are listed in
https://www.kernel.org/doc/Documentation/admin-guide/devices.txt

Some major numbers are grab-bags of miscellaneous devices, for example
major numbers 10 and 13; 13/0-31 (maj/min) are joysticks, whereas
13/32-62) are mice.

Maybe this can be specified as ranges on dev_t, so that it would be
possible to specify 13/32-62, without matching the joysticks too?


> > I think that it might be a feasible approach to start with the
> > LANDLOCK_ACCESS_FS_IOCTL approach and then look at its usage to
> > understand whether we see a significant number of programs whose
> > sandboxes are too coarse because of this.
> > 
> > If more fine-granular control is needed, we can still put the other
> > approach on top, and the additional complexity from
> > LANDLOCK_ACCESS_FS_IOCTL that we have to support is not that
> > dramatically high.
> >
> > [...]
> 
> I agree that IOCTLs are a security risk and that we should propose a simple
> solution short-term, and maybe a more complete one long-term.
> 
> The main issue with a unique IOCTL access right for a file hierarchy is that
> we may not know which device/driver will be the target/server, and hence if
> we need to allow some IOCTL for regular files (e.g., fscrypt), we might end
> up allowing all IOCTLs.
> 
> Here is a plan to incrementally develop a fine-grained IOCTL access control
> in 3 steps:
> 
> 1/ Add a simple IOCTL access right for path_beneath: what you initially
> proposed. For systems that already configure nodev mount points, it could be
> even more useful (e.g., safely allow IOCTL on /home for fscrypt, and
> specific /dev files otherwise).

Ack, I'll continue on that implementation then. 👍


> 2/ Create a new type of rule to identify file/device type:
> struct landlock_inode_type_attr {
>     __u64 allowed_access; /* same as for path_beneath */
>     __u64 flags; /* bit 0: ignores device minor */
>     dev_t device; /* same as stat's st_rdev */
>     __u16 file_type; /* same as stat's st_mode & S_IFMT */
> };
> 
> We'll probably need to differentiate the handled accesses for path_beneath
> rules and those for inode_type rules to be more useful.
> 
> One issue with this type of rule is that it could be used as an oracle to
> bypass stat restrictions. We could check if such (virtual) action is allowed
> without the current domain though.
> 
> 
> 3/ Add a new type of rule to match IOCTL commands, with a mechanism to tie
> this to inode_type rules (because a command ID is relative to a file
> type/device), and potentially the same mechanism to tie inode_type rules to
> path_beneath rules.
> 
> 
> Each of this step can be implemented one after the other, and each one is
> valuable. What do you think?

I think it is a good idea to do it in multiple steps,
as I also believe that step 1) already provides value on its own.

To make sure we are on the same page, let me paraphrase my understanding here:

1) is what I already sent as RFC, with tests and documentation etc.

   With 1), callers could allow and deny ioctls on newly opened files,
   independent of file path.

2) makes dev_t and file_type predicates which can be used to limit
   file accesses on already opened files.

   With 2), callers could allow and deny ioctls and other operations
   also on files which are already opened before enablement, such
   as the TTY FD inherited from the parent process.
   
3) would make it possible to restrict individual ioctl commands,
   depending on the dev_t, the file_type, and possibly the path.

Is that accurate?

In 2) and 3), I'm still contemplating a bit whether we have
alternative approaches, but I believe in any case, it's clear that
they can be built on top of each other without much overhead, and for
many programs, outright denying ioctl on newly opened files with 1)
might be the most straightforward approach which already delivers
value.

Some notes which I collected on the side:

* I'm still worried about the already-opened file descriptors like the
  TTY, through which it can be easy to escape a sandbox
  (c.f. https://nvd.nist.gov/vuln/detail/CVE-2017-5226) - I would like
  to have a solution for that.  Step (2) would fix it, but maybe I can
  find a workaround to at least detect the problem in step (1)
  already.

* I looked at OpenBSD's pledge and unveil.  In OpenBSD, the ioctl
  policy is based on the properties of an already opened file.  They
  have hardcoded their ioctl logic in the kernel, which would be a
  mistake to do in Linux due to stronger backwards compatibility
  guarantees.  OpenBSD is making the allow/deny decisions based on
  device major/minor numbers and file type (usually block/char
  device).

  I think it is noteworthy that OpenBSD does *not* use the file path
  to make that decision.

  I wonder whether we should replicate that in steps (2) and (3).  It
  would make for a simpler, more orthogonal API, where the ioctl
  policy would become a property of the task, and it would be enforced
  independently of the existing path-based logic.  When callers
  combine that with the overall ioctl check from (1), it might be
  flexible enough for practical purposes.

* We should not rely on the way that ioctl request numbers are
  structured, because it is used inconsistently.  More background:
  https://lwn.net/Articles/428140/ (Thank you for pointing out this
  article, Jeff!)

> > > > * Should we introduce a "landlock_fd_rights_limit()" syscall?
> > > [...]
> > >
> > > We should also think about batch operations on FD (see the
> > > close_range syscall), for instance to deny all IOCTLs on inherited
> > > or received FDs.
> > 
> > Hm, you mean a landlock_fd_rights_limit_range() syscall to limit the
> > rights for an entire range of FDs?
> > 
> > I have to admit, I'm not familiar with the real-life use cases of
> > close_range().  In most programs I work with, it's difficult to reason
> > about their ordering once the program has really started to run. So I
> > imagine that close_range() is mostly used to "sanitize" the open file
> > descriptors at the start of main(), and you have a similar use case in
> > mind for this one as well?
> > 
> > If it's just about closing the range from 0 to 2, I'm not sure it's
> > worth adding a custom syscall. :)
> 
> The advantage of this kind of range is to efficiently manage all potential
> FDs, and the main use case is to close (or change, see the flags) everything
> *except" 0-2 (i.e. 3-~0), and then avoid a lot of (potentially useless)
> syscalls.
> 
> The Landlock interface doesn't need to be a syscall. We could just add a new
> rule type which could take a FD range and restrict them when calling
> landlock_restrict_self(). Something like this:
> struct landlock_fd_attr {
>     __u64 allowed_access;
>     __u32 first;
>     __u32 last;
> }

Ack, I see it a bit better. Let's discuss this topic separately.

—Günther

-- 
Sent using Mutt 🐕 Woof Woof

Re: [RFC 0/4] Landlock: ioctl support

[Mickaël Salaün]

On 19/06/2023 18:21, Günther Noack wrote:
> Hello Mickaël!
> 
> On Sat, Jun 17, 2023 at 11:47:55AM +0200, Mickaël Salaün wrote:
>> This subject is not easy, but I think we're reaching a consensus (see my
>> 3-steps proposal plan below). I answered your questions about the (complex)
>> interface I proposed, but we should focus on the first step now (your
>> initial proposal) and get back to the other steps later in another email
>> thread.
> 
> Thanks for the review!
> 
> 
>> On 10/05/2023 21:21, Günther Noack wrote:
>>> [...]
>>> Some specific things I don't understand well are:
>>> [...]
> 
> Thanks, this all make sense now. 👍
> 
> 
>>>     Would it not be a more orthogonal API if the "file selection" part
>>>     of the Landlock API and the "policy adding" part for these selected
>>>     files were independent of each other?  Then the .device and
>>>     .file_type selection scheme could be used for the existing policies
>>>     as well?
>>
>> Both approaches have pros and cons. I propose a new incremental approach
>> below that starts with the simple case where there is no direct links
>> between different rule types (only the third step add that).
>>
>>>
>>> * When restricting by dev_t (major and minor number), aren't there use
>>>     cases where a system might have 256 CDROM drives, and you'd need to
>>>     allow-list all of these minor number combinations?
>>
>> Indeed, we should be able to just ignore device minors.
> 
> They device numbers are listed in
> https://www.kernel.org/doc/Documentation/admin-guide/devices.txt
> 
> Some major numbers are grab-bags of miscellaneous devices, for example
> major numbers 10 and 13; 13/0-31 (maj/min) are joysticks, whereas
> 13/32-62) are mice.
> 
> Maybe this can be specified as ranges on dev_t, so that it would be
> possible to specify 13/32-62, without matching the joysticks too?

Indeed, being able to specifying a range would be useful. I'm not sure 
about the whole dev_t range or only a minor range though. From a 
semantic POV, there is no links between majors.

> 
> 
>>> I think that it might be a feasible approach to start with the
>>> LANDLOCK_ACCESS_FS_IOCTL approach and then look at its usage to
>>> understand whether we see a significant number of programs whose
>>> sandboxes are too coarse because of this.
>>>
>>> If more fine-granular control is needed, we can still put the other
>>> approach on top, and the additional complexity from
>>> LANDLOCK_ACCESS_FS_IOCTL that we have to support is not that
>>> dramatically high.
>>>
>>> [...]
>>
>> I agree that IOCTLs are a security risk and that we should propose a simple
>> solution short-term, and maybe a more complete one long-term.
>>
>> The main issue with a unique IOCTL access right for a file hierarchy is that
>> we may not know which device/driver will be the target/server, and hence if
>> we need to allow some IOCTL for regular files (e.g., fscrypt), we might end
>> up allowing all IOCTLs.
>>
>> Here is a plan to incrementally develop a fine-grained IOCTL access control
>> in 3 steps:
>>
>> 1/ Add a simple IOCTL access right for path_beneath: what you initially
>> proposed. For systems that already configure nodev mount points, it could be
>> even more useful (e.g., safely allow IOCTL on /home for fscrypt, and
>> specific /dev files otherwise).
> 
> Ack, I'll continue on that implementation then. 👍
> 
> 
>> 2/ Create a new type of rule to identify file/device type:
>> struct landlock_inode_type_attr {
>>      __u64 allowed_access; /* same as for path_beneath */
>>      __u64 flags; /* bit 0: ignores device minor */
>>      dev_t device; /* same as stat's st_rdev */
>>      __u16 file_type; /* same as stat's st_mode & S_IFMT */
>> };
>>
>> We'll probably need to differentiate the handled accesses for path_beneath
>> rules and those for inode_type rules to be more useful.
>>
>> One issue with this type of rule is that it could be used as an oracle to
>> bypass stat restrictions. We could check if such (virtual) action is allowed
>> without the current domain though.
>>
>>
>> 3/ Add a new type of rule to match IOCTL commands, with a mechanism to tie
>> this to inode_type rules (because a command ID is relative to a file
>> type/device), and potentially the same mechanism to tie inode_type rules to
>> path_beneath rules.
>>
>>
>> Each of this step can be implemented one after the other, and each one is
>> valuable. What do you think?
> 
> I think it is a good idea to do it in multiple steps,
> as I also believe that step 1) already provides value on its own.
> 
> To make sure we are on the same page, let me paraphrase my understanding here:
> 
> 1) is what I already sent as RFC, with tests and documentation etc.
> 
>     With 1), callers could allow and deny ioctls on newly opened files,
>     independent of file path.

Yes

> 
> 2) makes dev_t and file_type predicates which can be used to limit
>     file accesses on already opened files.
> 
>     With 2), callers could allow and deny ioctls and other operations
>     also on files which are already opened before enablement, such
>     as the TTY FD inherited from the parent process.

This is not on already opened files, it would be a complementary rule 
type to path_beneath ones: if a rule with dev_t/file_type matches and 
allowed action FOO, and a path_beneath rules matches and also allowed 
action FOO, then FOO is allowed.

However, this could be extended in the future (with a new dedicated 
flag) to also support already-opened files.


>     
> 3) would make it possible to restrict individual ioctl commands,
>     depending on the dev_t, the file_type, and possibly the path.

Yes. I think we could even only extend the "landlock_inode_attr" struct 
to add an ioctl_command field, and potentially others too. I think it 
would make sense because IOCTLs are tied to a specific device/file type 
(or even the underlying filesystem).

In fact, we can split the third step in two:
3/ Extend landlock_inode_attr with ioctl_command.
4/ Define an inode_category field that contain an arbitrary number which 
can be match against a path_beneath rule (with a bitmask to be able to 
match others too).

Other steps could allow to match landlock_inode_attr with FDs, add a 
landlock_fd_range_attr…


> 
> Is that accurate?
> 
> In 2) and 3), I'm still contemplating a bit whether we have
> alternative approaches, but I believe in any case, it's clear that
> they can be built on top of each other without much overhead, and for
> many programs, outright denying ioctl on newly opened files with 1)
> might be the most straightforward approach which already delivers
> value.

Right

> 
> Some notes which I collected on the side:
> 
> * I'm still worried about the already-opened file descriptors like the
>    TTY, through which it can be easy to escape a sandbox
>    (c.f. https://nvd.nist.gov/vuln/detail/CVE-2017-5226) - I would like
>    to have a solution for that.  Step (2) would fix it, but maybe I can
>    find a workaround to at least detect the problem in step (1)
>    already.

Yes, good example, Landlock should be able to help with this issue 
thanks to the third step.


> 
> * I looked at OpenBSD's pledge and unveil.  In OpenBSD, the ioctl
>    policy is based on the properties of an already opened file.  They
>    have hardcoded their ioctl logic in the kernel, which would be a
>    mistake to do in Linux due to stronger backwards compatibility
>    guarantees.  OpenBSD is making the allow/deny decisions based on
>    device major/minor numbers and file type (usually block/char
>    device).
> 
>    I think it is noteworthy that OpenBSD does *not* use the file path
>    to make that decision.

Yes, we could have the same behavior by only using the rule type added 
with the second step, but we could easily improve that by combining a 
path_beneath rule thanks to the third step.


> 
>    I wonder whether we should replicate that in steps (2) and (3).  It
>    would make for a simpler, more orthogonal API, where the ioctl
>    policy would become a property of the task, and it would be enforced
>    independently of the existing path-based logic.  When callers
>    combine that with the overall ioctl check from (1), it might be
>    flexible enough for practical purposes.

Yes, this will be possible with the four steps.


> 
> * We should not rely on the way that ioctl request numbers are
>    structured, because it is used inconsistently.  More background:
>    https://lwn.net/Articles/428140/ (Thank you for pointing out this
>    article, Jeff!)

Definitely, but the user space libraries might help with that. ;)


> 
>>>>> * Should we introduce a "landlock_fd_rights_limit()" syscall?
>>>> [...]
>>>>
>>>> We should also think about batch operations on FD (see the
>>>> close_range syscall), for instance to deny all IOCTLs on inherited
>>>> or received FDs.
>>>
>>> Hm, you mean a landlock_fd_rights_limit_range() syscall to limit the
>>> rights for an entire range of FDs?
>>>
>>> I have to admit, I'm not familiar with the real-life use cases of
>>> close_range().  In most programs I work with, it's difficult to reason
>>> about their ordering once the program has really started to run. So I
>>> imagine that close_range() is mostly used to "sanitize" the open file
>>> descriptors at the start of main(), and you have a similar use case in
>>> mind for this one as well?
>>>
>>> If it's just about closing the range from 0 to 2, I'm not sure it's
>>> worth adding a custom syscall. :)
>>
>> The advantage of this kind of range is to efficiently manage all potential
>> FDs, and the main use case is to close (or change, see the flags) everything
>> *except" 0-2 (i.e. 3-~0), and then avoid a lot of (potentially useless)
>> syscalls.
>>
>> The Landlock interface doesn't need to be a syscall. We could just add a new
>> rule type which could take a FD range and restrict them when calling
>> landlock_restrict_self(). Something like this:
>> struct landlock_fd_attr {
>>      __u64 allowed_access;
>>      __u32 first;
>>      __u32 last;
>> }
> 
> Ack, I see it a bit better. Let's discuss this topic separately.
> 
> —Günther
> 

Re: [RFC 0/4] Landlock: ioctl support

[Günther Noack]

Hello!

On Sat, Jun 17, 2023 at 11:47:55AM +0200, Mickaël Salaün wrote:
> > > We should also think about batch operations on FD (see the
> > > close_range syscall), for instance to deny all IOCTLs on inherited
> > > or received FDs.
> > 
> > Hm, you mean a landlock_fd_rights_limit_range() syscall to limit the
> > rights for an entire range of FDs?
> > 
> > I have to admit, I'm not familiar with the real-life use cases of
> > close_range().  In most programs I work with, it's difficult to reason
> > about their ordering once the program has really started to run. So I
> > imagine that close_range() is mostly used to "sanitize" the open file
> > descriptors at the start of main(), and you have a similar use case in
> > mind for this one as well?
> > 
> > If it's just about closing the range from 0 to 2, I'm not sure it's
> > worth adding a custom syscall. :)
> 
> The advantage of this kind of range is to efficiently manage all potential
> FDs, and the main use case is to close (or change, see the flags) everything
> *except" 0-2 (i.e. 3-~0), and then avoid a lot of (potentially useless)
> syscalls.
> 
> The Landlock interface doesn't need to be a syscall. We could just add a new
> rule type which could take a FD range and restrict them when calling
> landlock_restrict_self(). Something like this:
> struct landlock_fd_attr {
>     __u64 allowed_access;
>     __u32 first;
>     __u32 last;
> }

FYI, regarding the idea of dropping rights on already-opened files:
I'm starting to have doubts about how feasible this is in practice.

The "obvious" approach is to just remove the access rights from the security
blob flags on the struct file.

But these opened "struct file"s might be shared with other processes already,
and mutating them in place would have undesired side effects on other processes.

For example, if brltty uses ioctls on the terminal and then one of the programs
running in that terminal drops ioctl rights on that open file, it would affect
brltty as well, because both the Landlocked program and brltty use the same
struct file.

It could be technically stored next to the file descriptor list, where the
close-on-exec flag is also stored, but that seems more cumbersome than I had
hoped.  I don't have a good approach for that idea yet, so I'll drop it for now.

Ideas are welcome. :)

—Günther

-- 
Sent using Mutt 🐕 Woof Woof

Re: [RFC 0/4] Landlock: ioctl support

[Mickaël Salaün]

On 12/07/2023 13:08, Günther Noack wrote:
> Hello!
> 
> On Sat, Jun 17, 2023 at 11:47:55AM +0200, Mickaël Salaün wrote:
>>>> We should also think about batch operations on FD (see the
>>>> close_range syscall), for instance to deny all IOCTLs on inherited
>>>> or received FDs.
>>>
>>> Hm, you mean a landlock_fd_rights_limit_range() syscall to limit the
>>> rights for an entire range of FDs?
>>>
>>> I have to admit, I'm not familiar with the real-life use cases of
>>> close_range().  In most programs I work with, it's difficult to reason
>>> about their ordering once the program has really started to run. So I
>>> imagine that close_range() is mostly used to "sanitize" the open file
>>> descriptors at the start of main(), and you have a similar use case in
>>> mind for this one as well?
>>>
>>> If it's just about closing the range from 0 to 2, I'm not sure it's
>>> worth adding a custom syscall. :)
>>
>> The advantage of this kind of range is to efficiently manage all potential
>> FDs, and the main use case is to close (or change, see the flags) everything
>> *except" 0-2 (i.e. 3-~0), and then avoid a lot of (potentially useless)
>> syscalls.
>>
>> The Landlock interface doesn't need to be a syscall. We could just add a new
>> rule type which could take a FD range and restrict them when calling
>> landlock_restrict_self(). Something like this:
>> struct landlock_fd_attr {
>>      __u64 allowed_access;
>>      __u32 first;
>>      __u32 last;
>> }
> 
> FYI, regarding the idea of dropping rights on already-opened files:
> I'm starting to have doubts about how feasible this is in practice.
> 
> The "obvious" approach is to just remove the access rights from the security
> blob flags on the struct file.
> 
> But these opened "struct file"s might be shared with other processes already,
> and mutating them in place would have undesired side effects on other processes.
> 
> For example, if brltty uses ioctls on the terminal and then one of the programs
> running in that terminal drops ioctl rights on that open file, it would affect
> brltty as well, because both the Landlocked program and brltty use the same
> struct file.
> 
> It could be technically stored next to the file descriptor list, where the
> close-on-exec flag is also stored, but that seems more cumbersome than I had
> hoped.  I don't have a good approach for that idea yet, so I'll drop it for now.

Indeed, as discussed in another thread (patch v9 network support), I now 
think that file descriptors should not be touched nor restricted by 
Landlock. Even if there are file *descriptions* and file descriptors, 
Landlock should focus on what user space cannot already do (i.e. close 
file descriptors). Already acquired file descriptors should be a concern 
for user space sandboxers and the whole system/services.

> 
> Ideas are welcome. :)
> 
> —Günther
>