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@xxxxxxxxxxx>
> ---
> 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.
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.
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 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.
*/
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");
---
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.
