Re: [PATCH v3 4/5] idmapped-mounts: Add new setgid_create_umask test

["xuyang2018.jy@fujitsu.com" <xuyang2018.jy@fujitsu.com>]

on 2022/4/13 16:59, Christian Brauner wrote:
> On Tue, Apr 12, 2022 at 07:33:45PM +0800, Yang Xu wrote:
>> The current_umask() is stripped from the mode directly in the vfs if the
>> filesystem either doesn't support acls or the filesystem has been
>> mounted without posic acl support.
>>
>> If the filesystem does support acls then current_umask() stripping is
>> deferred to posix_acl_create(). So when the filesystem calls
>> posix_acl_create() and there are no acls set or not supported then
>> current_umask() will be stripped.
>>
>> Here we only use umask(S_IXGRP) to check whether inode strip
>> S_ISGID works correctly.
>>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   src/idmapped-mounts/idmapped-mounts.c | 505 +++++++++++++++++++++++++-
>>   tests/generic/680                     |  26 ++
>>   tests/generic/680.out                 |   2 +
>>   3 files changed, 532 insertions(+), 1 deletion(-)
>>   create mode 100755 tests/generic/680
>>   create mode 100644 tests/generic/680.out
>>
>> diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
>> index 02f91558..e6c14586 100644
>> --- a/src/idmapped-mounts/idmapped-mounts.c
>> +++ b/src/idmapped-mounts/idmapped-mounts.c
>> @@ -14146,6 +14146,494 @@ out:
>>   	return fret;
>>   }
>>
>> +/* The following tests are concerned with setgid inheritance. These can be
>> + * filesystem type specific. For xfs, if a new file or directory or node is
>> + * created within a setgid directory and irix_sgid_inhiert is set then inheritthe
>> + * setgid bit if the caller is in the group of the directory.
>> + *
>> + * The current_umask() is stripped from the mode directly in the vfs if the
>> + * filesystem either doesn't support acls or the filesystem has been
>> + * mounted without posic acl support.
>> + *
>> + * If the filesystem does support acls then current_umask() stripping is
>> + * deferred to posix_acl_create(). So when the filesystem calls
>> + * posix_acl_create() and there are no acls set or not supported then
>> + * current_umask() will be stripped.
>> + *
>> + * Use umask(S_IXGRP) to check whether inode strip S_ISGID works correctly.
>> + */
>> +static int setgid_create_umask(void)
>> +{
>> +	int fret = -1;
>> +	int file1_fd = -EBADF;
>> +	int tmpfile_fd = -EBADF;
>> +	pid_t pid;
>> +	bool supported = false;
>> +	char path[PATH_MAX];
>> +	mode_t mode;
>> +
>> +	if (!caps_supported())
>> +		return 0;
>> +
>> +	if (fchmod(t_dir1_fd, S_IRUSR |
>> +			      S_IWUSR |
>> +			      S_IRGRP |
>> +			      S_IWGRP |
>> +			      S_IROTH |
>> +			      S_IWOTH |
>> +			      S_IXUSR |
>> +			      S_IXGRP |
>> +			      S_IXOTH |
>> +			      S_ISGID), 0) {
>> +		log_stderr("failure: fchmod");
>> +		goto out;
>> +	}
>> +
>> +	   /* Verify that the setgid bit got raised. */
>> +	if (!is_setgid(t_dir1_fd, "", AT_EMPTY_PATH)) {
>> +		log_stderr("failure: is_setgid");
>> +		goto out;
>> +	}
>> +
>> +	supported = openat_tmpfile_supported(t_dir1_fd);
>> +
>> +	/* Only umask with S_IXGRP because inode strip S_ISGID will check mode
>> +	 * whether has group execute or search permission.
>> +	 */
>> +	umask(S_IXGRP);
>> +	mode = umask(S_IXGRP);
>> +	if (!(mode&  S_IXGRP))
>> +		die("failure: umask");
>> +
>> +	pid = fork();
>> +	if (pid<  0) {
>> +		log_stderr("failure: fork");
>> +		goto out;
>> +	}
>> +	if (pid == 0) {
>> +		if (!switch_ids(0, 10000))
>> +			die("failure: switch_ids");
>> +
>> +		if (!caps_down_fsetid())
>> +			die("failure: caps_down_fsetid");
>> +
>> +		/* create regular file via open() */
>> +		file1_fd = openat(t_dir1_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
>> +		if (file1_fd<  0)
>> +			die("failure: create");
>> +
>> +		/* Neither in_group_p() nor capable_wrt_inode_uidgid() so setgid
>> +		 * bit needs to be stripped.
>> +		 */
>> +		if (is_setgid(t_dir1_fd, FILE1, 0))
>> +			die("failure: is_setgid");
>
> This test is wrong. Specifically, it is a false positive. I have
> explained this in more detail on v2 of this patchset.
>
> You want to test that umask(S_IXGRP) + setgid inheritance work together
> correctly. First, we need to establish what that means because from your
> patch series it at least seems to me as you're not completely clear
> about what you want to test just yet.
>
> A requested setgid bit for a non-directory object is only considered for
> stripping if S_IXGRP is simultaneously requested. In other words, both
> S_SISGID and S_IXGRP must be requested for the new file in order for
> additional checks such as CAP_FSETID to become relevant.
Yes, only keep S_IXGRP, then we can run into the next judgement for 
group and cap_fsetid.
>
> We need to distinguish two cases afaict:
>
> 1. The filesystem does support ACLs and has an applicable ACL
> -------------------------------------------------------------
>
> umask(S_IXGRP) is not used by the kernel and thus S_IXGRP is not
> stripped (unless the ACL does make it so) and so when e.g.
> inode_init_owner() is called we do not expect the file to inherit the
> setgid bit.
>
> This is what your test above is handling correctly. But it is a false
> positive because what you're trying to test is the behavior of
> umask(S_IXGRP) but it is made irrelevant by ACL support of the
> underlying filesystem.
I test this situation in the next patch as below:
umask(S_IXGRP);
snprintf(t_buf, sizeof(t_buf), "setfacl -d -m u::rwx,g::rwx,o::rwx,m:rw 
%s/%s", t_mountpoint, T_DIR1)

and
umask(S_IXGRP);
snprintf(t_buf, sizeof(t_buf), "setfacl -d -m u::rwx,g::rw,o::rwx, 
%s/%s", t_mountpoint, T_DIR1

>
> 2. The filesystem does not support ACLs, has been mounted without
> -----------------------------------------------------------------
> support for it, or has no applicable ACL
> ----------------------------------------
>
> umask(S_IXGRP) is used by the kernel and will be stripped from the mode.
> So when inode_init_owner() is called we expect the file to inherit the
> setgid bit.
This is why my kernel patch put strip setgid code into vfs.
xfs will inherit the setgid bit but ext4 not because the new_inode 
function and posix_acl_create function order(S_IXGRP mode has been 
stripped before pass to inode_init_owner).
>
> This means the test for this case needs to be:
>
> 	file1_fd = openat(t_dir1_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
> 	if (file1_fd<  0)
> 		die("failure: create");
> 	
> 	/*
> 	 * S_IXGRP will have been removed due to umask(S_IXGRP) so we expect the
> 	 * new file to be S_ISGID.
No No No, see the kernel patch url
https://patchwork.kernel.org/project/linux-fsdevel/patch/1648461389-2225-2-git-send-email-xuyang2018.jy@fujitsu.com/
> 	 */
> 	if (!is_setgid(t_dir1_fd, FILE1, 0))
> 		die("failure: is_setgid");
>
> And additionally you might want to also add a new helper is_ixgrp() to
> add an additional check:
>
> /*
>   * S_IXGRP will have been removed due to umask(S_IXGRP) so we expect the
>   * new file to not be S_IXGRP.
>   */
> if (is_ixgrp(t_dir1_fd, FILE1, 0))
> 	die("failure: is_ixgrp");
This sound reasonable.
>
>
> ---
>
> So for the umask() tests you need to first check whether the underlying
> filesystem does support ACLs and remember that somewhere. If it does
> support ACLs, then you should first remove any ACLs set on the
> directories you're performing the tests on. Then you can run your
> umask() tests.
Yes, I can just add a remove step in the beginning.

ps: When I writing my v2 kernel patch, I found I still miss the GRPID 
testcase in fstests.

Best Regrads
Yang Xu