[PATCH v5 0/4] landlock: truncate support

[PATCH v5 0/4] landlock: truncate support

[Günther Noack]

The goal of these patches is to work towards a more complete coverage
of file system operations that are restrictable with Landlock.

The known set of currently unsupported file system operations in
Landlock is described at [1]. Out of the operations listed there,
truncate is the only one that modifies file contents, so these patches
should make it possible to prevent the direct modification of file
contents with Landlock.

The patch introduces the truncation restriction feature as an
additional bit in the access_mask_t bitmap, in line with the existing
supported operations.

The truncation flag covers both the truncate(2) and ftruncate(2)
families of syscalls, as well as open(2) with the O_TRUNC flag.
This includes usages of creat() in the case where existing regular
files are overwritten.

Apart from Landlock, file truncation can also be restricted using
seccomp-bpf, but it is more difficult to use (requires BPF, requires
keeping up-to-date syscall lists) and it is not configurable by file
hierarchy, as Landlock is. The simplicity and flexibility of the
Landlock approach makes it worthwhile adding.

While it's possible to use the "write file" and "truncate" rights
independent of each other, it simplifies the mental model for
userspace callers to always use them together.

Specifically, the following behaviours might be surprising for users
when using these independently:

 * The commonly creat() syscall requires the truncate right when
   overwriting existing files, as it is equivalent to open(2) with
   O_TRUNC|O_CREAT|O_WRONLY.
 * The "write file" right is not always required to truncate a file,
   even through the open(2) syscall (when using O_RDONLY|O_TRUNC).

Nevertheless, keeping the two flags separate is the correct approach
to guarantee backwards compatibility for existing Landlock users.

These patches are based on version 6.0-rc1.

Best regards,
Günther

[1] https://docs.kernel.org/userspace-api/landlock.html#filesystem-flags

Past discussions:
V1: https://lore.kernel.org/all/20220707200612.132705-1-gnoack3000@gmail.com/
V2: https://lore.kernel.org/all/20220712211405.14705-1-gnoack3000@gmail.com/
V3: https://lore.kernel.org/all/20220804193746.9161-1-gnoack3000@gmail.com/
V4: https://lore.kernel.org/all/20220814192603.7387-1-gnoack3000@gmail.com/

Changelog:

V5:
* Documentation
  * Fix wording in userspace-api headers and in landlock.rst.
  * Move the truncation limitation section one to the bottom.
  * Move all .rst changes into the documentation commit.
* selftests
  * Remove _metadata argument from helpers where it became unnecessary.
  * Open writable file descriptors at the top of both tests, before Landlock
    is enabled, to exercise ftruncate() independently from open().
  * Simplify test_ftruncate and decouple it from exercising open().
  * test_creat(): Return errno on close() failure (it does not conflict).
  * Fix /* comment style */
  * Reorder blocks of EXPECT_EQ checks to be consistent within a test.
  * Add missing |O_TRUNC to a check in one test.
  * Put the truncate_unhandled test before the other.

V4:
 * Documentation
   * Clarify wording and syntax as discussed in review.
   * Use a less confusing error message in the example.
 * selftests:
   * Stop using ASSERT_EQ in test helpers, return EBADFD instead.
     (This is an intentionally uncommon error code, so that the source
     of the error is clear and the test can distinguish test setup
     failures from failures in the actual system call under test.)
 * samples/Documentation:
   * Use additional clarifying comments in the kernel backwards
     compatibility logic.

V3:
 * selftests:
   * Explicitly test ftruncate with readonly file descriptors
     (returns EINVAL).
   * Extract test_ftruncate, test_truncate, test_creat helpers,
     which simplified the previously mixed usage of EXPECT/ASSERT.
   * Test creat() behaviour as part of the big truncation test.
   * Stop testing the truncate64(2) and ftruncate64(2) syscalls.
     This simplifies the tests a bit. The kernel implementations are the
     same as for truncate(2) and ftruncate(2), so there is little benefit
     from testing them exhaustively. (We aren't testing all open(2)
     variants either.)
 * samples/landlock/sandboxer.c:
   * Use switch() to implement best effort mode.
 * Documentation:
   * Give more background on surprising truncation behaviour.
   * Use switch() in the example too, to stay in-line with the sample tool.
   * Small fixes in header file to address previous comments.
* misc:
  * Fix some typos and const usages.

V2:
 * Documentation: Mention the truncation flag where needed.
 * Documentation: Point out connection between truncation and file writing.
 * samples: Add file truncation to the landlock/sandboxer.c sample tool.
 * selftests: Exercise open(2) with O_TRUNC and creat(2) exhaustively.
 * selftests: Exercise truncation syscalls when the truncate right
   is not handled by Landlock.

Günther Noack (4):
  landlock: Support file truncation
  selftests/landlock: Selftests for file truncation support
  samples/landlock: Extend sample tool to support
    LANDLOCK_ACCESS_FS_TRUNCATE
  landlock: Document Landlock's file truncation support

 Documentation/userspace-api/landlock.rst     |  52 +++-
 include/uapi/linux/landlock.h                |  17 +-
 samples/landlock/sandboxer.c                 |  23 +-
 security/landlock/fs.c                       |   9 +-
 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   | 257 ++++++++++++++++++-
 8 files changed, 336 insertions(+), 28 deletions(-)


base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
--
2.37.2

[PATCH v5 1/4] landlock: Support file truncation

[Günther Noack]

Introduce the LANDLOCK_ACCESS_FS_TRUNCATE flag for file truncation.

This flag hooks into the path_truncate LSM hook and covers file
truncation using truncate(2), ftruncate(2), open(2) with O_TRUNC, as
well as creat().

This change also increments the Landlock ABI version, updates
corresponding selftests, and updates code documentation to document
the flag.

