[PATCH v2 2/4] selftests/landlock: Selftests for file truncation support
Mickaël Salaün
mic at digikod.net
Fri Jul 29 09:39:40 UTC 2022
On 12/07/2022 23:14, Günther Noack wrote:
> These tests exercise the following truncation operations:
>
> * truncate() and trunate64() (truncate by path)
> * ftruncate() and ftruncate64() (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:
>
> * The truncate right is required to use ftruncate,
> 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 at gmail.com>
> Link: https://lore.kernel.org/all/20220707200612.132705-3-gnoack3000@gmail.com/
> ---
> tools/testing/selftests/landlock/fs_test.c | 238 +++++++++++++++++++++
> 1 file changed, 238 insertions(+)
>
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index cb77eaa01c91..1e5660118bce 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -3023,6 +3023,244 @@ TEST_F_FORK(layout1, proc_pipe)
> ASSERT_EQ(0, close(pipe_fds[1]));
> }
>
> +TEST_F_FORK(layout1, truncate)
> +{
> + const char *file_rwt = file1_s1d1;
You can make file_rwt (and others) const too:
const char *const file_rwt = file1_s1d1;
> + const char *file_rw = file2_s1d1;
> + const char *file_rt = file1_s1d2;
> + const char *file_t = file2_s1d2;
> + const char *file_none = file1_s1d3;
> + const char *dir_t = dir_s2d1;
> + const char *file_in_dir_t = file1_s2d1;
> + 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,
> + },
> + {},
> + };
> + const __u64 handled = LANDLOCK_ACCESS_FS_READ_FILE |
> + LANDLOCK_ACCESS_FS_WRITE_FILE |
> + LANDLOCK_ACCESS_FS_TRUNCATE;
> + const int ruleset_fd = create_ruleset(_metadata, handled, rules);
> + struct stat statbuf;
> + int reg_fd;
> +
> + ASSERT_LE(0, ruleset_fd);
> + enforce_ruleset(_metadata, ruleset_fd);
> + ASSERT_EQ(0, close(ruleset_fd));
> +
> + /* Checks read, write and truncate rights: truncate and ftruncate work. */
> + reg_fd = open(file_rwt, O_WRONLY | O_CLOEXEC);
> + ASSERT_LE(0, reg_fd);
> + EXPECT_EQ(0, ftruncate(reg_fd, 10));
> + EXPECT_EQ(0, ftruncate64(reg_fd, 20));
> + ASSERT_EQ(0, close(reg_fd));
Could you add the same ftruncate* checks with a O_RDONLY FD?
> +
> + EXPECT_EQ(0, truncate(file_rwt, 10));
> + EXPECT_EQ(0, truncate64(file_rwt, 20));
> +
> + reg_fd = open(file_rwt, O_WRONLY | O_TRUNC | O_CLOEXEC);
> + ASSERT_LE(0, reg_fd);
> + ASSERT_EQ(0, close(reg_fd));
> +
> + reg_fd = open(file_rwt, O_RDONLY | O_TRUNC | O_CLOEXEC);
> + ASSERT_LE(0, reg_fd);
> + ASSERT_EQ(0, close(reg_fd));
I'm not sure if it would be worth it, but can you try to move these
checks in a standalone helper (like test_execute) that could be use for
all/most scenarios?
> +
> + /* Checks read and write rights: no truncate variant works. */
> + reg_fd = open(file_rw, O_WRONLY | O_CLOEXEC);
> + ASSERT_LE(0, reg_fd);
> + EXPECT_EQ(-1, ftruncate(reg_fd, 10));
> + EXPECT_EQ(EACCES, errno);
> + EXPECT_EQ(-1, ftruncate64(reg_fd, 20));
> + EXPECT_EQ(EACCES, errno);
> + ASSERT_EQ(0, close(reg_fd));
> +
> + EXPECT_EQ(-1, truncate(file_rw, 10));
> + EXPECT_EQ(EACCES, errno);
> + EXPECT_EQ(-1, truncate64(file_rw, 20));
> + EXPECT_EQ(EACCES, errno);
> +
> + EXPECT_EQ(-1, open(file_rw, O_WRONLY | O_TRUNC | O_CLOEXEC));
> + EXPECT_EQ(EACCES, errno);
> +
> + EXPECT_EQ(-1, open(file_rw, O_RDONLY | O_TRUNC | O_CLOEXEC));
> + EXPECT_EQ(EACCES, errno);
> +
> + /* Checks read and truncate right: truncate works, also with open(2). */
"right*s*" ? Just keep it consistent with other comments.
> + EXPECT_EQ(-1, open(file_rt, O_WRONLY | O_CLOEXEC));
> + EXPECT_EQ(EACCES, errno);
> +
> + EXPECT_EQ(0, truncate(file_rt, 10));
> + EXPECT_EQ(0, truncate64(file_rt, 20));
> +
> + reg_fd = open(file_rt, O_RDONLY | O_TRUNC | O_CLOEXEC);
> + ASSERT_LE(0, reg_fd);
> + ASSERT_EQ(0, fstat(reg_fd, &statbuf));
> + EXPECT_EQ(0, statbuf.st_size);
> + ASSERT_EQ(0, close(reg_fd));
Why checking fstat? And why only here?
> +
> + /* Checks truncate right: truncate works, but can't open file. */
> + EXPECT_EQ(-1, open(file_t, O_WRONLY | O_CLOEXEC));
> + EXPECT_EQ(EACCES, errno);
> +
> + EXPECT_EQ(0, truncate(file_t, 10));
> + EXPECT_EQ(0, truncate64(file_t, 20));
> +
> + EXPECT_EQ(-1, open(file_t, O_RDONLY | O_TRUNC | O_CLOEXEC));
> + EXPECT_EQ(EACCES, errno);
> +
> + /* Checks "no rights" case: No form of truncation works. */
Nit: Do we need an "s" if there is none?
> + EXPECT_EQ(-1, open(file_none, O_WRONLY | O_CLOEXEC));
> + EXPECT_EQ(EACCES, errno);
> +
> + EXPECT_EQ(-1, truncate(file_none, 10));
> + EXPECT_EQ(EACCES, errno);
> + EXPECT_EQ(-1, truncate64(file_none, 20));
> + EXPECT_EQ(EACCES, errno);
> +
> + EXPECT_EQ(-1, open(file_none, O_RDONLY | O_TRUNC | O_CLOEXEC));
> + EXPECT_EQ(EACCES, errno);
> +
> + /* Checks truncate right on directory: */
> + EXPECT_EQ(-1, open(file_in_dir_t, O_WRONLY | O_CLOEXEC));
> + EXPECT_EQ(EACCES, errno);
> +
> + EXPECT_EQ(0, truncate(file_in_dir_t, 10));
> + EXPECT_EQ(0, truncate64(file_in_dir_t, 20));
> +
> + EXPECT_EQ(-1, open(file_in_dir_t, O_RDONLY | O_TRUNC | O_CLOEXEC));
> + EXPECT_EQ(EACCES, errno);
> +}
> +
> +/*
> + * 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)
Could you merge these truncate_unhandled tests with TEST_F_FORK(layout1,
truncate)? You can use it as a first layer of rules. This will make sure
that nested stacking with different handled access rights works as expected.
> +{
> + const char *file_r = file1_s1d1;
> + const char *file_w = file2_s1d1;
> + const char *file_none = file1_s1d2;
> + 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;
> + const int ruleset_fd = create_ruleset(_metadata, handled, rules);
> + int reg_fd;
> +
> + ASSERT_LE(0, ruleset_fd);
> + enforce_ruleset(_metadata, ruleset_fd);
> + ASSERT_EQ(0, close(ruleset_fd));
> +
> + /* Checks read right: truncation should work through truncate and open. */
> + EXPECT_EQ(0, truncate(file_r, 10));
> + EXPECT_EQ(0, truncate64(file_r, 20));
> +
> + reg_fd = open(file_r, O_RDONLY | O_TRUNC | O_CLOEXEC);
> + ASSERT_LE(0, reg_fd);
> + ASSERT_EQ(0, close(reg_fd));
> +
> + EXPECT_EQ(-1, open(file_r, O_WRONLY | O_TRUNC | O_CLOEXEC));
> + EXPECT_EQ(EACCES, errno);
> +
> + /* Checks write right: truncation should work through truncate, ftruncate and open. */
> + EXPECT_EQ(0, truncate(file_w, 10));
> + EXPECT_EQ(0, truncate64(file_w, 20));
> +
> + EXPECT_EQ(-1, open(file_w, O_RDONLY | O_TRUNC | O_CLOEXEC));
> + EXPECT_EQ(EACCES, errno);
> +
> + reg_fd = open(file_w, O_WRONLY | O_TRUNC | O_CLOEXEC);
> + ASSERT_LE(0, reg_fd);
> + ASSERT_EQ(0, close(reg_fd));
> +
> + reg_fd = open(file_w, O_WRONLY);
> + ASSERT_LE(0, reg_fd);
> + EXPECT_EQ(0, ftruncate(reg_fd, 10));
> + EXPECT_EQ(0, ftruncate64(reg_fd, 20));
> + ASSERT_EQ(0, close(reg_fd));
> +
> + /* Checks "no rights" case: truncate works but all open attempts fail. */
> + EXPECT_EQ(0, truncate(file_none, 10));
> + EXPECT_EQ(0, truncate64(file_none, 20));
> +
> + EXPECT_EQ(-1, open(file_none, O_RDONLY | O_TRUNC | O_CLOEXEC));
> + EXPECT_EQ(EACCES, errno);
> +
> + EXPECT_EQ(-1, open(file_none, O_WRONLY | O_TRUNC | O_CLOEXEC));
> + EXPECT_EQ(EACCES, errno);
> +
> + EXPECT_EQ(-1, open(file_none, O_WRONLY | O_CLOEXEC));
> + EXPECT_EQ(EACCES, errno);
> +}
> +
> +/* Exercises creat(), which is equivalent to an open with O_CREAT|O_WRONLY|O_TRUNC. */
> +TEST_F_FORK(layout1, truncate_creat)
You can merge these truncate_creat tests too in TEST_F_FORK(layout1,
truncate) (without dedicated layer though).
> +{
> + const struct rule rules[] = {
> + {
> + .path = dir_s1d1,
> + .access = LANDLOCK_ACCESS_FS_WRITE_FILE,
> + },
> + {}
> + };
> + const __u64 handled = LANDLOCK_ACCESS_FS_WRITE_FILE |
> + LANDLOCK_ACCESS_FS_TRUNCATE;
> + const int ruleset_fd = create_ruleset(_metadata, handled, rules);
> + int reg_fd;
> +
> + ASSERT_LE(0, ruleset_fd);
> + enforce_ruleset(_metadata, ruleset_fd);
> + ASSERT_EQ(0, close(ruleset_fd));
> +
> + /* Checks creating a new file: This should work even without the truncate right. */
> + ASSERT_EQ(0, unlink(file1_s1d1));
> +
> + reg_fd = creat(file1_s1d1, 0600);
> + ASSERT_LE(0, reg_fd);
> + ASSERT_EQ(0, close(reg_fd));
> +
> + /*
> + * Checks creating a file over an existing one:
> + * This should fail. It would truncate the file, and we don't have that right.
> + */
> + EXPECT_EQ(-1, creat(file2_s1d1, 0600));
> + EXPECT_EQ(EACCES, errno);
> +}
> +
> /* clang-format off */
> FIXTURE(layout1_bind) {};
> /* clang-format on */
More information about the Linux-security-module-archive
mailing list