Re: [RFC RESEND] xfs: fix up non-directory creation in SGID directories when umask S_IXGRP

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

on 2022/3/22 17:23, Dave Chinner wrote:
> On Tue, Mar 22, 2022 at 07:43:20PM +1100, Dave Chinner wrote:
>> On Tue, Mar 22, 2022 at 04:04:17PM +0800, Yang Xu wrote:
>>> Petr reported a problem that S_ISGID bit was not clean when testing ltp
>>> case create09[1] by using umask(077).
>>
>> Ok. So what is the failure message from the test?
>>
>> When did the test start failing? Is this a recent failure or
>> something that has been around for years? If it's recent, what
>> commit broke it?
>
> Ok, I went and looked at the test, and it confirmed my suspicion.  I
> can't find the patch that introduced this change on lore.kernel.org.
> Looks like one of those silent security fixes that nobody gets told
> about, gets no real review, has no test cases written for it, etc.
>
> And nobody wrote a test for until August 2021 and that's when people
> started to notice broken filesystems.
>
> This is the commit that failed to fix several filesystems:
>
> commit 0fa3ecd87848c9c93c2c828ef4c3a8ca36ce46c7
> Author: Linus Torvalds<torvalds@linux-foundation.org>
> Date:   Tue Jul 3 17:10:19 2018 -0700
>
>      Fix up non-directory creation in SGID directories
>
>      sgid directories have special semantics, making newly created files in
>      the directory belong to the group of the directory, and newly created
>      subdirectories will also become sgid.  This is historically used for
>      group-shared directories.
>
>      But group directories writable by non-group members should not imply
>      that such non-group members can magically join the group, so make sure
>      to clear the sgid bit on non-directories for non-members (but remember
>      that sgid without group execute means "mandatory locking", just to
>      confuse things even more).
>
>      Reported-by: Jann Horn<jannh@google.com>
>      Cc: Andy Lutomirski<luto@kernel.org>
>      Cc: Al Viro<viro@zeniv.linux.org.uk>
>      Signed-off-by: Linus Torvalds<torvalds@linux-foundation.org>
>
> diff --git a/fs/inode.c b/fs/inode.c
> index 2c300e981796..8c86c809ca17 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1999,8 +1999,14 @@ void inode_init_owner(struct inode *inode, const struct inode *dir,
>          inode->i_uid = current_fsuid();
>          if (dir&&  dir->i_mode&  S_ISGID) {
>                  inode->i_gid = dir->i_gid;
> +
> +               /* Directories are special, and always inherit S_ISGID */
>                  if (S_ISDIR(mode))
>                          mode |= S_ISGID;
> +               else if ((mode&  (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)&&
> +                        !in_group_p(inode->i_gid)&&
> +                        !capable_wrt_inode_uidgid(dir, CAP_FSETID))
> +                       mode&= ~S_ISGID;
>          } else
>                  inode->i_gid = current_fsgid();
>          inode->i_mode = mode;
>
> The problem is it takes away mode bits that the VFS passed to us
> deep in the VFS inode initialisation done during on-disk inode
> initialisation, and it's hidden well away from sight of the
> filesystems.
>
> Oh, what a mess - this in_group_p()&&  capable_wrt_inode_uidgid()
> check is splattered all over filesystems in random places to clear
> SGID bits. e.g ceph_finish_async_create() is an open coded
> inode_init_owner() call. There's special case
> code in fuse_set_acl() to clear SGID. There's a special case in
> ovl_posix_acl_xattr_set() for ACL xattrs to clear SGID. And so on.
>
> No consistency anywhere - shouldn't the VFS just be stripping the
> SGID bit before it passes the mode down to filesystems? It has all
> the info it needs - it doesn't need the filesystems to do everything
> correctly with the mode and ensuring that they order things like
> posix acl setup functions correctly with inode_init_owner() to strip
> the SGID bit...
>
> Just strip the SGID bit at the VFS, and then the filesystems can't
> get it wrong...
Thanks for your reply.

Sound reasonable, but I am not sure I have the ability to do this.
Will try ...


Best Regards
Yang Xu
>
> Cheers,
>
> Dave.