Re: [PATCH v2 2/3] vfs: strip file's S_ISGID mode on vfs instead of on underlying filesystem

From: "xuyang2018.jy@fujitsu.com" <xuyang2018.jy@fujitsu.com>
To: Christian Brauner <brauner@kernel.org>,
	"djwong@kernel.org" <djwong@kernel.org>
Cc: "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	"ceph-devel@vger.kernel.org" <ceph-devel@vger.kernel.org>,
	"ocfs2-devel@oss.oracle.com" <ocfs2-devel@oss.oracle.com>,
	"viro@zeniv.linux.org.uk" <viro@zeniv.linux.org.uk>,
	"david@fromorbit.com" <david@fromorbit.com>,
	"jlayton@kernel.org" <jlayton@kernel.org>
Subject: Re: [PATCH v2 2/3] vfs: strip file's S_ISGID mode on vfs instead of on underlying filesystem
Date: Fri, 15 Apr 2022 09:06:25 +0000	[thread overview]
Message-ID: <625943E1.2000603@fujitsu.com> (raw)
In-Reply-To: <6258F17C.7010705@fujitsu.com>

on 2022/4/15 12:15, Yang Xu wrote:
> on 2022/4/14 20:45, Christian Brauner wrote:
>> On Thu, Apr 14, 2022 at 03:57:18PM +0800, Yang Xu wrote:
>>> Currently, vfs only passes mode argument to filesystem, then use
>>> inode_init_owner()
>>> to strip S_ISGID. Some filesystem(ie ext4/btrfs) will call
>>> inode_init_owner
>>> firstly, then posxi acl setup, but xfs uses the contrary order. It
>>> will affect
>>> S_ISGID clear especially we filter S_IXGRP by umask or acl.
>>>
>>> Regardless of which filesystem is in use, failure to strip the SGID
>>> correctly is
>>> considered a security failure that needs to be fixed. The current VFS
>>> infrastructure
>>> requires the filesystem to do everything right and not step on any
>>> landmines to
>>> strip the SGID bit, when in fact it can easily be done at the VFS and
>>> the filesystems
>>> then don't even need to be aware that the SGID needs to be (or has
>>> been stripped) by
>>> the operation the user asked to be done.
>>>
>>> Vfs 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.
>>>
>>> Also, the inode_sgid_strip() api should be used before IS_POSIXACL()
>>> because
>>> this api may change mode.
>>>
>>> Only the following places use inode_init_owner
>>> "hugetlbfs/inode.c:846: inode_init_owner(&init_user_ns, inode, dir,
>>> mode);
>>> nilfs2/inode.c:354: inode_init_owner(&init_user_ns, inode, dir, mode);
>>> zonefs/super.c:1289: inode_init_owner(&init_user_ns, inode, parent,
>>> S_IFDIR | 0555);
>>> reiserfs/namei.c:619: inode_init_owner(&init_user_ns, inode, dir, mode);
>>> jfs/jfs_inode.c:67: inode_init_owner(&init_user_ns, inode, parent,
>>> mode);
>>> f2fs/namei.c:50: inode_init_owner(mnt_userns, inode, dir, mode);
>>> ext2/ialloc.c:549: inode_init_owner(&init_user_ns, inode, dir, mode);
>>> overlayfs/dir.c:643: inode_init_owner(&init_user_ns, inode,
>>> dentry->d_parent->d_inode, mode);
>>> ufs/ialloc.c:292: inode_init_owner(&init_user_ns, inode, dir, mode);
>>> ntfs3/inode.c:1283: inode_init_owner(mnt_userns, inode, dir, mode);
>>> ramfs/inode.c:64: inode_init_owner(&init_user_ns, inode, dir, mode);
>>> 9p/vfs_inode.c:263: inode_init_owner(&init_user_ns, inode, NULL, mode);
>>> btrfs/tests/btrfs-tests.c:65: inode_init_owner(&init_user_ns, inode,
>>> NULL, S_IFREG);
>>> btrfs/inode.c:6215: inode_init_owner(mnt_userns, inode, dir, mode);
>>> sysv/ialloc.c:166: inode_init_owner(&init_user_ns, inode, dir, mode);
>>> omfs/inode.c:51: inode_init_owner(&init_user_ns, inode, NULL, mode);
>>> ubifs/dir.c:97: inode_init_owner(&init_user_ns, inode, dir, mode);
>>> udf/ialloc.c:108: inode_init_owner(&init_user_ns, inode, dir, mode);
>>> ext4/ialloc.c:979: inode_init_owner(mnt_userns, inode, dir, mode);
>>> hfsplus/inode.c:393: inode_init_owner(&init_user_ns, inode, dir, mode);
>>> xfs/xfs_inode.c:840: inode_init_owner(mnt_userns, inode, dir, mode);
>>> ocfs2/dlmfs/dlmfs.c:331: inode_init_owner(&init_user_ns, inode, NULL,
>>> mode);
>>> ocfs2/dlmfs/dlmfs.c:354: inode_init_owner(&init_user_ns, inode,
>>> parent, mode);
>>> ocfs2/namei.c:200: inode_init_owner(&init_user_ns, inode, dir, mode);
>>> minix/bitmap.c:255: inode_init_owner(&init_user_ns, inode, dir, mode);
>>> bfs/dir.c:99: inode_init_owner(&init_user_ns, inode, dir, mode);
>>> "
>>
>> For completeness sake, there's also spufs which doesn't really go
>> through the regular VFS callpath because it has separate system calls
>> like:
>>
>> SYSCALL_DEFINE4(spu_create, const char __user *, name, unsigned int,
>> flags,
>> umode_t, mode, int, neighbor_fd)
>>
>> but looking through the code it only allows the creation of
>> directories and only
>> allows bits in 0777.
> IMO, this fs also doesn't use inode_init_owner, so it should be not
> affected. We add indo_sgid_strip into vfs, IMO, it only happen new sgid
> strip situation and doesn't happen to remove old sgid strip situation.
> So I think it is "safe".
>>
>>>
>>> They are used in filesystem init new inode function and these init
>>> inode functions are used
>>> by following operations:
>>> mkdir
>>> symlink
>>> mknod
>>> create
>>> tmpfile
>>> rename
>>>
>>> We don't care about mkdir because we don't strip SGID bit for
>>> directory except fs.xfs.irix_sgid_inherit.
>>> symlink and rename only use valid mode that doesn't have SGID bit.
>>>
>>> We have added inode_sgid_strip api for the remaining operations.
>>>
>>> In addition to the above six operations, two filesystems has a little
>>> difference
>>> 1) btrfs has btrfs_create_subvol_root to create new inode but used
>>> non SGID bit mode and can ignore
>>> 2) ocfs2 reflink function should add inode_sgid_strip api manually
>>> because we don't add it in vfs
>>>
>>> Last but not least, this patch also changed grpid behaviour for
>>> ext4/xfs because the mode passed to
>>> them may been changed by inode_sgid_strip.
>>
>> I think the patch itself is useful as it would move a security sensitive
>> operation that is currently burried in individual filesystems into the
>> vfs layer. But it has a decent regression potential since it might trip
>> filesystems that have so far relied on getting the S_ISGID bit with a
>> mode argument. The example being network filesystems that Jeff brought
>> up earlier. So this needs a lot of testing and long exposure in -next
>> for at least one full kernel cycle imho.
> Agreed.
>>
>>>
>>> Suggested-by: Dave Chinner<david@fromorbit.com>
>>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>>> ---
>>> fs/inode.c | 4 ----
>>> fs/namei.c | 5 ++++-
>>> fs/ocfs2/namei.c | 1 +
>>> 3 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/inode.c b/fs/inode.c
>>> index d63264998855..b08bdd73e116 100644
>>> --- a/fs/inode.c
>>> +++ b/fs/inode.c
>>> @@ -2246,10 +2246,6 @@ void inode_init_owner(struct user_namespace
>>> *mnt_userns, struct inode *inode,
>>> /* 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(i_gid_into_mnt(mnt_userns, dir))&&
>>> - !capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
>>> - mode&= ~S_ISGID;
>>> } else
>>> inode_fsgid_set(inode, mnt_userns);
>>> inode->i_mode = mode;
>>> diff --git a/fs/namei.c b/fs/namei.c
>>> index 3f1829b3ab5b..e03f7defdd30 100644
>>> --- a/fs/namei.c
>>> +++ b/fs/namei.c
>>> @@ -3287,6 +3287,7 @@ static struct dentry *lookup_open(struct
>>> nameidata *nd, struct file *file,
>>> if (open_flag& O_CREAT) {
>>> if (open_flag& O_EXCL)
>>> open_flag&= ~O_TRUNC;
>>> + inode_sgid_strip(mnt_userns, dir->d_inode,&mode);
>>> if (!IS_POSIXACL(dir->d_inode))
>>> mode&= ~current_umask();
>>> if (likely(got_write))
>>> @@ -3521,6 +3522,7 @@ struct dentry *vfs_tmpfile(struct
>>> user_namespace *mnt_userns,
>>> child = d_alloc(dentry,&slash_name);
>>> if (unlikely(!child))
>>> goto out_err;
>>> + inode_sgid_strip(mnt_userns, dir,&mode);
>>
>> Hm, an additional question: how is umask stripping currently handled in
>> vfs_tmpfile()? I don't see it anywhere. That seems like a bug?
> Yes, I think it is a bug.
Since you found this bug and I have finished my v3 kernel patch set(also 
fix this tmpfile umask problem and add your reported-by, also two 
patches about other problem in xfs/nfs ), so do you will fix this kernel 
bug or I send a v3 directly?

Best Regards
Yang Xu
>
> You can verify this by setting
> export MOUNT_OPTIONS='-o noacl'
> in your xfstests config.
>
> then in my setgid_create_umask test, you can add is_ixgrp check for
> tmpfile and it still have S_IXGRP mode(So yesterday, add additional
> check for this mode is meaningful).
>
> For xfs, it seems doesn't have noacl mount options, just disable
> CONFIG_XFS_POSIX_ACL.
> But, xfs doesn't have this problem becuase it doesn't put
> posix_acl_create under CONFIG_XFS_POSIX_ACL situation.
>
> I think we should add this umask stripping here and also should
> put xfs posix_acl_create code into CONFIG_XFS_POSIX_ACL situation.
>
> @Darrick What do you think the xfs code change ?
>
> commit mesage may as below:
> xfs/xfs_iops: Only do posix acl setup operation under CONFIG_XFS_POSIX_ACL
>
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index b34e8e4344a8..61c1d4e85891 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -146,10 +146,13 @@ xfs_create_need_xattr(
> struct posix_acl *default_acl,
> struct posix_acl *acl)
> {
> +#ifdef CONFIG_XFS_POSIX_ACL
> if (acl)
> return true;
> if (default_acl)
> return true;
> +#endif
> +
> #if IS_ENABLED(CONFIG_SECURITY)
> if (dir->i_sb->s_security)
> return true;
> @@ -183,10 +186,11 @@ xfs_generic_create(
> } else {
> rdev = 0;
> }
> -
> +#ifdef CONFIG_XFS_POSIX_ACL
> error = posix_acl_create(dir, &mode, &default_acl, &acl);
> if (error)
> return error;
> +#endif
>
> /* Verify mode is valid also for tmpfile case */
> error = xfs_dentry_mode_to_name(&name, dentry, mode);
>
>
> Best Regards
> Yang Xu
>>
>>> error = dir->i_op->tmpfile(mnt_userns, dir, child, mode);
>>> if (error)
>>> goto out_err;
>>> @@ -3850,13 +3852,14 @@ static int do_mknodat(int dfd, struct
>>> filename *name, umode_t mode,
>>> if (IS_ERR(dentry))
>>> goto out1;
>>>
>>> + mnt_userns = mnt_user_ns(path.mnt);
>>> + inode_sgid_strip(mnt_userns, path.dentry->d_inode,&mode);
>>> if (!IS_POSIXACL(path.dentry->d_inode))
>>> mode&= ~current_umask();
>>
>> It would be worth to add another helper prepare_mode() that calls
>> inode_sgid_strip() and does the umask stripping as well and then call it
>> in all these places. You should even call it in do_mkdirat() since
>> inode_sgid_strip() will skip directories anyway. This will enforce the
>> same ordering for all relevant operations and it will make the code more
>> uniform and easier to understand.
> Sound reasonable. I will think of it.
>>
>>> error = security_path_mknod(&path, dentry, mode, dev);
>>> if (error)
>>> goto out2;
>>>
>>> - mnt_userns = mnt_user_ns(path.mnt);
>>> switch (mode& S_IFMT) {
>>> case 0: case S_IFREG:
>>> error = vfs_create(mnt_userns, path.dentry->d_inode,
>>> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
>>> index c75fd54b9185..f1d626697302 100644
>>> --- a/fs/ocfs2/namei.c
>>> +++ b/fs/ocfs2/namei.c
>>> @@ -197,6 +197,7 @@ static struct inode *ocfs2_get_init_inode(struct
>>> inode *dir, umode_t mode)
>>> * callers. */
>>> if (S_ISDIR(mode))
>>> set_nlink(inode, 2);
>>> + inode_sgid_strip(&init_user_ns, dir,&mode);
>>> inode_init_owner(&init_user_ns, inode, dir, mode);
>>> status = dquot_initialize(inode);
>>> if (status)
>>> --
>>> 2.27.0
>>>
>>>