| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <223344b1-036a-2f25-727f-6262e1b7a763@digikod.net>
Date: Mon, 22 Aug 2022 23:28:37 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: Günther Noack <gnoack3000@...il.com>,
Xiu Jianfeng <xiujianfeng@...wei.com>
Cc: paul@...l-moore.com, jmorris@...ei.org, serge@...lyn.com,
shuah@...nel.org, corbet@....net,
linux-security-module@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
linux-doc@...r.kernel.org
Subject: Re: [PATCH -next 3/5] landlock/selftests: add selftests for chmod and
chown
Agree with Günther's review. :)
On 22/08/2022 20:53, Günther Noack wrote:
> On Mon, Aug 22, 2022 at 07:46:59PM +0800, Xiu Jianfeng wrote:
>> Add the following simple testcases:
>> 1. chmod/fchmod: remove S_IWUSR and restore S_IWUSR with or without
>> restriction.
>> 2. chown/fchown: set original uid and gid with or without restriction,
>> because chown needs CAP_CHOWN and testcase framework don't have this
>> capability, setting original uid and gid is ok to cover landlock
>> function.
>>
>> Signed-off-by: Xiu Jianfeng <xiujianfeng@...wei.com>
>> ---
>> tools/testing/selftests/landlock/fs_test.c | 228 +++++++++++++++++++++
>> 1 file changed, 228 insertions(+)
>>
>> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
>> index 5b55b93b5570..f47b4ccd2b26 100644
>> --- a/tools/testing/selftests/landlock/fs_test.c
>> +++ b/tools/testing/selftests/landlock/fs_test.c
>> @@ -59,6 +59,9 @@ 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";
>> +static const char file2_s3d1[] = TMP_DIR "/s3d1/f2";
>> +static const char file3_s3d1[] = TMP_DIR "/s3d1/f3";
>> +
>> /* 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";
>> @@ -211,6 +214,8 @@ static void create_layout1(struct __test_metadata *const _metadata)
>> create_file(_metadata, file2_s2d3);
>>
>> create_file(_metadata, file1_s3d1);
>> + create_file(_metadata, file2_s3d1);
>> + create_file(_metadata, file3_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"));
>> @@ -234,6 +239,8 @@ static void remove_layout1(struct __test_metadata *const _metadata)
>> EXPECT_EQ(0, remove_path(file1_s2d1));
>>
>> EXPECT_EQ(0, remove_path(file1_s3d1));
>> + EXPECT_EQ(0, remove_path(file2_s3d1));
>> + EXPECT_EQ(0, remove_path(file3_s3d1));
>> EXPECT_EQ(0, remove_path(dir_s3d3));
>> set_cap(_metadata, CAP_SYS_ADMIN);
>> umount(dir_s3d2);
>> @@ -3272,6 +3279,227 @@ TEST_F_FORK(layout1, truncate)
>> EXPECT_EQ(0, test_creat(file_in_dir_w));
>> }
>>
>> +static int test_chmod(const char *path)
>
> Nitpicks:
> - const char *const path
> - short documentation? :)
>
>> +{
>> + int ret;
>> + struct stat st;
>> + mode_t mode;
>> +
>> + ret = stat(path, &st);
>> + if (ret < 0)
>> + return errno;
>> + /* save original mode in order to restore */
>> + mode = st.st_mode & 0777;
>> + /* remove S_IWUSR */
>> + ret = chmod(path, mode & ~0200);
>> + if (ret < 0)
>> + return errno;
>> + ret = stat(path, &st);
>> + if (ret < 0)
>> + return errno;
>> + /* check if still has S_IWUSR */
>> + if (st.st_mode & 0200)
>> + return -EFAULT;
>> + /* restore the original mode */
>> + ret = chmod(path, mode);
>> + if (ret < 0)
>> + return errno;
>> + return 0;
>> +}
>
> I would argue this can be simpler, with the following reasoning:
>
> - Does the file have the right mode after chmod()?
>
> I claim that fs_test should care only about the question of whether
> EACCES is returned or not. If fs_test were to also check for the
> side effects of these operations, it would eventually contain tests
> for the full file system API, not just for Landlock. That seems out
> of scope :)
>
> - Undoing the chmod() operation
>
> I'm not sure whether it's worth the effort to restore the exact
> state before that function returns. As long as the flags suffice to
> remove the test directory at the end, it probably doesn't matter
> much what exact mode they have?
>
> I think this could just be
>
> if (chmod(path, mode) < 0)
> return errno;
> return 0
>
> and it would be a bit simpler to understand :)
>
> The same argument applies also to the other test_...() functions.
>
>> +static int test_fchmod(const char *path)
>
> I initially took the same approach for test_ftruncate() but eventually
> settled on using an approach where the file is open()ed before
> restricting the thread with Landlock. This eliminates the potential
> confusion where test_ftruncate() returns an error but the caller can't
> distinguish whether the error is from open() or from ftruncate(). It
> also makes fchmod testable even in scenarios where the file cannot be
> opened because of missing Landlock rights.
>
>> +{
>> + int ret, fd;
>> + struct stat st;
>> + mode_t mode;
>> +
>> + ret = stat(path, &st);
>> + if (ret < 0)
>> + return errno;
>> + /* save original mode in order to restore */
>> + mode = st.st_mode & 0777;
>> +
>> + fd = openat(AT_FDCWD, path, O_RDWR | O_CLOEXEC);
>> + if (fd < 0)
>> + return errno;
>> + /* remove S_IWUSR */
>> + ret = fchmod(fd, mode & ~0200);
>> + if (ret < 0)
>> + goto err;
>> + ret = stat(path, &st);
>> + if (ret < 0)
>> + goto err;
>> + /* check if still has S_IWUSR */
>> + if (st.st_mode & 0200) {
>> + ret = -1;
>> + errno = -EFAULT;
>> + goto err;
>> + }
>> + /* restore the original mode */
>> + ret = fchmod(fd, mode);
>> +err:
>> + if (close(fd) < 0)
>> + return errno;
>> + return ret ? errno : 0;
>> +}
>
>> +static int test_chown(const char *path)
>> +{
>> + int ret;
>> + struct stat st;
>> +
>> + ret = stat(path, &st);
>> + if (ret < 0)
>> + return errno;
>> + /*
>> + * chown needs CAP_CHOWN to modify uid and/or gid, however
>> + * there is no such capability when the testcases framework
>> + * setup, so just chown to original uid/gid, which can also
>> + * cover the function in landlock.
>> + */
>> + ret = chown(path, st.st_uid, st.st_gid);
>> + if (ret < 0)
>> + return errno;
>> + return 0;
>> +}
>> +
>> +static int test_fchown(const char *path)
>> +{
>> + int ret, fd;
>> + struct stat st;
>> +
>> + ret = stat(path, &st);
>> + if (ret < 0)
>> + return errno;
>> + fd = openat(AT_FDCWD, path, O_RDWR | O_CLOEXEC);
>> + if (fd < 0)
>> + return errno;
>> + /*
>> + * fchown needs CAP_CHOWN to modify uid and/or gid, however
>> + * there is no such capability when the testcases framework
>> + * setup, so just fchown to original uid/gid, which can also
>> + * cover the function in landlock.
>> + */
>> + ret = fchown(fd, st.st_uid, st.st_gid);
>> + if (close(fd) < 0)
>> + return errno;
>> + return ret ? errno : 0;
>> +}
>> +
>> +TEST_F_FORK(layout1, unhandled_chmod)
>> +{
>> + const struct rule rules[] = {
>> + {
>> + .path = file2_s3d1,
>> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
>> + LANDLOCK_ACCESS_FS_WRITE_FILE,
>> + },
>> + {
>> + .path = file3_s3d1,
>> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
>> + LANDLOCK_ACCESS_FS_WRITE_FILE,
>> + },
>> + {},
>> + };
>> + const int ruleset_fd =
>> + create_ruleset(_metadata, ACCESS_RW, rules);
>> +
>> + ASSERT_LE(0, ruleset_fd);
>> + enforce_ruleset(_metadata, ruleset_fd);
>> + ASSERT_EQ(0, close(ruleset_fd));
>> +
>> + ASSERT_EQ(0, test_chmod(file2_s3d1));
>> + ASSERT_EQ(0, test_fchmod(file2_s3d1));
>> + ASSERT_EQ(0, test_chmod(file3_s3d1));
>> + ASSERT_EQ(0, test_chmod(dir_s3d1));
>
> *optional* because the existing tests are already inconsistent about it
>
> These four ASSERT_EQ() calls are independent scenarios and could be
> done with EXPECT_EQ(), which would be more in line with the approach
> that this test framework takes. (Same for the other tests below)
>
> Compare previous discussion at:
> https://lore.kernel.org/all/Yvd3+fy+mDBop+YA@nuc/
>
>> +}
>> +
>> +TEST_F_FORK(layout1, chmod)
>> +{
>> + const struct rule rules[] = {
>> + {
>> + .path = file2_s3d1,
>> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
>> + LANDLOCK_ACCESS_FS_WRITE_FILE |
>> + LANDLOCK_ACCESS_FS_CHMOD,
>> + },
>> + {
>> + .path = file3_s3d1,
>> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
>> + LANDLOCK_ACCESS_FS_WRITE_FILE,
>> + },
>> + {},
>> + };
>> + const int ruleset_fd =
>> + create_ruleset(_metadata, ACCESS_RW | LANDLOCK_ACCESS_FS_CHMOD, rules);
>> +
>> + ASSERT_LE(0, ruleset_fd);
>> + enforce_ruleset(_metadata, ruleset_fd);
>> + ASSERT_EQ(0, close(ruleset_fd));
>> +
>> + ASSERT_EQ(0, test_chmod(file2_s3d1));
>> + ASSERT_EQ(0, test_fchmod(file2_s3d1));
>> + ASSERT_EQ(EACCES, test_chmod(file3_s3d1));
>> + ASSERT_EQ(EACCES, test_chmod(dir_s3d1));
>> +}
>> +
>> +TEST_F_FORK(layout1, no_chown)
>
> "unhandled_chown" to be consistent with the other one above?
>
>> +{
>> + const struct rule rules[] = {
>> + {
>> + .path = file2_s3d1,
>> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
>> + LANDLOCK_ACCESS_FS_WRITE_FILE,
>> + },
>> + {
>> + .path = file3_s3d1,
>> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
>> + LANDLOCK_ACCESS_FS_WRITE_FILE,
>> + },
>> + {},
>> + };
>> + const int ruleset_fd =
>> + create_ruleset(_metadata, ACCESS_RW, rules);
>> +
>> + ASSERT_LE(0, ruleset_fd);
>> + enforce_ruleset(_metadata, ruleset_fd);
>> + ASSERT_EQ(0, close(ruleset_fd));
>> +
>> + ASSERT_EQ(0, test_chown(file2_s3d1));
>> + ASSERT_EQ(0, test_fchown(file2_s3d1));
>> + ASSERT_EQ(0, test_chown(file3_s3d1));
>> + ASSERT_EQ(0, test_chown(dir_s3d1));
>> +}
>> +
>> +TEST_F_FORK(layout1, chown)
>> +{
>> + const struct rule rules[] = {
>> + {
>> + .path = file2_s3d1,
>> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
>> + LANDLOCK_ACCESS_FS_WRITE_FILE |
>> + LANDLOCK_ACCESS_FS_CHOWN,
>
> It might be useful to also check a scenario where the chown right is
> granted on a directory (and as a consequence, both the directory
> itself as well as its contents can be chowned)? (Same for chmod)
>
>> + },
>> + {
>> + .path = file3_s3d1,
>> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
>> + LANDLOCK_ACCESS_FS_WRITE_FILE,
>> + },
>> + {},
>> + };
>> + const int ruleset_fd =
>> + create_ruleset(_metadata, ACCESS_RW | LANDLOCK_ACCESS_FS_CHOWN, rules);
>> +
>> + ASSERT_LE(0, ruleset_fd);
>> + enforce_ruleset(_metadata, ruleset_fd);
>> + ASSERT_EQ(0, close(ruleset_fd));
>> +
>> + ASSERT_EQ(0, test_chown(file2_s3d1));
>> + ASSERT_EQ(0, test_fchown(file2_s3d1));
>> + ASSERT_EQ(EACCES, test_chown(file3_s3d1));
>> + ASSERT_EQ(EACCES, test_chown(dir_s3d1));
>> +}
>> +
>> /* clang-format off */
>> FIXTURE(layout1_bind) {};
>> /* clang-format on */
>> --
>> 2.17.1
>>
>
> --
Powered by blists - more mailing lists