Re: [PATCH -next 3/5] landlock/selftests: add selftests for chmod and chown

From: "Mickaël Salaün" <mic@digikod.net>
To: "Günther Noack" <gnoack3000@gmail.com>,
	"Xiu Jianfeng" <xiujianfeng@huawei.com>
Cc: paul@paul-moore.com, jmorris@namei.org, serge@hallyn.com,
	shuah@kernel.org, corbet@lwn.net,
	linux-security-module@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
	linux-doc@vger.kernel.org
Subject: Re: [PATCH -next 3/5] landlock/selftests: add selftests for chmod and chown
Date: Mon, 22 Aug 2022 23:28:37 +0200	[thread overview]
Message-ID: <223344b1-036a-2f25-727f-6262e1b7a763@digikod.net> (raw)
In-Reply-To: <YwPQpz0lV5CVBVeK@nuc>

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@huawei.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
>>
> 
> --