Signed-off-by: Günther Noack <gnoack3000@gmail.com>
---
 include/uapi/linux/landlock.h                | 17 ++++++++++++-----
 security/landlock/fs.c                       |  9 ++++++++-
 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   |  7 ++++---
 6 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 23df4e0e8ace..50c35404c4d3 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -95,8 +95,15 @@ struct landlock_path_beneath_attr {
  * A file can only receive these access rights:
  *
  * - %LANDLOCK_ACCESS_FS_EXECUTE: Execute a file.
- * - %LANDLOCK_ACCESS_FS_WRITE_FILE: Open a file with write access.
+ * - %LANDLOCK_ACCESS_FS_WRITE_FILE: Open a file with write access. Note that
+ *   you might additionally need the `LANDLOCK_ACCESS_FS_TRUNCATE` right in
+ *   order to overwrite files with :manpage:`open(2)` using `O_TRUNC` or
+ *   :manpage:`creat(2)`.
  * - %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:`open(2)` with `O_TRUNC` or
+ *   :manpage:`creat(2)`. This access right is available since the third version
+ *   of the Landlock ABI.
  *
  * A directory can receive access rights related to files or directories.  The
  * following access right is applied to the directory itself, and the
@@ -139,10 +146,9 @@ struct landlock_path_beneath_attr {
  *
  *   It is currently not possible to restrict some file-related actions
  *   accessible through these syscall families: :manpage:`chdir(2)`,
- *   :manpage:`truncate(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:`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)`.
  *   Future Landlock evolutions will enable to restrict them.
  */
 /* clang-format off */
@@ -160,6 +166,7 @@ struct landlock_path_beneath_attr {
 #define LANDLOCK_ACCESS_FS_MAKE_BLOCK			(1ULL << 11)
 #define LANDLOCK_ACCESS_FS_MAKE_SYM			(1ULL << 12)
 #define LANDLOCK_ACCESS_FS_REFER			(1ULL << 13)
+#define LANDLOCK_ACCESS_FS_TRUNCATE			(1ULL << 14)
 /* clang-format on */
 
 #endif /* _UAPI_LINUX_LANDLOCK_H */
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index ec5a6247cd3e..c57f581a9cd5 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -146,7 +146,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
 #define ACCESS_FILE ( \
 	LANDLOCK_ACCESS_FS_EXECUTE | \
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
-	LANDLOCK_ACCESS_FS_READ_FILE)
+	LANDLOCK_ACCESS_FS_READ_FILE | \
+	LANDLOCK_ACCESS_FS_TRUNCATE)
 /* clang-format on */
 
 /*
@@ -1140,6 +1141,11 @@ static int hook_path_rmdir(const struct path *const dir,
 	return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_DIR);
 }
 
+static int hook_path_truncate(const struct path *const path)
+{
+	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
+}
+
 /* File hooks */
 
 static inline access_mask_t get_file_access(const struct file *const file)
@@ -1192,6 +1198,7 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(path_symlink, hook_path_symlink),
 	LSM_HOOK_INIT(path_unlink, hook_path_unlink),
 	LSM_HOOK_INIT(path_rmdir, hook_path_rmdir),
+	LSM_HOOK_INIT(path_truncate, hook_path_truncate),
 
 	LSM_HOOK_INIT(file_open, hook_file_open),
 };
diff --git a/security/landlock/limits.h b/security/landlock/limits.h
index b54184ab9439..82288f0e9e5e 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_REFER
+#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_TRUNCATE
 #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/security/landlock/syscalls.c b/security/landlock/syscalls.c
index 735a0865ea11..f4d6fc7ed17f 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 2
+#define LANDLOCK_ABI_VERSION 3
 
 /**
  * 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 da9290817866..72cdae277b02 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(2, landlock_create_ruleset(NULL, 0,
+	ASSERT_EQ(3, landlock_create_ruleset(NULL, 0,
 					     LANDLOCK_CREATE_RULESET_VERSION));
 
 	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 21a2ce8fa739..cb77eaa01c91 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -399,9 +399,10 @@ TEST_F_FORK(layout1, inval)
 #define ACCESS_FILE ( \
 	LANDLOCK_ACCESS_FS_EXECUTE | \
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
-	LANDLOCK_ACCESS_FS_READ_FILE)
+	LANDLOCK_ACCESS_FS_READ_FILE | \
+	LANDLOCK_ACCESS_FS_TRUNCATE)
 
-#define ACCESS_LAST LANDLOCK_ACCESS_FS_REFER
+#define ACCESS_LAST LANDLOCK_ACCESS_FS_TRUNCATE
 
 #define ACCESS_ALL ( \
 	ACCESS_FILE | \
@@ -415,7 +416,7 @@ TEST_F_FORK(layout1, inval)
 	LANDLOCK_ACCESS_FS_MAKE_FIFO | \
 	LANDLOCK_ACCESS_FS_MAKE_BLOCK | \
 	LANDLOCK_ACCESS_FS_MAKE_SYM | \
-	ACCESS_LAST)
+	LANDLOCK_ACCESS_FS_REFER)
 
 /* clang-format on */
 
-- 
2.37.2

[PATCH v5 2/4] selftests/landlock: Selftests for file truncation support

[Günther Noack]

These tests exercise the following truncation operations:

* truncate() (truncate by path)
* ftruncate() (truncate by file descriptor)
* open with the O_TRUNC flag
* special case: creat(), which is open with O_CREAT|O_WRONLY|O_TRUNC.

in the following scenarios:

* Files with read, write and truncate rights.
* Files with read and truncate rights.
* Files with the truncate right.
* Files without the truncate right.

In particular, the following scenarios are enforced with the test:

* ftruncate() requires the truncate right,
  even when the thread already has the right to write to the file.
* open() with O_TRUNC requires the truncate right, if it truncates a file.
  open() already checks security_path_truncate() in this case,
  and it required no additional check in the Landlock LSM's file_open hook.
* creat() requires the truncate right
  when called with an existing filename.
* creat() does *not* require the truncate right
  when it's creating a new file.

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

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index cb77eaa01c91..010d4c59139e 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -58,6 +58,7 @@ static const char file1_s2d3[] = TMP_DIR "/s2d1/s2d2/s2d3/f1";
 static const char file2_s2d3[] = TMP_DIR "/s2d1/s2d2/s2d3/f2";
 
 static const char dir_s3d1[] = TMP_DIR "/s3d1";
+static const char file1_s3d1[] = TMP_DIR "/s3d1/f1";
 /* dir_s3d2 is a mount point. */
 static const char dir_s3d2[] = TMP_DIR "/s3d1/s3d2";
 static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3";
@@ -83,6 +84,7 @@ static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3";
  * │           ├── f1
  * │           └── f2
  * └── s3d1
+ *     ├── f1
  *     └── s3d2
  *         └── s3d3
  */
@@ -208,6 +210,7 @@ static void create_layout1(struct __test_metadata *const _metadata)
 	create_file(_metadata, file1_s2d3);
 	create_file(_metadata, file2_s2d3);
 
+	create_file(_metadata, file1_s3d1);
 	create_directory(_metadata, dir_s3d2);
 	set_cap(_metadata, CAP_SYS_ADMIN);
 	ASSERT_EQ(0, mount("tmp", dir_s3d2, "tmpfs", 0, "size=4m,mode=700"));
@@ -230,6 +233,7 @@ static void remove_layout1(struct __test_metadata *const _metadata)
 	EXPECT_EQ(0, remove_path(file1_s2d2));
 	EXPECT_EQ(0, remove_path(file1_s2d1));
 
+	EXPECT_EQ(0, remove_path(file1_s3d1));
 	EXPECT_EQ(0, remove_path(dir_s3d3));
 	set_cap(_metadata, CAP_SYS_ADMIN);
 	umount(dir_s3d2);
@@ -3023,6 +3027,252 @@ TEST_F_FORK(layout1, proc_pipe)
 	ASSERT_EQ(0, close(pipe_fds[1]));
 }
 
+/* Invokes truncate(2) and returns its errno or 0. */
+static int test_truncate(const char *const path)
+{
+	if (truncate(path, 10) < 0)
+		return errno;
+	return 0;
+}
+
+/* Invokes ftruncate(2) and returns its errno or 0. */
+static int test_ftruncate(int fd)
+{
+	if (ftruncate(fd, 10) < 0)
+		return errno;
+	return 0;
+}
+
+/*
+ * Invokes creat(2) and returns its errno or 0.
+ * Closes the opened file descriptor on success.
+ */
+static int test_creat(const char *const path, mode_t mode)
+{
+	int fd = creat(path, mode);
+
+	if (fd < 0)
+		return errno;
+
+	/*
+	 * Mixing error codes from close(2) and creat(2) should not lead to any
+	 * (access type) confusion for this test.
+	 */
+	if (close(fd) < 0)
+		return errno;
+	return 0;
+}
+
+/*
+ * Exercises file truncation when it's not restricted,
+ * as it was the case before LANDLOCK_ACCESS_FS_TRUNCATE existed.
+ */
+TEST_F_FORK(layout1, truncate_unhandled)
+{
+	const char *const file_r = file1_s1d1;
+	const char *const file_w = file2_s1d1;
+	const char *const file_none = file1_s1d2;
+	int file_r_fd, file_w_fd, file_none_fd;
+	const struct rule rules[] = {
+		{
+			.path = file_r,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE,
+		},
+		{
+			.path = file_w,
+			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
+		},
+		/* Implicitly: No rights for file_none. */
+		{},
+	};
+
+	const __u64 handled = LANDLOCK_ACCESS_FS_READ_FILE |
+			      LANDLOCK_ACCESS_FS_WRITE_FILE;
+	int ruleset_fd;
+
+	/*
+	 * Open some writable file descriptors before enabling Landlock, so that
+	 * we can test ftruncate() without making open() a prerequisite.
+	 */
+	file_r_fd = open(file_r, O_WRONLY | O_CLOEXEC);
+	ASSERT_LE(0, file_r_fd);
+	file_w_fd = open(file_w, O_WRONLY | O_CLOEXEC);
+	ASSERT_LE(0, file_w_fd);
+	file_none_fd = open(file_none, O_WRONLY | O_CLOEXEC);
+	ASSERT_LE(0, file_none_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 read right: truncate, ftruncate and open with O_TRUNC work,
+	 * unless the file is attempted to be opened for writing.
+	 */
+	EXPECT_EQ(0, test_truncate(file_r));
+	EXPECT_EQ(0, test_ftruncate(file_r_fd));
+	EXPECT_EQ(0, test_open(file_r, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_r, O_WRONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_creat(file_r, 0600));
+
+	/*
+	 * Checks write right: truncate, ftruncate and open with O_TRUNC work,
+	 * unless the file is attempted to be opened for reading.
+	 */
+	EXPECT_EQ(0, test_truncate(file_w));
+	EXPECT_EQ(0, test_ftruncate(file_w_fd));
+	EXPECT_EQ(EACCES, test_open(file_w, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(0, test_open(file_w, O_WRONLY | O_TRUNC));
+	EXPECT_EQ(0, test_creat(file_w, 0600));
+
+	/*
+	 * Checks "no rights" case: truncate and ftruncate work but all open
+	 * attempts fail, including creat.
+	 */
+	EXPECT_EQ(0, test_truncate(file_none));
+	EXPECT_EQ(0, test_ftruncate(file_none_fd));
+	EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_creat(file_none, 0600));
+
+	ASSERT_EQ(0, close(file_r_fd));
+	ASSERT_EQ(0, close(file_w_fd));
+	ASSERT_EQ(0, close(file_none_fd));
+}
+
+TEST_F_FORK(layout1, truncate)
+{
+	const char *const file_rwt = file1_s1d1;
+	const char *const file_rw = file2_s1d1;
+	const char *const file_rt = file1_s1d2;
+	const char *const file_t = file2_s1d2;
+	const char *const file_none = file1_s1d3;
+	const char *const dir_t = dir_s2d1;
+	const char *const file_in_dir_t = file1_s2d1;
+	const char *const dir_w = dir_s3d1;
+	const char *const file_in_dir_w = file1_s3d1;
+	int file_rwt_fd, file_rw_fd, file_rt_fd, file_t_fd, file_none_fd;
+	int file_in_dir_t_fd, file_in_dir_w_fd;
+	const struct rule rules[] = {
+		{
+			.path = file_rwt,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE |
+				  LANDLOCK_ACCESS_FS_WRITE_FILE |
+				  LANDLOCK_ACCESS_FS_TRUNCATE,
+		},
+		{
+			.path = file_rw,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE |
+				  LANDLOCK_ACCESS_FS_WRITE_FILE,
+		},
+		{
+			.path = file_rt,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE |
+				  LANDLOCK_ACCESS_FS_TRUNCATE,
+		},
+		{
+			.path = file_t,
+			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
+		},
+		/* Implicitly: No access rights for file_none. */
+		{
+			.path = dir_t,
+			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
+		},
+		{
+			.path = dir_w,
+			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
+		},
+		{},
+	};
+	const __u64 handled = LANDLOCK_ACCESS_FS_READ_FILE |
+			      LANDLOCK_ACCESS_FS_WRITE_FILE |
+			      LANDLOCK_ACCESS_FS_TRUNCATE;
+	int ruleset_fd;
+
+	/*
+	 * Open some writable file descriptors before enabling Landlock, so that
+	 * we can test ftruncate() without making open() a prerequisite.
+	 */
+	file_rwt_fd = open(file_rwt, O_WRONLY | O_CLOEXEC);
+	ASSERT_LE(0, file_rwt_fd);
+	file_rw_fd = open(file_rw, O_WRONLY | O_CLOEXEC);
+	ASSERT_LE(0, file_rw_fd);
+	file_rt_fd = open(file_rt, O_WRONLY | O_CLOEXEC);
+	ASSERT_LE(0, file_rt_fd);
+	file_t_fd = open(file_t, O_WRONLY | O_CLOEXEC);
+	ASSERT_LE(0, file_t_fd);
+	file_none_fd = open(file_none, O_WRONLY | O_CLOEXEC);
+	ASSERT_LE(0, file_none_fd);
+	file_in_dir_t_fd = open(file_in_dir_t, O_WRONLY | O_CLOEXEC);
+	ASSERT_LE(0, file_in_dir_t_fd);
+	file_in_dir_w_fd = open(file_in_dir_w, O_WRONLY | O_CLOEXEC);
+	ASSERT_LE(0, file_in_dir_w_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 read, write and truncate rights: truncation works. */
+	EXPECT_EQ(0, test_truncate(file_rwt));
+	EXPECT_EQ(0, test_ftruncate(file_rwt_fd));
+	EXPECT_EQ(0, test_open(file_rwt, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(0, test_open(file_rwt, O_WRONLY | O_TRUNC));
+
+	/* Checks read and write rights: no truncate variant works. */
+	EXPECT_EQ(EACCES, test_truncate(file_rw));
+	EXPECT_EQ(EACCES, test_ftruncate(file_rw_fd));
+	EXPECT_EQ(EACCES, test_open(file_rw, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_rw, O_WRONLY | O_TRUNC));
+
+	/*
+	 * Checks read and truncate rights: truncation works.
+	 *
+	 * Note: Files can get truncated using open() even with O_RDONLY.
+	 */
+	EXPECT_EQ(0, test_truncate(file_rt));
+	EXPECT_EQ(0, test_ftruncate(file_rt_fd));
+	EXPECT_EQ(0, test_open(file_rt, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_rt, O_WRONLY | O_TRUNC));
+
+	/* Checks truncate right: truncate works, but can't open file. */
+	EXPECT_EQ(0, test_truncate(file_t));
+	EXPECT_EQ(0, test_ftruncate(file_t_fd));
+	EXPECT_EQ(EACCES, test_open(file_t, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_t, O_WRONLY | O_TRUNC));
+
+	/* Checks "no rights" case: No form of truncation works. */
+	EXPECT_EQ(EACCES, test_truncate(file_none));
+	EXPECT_EQ(EACCES, test_ftruncate(file_none_fd));
+	EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC));
+
+	/*
+	 * Checks truncate right on directory: truncate works on contained
+	 * files.
+	 */
+	EXPECT_EQ(0, test_truncate(file_in_dir_t));
+	EXPECT_EQ(0, test_ftruncate(file_in_dir_t_fd));
+	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_RDONLY | O_TRUNC));
+	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_WRONLY | O_TRUNC));
+
+	/*
+	 * Checks creat in dir_w: This requires the truncate right when
+	 * overwriting an existing file, but does not require it when the file
+	 * is new.
+	 */
+	EXPECT_EQ(EACCES, test_creat(file_in_dir_w, 0600));
+
+	ASSERT_EQ(0, unlink(file_in_dir_w));
+	EXPECT_EQ(0, test_creat(file_in_dir_w, 0600));
+}
+
 /* clang-format off */
 FIXTURE(layout1_bind) {};
 /* clang-format on */
-- 
2.37.2

Re: [PATCH v5 2/4] selftests/landlock: Selftests for file truncation support

[Mickaël Salaün]

On 17/08/2022 22:30, Günther Noack wrote:
> These tests exercise the following truncation operations:
> 
> * truncate() (truncate by path)
> * ftruncate() (truncate by file descriptor)
> * open with the O_TRUNC flag
> * special case: creat(), which is open with O_CREAT|O_WRONLY|O_TRUNC.
> 
> in the following scenarios:
> 
> * Files with read, write and truncate rights.
> * Files with read and truncate rights.
> * Files with the truncate right.
> * Files without the truncate right.
> 
> In particular, the following scenarios are enforced with the test:
> 
> * ftruncate() requires the truncate right,
>    even when the thread already has the right to write to the file.
> * open() with O_TRUNC requires the truncate right, if it truncates a file.
>    open() already checks security_path_truncate() in this case,
>    and it required no additional check in the Landlock LSM's file_open hook.
> * creat() requires the truncate right
>    when called with an existing filename.
> * creat() does *not* require the truncate right
>    when it's creating a new file.
> 
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> ---
>   tools/testing/selftests/landlock/fs_test.c | 250 +++++++++++++++++++++
>   1 file changed, 250 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index cb77eaa01c91..010d4c59139e 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -58,6 +58,7 @@ static const char file1_s2d3[] = TMP_DIR "/s2d1/s2d2/s2d3/f1";
>   static const char file2_s2d3[] = TMP_DIR "/s2d1/s2d2/s2d3/f2";
>   
>   static const char dir_s3d1[] = TMP_DIR "/s3d1";
> +static const char file1_s3d1[] = TMP_DIR "/s3d1/f1";
>   /* dir_s3d2 is a mount point. */
>   static const char dir_s3d2[] = TMP_DIR "/s3d1/s3d2";
>   static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3";
> @@ -83,6 +84,7 @@ static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3";
>    * │           ├── f1
>    * │           └── f2
>    * └── s3d1
> + *     ├── f1
>    *     └── s3d2
>    *         └── s3d3
>    */
> @@ -208,6 +210,7 @@ static void create_layout1(struct __test_metadata *const _metadata)
>   	create_file(_metadata, file1_s2d3);
>   	create_file(_metadata, file2_s2d3);
>   
> +	create_file(_metadata, file1_s3d1);
>   	create_directory(_metadata, dir_s3d2);
>   	set_cap(_metadata, CAP_SYS_ADMIN);
>   	ASSERT_EQ(0, mount("tmp", dir_s3d2, "tmpfs", 0, "size=4m,mode=700"));
> @@ -230,6 +233,7 @@ static void remove_layout1(struct __test_metadata *const _metadata)
>   	EXPECT_EQ(0, remove_path(file1_s2d2));
>   	EXPECT_EQ(0, remove_path(file1_s2d1));
>   
> +	EXPECT_EQ(0, remove_path(file1_s3d1));
>   	EXPECT_EQ(0, remove_path(dir_s3d3));
>   	set_cap(_metadata, CAP_SYS_ADMIN);
>   	umount(dir_s3d2);
> @@ -3023,6 +3027,252 @@ TEST_F_FORK(layout1, proc_pipe)
>   	ASSERT_EQ(0, close(pipe_fds[1]));
>   }
>   
> +/* Invokes truncate(2) and returns its errno or 0. */
> +static int test_truncate(const char *const path)
> +{
> +	if (truncate(path, 10) < 0)
> +		return errno;
> +	return 0;
> +}
> +
> +/* Invokes ftruncate(2) and returns its errno or 0. */
> +static int test_ftruncate(int fd)
> +{
> +	if (ftruncate(fd, 10) < 0)
> +		return errno;
> +	return 0;
> +}
> +
> +/*
> + * Invokes creat(2) and returns its errno or 0.
> + * Closes the opened file descriptor on success.
> + */
> +static int test_creat(const char *const path, mode_t mode)

This "mode" argument is always 0600. If it's OK with you, I hard code 
this mode and push this series to -next with some small cosmetic fixes.


> +{
> +	int fd = creat(path, mode);
> +
> +	if (fd < 0)
> +		return errno;
> +
> +	/*
> +	 * Mixing error codes from close(2) and creat(2) should not lead to any
> +	 * (access type) confusion for this test.
> +	 */
> +	if (close(fd) < 0)
> +		return errno;
> +	return 0;
> +}
> +
> +/*
> + * Exercises file truncation when it's not restricted,
> + * as it was the case before LANDLOCK_ACCESS_FS_TRUNCATE existed.
> + */
> +TEST_F_FORK(layout1, truncate_unhandled)
> +{
> +	const char *const file_r = file1_s1d1;
> +	const char *const file_w = file2_s1d1;
> +	const char *const file_none = file1_s1d2;
> +	int file_r_fd, file_w_fd, file_none_fd;
> +	const struct rule rules[] = {
> +		{
> +			.path = file_r,
> +			.access = LANDLOCK_ACCESS_FS_READ_FILE,
> +		},
> +		{
> +			.path = file_w,
> +			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
> +		},
> +		/* Implicitly: No rights for file_none. */
> +		{},
> +	};
> +
> +	const __u64 handled = LANDLOCK_ACCESS_FS_READ_FILE |
> +			      LANDLOCK_ACCESS_FS_WRITE_FILE;
> +	int ruleset_fd;
> +
> +	/*
> +	 * Open some writable file descriptors before enabling Landlock, so that
> +	 * we can test ftruncate() without making open() a prerequisite.
> +	 */
> +	file_r_fd = open(file_r, O_WRONLY | O_CLOEXEC);
> +	ASSERT_LE(0, file_r_fd);
> +	file_w_fd = open(file_w, O_WRONLY | O_CLOEXEC);
> +	ASSERT_LE(0, file_w_fd);
> +	file_none_fd = open(file_none, O_WRONLY | O_CLOEXEC);
> +	ASSERT_LE(0, file_none_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 read right: truncate, ftruncate and open with O_TRUNC work,
> +	 * unless the file is attempted to be opened for writing.
> +	 */
> +	EXPECT_EQ(0, test_truncate(file_r));
> +	EXPECT_EQ(0, test_ftruncate(file_r_fd));
> +	EXPECT_EQ(0, test_open(file_r, O_RDONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_open(file_r, O_WRONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_creat(file_r, 0600));
> +
> +	/*
> +	 * Checks write right: truncate, ftruncate and open with O_TRUNC work,
> +	 * unless the file is attempted to be opened for reading.
> +	 */
> +	EXPECT_EQ(0, test_truncate(file_w));
> +	EXPECT_EQ(0, test_ftruncate(file_w_fd));
> +	EXPECT_EQ(EACCES, test_open(file_w, O_RDONLY | O_TRUNC));
> +	EXPECT_EQ(0, test_open(file_w, O_WRONLY | O_TRUNC));
> +	EXPECT_EQ(0, test_creat(file_w, 0600));
> +
> +	/*
> +	 * Checks "no rights" case: truncate and ftruncate work but all open
> +	 * attempts fail, including creat.
> +	 */
> +	EXPECT_EQ(0, test_truncate(file_none));
> +	EXPECT_EQ(0, test_ftruncate(file_none_fd));
> +	EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_creat(file_none, 0600));
> +
> +	ASSERT_EQ(0, close(file_r_fd));
> +	ASSERT_EQ(0, close(file_w_fd));
> +	ASSERT_EQ(0, close(file_none_fd));
> +}
> +
> +TEST_F_FORK(layout1, truncate)
> +{
> +	const char *const file_rwt = file1_s1d1;
> +	const char *const file_rw = file2_s1d1;
> +	const char *const file_rt = file1_s1d2;
> +	const char *const file_t = file2_s1d2;
> +	const char *const file_none = file1_s1d3;
> +	const char *const dir_t = dir_s2d1;
> +	const char *const file_in_dir_t = file1_s2d1;
> +	const char *const dir_w = dir_s3d1;
> +	const char *const file_in_dir_w = file1_s3d1;
> +	int file_rwt_fd, file_rw_fd, file_rt_fd, file_t_fd, file_none_fd;
> +	int file_in_dir_t_fd, file_in_dir_w_fd;
> +	const struct rule rules[] = {
> +		{
> +			.path = file_rwt,
> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
> +				  LANDLOCK_ACCESS_FS_WRITE_FILE |
> +				  LANDLOCK_ACCESS_FS_TRUNCATE,
> +		},
> +		{
> +			.path = file_rw,
> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
> +				  LANDLOCK_ACCESS_FS_WRITE_FILE,
> +		},
> +		{
> +			.path = file_rt,
> +			.access = LANDLOCK_ACCESS_FS_READ_FILE |
> +				  LANDLOCK_ACCESS_FS_TRUNCATE,
> +		},
> +		{
> +			.path = file_t,
> +			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
> +		},
> +		/* Implicitly: No access rights for file_none. */
> +		{
> +			.path = dir_t,
> +			.access = LANDLOCK_ACCESS_FS_TRUNCATE,
> +		},
> +		{
> +			.path = dir_w,
> +			.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
> +		},
> +		{},
> +	};
> +	const __u64 handled = LANDLOCK_ACCESS_FS_READ_FILE |
> +			      LANDLOCK_ACCESS_FS_WRITE_FILE |
> +			      LANDLOCK_ACCESS_FS_TRUNCATE;
> +	int ruleset_fd;
> +
> +	/*
> +	 * Open some writable file descriptors before enabling Landlock, so that
> +	 * we can test ftruncate() without making open() a prerequisite.
> +	 */
> +	file_rwt_fd = open(file_rwt, O_WRONLY | O_CLOEXEC);
> +	ASSERT_LE(0, file_rwt_fd);
> +	file_rw_fd = open(file_rw, O_WRONLY | O_CLOEXEC);
> +	ASSERT_LE(0, file_rw_fd);
> +	file_rt_fd = open(file_rt, O_WRONLY | O_CLOEXEC);
> +	ASSERT_LE(0, file_rt_fd);
> +	file_t_fd = open(file_t, O_WRONLY | O_CLOEXEC);
> +	ASSERT_LE(0, file_t_fd);
> +	file_none_fd = open(file_none, O_WRONLY | O_CLOEXEC);
> +	ASSERT_LE(0, file_none_fd);
> +	file_in_dir_t_fd = open(file_in_dir_t, O_WRONLY | O_CLOEXEC);
> +	ASSERT_LE(0, file_in_dir_t_fd);
> +	file_in_dir_w_fd = open(file_in_dir_w, O_WRONLY | O_CLOEXEC);
> +	ASSERT_LE(0, file_in_dir_w_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 read, write and truncate rights: truncation works. */
> +	EXPECT_EQ(0, test_truncate(file_rwt));
> +	EXPECT_EQ(0, test_ftruncate(file_rwt_fd));
> +	EXPECT_EQ(0, test_open(file_rwt, O_RDONLY | O_TRUNC));
> +	EXPECT_EQ(0, test_open(file_rwt, O_WRONLY | O_TRUNC));
> +
> +	/* Checks read and write rights: no truncate variant works. */
> +	EXPECT_EQ(EACCES, test_truncate(file_rw));
> +	EXPECT_EQ(EACCES, test_ftruncate(file_rw_fd));
> +	EXPECT_EQ(EACCES, test_open(file_rw, O_RDONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_open(file_rw, O_WRONLY | O_TRUNC));
> +
> +	/*
> +	 * Checks read and truncate rights: truncation works.
> +	 *
> +	 * Note: Files can get truncated using open() even with O_RDONLY.
> +	 */
> +	EXPECT_EQ(0, test_truncate(file_rt));
> +	EXPECT_EQ(0, test_ftruncate(file_rt_fd));
> +	EXPECT_EQ(0, test_open(file_rt, O_RDONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_open(file_rt, O_WRONLY | O_TRUNC));
> +
> +	/* Checks truncate right: truncate works, but can't open file. */
> +	EXPECT_EQ(0, test_truncate(file_t));
> +	EXPECT_EQ(0, test_ftruncate(file_t_fd));
> +	EXPECT_EQ(EACCES, test_open(file_t, O_RDONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_open(file_t, O_WRONLY | O_TRUNC));
> +
> +	/* Checks "no rights" case: No form of truncation works. */
> +	EXPECT_EQ(EACCES, test_truncate(file_none));
> +	EXPECT_EQ(EACCES, test_ftruncate(file_none_fd));

This test is interesting because it shows that the access control may 
still restrict opened FD (when it makes sense). The truncate access 
right is kind of the first one to be testable this way.


> +	EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC));
> +
> +	/*
> +	 * Checks truncate right on directory: truncate works on contained
> +	 * files.
> +	 */
> +	EXPECT_EQ(0, test_truncate(file_in_dir_t));
> +	EXPECT_EQ(0, test_ftruncate(file_in_dir_t_fd));
> +	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_RDONLY | O_TRUNC));
> +	EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_WRONLY | O_TRUNC));
> +
> +	/*
> +	 * Checks creat in dir_w: This requires the truncate right when
> +	 * overwriting an existing file, but does not require it when the file
> +	 * is new.
> +	 */
> +	EXPECT_EQ(EACCES, test_creat(file_in_dir_w, 0600));
> +
> +	ASSERT_EQ(0, unlink(file_in_dir_w));
> +	EXPECT_EQ(0, test_creat(file_in_dir_w, 0600));
> +}
> +
>   /* clang-format off */
>   FIXTURE(layout1_bind) {};
>   /* clang-format on */

Re: [PATCH v5 2/4] selftests/landlock: Selftests for file truncation support

[Günther Noack]

On Thu, Aug 18, 2022 at 10:39:27PM +0200, Mickaël Salaün wrote:
> On 17/08/2022 22:30, Günther Noack wrote:
> > +/*
> > + * Invokes creat(2) and returns its errno or 0.
> > + * Closes the opened file descriptor on success.
> > + */
> > +static int test_creat(const char *const path, mode_t mode)
>
> This "mode" argument is always 0600. If it's OK with you, I hard code this
> mode and push this series to -next with some small cosmetic fixes.

Yes, absolutely. Please do these fixes and push it to -next. :)

Thanks,
—Günther

--

Re: [PATCH v5 2/4] selftests/landlock: Selftests for file truncation support

[Mickaël Salaün]

Ok, it should be in -next soon. Thanks for your contribution!

Would you like to write a syzkaller test to cover this new access right? 
You only need to update the landlock_fs_accesses file with a call to 
truncate() returning EACCES and check that it covers 
hook_path_truncate(). You can take inspiration from this PR: 
https://github.com/google/syzkaller/pull/3133
Please CC me, I can help.

Regards,
  Mickaël


On 19/08/2022 07:24, Günther Noack wrote:
> On Thu, Aug 18, 2022 at 10:39:27PM +0200, Mickaël Salaün wrote:
>> On 17/08/2022 22:30, Günther Noack wrote:
>>> +/*
>>> + * Invokes creat(2) and returns its errno or 0.
>>> + * Closes the opened file descriptor on success.
>>> + */
>>> +static int test_creat(const char *const path, mode_t mode)
>>
>> This "mode" argument is always 0600. If it's OK with you, I hard code this
>> mode and push this series to -next with some small cosmetic fixes.
> 
> Yes, absolutely. Please do these fixes and push it to -next. :)
> 
> Thanks,
> —Günther
> 
> --

Re: [PATCH v5 2/4] selftests/landlock: Selftests for file truncation support

[Mickaël Salaün]

FYI, my -next branch is here: 
https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/log/?h=next

Günther, let me know if everything is OK.

Konstantin, please rebase your work on it. It should mainly conflict 
with changes related to the Landlock ABI version.


On 19/08/2022 10:15, Mickaël Salaün wrote:
> Ok, it should be in -next soon. Thanks for your contribution!
> 
> Would you like to write a syzkaller test to cover this new access right?
> You only need to update the landlock_fs_accesses file with a call to
> truncate() returning EACCES and check that it covers
> hook_path_truncate(). You can take inspiration from this PR:
> https://github.com/google/syzkaller/pull/3133
> Please CC me, I can help.
> 
> Regards,
>    Mickaël
> 
> 
> On 19/08/2022 07:24, Günther Noack wrote:
>> On Thu, Aug 18, 2022 at 10:39:27PM +0200, Mickaël Salaün wrote:
>>> On 17/08/2022 22:30, Günther Noack wrote:
>>>> +/*
>>>> + * Invokes creat(2) and returns its errno or 0.
>>>> + * Closes the opened file descriptor on success.
>>>> + */
>>>> +static int test_creat(const char *const path, mode_t mode)
>>>
>>> This "mode" argument is always 0600. If it's OK with you, I hard code this
>>> mode and push this series to -next with some small cosmetic fixes.
>>
>> Yes, absolutely. Please do these fixes and push it to -next. :)
>>
>> Thanks,
>> —Günther
>>
>> --

Re: [PATCH v5 2/4] selftests/landlock: Selftests for file truncation support

[Konstantin Meskhidze (A)]

8/19/2022 11:36 AM, Mickaël Salaün пишет:
> FYI, my -next branch is here:
> https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/log/?h=next
> 
> Günther, let me know if everything is OK.
> 
> Konstantin, please rebase your work on it. It should mainly conflict
> with changes related to the Landlock ABI version.

   Ok. I will rebase. Thnaks. Do I need to keep my next versions on your 
-next branch?
> 
> 
> On 19/08/2022 10:15, Mickaël Salaün wrote:
>> Ok, it should be in -next soon. Thanks for your contribution!
>> 
>> Would you like to write a syzkaller test to cover this new access right?
>> You only need to update the landlock_fs_accesses file with a call to
>> truncate() returning EACCES and check that it covers
>> hook_path_truncate(). You can take inspiration from this PR:
>> https://github.com/google/syzkaller/pull/3133
>> Please CC me, I can help.
>> 
>> Regards,
>>    Mickaël
>> 
>> 
>> On 19/08/2022 07:24, Günther Noack wrote:
>>> On Thu, Aug 18, 2022 at 10:39:27PM +0200, Mickaël Salaün wrote:
>>>> On 17/08/2022 22:30, Günther Noack wrote:
>>>>> +/*
>>>>> + * Invokes creat(2) and returns its errno or 0.
>>>>> + * Closes the opened file descriptor on success.
>>>>> + */
>>>>> +static int test_creat(const char *const path, mode_t mode)
>>>>
>>>> This "mode" argument is always 0600. If it's OK with you, I hard code this
>>>> mode and push this series to -next with some small cosmetic fixes.
>>>
>>> Yes, absolutely. Please do these fixes and push it to -next. :)
>>>
>>> Thanks,
>>> —Günther
>>>
>>> --
> .

Re: [PATCH v5 2/4] selftests/landlock: Selftests for file truncation support

[Mickaël Salaün]

On 19/08/2022 10:55, Konstantin Meskhidze (A) wrote:
> 
> 
> 8/19/2022 11:36 AM, Mickaël Salaün пишет:
>> FYI, my -next branch is here:
>> https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/log/?h=next
>>
>> Günther, let me know if everything is OK.
>>
>> Konstantin, please rebase your work on it. It should mainly conflict
>> with changes related to the Landlock ABI version.
> 
>     Ok. I will rebase. Thnaks. Do I need to keep my next versions on your
> -next branch?

Yes please.

Re: [PATCH v5 2/4] selftests/landlock: Selftests for file truncation support

[Günther Noack]

On Fri, Aug 19, 2022 at 10:36:15AM +0200, Mickaël Salaün wrote:
> FYI, my -next branch is here:
> https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/log/?h=next
>
> Günther, let me know if everything is OK.

Thanks, looks good.

Good idea to also fix the date in the documentation, I had overlooked that.


> Konstantin, please rebase your work on it. It should mainly conflict with
> changes related to the Landlock ABI version.

+1, I believe we have both increased the ABI version to 3 in the two patch sets.

--

[PATCH v5 3/4] samples/landlock: Extend sample tool to support LANDLOCK_ACCESS_FS_TRUNCATE

[Günther Noack]

Update the sandboxer sample to restrict truncate actions. This is
automatically enabled by default if the running kernel supports
LANDLOCK_ACCESS_FS_TRUNCATE, expect for the paths listed in the
LL_FS_RW environment variable.

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

diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index 3e404e51ec64..771b6b10d519 100644
--- a/samples/landlock/sandboxer.c
+++ b/samples/landlock/sandboxer.c
@@ -76,7 +76,8 @@ static int parse_path(char *env_path, const char ***const path_list)
 #define ACCESS_FILE ( \
 	LANDLOCK_ACCESS_FS_EXECUTE | \
 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
-	LANDLOCK_ACCESS_FS_READ_FILE)
+	LANDLOCK_ACCESS_FS_READ_FILE | \
+	LANDLOCK_ACCESS_FS_TRUNCATE)
 
 /* clang-format on */
 
@@ -160,10 +161,8 @@ static int populate_ruleset(const char *const env_var, const int ruleset_fd,
 	LANDLOCK_ACCESS_FS_MAKE_FIFO | \
 	LANDLOCK_ACCESS_FS_MAKE_BLOCK | \
 	LANDLOCK_ACCESS_FS_MAKE_SYM | \
-	LANDLOCK_ACCESS_FS_REFER)
-
-#define ACCESS_ABI_2 ( \
-	LANDLOCK_ACCESS_FS_REFER)
+	LANDLOCK_ACCESS_FS_REFER | \
+	LANDLOCK_ACCESS_FS_TRUNCATE)
 
 /* clang-format on */
 
@@ -226,11 +225,17 @@ int main(const int argc, char *const argv[], char *const *const envp)
 		return 1;
 	}
 	/* Best-effort security. */
-	if (abi < 2) {
-		ruleset_attr.handled_access_fs &= ~ACCESS_ABI_2;
-		access_fs_ro &= ~ACCESS_ABI_2;
-		access_fs_rw &= ~ACCESS_ABI_2;
+	switch (abi) {
+	case 1:
+		/* Removes LANDLOCK_ACCESS_FS_REFER for ABI < 2 */
+		ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_REFER;
+		__attribute__((fallthrough));
+	case 2:
+		/* Removes LANDLOCK_ACCESS_FS_TRUNCATE for ABI < 3 */
+		ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
 	}
+	access_fs_ro &= ruleset_attr.handled_access_fs;
+	access_fs_rw &= ruleset_attr.handled_access_fs;
 
 	ruleset_fd =
 		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
-- 
2.37.2

[PATCH v5 4/4] landlock: Document Landlock's file truncation support

[Günther Noack]

Use the LANDLOCK_ACCESS_FS_TRUNCATE flag in the tutorial.

Adapt the backwards compatibility example and discussion to remove the
truncation flag where needed.

Point out potential surprising behaviour related to truncate.

Signed-off-by: Günther Noack <gnoack3000@gmail.com>
---
 Documentation/userspace-api/landlock.rst | 52 ++++++++++++++++++++----
 1 file changed, 45 insertions(+), 7 deletions(-)

diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
index b8ea59493964..5c10872cb795 100644
--- a/Documentation/userspace-api/landlock.rst
+++ b/Documentation/userspace-api/landlock.rst
@@ -60,7 +60,8 @@ the need to be explicit about the denied-by-default access rights.
             LANDLOCK_ACCESS_FS_MAKE_FIFO |
             LANDLOCK_ACCESS_FS_MAKE_BLOCK |
             LANDLOCK_ACCESS_FS_MAKE_SYM |
-            LANDLOCK_ACCESS_FS_REFER,
+            LANDLOCK_ACCESS_FS_REFER |
+            LANDLOCK_ACCESS_FS_TRUNCATE,
     };
 
 Because we may not know on which kernel version an application will be
@@ -69,16 +70,26 @@ should try to protect users as much as possible whatever the kernel they are
 using.  To avoid binary enforcement (i.e. either all security features or
 none), we can leverage a dedicated Landlock command to get the current version
 of the Landlock ABI and adapt the handled accesses.  Let's check if we should
-remove the `LANDLOCK_ACCESS_FS_REFER` access right which is only supported
-starting with the second version of the ABI.
+remove the `LANDLOCK_ACCESS_FS_REFER` or `LANDLOCK_ACCESS_FS_TRUNCATE` access
+rights, which are only supported starting with the second and third version of
+the ABI.
 
 .. code-block:: c
 
     int abi;
 
     abi = landlock_create_ruleset(NULL, 0, LANDLOCK_CREATE_RULESET_VERSION);
-    if (abi < 2) {
-        ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_REFER;
+    switch (abi) {
+    case -1:
+            perror("The running kernel does not enable to use Landlock");
+            return 1;
+    case 1:
+            /* Removes LANDLOCK_ACCESS_FS_REFER for ABI < 2 */
+            ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_REFER;
+            __attribute__((fallthrough));
+    case 2:
+            /* Removes LANDLOCK_ACCESS_FS_TRUNCATE for ABI < 3 */
+            ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
     }
 
 This enables to create an inclusive ruleset that will contain our rules.
@@ -127,8 +138,8 @@ descriptor.
 
 It may also be required to create rules following the same logic as explained
 for the ruleset creation, by filtering access rights according to the Landlock
-ABI version.  In this example, this is not required because
-`LANDLOCK_ACCESS_FS_REFER` is not allowed by any rule.
+ABI version.  In this example, this is not required because all of the requested
+``allowed_access`` rights are already available in ABI 1.
 
 We now have a ruleset with one rule allowing read access to ``/usr`` while
 denying all other handled accesses for the filesystem.  The next step is to
@@ -251,6 +262,24 @@ To be allowed to use :manpage:`ptrace(2)` and related syscalls on a target
 process, a sandboxed process should have a subset of the target process rules,
 which means the tracee must be in a sub-domain of the tracer.
 
+Truncating files
+----------------
+
+The operations covered by `LANDLOCK_ACCESS_FS_WRITE_FILE` and
+`LANDLOCK_ACCESS_FS_TRUNCATE` both change the contents of a file and sometimes
+overlap in non-intuitive ways.  It is recommended to always specify both of
+these together.
+
+A particularly surprising example is :manpage:`creat(2)`.  The name suggests
+that this system call requires the rights to create and write files.  However,
+it also requires the truncate right if an existing file under the same name is
+already present.
+
+It should also be noted that truncating files does not require the
+`LANDLOCK_ACCESS_FS_WRITE_FILE` right.  Apart from the :manpage:`truncate(2)`
+system call, this can also be done through :manpage:`open(2)` with the flags
+`O_RDONLY | O_TRUNC`.
+
 Compatibility
 =============
 
@@ -397,6 +426,15 @@ Starting with the Landlock ABI version 2, it is now possible to securely
 control renaming and linking thanks to the new `LANDLOCK_ACCESS_FS_REFER`
 access right.
 
+File truncation (ABI < 3)
+-------------------------
+
+File truncation could not be denied before the third Landlock ABI, so it is
+always allowed when using a kernel that only supports the first or second ABI.
+
+Starting with the Landlock ABI version 3, it is now possible to securely control
+truncation thanks to the new `LANDLOCK_ACCESS_FS_TRUNCATE` access right.
+
 .. _kernel_support:
 
 Kernel support
-- 
2.37.2

Re: [PATCH v5 0/4] landlock: truncate support

[Mickaël Salaün]

Hmm, I think there is an issue with this series. Landlock only enforces 
restrictions at open time or when dealing with user-supplied file paths 
(relative or absolute). The use of the path_truncate hook in this series 
doesn't distinguish between file descriptor from before the current 
sandbox or from after being sandboxed. For instance, if a file 
descriptor is received through a unix socket, it is assumed that this is 
legitimate and no Landlock restriction apply on it, which is not the 
case with this series anymore. It is the same for files opened before 
the process sandbox itself.

To be able to follow the current semantic, I think we should control the 
truncate access at open time (or when dealing with a user-supplied path) 
but not on any file descriptor as it is currently done.


On 17/08/2022 22:30, Günther Noack wrote:
> The goal of these patches is to work towards a more complete coverage
> of file system operations that are restrictable with Landlock.
> 
> The known set of currently unsupported file system operations in
> Landlock is described at [1]. Out of the operations listed there,
> truncate is the only one that modifies file contents, so these patches
> should make it possible to prevent the direct modification of file
> contents with Landlock.
> 
> The patch introduces the truncation restriction feature as an
> additional bit in the access_mask_t bitmap, in line with the existing
> supported operations.
> 
> The truncation flag covers both the truncate(2) and ftruncate(2)
> families of syscalls, as well as open(2) with the O_TRUNC flag.
> This includes usages of creat() in the case where existing regular
> files are overwritten.
> 
> Apart from Landlock, file truncation can also be restricted using
> seccomp-bpf, but it is more difficult to use (requires BPF, requires
> keeping up-to-date syscall lists) and it is not configurable by file
> hierarchy, as Landlock is. The simplicity and flexibility of the
> Landlock approach makes it worthwhile adding.
> 
> While it's possible to use the "write file" and "truncate" rights
> independent of each other, it simplifies the mental model for
> userspace callers to always use them together.
> 
> Specifically, the following behaviours might be surprising for users
> when using these independently:
> 
>   * The commonly creat() syscall requires the truncate right when
>     overwriting existing files, as it is equivalent to open(2) with
>     O_TRUNC|O_CREAT|O_WRONLY.
>   * The "write file" right is not always required to truncate a file,
>     even through the open(2) syscall (when using O_RDONLY|O_TRUNC).
> 
> Nevertheless, keeping the two flags separate is the correct approach
> to guarantee backwards compatibility for existing Landlock users.
> 
> These patches are based on version 6.0-rc1.
> 
> Best regards,
> Günther
> 
> [1] https://docs.kernel.org/userspace-api/landlock.html#filesystem-flags
> 
> Past discussions:
> V1: https://lore.kernel.org/all/20220707200612.132705-1-gnoack3000@gmail.com/
> V2: https://lore.kernel.org/all/20220712211405.14705-1-gnoack3000@gmail.com/
> V3: https://lore.kernel.org/all/20220804193746.9161-1-gnoack3000@gmail.com/
> V4: https://lore.kernel.org/all/20220814192603.7387-1-gnoack3000@gmail.com/
> 
> Changelog:
> 
> V5:
> * Documentation
>    * Fix wording in userspace-api headers and in landlock.rst.
>    * Move the truncation limitation section one to the bottom.
>    * Move all .rst changes into the documentation commit.
> * selftests
>    * Remove _metadata argument from helpers where it became unnecessary.
>    * Open writable file descriptors at the top of both tests, before Landlock
>      is enabled, to exercise ftruncate() independently from open().
>    * Simplify test_ftruncate and decouple it from exercising open().
>    * test_creat(): Return errno on close() failure (it does not conflict).
>    * Fix /* comment style */
>    * Reorder blocks of EXPECT_EQ checks to be consistent within a test.
>    * Add missing |O_TRUNC to a check in one test.
>    * Put the truncate_unhandled test before the other.
> 
> V4:
>   * Documentation
>     * Clarify wording and syntax as discussed in review.
>     * Use a less confusing error message in the example.
>   * selftests:
>     * Stop using ASSERT_EQ in test helpers, return EBADFD instead.
>       (This is an intentionally uncommon error code, so that the source
>       of the error is clear and the test can distinguish test setup
>       failures from failures in the actual system call under test.)
>   * samples/Documentation:
>     * Use additional clarifying comments in the kernel backwards
>       compatibility logic.
> 
> V3:
>   * selftests:
>     * Explicitly test ftruncate with readonly file descriptors
>       (returns EINVAL).
>     * Extract test_ftruncate, test_truncate, test_creat helpers,
>       which simplified the previously mixed usage of EXPECT/ASSERT.
>     * Test creat() behaviour as part of the big truncation test.
>     * Stop testing the truncate64(2) and ftruncate64(2) syscalls.
>       This simplifies the tests a bit. The kernel implementations are the
>       same as for truncate(2) and ftruncate(2), so there is little benefit
>       from testing them exhaustively. (We aren't testing all open(2)
>       variants either.)
>   * samples/landlock/sandboxer.c:
>     * Use switch() to implement best effort mode.
>   * Documentation:
>     * Give more background on surprising truncation behaviour.
>     * Use switch() in the example too, to stay in-line with the sample tool.
>     * Small fixes in header file to address previous comments.
> * misc:
>    * Fix some typos and const usages.
> 
> V2:
>   * Documentation: Mention the truncation flag where needed.
>   * Documentation: Point out connection between truncation and file writing.
>   * samples: Add file truncation to the landlock/sandboxer.c sample tool.
>   * selftests: Exercise open(2) with O_TRUNC and creat(2) exhaustively.
>   * selftests: Exercise truncation syscalls when the truncate right
>     is not handled by Landlock.
> 
> Günther Noack (4):
>    landlock: Support file truncation
>    selftests/landlock: Selftests for file truncation support
>    samples/landlock: Extend sample tool to support
>      LANDLOCK_ACCESS_FS_TRUNCATE
>    landlock: Document Landlock's file truncation support
> 
>   Documentation/userspace-api/landlock.rst     |  52 +++-
>   include/uapi/linux/landlock.h                |  17 +-
>   samples/landlock/sandboxer.c                 |  23 +-
>   security/landlock/fs.c                       |   9 +-
>   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   | 257 ++++++++++++++++++-
>   8 files changed, 336 insertions(+), 28 deletions(-)
> 
> 
> base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
> --
> 2.37.2

Re: [PATCH v5 0/4] landlock: truncate support

[Günther Noack]

On Thu, Sep 01, 2022 at 07:10:38PM +0200, Mickaël Salaün wrote:
> Hmm, I think there is an issue with this series. Landlock only enforces
> restrictions at open time or when dealing with user-supplied file paths
> (relative or absolute).

Argh, ok. That sounds like a desirable property, although it would
mean reworking the patch set.

> The use of the path_truncate hook in this series
> doesn't distinguish between file descriptor from before the current sandbox
> or from after being sandboxed. For instance, if a file descriptor is
> received through a unix socket, it is assumed that this is legitimate and no
> Landlock restriction apply on it, which is not the case with this series
> anymore. It is the same for files opened before the process sandbox itself.
>
> To be able to follow the current semantic, I think we should control the
> truncate access at open time (or when dealing with a user-supplied path) but
> not on any file descriptor as it is currently done.

OK - so let me try to make a constructive proposal. We have previously
identified a few operations where a truncation happens, and I would
propose that the following Landlock rights should be needed for these:

* truncate() (operating on char *path): Require LL_ACCESS_FS_TRUNCATE
* ftruncate() (operating on fd): No Landlock rights required
* open() for reading with O_TRUNC: Require LL_ACCESS_FS_TRUNCATE
* open() for writing with O_TRUNC: Require LL_ACCESS_FS_WRITE_FILE

The rationale goes as follows:

* ftruncate() is already adequately protected by the
  LL_ACCESS_FS_WRITE_FILE right. ftruncate is only permitted on fds
  that are open for writing.
* truncate() is not Landlock-restrictable in Landlock ABI v1,
  so needs to be covered by the new truncate right.
* open() for reading with O_TRUNC is also not Landlock-restrictable in
  Landlock ABI v1, so needs to be covered by the new truncate right.
* open() for writing with O_TRUNC is also not Landlock-restrictable in
  Landlock ABI v1. BUT: A caller who can open the file for writing
  will also be able to ftruncate it - so it doesn't really make sense
  to ask for a different Landlock right here.

Does that approach make sense to you?

I think in terms of changs required for it, it sounds like it would
require a change to the path_truncate LSM hook to distinguish the
cases above.

Do you want a new patch on top of the existing one, or should I rather
create a new version of the old truncate patch set?

--Günther

> On 17/08/2022 22:30, Günther Noack wrote:
> > The goal of these patches is to work towards a more complete coverage
> > of file system operations that are restrictable with Landlock.
> >
> > The known set of currently unsupported file system operations in
> > Landlock is described at [1]. Out of the operations listed there,
> > truncate is the only one that modifies file contents, so these patches
> > should make it possible to prevent the direct modification of file
> > contents with Landlock.
> >
> > The patch introduces the truncation restriction feature as an
> > additional bit in the access_mask_t bitmap, in line with the existing
> > supported operations.
> >
> > The truncation flag covers both the truncate(2) and ftruncate(2)
> > families of syscalls, as well as open(2) with the O_TRUNC flag.
> > This includes usages of creat() in the case where existing regular
> > files are overwritten.
> >
> > Apart from Landlock, file truncation can also be restricted using
> > seccomp-bpf, but it is more difficult to use (requires BPF, requires
> > keeping up-to-date syscall lists) and it is not configurable by file
> > hierarchy, as Landlock is. The simplicity and flexibility of the
> > Landlock approach makes it worthwhile adding.
> >
> > While it's possible to use the "write file" and "truncate" rights
> > independent of each other, it simplifies the mental model for
> > userspace callers to always use them together.
> >
> > Specifically, the following behaviours might be surprising for users
> > when using these independently:
> >
> >   * The commonly creat() syscall requires the truncate right when
> >     overwriting existing files, as it is equivalent to open(2) with
> >     O_TRUNC|O_CREAT|O_WRONLY.
> >   * The "write file" right is not always required to truncate a file,
> >     even through the open(2) syscall (when using O_RDONLY|O_TRUNC).
> >
> > Nevertheless, keeping the two flags separate is the correct approach
> > to guarantee backwards compatibility for existing Landlock users.
> >
> > These patches are based on version 6.0-rc1.
> >
> > Best regards,
> > Günther
> >
> > [1] https://docs.kernel.org/userspace-api/landlock.html#filesystem-flags
> >
> > Past discussions:
> > V1: https://lore.kernel.org/all/20220707200612.132705-1-gnoack3000@gmail.com/
> > V2: https://lore.kernel.org/all/20220712211405.14705-1-gnoack3000@gmail.com/
> > V3: https://lore.kernel.org/all/20220804193746.9161-1-gnoack3000@gmail.com/
> > V4: https://lore.kernel.org/all/20220814192603.7387-1-gnoack3000@gmail.com/
> >
> > Changelog:
> >
> > V5:
> > * Documentation
> >    * Fix wording in userspace-api headers and in landlock.rst.
> >    * Move the truncation limitation section one to the bottom.
> >    * Move all .rst changes into the documentation commit.
> > * selftests
> >    * Remove _metadata argument from helpers where it became unnecessary.
> >    * Open writable file descriptors at the top of both tests, before Landlock
> >      is enabled, to exercise ftruncate() independently from open().
> >    * Simplify test_ftruncate and decouple it from exercising open().
> >    * test_creat(): Return errno on close() failure (it does not conflict).
> >    * Fix /* comment style */
> >    * Reorder blocks of EXPECT_EQ checks to be consistent within a test.
> >    * Add missing |O_TRUNC to a check in one test.
> >    * Put the truncate_unhandled test before the other.
> >
> > V4:
> >   * Documentation
> >     * Clarify wording and syntax as discussed in review.
> >     * Use a less confusing error message in the example.
> >   * selftests:
> >     * Stop using ASSERT_EQ in test helpers, return EBADFD instead.
> >       (This is an intentionally uncommon error code, so that the source
> >       of the error is clear and the test can distinguish test setup
> >       failures from failures in the actual system call under test.)
> >   * samples/Documentation:
> >     * Use additional clarifying comments in the kernel backwards
> >       compatibility logic.
> >
> > V3:
> >   * selftests:
> >     * Explicitly test ftruncate with readonly file descriptors
> >       (returns EINVAL).
> >     * Extract test_ftruncate, test_truncate, test_creat helpers,
> >       which simplified the previously mixed usage of EXPECT/ASSERT.
> >     * Test creat() behaviour as part of the big truncation test.
> >     * Stop testing the truncate64(2) and ftruncate64(2) syscalls.
> >       This simplifies the tests a bit. The kernel implementations are the
> >       same as for truncate(2) and ftruncate(2), so there is little benefit
> >       from testing them exhaustively. (We aren't testing all open(2)
> >       variants either.)
> >   * samples/landlock/sandboxer.c:
> >     * Use switch() to implement best effort mode.
> >   * Documentation:
> >     * Give more background on surprising truncation behaviour.
> >     * Use switch() in the example too, to stay in-line with the sample tool.
> >     * Small fixes in header file to address previous comments.
> > * misc:
> >    * Fix some typos and const usages.
> >
> > V2:
> >   * Documentation: Mention the truncation flag where needed.
> >   * Documentation: Point out connection between truncation and file writing.
> >   * samples: Add file truncation to the landlock/sandboxer.c sample tool.
> >   * selftests: Exercise open(2) with O_TRUNC and creat(2) exhaustively.
> >   * selftests: Exercise truncation syscalls when the truncate right
> >     is not handled by Landlock.
> >
> > Günther Noack (4):
> >    landlock: Support file truncation
> >    selftests/landlock: Selftests for file truncation support
> >    samples/landlock: Extend sample tool to support
> >      LANDLOCK_ACCESS_FS_TRUNCATE
> >    landlock: Document Landlock's file truncation support
> >
> >   Documentation/userspace-api/landlock.rst     |  52 +++-
> >   include/uapi/linux/landlock.h                |  17 +-
> >   samples/landlock/sandboxer.c                 |  23 +-
> >   security/landlock/fs.c                       |   9 +-
> >   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   | 257 ++++++++++++++++++-
> >   8 files changed, 336 insertions(+), 28 deletions(-)
> >
> >
> > base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
> > --
> > 2.37.2

--

Re: [PATCH v5 0/4] landlock: truncate support

[Günther Noack]

On Fri, Sep 02, 2022 at 07:32:49AM +0200, Günther Noack wrote:
> On Thu, Sep 01, 2022 at 07:10:38PM +0200, Mickaël Salaün wrote:
> > Hmm, I think there is an issue with this series. Landlock only enforces
> > restrictions at open time or when dealing with user-supplied file paths
> > (relative or absolute).
>
> Argh, ok. That sounds like a desirable property, although it would
> mean reworking the patch set.
>
> > The use of the path_truncate hook in this series
> > doesn't distinguish between file descriptor from before the current sandbox
> > or from after being sandboxed. For instance, if a file descriptor is
> > received through a unix socket, it is assumed that this is legitimate and no
> > Landlock restriction apply on it, which is not the case with this series
> > anymore. It is the same for files opened before the process sandbox itself.
> >
> > To be able to follow the current semantic, I think we should control the
> > truncate access at open time (or when dealing with a user-supplied path) but
> > not on any file descriptor as it is currently done.
>
> OK - so let me try to make a constructive proposal. We have previously
> identified a few operations where a truncation happens, and I would
> propose that the following Landlock rights should be needed for these:
>
> * truncate() (operating on char *path): Require LL_ACCESS_FS_TRUNCATE
> * ftruncate() (operating on fd): No Landlock rights required
> * open() for reading with O_TRUNC: Require LL_ACCESS_FS_TRUNCATE
> * open() for writing with O_TRUNC: Require LL_ACCESS_FS_WRITE_FILE

Thinking about it again, another alternative would be to require
TRUNCATE as well when opening a file for writing - it would be
logical, because the resulting FD can be truncated. It would also
require people to provide the truncate right in order to open files
for writing, but this may be the logical thing to do.

Let me know what you think!

—Günther

>
> The rationale goes as follows:
>
> * ftruncate() is already adequately protected by the
>   LL_ACCESS_FS_WRITE_FILE right. ftruncate is only permitted on fds
>   that are open for writing.
> * truncate() is not Landlock-restrictable in Landlock ABI v1,
>   so needs to be covered by the new truncate right.
> * open() for reading with O_TRUNC is also not Landlock-restrictable in
>   Landlock ABI v1, so needs to be covered by the new truncate right.
> * open() for writing with O_TRUNC is also not Landlock-restrictable in
>   Landlock ABI v1. BUT: A caller who can open the file for writing
>   will also be able to ftruncate it - so it doesn't really make sense
>   to ask for a different Landlock right here.
>
> Does that approach make sense to you?
>
> I think in terms of changs required for it, it sounds like it would
> require a change to the path_truncate LSM hook to distinguish the
> cases above.
>
> Do you want a new patch on top of the existing one, or should I rather
> create a new version of the old truncate patch set?
>
> --Günther
>
> > On 17/08/2022 22:30, Günther Noack wrote:
> > > The goal of these patches is to work towards a more complete coverage
> > > of file system operations that are restrictable with Landlock.
> > >
> > > The known set of currently unsupported file system operations in
> > > Landlock is described at [1]. Out of the operations listed there,
> > > truncate is the only one that modifies file contents, so these patches
> > > should make it possible to prevent the direct modification of file
> > > contents with Landlock.
> > >
> > > The patch introduces the truncation restriction feature as an
> > > additional bit in the access_mask_t bitmap, in line with the existing
> > > supported operations.
> > >
> > > The truncation flag covers both the truncate(2) and ftruncate(2)
> > > families of syscalls, as well as open(2) with the O_TRUNC flag.
> > > This includes usages of creat() in the case where existing regular
> > > files are overwritten.
> > >
> > > Apart from Landlock, file truncation can also be restricted using
> > > seccomp-bpf, but it is more difficult to use (requires BPF, requires
> > > keeping up-to-date syscall lists) and it is not configurable by file
> > > hierarchy, as Landlock is. The simplicity and flexibility of the
> > > Landlock approach makes it worthwhile adding.
> > >
> > > While it's possible to use the "write file" and "truncate" rights
> > > independent of each other, it simplifies the mental model for
> > > userspace callers to always use them together.
> > >
> > > Specifically, the following behaviours might be surprising for users
> > > when using these independently:
> > >
> > >   * The commonly creat() syscall requires the truncate right when
> > >     overwriting existing files, as it is equivalent to open(2) with
> > >     O_TRUNC|O_CREAT|O_WRONLY.
> > >   * The "write file" right is not always required to truncate a file,
> > >     even through the open(2) syscall (when using O_RDONLY|O_TRUNC).
> > >
> > > Nevertheless, keeping the two flags separate is the correct approach
> > > to guarantee backwards compatibility for existing Landlock users.
> > >
> > > These patches are based on version 6.0-rc1.
> > >
> > > Best regards,
> > > Günther
> > >
> > > [1] https://docs.kernel.org/userspace-api/landlock.html#filesystem-flags
> > >
> > > Past discussions:
> > > V1: https://lore.kernel.org/all/20220707200612.132705-1-gnoack3000@gmail.com/
> > > V2: https://lore.kernel.org/all/20220712211405.14705-1-gnoack3000@gmail.com/
> > > V3: https://lore.kernel.org/all/20220804193746.9161-1-gnoack3000@gmail.com/
> > > V4: https://lore.kernel.org/all/20220814192603.7387-1-gnoack3000@gmail.com/
> > >
> > > Changelog:
> > >
> > > V5:
> > > * Documentation
> > >    * Fix wording in userspace-api headers and in landlock.rst.
> > >    * Move the truncation limitation section one to the bottom.
> > >    * Move all .rst changes into the documentation commit.
> > > * selftests
> > >    * Remove _metadata argument from helpers where it became unnecessary.
> > >    * Open writable file descriptors at the top of both tests, before Landlock
> > >      is enabled, to exercise ftruncate() independently from open().
> > >    * Simplify test_ftruncate and decouple it from exercising open().
> > >    * test_creat(): Return errno on close() failure (it does not conflict).
> > >    * Fix /* comment style */
> > >    * Reorder blocks of EXPECT_EQ checks to be consistent within a test.
> > >    * Add missing |O_TRUNC to a check in one test.
> > >    * Put the truncate_unhandled test before the other.
> > >
> > > V4:
> > >   * Documentation
> > >     * Clarify wording and syntax as discussed in review.
> > >     * Use a less confusing error message in the example.
> > >   * selftests:
> > >     * Stop using ASSERT_EQ in test helpers, return EBADFD instead.
> > >       (This is an intentionally uncommon error code, so that the source
> > >       of the error is clear and the test can distinguish test setup
> > >       failures from failures in the actual system call under test.)
> > >   * samples/Documentation:
> > >     * Use additional clarifying comments in the kernel backwards
> > >       compatibility logic.
> > >
> > > V3:
> > >   * selftests:
> > >     * Explicitly test ftruncate with readonly file descriptors
> > >       (returns EINVAL).
> > >     * Extract test_ftruncate, test_truncate, test_creat helpers,
> > >       which simplified the previously mixed usage of EXPECT/ASSERT.
> > >     * Test creat() behaviour as part of the big truncation test.
> > >     * Stop testing the truncate64(2) and ftruncate64(2) syscalls.
> > >       This simplifies the tests a bit. The kernel implementations are the
> > >       same as for truncate(2) and ftruncate(2), so there is little benefit
> > >       from testing them exhaustively. (We aren't testing all open(2)
> > >       variants either.)
> > >   * samples/landlock/sandboxer.c:
> > >     * Use switch() to implement best effort mode.
> > >   * Documentation:
> > >     * Give more background on surprising truncation behaviour.
> > >     * Use switch() in the example too, to stay in-line with the sample tool.
> > >     * Small fixes in header file to address previous comments.
> > > * misc:
> > >    * Fix some typos and const usages.
> > >
> > > V2:
> > >   * Documentation: Mention the truncation flag where needed.
> > >   * Documentation: Point out connection between truncation and file writing.
> > >   * samples: Add file truncation to the landlock/sandboxer.c sample tool.
> > >   * selftests: Exercise open(2) with O_TRUNC and creat(2) exhaustively.
> > >   * selftests: Exercise truncation syscalls when the truncate right
> > >     is not handled by Landlock.
> > >
> > > Günther Noack (4):
> > >    landlock: Support file truncation
> > >    selftests/landlock: Selftests for file truncation support
> > >    samples/landlock: Extend sample tool to support
> > >      LANDLOCK_ACCESS_FS_TRUNCATE
> > >    landlock: Document Landlock's file truncation support
> > >
> > >   Documentation/userspace-api/landlock.rst     |  52 +++-
> > >   include/uapi/linux/landlock.h                |  17 +-
> > >   samples/landlock/sandboxer.c                 |  23 +-
> > >   security/landlock/fs.c                       |   9 +-
> > >   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   | 257 ++++++++++++++++++-
> > >   8 files changed, 336 insertions(+), 28 deletions(-)
> > >
> > >
> > > base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
> > > --
> > > 2.37.2
>
> --

--

Re: [PATCH v5 0/4] landlock: truncate support

[Mickaël Salaün]

On 02/09/2022 08:16, Günther Noack wrote:
> On Fri, Sep 02, 2022 at 07:32:49AM +0200, Günther Noack wrote:
>> On Thu, Sep 01, 2022 at 07:10:38PM +0200, Mickaël Salaün wrote:
>>> Hmm, I think there is an issue with this series. Landlock only enforces
>>> restrictions at open time or when dealing with user-supplied file paths
>>> (relative or absolute).
>>
>> Argh, ok. That sounds like a desirable property, although it would
>> mean reworking the patch set.
>>
>>> The use of the path_truncate hook in this series
>>> doesn't distinguish between file descriptor from before the current sandbox
>>> or from after being sandboxed. For instance, if a file descriptor is
>>> received through a unix socket, it is assumed that this is legitimate and no
>>> Landlock restriction apply on it, which is not the case with this series
>>> anymore. It is the same for files opened before the process sandbox itself.
>>>
>>> To be able to follow the current semantic, I think we should control the
>>> truncate access at open time (or when dealing with a user-supplied path) but
>>> not on any file descriptor as it is currently done.
>>
>> OK - so let me try to make a constructive proposal. We have previously
>> identified a few operations where a truncation happens, and I would
>> propose that the following Landlock rights should be needed for these:
>>
>> * truncate() (operating on char *path): Require LL_ACCESS_FS_TRUNCATE
>> * ftruncate() (operating on fd): No Landlock rights required
>> * open() for reading with O_TRUNC: Require LL_ACCESS_FS_TRUNCATE
>> * open() for writing with O_TRUNC: Require LL_ACCESS_FS_WRITE_FILE
> 
> Thinking about it again, another alternative would be to require
> TRUNCATE as well when opening a file for writing - it would be
> logical, because the resulting FD can be truncated. It would also
> require people to provide the truncate right in order to open files
> for writing, but this may be the logical thing to do.

Another alternative would be to keep the current semantic but ignore 
file descriptors from not-sandboxed processes. This could be possible by 
following the current file->f_mode logic but using the Landlock's 
file->f_security instead to store if the file descriptor was opened in a 
context allowing it to be truncated: file opened outside of a landlocked 
process, or in a sandbox allowing LANDLOCK_ACCESS_FS_TRUNCATE on the 
related path.


> 
> Let me know what you think!
> 
> —Günther
> 
>>
>> The rationale goes as follows:
>>
>> * ftruncate() is already adequately protected by the
>>    LL_ACCESS_FS_WRITE_FILE right. ftruncate is only permitted on fds
>>    that are open for writing.
>> * truncate() is not Landlock-restrictable in Landlock ABI v1,
>>    so needs to be covered by the new truncate right.
>> * open() for reading with O_TRUNC is also not Landlock-restrictable in
>>    Landlock ABI v1, so needs to be covered by the new truncate right.
>> * open() for writing with O_TRUNC is also not Landlock-restrictable in
>>    Landlock ABI v1. BUT: A caller who can open the file for writing
>>    will also be able to ftruncate it - so it doesn't really make sense
>>    to ask for a different Landlock right here.
>>
>> Does that approach make sense to you?
>>
>> I think in terms of changs required for it, it sounds like it would
>> require a change to the path_truncate LSM hook to distinguish the
>> cases above.

Yes, it requires some changes to the path_truncate hook. I think 
providing a struct file, when available, as a second argument looks good.

Serge, Paul, what do you think about that?


>>
>> Do you want a new patch on top of the existing one, or should I rather
>> create a new version of the old truncate patch set?

Please create a sixth patch series also including my (slight) changes.


>>
>> --Günther
>>
>>> On 17/08/2022 22:30, Günther Noack wrote:
>>>> The goal of these patches is to work towards a more complete coverage
>>>> of file system operations that are restrictable with Landlock.
>>>>
>>>> The known set of currently unsupported file system operations in
>>>> Landlock is described at [1]. Out of the operations listed there,
>>>> truncate is the only one that modifies file contents, so these patches
>>>> should make it possible to prevent the direct modification of file
>>>> contents with Landlock.
>>>>
>>>> The patch introduces the truncation restriction feature as an
>>>> additional bit in the access_mask_t bitmap, in line with the existing
>>>> supported operations.
>>>>
>>>> The truncation flag covers both the truncate(2) and ftruncate(2)
>>>> families of syscalls, as well as open(2) with the O_TRUNC flag.
>>>> This includes usages of creat() in the case where existing regular
>>>> files are overwritten.
>>>>
>>>> Apart from Landlock, file truncation can also be restricted using
>>>> seccomp-bpf, but it is more difficult to use (requires BPF, requires
>>>> keeping up-to-date syscall lists) and it is not configurable by file
>>>> hierarchy, as Landlock is. The simplicity and flexibility of the
>>>> Landlock approach makes it worthwhile adding.
>>>>
>>>> While it's possible to use the "write file" and "truncate" rights
>>>> independent of each other, it simplifies the mental model for
>>>> userspace callers to always use them together.
>>>>
>>>> Specifically, the following behaviours might be surprising for users
>>>> when using these independently:
>>>>
>>>>    * The commonly creat() syscall requires the truncate right when
>>>>      overwriting existing files, as it is equivalent to open(2) with
>>>>      O_TRUNC|O_CREAT|O_WRONLY.
>>>>    * The "write file" right is not always required to truncate a file,
>>>>      even through the open(2) syscall (when using O_RDONLY|O_TRUNC).
>>>>
>>>> Nevertheless, keeping the two flags separate is the correct approach
>>>> to guarantee backwards compatibility for existing Landlock users.
>>>>
>>>> These patches are based on version 6.0-rc1.
>>>>
>>>> Best regards,
>>>> Günther
>>>>
>>>> [1] https://docs.kernel.org/userspace-api/landlock.html#filesystem-flags
>>>>
>>>> Past discussions:
>>>> V1: https://lore.kernel.org/all/20220707200612.132705-1-gnoack3000@gmail.com/
>>>> V2: https://lore.kernel.org/all/20220712211405.14705-1-gnoack3000@gmail.com/
>>>> V3: https://lore.kernel.org/all/20220804193746.9161-1-gnoack3000@gmail.com/
>>>> V4: https://lore.kernel.org/all/20220814192603.7387-1-gnoack3000@gmail.com/
>>>>
>>>> Changelog:
>>>>
>>>> V5:
>>>> * Documentation
>>>>     * Fix wording in userspace-api headers and in landlock.rst.
>>>>     * Move the truncation limitation section one to the bottom.
>>>>     * Move all .rst changes into the documentation commit.
>>>> * selftests
>>>>     * Remove _metadata argument from helpers where it became unnecessary.
>>>>     * Open writable file descriptors at the top of both tests, before Landlock
>>>>       is enabled, to exercise ftruncate() independently from open().
>>>>     * Simplify test_ftruncate and decouple it from exercising open().
>>>>     * test_creat(): Return errno on close() failure (it does not conflict).
>>>>     * Fix /* comment style */
>>>>     * Reorder blocks of EXPECT_EQ checks to be consistent within a test.
>>>>     * Add missing |O_TRUNC to a check in one test.
>>>>     * Put the truncate_unhandled test before the other.
>>>>
>>>> V4:
>>>>    * Documentation
>>>>      * Clarify wording and syntax as discussed in review.
>>>>      * Use a less confusing error message in the example.
>>>>    * selftests:
>>>>      * Stop using ASSERT_EQ in test helpers, return EBADFD instead.
>>>>        (This is an intentionally uncommon error code, so that the source
>>>>        of the error is clear and the test can distinguish test setup
>>>>        failures from failures in the actual system call under test.)
>>>>    * samples/Documentation:
>>>>      * Use additional clarifying comments in the kernel backwards
>>>>        compatibility logic.
>>>>
>>>> V3:
>>>>    * selftests:
>>>>      * Explicitly test ftruncate with readonly file descriptors
>>>>        (returns EINVAL).
>>>>      * Extract test_ftruncate, test_truncate, test_creat helpers,
>>>>        which simplified the previously mixed usage of EXPECT/ASSERT.
>>>>      * Test creat() behaviour as part of the big truncation test.
>>>>      * Stop testing the truncate64(2) and ftruncate64(2) syscalls.
>>>>        This simplifies the tests a bit. The kernel implementations are the
>>>>        same as for truncate(2) and ftruncate(2), so there is little benefit
>>>>        from testing them exhaustively. (We aren't testing all open(2)
>>>>        variants either.)
>>>>    * samples/landlock/sandboxer.c:
>>>>      * Use switch() to implement best effort mode.
>>>>    * Documentation:
>>>>      * Give more background on surprising truncation behaviour.
>>>>      * Use switch() in the example too, to stay in-line with the sample tool.
>>>>      * Small fixes in header file to address previous comments.
>>>> * misc:
>>>>     * Fix some typos and const usages.
>>>>
>>>> V2:
>>>>    * Documentation: Mention the truncation flag where needed.
>>>>    * Documentation: Point out connection between truncation and file writing.
>>>>    * samples: Add file truncation to the landlock/sandboxer.c sample tool.
>>>>    * selftests: Exercise open(2) with O_TRUNC and creat(2) exhaustively.
>>>>    * selftests: Exercise truncation syscalls when the truncate right
>>>>      is not handled by Landlock.
>>>>
>>>> Günther Noack (4):
>>>>     landlock: Support file truncation
>>>>     selftests/landlock: Selftests for file truncation support
>>>>     samples/landlock: Extend sample tool to support
>>>>       LANDLOCK_ACCESS_FS_TRUNCATE
>>>>     landlock: Document Landlock's file truncation support
>>>>
>>>>    Documentation/userspace-api/landlock.rst     |  52 +++-
>>>>    include/uapi/linux/landlock.h                |  17 +-
>>>>    samples/landlock/sandboxer.c                 |  23 +-
>>>>    security/landlock/fs.c                       |   9 +-
>>>>    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   | 257 ++++++++++++++++++-
>>>>    8 files changed, 336 insertions(+), 28 deletions(-)
>>>>
>>>>
>>>> base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
>>>> --
>>>> 2.37.2
>>
>> --
> 
> --

Re: [PATCH v5 0/4] landlock: truncate support

[Günther Noack]

On Fri, Sep 02, 2022 at 10:40:57AM +0200, Mickaël Salaün wrote:
> On 02/09/2022 08:16, Günther Noack wrote:
> > On Fri, Sep 02, 2022 at 07:32:49AM +0200, Günther Noack wrote:
> > > On Thu, Sep 01, 2022 at 07:10:38PM +0200, Mickaël Salaün wrote:
> > > > Hmm, I think there is an issue with this series. Landlock only enforces
> > > > restrictions at open time or when dealing with user-supplied file paths
> > > > (relative or absolute).
> > >
> > > Argh, ok. That sounds like a desirable property, although it would
> > > mean reworking the patch set.
> > >
> > > > The use of the path_truncate hook in this series
> > > > doesn't distinguish between file descriptor from before the current sandbox
> > > > or from after being sandboxed. For instance, if a file descriptor is
> > > > received through a unix socket, it is assumed that this is legitimate and no
> > > > Landlock restriction apply on it, which is not the case with this series
> > > > anymore. It is the same for files opened before the process sandbox itself.
> > > >
> > > > To be able to follow the current semantic, I think we should control the
> > > > truncate access at open time (or when dealing with a user-supplied path) but
> > > > not on any file descriptor as it is currently done.
> > >
> > > OK - so let me try to make a constructive proposal. We have previously
> > > identified a few operations where a truncation happens, and I would
> > > propose that the following Landlock rights should be needed for these:
> > >
> > > * truncate() (operating on char *path): Require LL_ACCESS_FS_TRUNCATE
> > > * ftruncate() (operating on fd): No Landlock rights required
> > > * open() for reading with O_TRUNC: Require LL_ACCESS_FS_TRUNCATE
> > > * open() for writing with O_TRUNC: Require LL_ACCESS_FS_WRITE_FILE
> >
> > Thinking about it again, another alternative would be to require
> > TRUNCATE as well when opening a file for writing - it would be
> > logical, because the resulting FD can be truncated. It would also
> > require people to provide the truncate right in order to open files
> > for writing, but this may be the logical thing to do.
>
> Another alternative would be to keep the current semantic but ignore file
> descriptors from not-sandboxed processes. This could be possible by
> following the current file->f_mode logic but using the Landlock's
> file->f_security instead to store if the file descriptor was opened in a
> context allowing it to be truncated: file opened outside of a landlocked
> process, or in a sandbox allowing LANDLOCK_ACCESS_FS_TRUNCATE on the related
> path.

I'm not convinced that it'll be worth distinguishing between a FD
opened for writing and a FD opened for writing+truncation. And whether
the FD is open for writing is already tracked by default and
ftruncate() checks that.

I'm having a hard time constructing a scenario where write() should be
permitted on an FD but ftruncate() should be forbidden. It seems that
write() is the more dangerous operation of the two, with more
potential to modify a file to one's liking, whereas the modifications
possible through TRUNCATE are relatively benign?

The opposite scenario (where ftruncate() is permitted and write() is
forbidden) simply can't exist because an FD must already be writable
in order to use ftruncate(). (see man page)

Additionally, if we recall previous discussions on the truncate patch
sets, there is the very commonly used creat() syscall (a.k.a. open()
with O_CREAT|O_WRONLY|O_TRUNC), which anyway requires the Landlock
truncate right in many cases. So I still think you can't actually use
LANDLOCK_ACCESS_FS_FILE_WRITE without also providing the
LANDLOCK_ACCESS_FS_TRUNCATE right?

In conclusion, I'd be in favor of not tracking the truncate right
separately as a property of an open file descriptor. Does that
rationale sound reasonable?

--

Re: [PATCH v5 0/4] landlock: truncate support

[Mickaël Salaün]

On 02/09/2022 18:34, Günther Noack wrote:
> On Fri, Sep 02, 2022 at 10:40:57AM +0200, Mickaël Salaün wrote:
>> On 02/09/2022 08:16, Günther Noack wrote:
>>> On Fri, Sep 02, 2022 at 07:32:49AM +0200, Günther Noack wrote:
>>>> On Thu, Sep 01, 2022 at 07:10:38PM +0200, Mickaël Salaün wrote:
>>>>> Hmm, I think there is an issue with this series. Landlock only enforces
>>>>> restrictions at open time or when dealing with user-supplied file paths
>>>>> (relative or absolute).
>>>>
>>>> Argh, ok. That sounds like a desirable property, although it would
>>>> mean reworking the patch set.
>>>>
>>>>> The use of the path_truncate hook in this series
>>>>> doesn't distinguish between file descriptor from before the current sandbox
>>>>> or from after being sandboxed. For instance, if a file descriptor is
>>>>> received through a unix socket, it is assumed that this is legitimate and no
>>>>> Landlock restriction apply on it, which is not the case with this series
>>>>> anymore. It is the same for files opened before the process sandbox itself.
>>>>>
>>>>> To be able to follow the current semantic, I think we should control the
>>>>> truncate access at open time (or when dealing with a user-supplied path) but
>>>>> not on any file descriptor as it is currently done.
>>>>
>>>> OK - so let me try to make a constructive proposal. We have previously
>>>> identified a few operations where a truncation happens, and I would
>>>> propose that the following Landlock rights should be needed for these:
>>>>
>>>> * truncate() (operating on char *path): Require LL_ACCESS_FS_TRUNCATE
>>>> * ftruncate() (operating on fd): No Landlock rights required
>>>> * open() for reading with O_TRUNC: Require LL_ACCESS_FS_TRUNCATE
>>>> * open() for writing with O_TRUNC: Require LL_ACCESS_FS_WRITE_FILE
>>>
>>> Thinking about it again, another alternative would be to require
>>> TRUNCATE as well when opening a file for writing - it would be
>>> logical, because the resulting FD can be truncated. It would also
>>> require people to provide the truncate right in order to open files
>>> for writing, but this may be the logical thing to do.
>>
>> Another alternative would be to keep the current semantic but ignore file
>> descriptors from not-sandboxed processes. This could be possible by
>> following the current file->f_mode logic but using the Landlock's
>> file->f_security instead to store if the file descriptor was opened in a
>> context allowing it to be truncated: file opened outside of a landlocked
>> process, or in a sandbox allowing LANDLOCK_ACCESS_FS_TRUNCATE on the related
>> path.
> 
> I'm not convinced that it'll be worth distinguishing between a FD
> opened for writing and a FD opened for writing+truncation. And whether
> the FD is open for writing is already tracked by default and
> ftruncate() checks that.

That might be a misunderstanding. What I'm proposing is to keep the same 
semantic as this fifth patch series, only to keep scoped Landlock 
restrictions and propagate them (which is already how Landlock works).

The layout1.truncate tests should work the same except that 
test_ftruncate(file_*_fd) will always be allowed because such FD they 
are opened before the thread being sandboxed.

> 
> I'm having a hard time constructing a scenario where write() should be
> permitted on an FD but ftruncate() should be forbidden. It seems that
> write() is the more dangerous operation of the two, with more
> potential to modify a file to one's liking, whereas the modifications
> possible through TRUNCATE are relatively benign?

I don't understand, this is how this fifth series already restrict 
truncate. I'm not proposing to change the FD minimal requirement to be 
"truncatable", and it would not be possible with the LSM framework anyway.


> 
> The opposite scenario (where ftruncate() is permitted and write() is
> forbidden) simply can't exist because an FD must already be writable
> in order to use ftruncate(). (see man page)

Right, and I'm not proposing to change that. Currently, the kernel 
tracks how a FD was opened (e.g. read or write mode). I'm proposing to 
add another *complementary* Landlock-specific mode for truncate because 
it is a more fine-grained access right with Landlock, hence this patch 
series.


> 
> Additionally, if we recall previous discussions on the truncate patch
> sets, there is the very commonly used creat() syscall (a.k.a. open()
> with O_CREAT|O_WRONLY|O_TRUNC), which anyway requires the Landlock
> truncate right in many cases. So I still think you can't actually use
> LANDLOCK_ACCESS_FS_FILE_WRITE without also providing the
> LANDLOCK_ACCESS_FS_TRUNCATE right?

Hmm, that is definitely not a common case, but this series permit that, 
see test_truncate(file_t).

> 
> In conclusion, I'd be in favor of not tracking the truncate right
> separately as a property of an open file descriptor. Does that
> rationale sound reasonable?

No, but I think there is a misunderstanding. :)

The idea is first to change hook_file_open() to set 
landlock_file(file)->access = LANDLOCK_ACCESS_FS_TRUNCATE if it is 
allowed by the policy: current thread being either not in a sandbox, or 
in a sandbox that allows truncate.
Then, in hook_path_truncate(), we allow the operation if `file && 
(landlock_file(file)->access & LANDLOCK_ACCESS_FS_TRUNCATE)`. Otherwise, 
it there is only a path available (because it comes from truncate(2)), 
we (almost) call current_check_access_path(path, 
LANDLOCK_ACCESS_FS_TRUNCATE).

Re: [PATCH v5 0/4] landlock: truncate support

[xiujianfeng]

Hi,

在 2022/9/2 1:10, Mickaël Salaün 写道:
> Hmm, I think there is an issue with this series. Landlock only enforces 
> restrictions at open time or when dealing with user-supplied file paths 
> (relative or absolute). The use of the path_truncate hook in this series 
> doesn't distinguish between file descriptor from before the current 
> sandbox or from after being sandboxed. For instance, if a file 
> descriptor is received through a unix socket, it is assumed that this is 
> legitimate and no Landlock restriction apply on it, which is not the 
> case with this series anymore. It is the same for files opened before 
> the process sandbox itself.

so I think this issue also exists in the chown/chmod series, right? 
there is a testcase in that patchset verify the corresponding rights 
inside the sanbox with a fd opened before sanboxing.
> 
> To be able to follow the current semantic, I think we should control the 
> truncate access at open time (or when dealing with a user-supplied path) 
> but not on any file descriptor as it is currently done. >
> 
> On 17/08/2022 22:30, Günther Noack wrote:
>> The goal of these patches is to work towards a more complete coverage
>> of file system operations that are restrictable with Landlock.
>>
>> The known set of currently unsupported file system operations in
>> Landlock is described at [1]. Out of the operations listed there,
>> truncate is the only one that modifies file contents, so these patches
>> should make it possible to prevent the direct modification of file
>> contents with Landlock.
>>
>> The patch introduces the truncation restriction feature as an
>> additional bit in the access_mask_t bitmap, in line with the existing
>> supported operations.
>>
>> The truncation flag covers both the truncate(2) and ftruncate(2)
>> families of syscalls, as well as open(2) with the O_TRUNC flag.
>> This includes usages of creat() in the case where existing regular
>> files are overwritten.
>>
>> Apart from Landlock, file truncation can also be restricted using
>> seccomp-bpf, but it is more difficult to use (requires BPF, requires
>> keeping up-to-date syscall lists) and it is not configurable by file
>> hierarchy, as Landlock is. The simplicity and flexibility of the
>> Landlock approach makes it worthwhile adding.
>>
>> While it's possible to use the "write file" and "truncate" rights
>> independent of each other, it simplifies the mental model for
>> userspace callers to always use them together.
>>
>> Specifically, the following behaviours might be surprising for users
>> when using these independently:
>>
>>   * The commonly creat() syscall requires the truncate right when
>>     overwriting existing files, as it is equivalent to open(2) with
>>     O_TRUNC|O_CREAT|O_WRONLY.
>>   * The "write file" right is not always required to truncate a file,
>>     even through the open(2) syscall (when using O_RDONLY|O_TRUNC).
>>
>> Nevertheless, keeping the two flags separate is the correct approach
>> to guarantee backwards compatibility for existing Landlock users.
>>
>> These patches are based on version 6.0-rc1.
>>
>> Best regards,
>> Günther
>>
>> [1] https://docs.kernel.org/userspace-api/landlock.html#filesystem-flags
>>
>> Past discussions:
>> V1: 
>> https://lore.kernel.org/all/20220707200612.132705-1-gnoack3000@gmail.com/
>> V2: 
>> https://lore.kernel.org/all/20220712211405.14705-1-gnoack3000@gmail.com/
>> V3: 
>> https://lore.kernel.org/all/20220804193746.9161-1-gnoack3000@gmail.com/
>> V4: 
>> https://lore.kernel.org/all/20220814192603.7387-1-gnoack3000@gmail.com/
>>
>> Changelog:
>>
>> V5:
>> * Documentation
>>    * Fix wording in userspace-api headers and in landlock.rst.
>>    * Move the truncation limitation section one to the bottom.
>>    * Move all .rst changes into the documentation commit.
>> * selftests
>>    * Remove _metadata argument from helpers where it became unnecessary.
>>    * Open writable file descriptors at the top of both tests, before 
>> Landlock
>>      is enabled, to exercise ftruncate() independently from open().
>>    * Simplify test_ftruncate and decouple it from exercising open().
>>    * test_creat(): Return errno on close() failure (it does not 
>> conflict).
>>    * Fix /* comment style */
>>    * Reorder blocks of EXPECT_EQ checks to be consistent within a test.
>>    * Add missing |O_TRUNC to a check in one test.
>>    * Put the truncate_unhandled test before the other.
>>
>> V4:
>>   * Documentation
>>     * Clarify wording and syntax as discussed in review.
>>     * Use a less confusing error message in the example.
>>   * selftests:
>>     * Stop using ASSERT_EQ in test helpers, return EBADFD instead.
>>       (This is an intentionally uncommon error code, so that the source
>>       of the error is clear and the test can distinguish test setup
>>       failures from failures in the actual system call under test.)
>>   * samples/Documentation:
>>     * Use additional clarifying comments in the kernel backwards
>>       compatibility logic.
>>
>> V3:
>>   * selftests:
>>     * Explicitly test ftruncate with readonly file descriptors
>>       (returns EINVAL).
>>     * Extract test_ftruncate, test_truncate, test_creat helpers,
>>       which simplified the previously mixed usage of EXPECT/ASSERT.
>>     * Test creat() behaviour as part of the big truncation test.
>>     * Stop testing the truncate64(2) and ftruncate64(2) syscalls.
>>       This simplifies the tests a bit. The kernel implementations are the
>>       same as for truncate(2) and ftruncate(2), so there is little 
>> benefit
>>       from testing them exhaustively. (We aren't testing all open(2)
>>       variants either.)
>>   * samples/landlock/sandboxer.c:
>>     * Use switch() to implement best effort mode.
>>   * Documentation:
>>     * Give more background on surprising truncation behaviour.
>>     * Use switch() in the example too, to stay in-line with the sample 
>> tool.
>>     * Small fixes in header file to address previous comments.
>> * misc:
>>    * Fix some typos and const usages.
>>
>> V2:
>>   * Documentation: Mention the truncation flag where needed.
>>   * Documentation: Point out connection between truncation and file 
>> writing.
>>   * samples: Add file truncation to the landlock/sandboxer.c sample tool.
>>   * selftests: Exercise open(2) with O_TRUNC and creat(2) exhaustively.
>>   * selftests: Exercise truncation syscalls when the truncate right
>>     is not handled by Landlock.
>>
>> Günther Noack (4):
>>    landlock: Support file truncation
>>    selftests/landlock: Selftests for file truncation support
>>    samples/landlock: Extend sample tool to support
>>      LANDLOCK_ACCESS_FS_TRUNCATE
>>    landlock: Document Landlock's file truncation support
>>
>>   Documentation/userspace-api/landlock.rst     |  52 +++-
>>   include/uapi/linux/landlock.h                |  17 +-
>>   samples/landlock/sandboxer.c                 |  23 +-
>>   security/landlock/fs.c                       |   9 +-
>>   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   | 257 ++++++++++++++++++-
>>   8 files changed, 336 insertions(+), 28 deletions(-)
>>
>>
>> base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
>> -- 
>> 2.37.2
> .

Re: [PATCH v5 0/4] landlock: truncate support

[Mickaël Salaün]

On 02/09/2022 14:26, xiujianfeng wrote:
> Hi,
> 
> 在 2022/9/2 1:10, Mickaël Salaün 写道:
>> Hmm, I think there is an issue with this series. Landlock only enforces
>> restrictions at open time or when dealing with user-supplied file paths
>> (relative or absolute). The use of the path_truncate hook in this series
>> doesn't distinguish between file descriptor from before the current
>> sandbox or from after being sandboxed. For instance, if a file
>> descriptor is received through a unix socket, it is assumed that this is
>> legitimate and no Landlock restriction apply on it, which is not the
>> case with this series anymore. It is the same for files opened before
>> the process sandbox itself.
> 
> so I think this issue also exists in the chown/chmod series, right?
> there is a testcase in that patchset verify the corresponding rights
> inside the sanbox with a fd opened before sanboxing.

Correct. For LANDLOCK_ACCESS_FS_TRUNCATE, we need to add tests to make 
sure that:
* a sandboxed process with the truncate restriction can open a file in 
write mode, forward it to an un-sandboxed process, and make sure this 
receiver cannot truncate the file descriptor, nor its dup.
* an inherited file descriptor can be truncated even if done by a 
sandboxed process, except if it was created by a sandboxed process and 
the truncate restriction applied on it.

However, for the file metadata accesses, I suggest you first focus on 
the inode_setattr and inode_setxattr hook modifications. We'll get back 
to this FD-based restrictions later.