| 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: <76c05799-91e1-dea3-92dd-beb299d0945c@huawei.com>
Date: Mon, 29 Aug 2022 09:49:18 +0800
From: xiujianfeng <xiujianfeng@...wei.com>
To: Günther Noack <gnoack3000@...il.com>
CC: <mic@...ikod.net>, <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 v2 4/6] landlock/selftests: add selftests for chmod
and chown
Hi,
在 2022/8/28 1:48, Günther Noack 写道:
> On Sat, Aug 27, 2022 at 07:12:13PM +0800, Xiu Jianfeng wrote:
>> The following APIs are tested with simple scenarios.
>> 1. chmod/fchmod/fchmodat;
>> 2. chmod/fchmod/lchown/fchownat;
>>
>> The key point is that set these access rights on a directory but only for
>> its content, not the directory itself. this scenario is covered.
>>
>> Signed-off-by: Xiu Jianfeng <xiujianfeng@...wei.com>
>> ---
>> tools/testing/selftests/landlock/fs_test.c | 261 +++++++++++++++++++++
>> 1 file changed, 261 insertions(+)
>>
>> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
>> index f513cd8d9d51..982cb824967c 100644
>> --- a/tools/testing/selftests/landlock/fs_test.c
>> +++ b/tools/testing/selftests/landlock/fs_test.c
>> @@ -3272,6 +3272,267 @@ TEST_F_FORK(layout1, truncate)
>> EXPECT_EQ(0, test_creat(file_in_dir_w));
>> }
>>
>> +/* Invokes chmod(2) and returns its errno or 0. */
>> +static int test_chmod(const char *const path, mode_t mode)
>> +{
>> + if (chmod(path, mode) < 0)
>> + return errno;
>> + return 0;
>> +}
>
> Nice, this is much simpler than in the last revision :)
>
>> +
>> +/* Invokes fchmod(2) and returns its errno or 0. */
>> +static int test_fchmod(int fd, mode_t mode)
>> +{
>> + if (fchmod(fd, mode) < 0)
>> + return errno;
>> + return 0;
>> +}
>> +
>> +/* Invokes fchmodat(2) and returns its errno or 0. */
>> +static int test_fchmodat(int dirfd, const char *path, mode_t mode, int flags)
>
> Nitpick: Some of these functions have path arguments declared as
>
> const char *path
>
> and others as
>
> const char *const path
>
> -- would be nice to stay consistent.
Thanks, const char *const path would be good to me.
>
>> +{
>> + if (fchmodat(dirfd, path, mode, flags) < 0)
>> + return errno;
>> + return 0;
>> +}
>> +
>> +/* Invokes chown(2) and returns its errno or 0. */
>> +static int test_chown(const char *path, uid_t uid, gid_t gid)
>> +{
>> + if (chown(path, uid, gid) < 0)
>> + return errno;
>> + return 0;
>> +}
>> +
>> +/* Invokes fchown(2) and returns its errno or 0. */
>> +static int test_fchown(int fd, uid_t uid, gid_t gid)
>> +{
>> + if (fchown(fd, uid, gid) < 0)
>> + return errno;
>> + return 0;
>> +}
>> +
>> +/* Invokes lchown(2) and returns its errno or 0. */
>> +static int test_lchown(const char *path, uid_t uid, gid_t gid)
>> +{
>> + if (lchown(path, uid, gid) < 0)
>> + return errno;
>> + return 0;
>> +}
>> +
>> +/* Invokes fchownat(2) and returns its errno or 0. */
>> +static int test_fchownat(int dirfd, const char *path,
>> + uid_t uid, gid_t gid, int flags)
>> +{
>> + if (fchownat(dirfd, path, uid, gid, flags) < 0)
>> + return errno;
>> + return 0;
>> +}
>> +
>> +TEST_F_FORK(layout1, unhandled_chmod)
>> +{
>> + int ruleset_fd, file1_fd;
>> + const char *file1 = file1_s1d1;
>> + const char *file2 = file2_s1d1;
>> + const char *dir1 = dir_s1d1;
>
> I'd suggest to name these kinds of variables according to the rights
> that are granted on these files and directories, for example as as
> w_file and rw_file. (Then, when looking at the EXPECT_EQ checks below,
> you don't need to jump back up to the start of the test to remember
> which of the rights is being tested.)
>
> Nitpick: The same remark as above about the 'const' modifier applies
> here as well. The rest of the file uses "const char *const varname".
Good point, will do it in v3.
>
>> + const struct rule rules[] = {
>> + {
>> + .path = file1,
>> + .access = LANDLOCK_ACCESS_FS_WRITE_FILE,
>> + },
>> + {
>> + .path = file2,
>> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
>> + LANDLOCK_ACCESS_FS_WRITE_FILE,
>> + },
>> + {
>> + .path = dir1,
>> + .access = ACCESS_RW,
>> + },
>> + {},
>> + };
>> +
>> + ruleset_fd = create_ruleset(_metadata, ACCESS_RW, rules);
>> + ASSERT_LE(0, ruleset_fd);
>> + file1_fd = open(file1, O_WRONLY | O_CLOEXEC);
>> + ASSERT_LE(0, file1_fd);
>> +
>> + enforce_ruleset(_metadata, ruleset_fd);
>> + ASSERT_EQ(0, close(ruleset_fd));
>> +
>> + EXPECT_EQ(0, test_chmod(file1, 0400));
>> + EXPECT_EQ(0, test_fchmod(file1_fd, 0400));
>> + EXPECT_EQ(0, test_fchmodat(AT_FDCWD, file1, 0400, 0));
>> + EXPECT_EQ(0, test_chmod(file2, 0400));
>> + EXPECT_EQ(0, test_chmod(dir1, 0700));
>> + ASSERT_EQ(0, close(file1_fd));
>> +}
>> +
>> +TEST_F_FORK(layout1, chmod)
>> +{
>> + int ruleset_fd, file1_fd;
>> + const char *file1 = file1_s1d1;
>> + const char *file2 = file2_s1d1;
>> + const char *file3 = file1_s2d1;
>> + const char *dir1 = dir_s1d1;
>> + const char *dir2 = dir_s2d1;
>> + const struct rule rules[] = {
>> + {
>> + .path = file1,
>> + .access = LANDLOCK_ACCESS_FS_WRITE_FILE |
>> + LANDLOCK_ACCESS_FS_CHMOD,
>> + },
>> + {
>> + .path = file2,
>> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
>> + LANDLOCK_ACCESS_FS_WRITE_FILE,
>> + },
>> + {
>> + .path = dir1,
>> + .access = ACCESS_RW,
>> + },
>> + {
>> + .path = dir2,
>> + .access = ACCESS_RW | LANDLOCK_ACCESS_FS_CHMOD,
>> + },
>> + {},
>> + };
>> +
>> + ruleset_fd = create_ruleset(_metadata, ACCESS_RW | LANDLOCK_ACCESS_FS_CHMOD, rules);
>> + ASSERT_LE(0, ruleset_fd);
>> + file1_fd = open(file1, O_WRONLY | O_CLOEXEC);
>> + ASSERT_LE(0, file1_fd);
>> +
>> + enforce_ruleset(_metadata, ruleset_fd);
>> + ASSERT_EQ(0, close(ruleset_fd));
>> +
>> + EXPECT_EQ(0, test_chmod(file1, 0400));
>> + EXPECT_EQ(0, test_fchmod(file1_fd, 0400));
>> + EXPECT_EQ(0, test_fchmodat(AT_FDCWD, file1, 0400, 0));
>> + EXPECT_EQ(EACCES, test_chmod(file2, 0400));
>> + EXPECT_EQ(EACCES, test_chmod(dir1, 0700));
>> + /* set CHMOD right on dir will only affect its context not dir itself*/
>> + EXPECT_EQ(0, test_chmod(file3, 0400));
>> + EXPECT_EQ(0, test_fchmodat(AT_FDCWD, file3, 0400, 0));
>> + EXPECT_EQ(EACCES, test_chmod(dir2, 0700));
>> + EXPECT_EQ(EACCES, test_fchmodat(AT_FDCWD, dir2, 0700, 0));
>> + ASSERT_EQ(0, close(file1_fd));
>> +}
>> +
>> +TEST_F_FORK(layout1, unhandled_chown)
>> +{
>> + int ruleset_fd, file1_fd;
>> + const char *file1 = file1_s1d1;
>> + const char *file2 = file2_s1d1;
>> + const char *dir1 = dir_s1d1;
>> + const struct rule rules[] = {
>> + {
>> + .path = file1,
>> + .access = LANDLOCK_ACCESS_FS_WRITE_FILE,
>> + },
>> + {
>> + .path = file2,
>> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
>> + LANDLOCK_ACCESS_FS_WRITE_FILE,
>> + },
>> + {
>> + .path = dir1,
>> + .access = ACCESS_RW,
>> + },
>> + {},
>> + };
>> + struct stat st;
>> + uid_t uid;
>> + gid_t gid;
>> +
>> + ruleset_fd = create_ruleset(_metadata, ACCESS_RW, rules);
>> + ASSERT_LE(0, ruleset_fd);
>> + file1_fd = open(file1, O_WRONLY | O_CLOEXEC);
>> + ASSERT_LE(0, file1_fd);
>> + /*
>> + * there is no CAP_CHOWN when the testcases framework setup,
>> + * and we cannot assume the testcases are run as root, to make
>> + * sure {f}chown syscall won't fail, get the original uid/gid and
>> + * use them in test_{f}chown.
>> + */
>> + ASSERT_EQ(0, stat(dir1, &st));
>> + uid = st.st_uid;
>> + gid = st.st_gid;
>> +
>> + enforce_ruleset(_metadata, ruleset_fd);
>> + ASSERT_EQ(0, close(ruleset_fd));
>> +
>> + EXPECT_EQ(0, test_chown(file1, uid, gid));
>> + EXPECT_EQ(0, test_fchown(file1_fd, uid, gid));
>> + EXPECT_EQ(0, test_lchown(file1, uid, gid));
>> + EXPECT_EQ(0, test_fchownat(AT_FDCWD, file1, uid, gid, 0));
>> + EXPECT_EQ(0, test_chown(file2, uid, gid));
>> + EXPECT_EQ(0, test_chown(dir1, uid, gid));
>> + ASSERT_EQ(0, close(file1_fd));
>> +}
>> +
>> +TEST_F_FORK(layout1, chown)
>> +{
>> + int ruleset_fd, file1_fd;
>> + const char *file1 = file1_s1d1;
>> + const char *file2 = file2_s1d1;
>> + const char *file3 = file1_s2d1;
>> + const char *dir1 = dir_s1d1;
>> + const char *dir2 = dir_s2d1;
>> + const struct rule rules[] = {
>> + {
>> + .path = file1,
>> + .access = LANDLOCK_ACCESS_FS_WRITE_FILE |
>> + LANDLOCK_ACCESS_FS_CHGRP,
>> + },
>> + {
>> + .path = file2,
>> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
>> + LANDLOCK_ACCESS_FS_WRITE_FILE,
>> + },
>> + {
>> + .path = dir1,
>> + .access = ACCESS_RW,
>> + },
>> + {
>> + .path = dir2,
>> + .access = ACCESS_RW | LANDLOCK_ACCESS_FS_CHGRP,
>> + },
>> + {},
>> + };
>> + struct stat st;
>> + uid_t uid;
>> + gid_t gid;
>> +
>> + ruleset_fd = create_ruleset(_metadata, ACCESS_RW | LANDLOCK_ACCESS_FS_CHGRP, rules);
>> + ASSERT_LE(0, ruleset_fd);
>> + file1_fd = open(file1, O_WRONLY | O_CLOEXEC);
>> + ASSERT_LE(0, file1_fd);
>> + /*
>> + * there is no CAP_CHOWN when the testcases framework setup,
>> + * and we cannot assume the testcases are run as root, to make
>> + * sure {f}chown syscall won't fail, get the original uid/gid and
>> + * use them in test_{f}chown.
>> + */
>> + ASSERT_EQ(0, stat(dir1, &st));
>> + uid = st.st_uid;
>> + gid = st.st_gid;
>> +
>> + enforce_ruleset(_metadata, ruleset_fd);
>> + ASSERT_EQ(0, close(ruleset_fd));
>> +
>> + EXPECT_EQ(0, test_chown(file1, uid, gid));
>> + EXPECT_EQ(0, test_fchown(file1_fd, uid, gid));
>> + EXPECT_EQ(0, test_lchown(file1, uid, gid));
>> + EXPECT_EQ(0, test_fchownat(AT_FDCWD, file1, uid, gid, 0));
>> + EXPECT_EQ(EACCES, test_chown(file2, uid, gid));
>> + EXPECT_EQ(EACCES, test_chown(dir1, uid, gid));
>> + /* set CHOWN right on dir will only affect its context not dir itself*/
>> + EXPECT_EQ(0, test_chown(file3, uid, gid));
>> + EXPECT_EQ(EACCES, test_chown(dir2, uid, gid));
>> + ASSERT_EQ(0, close(file1_fd));
>> +}
>> +
>> /* clang-format off */
>> FIXTURE(layout1_bind) {};
>> /* clang-format on */
>> --
>> 2.17.1
>>
>
> --
> .
>
Powered by blists - more mailing lists