| Message ID | 1648461389-2225-2-git-send-email-xuyang2018.jy@fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/3] vfs: Add inode_sgid_strip() api | expand |
On Mon, Mar 28, 2022 at 05:56:28PM +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 umask with S_IXGRP. > > 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 by using umask but S_ISGID clear isn't related to > SB_POSIXACL flag. > > Suggested-by: Dave Chinner <david@fromorbit.com> > Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> > --- I think adding that helper and using it in the vfs already is a good idea. But I wonder whether leaving this in inode_init_owner() might be desirable as well. I don't know how likely it is but if any filesystem is somehow internally creating a new inode without using vfs_*() helpers and botches the job then inode_init_owner() would still correctly strip the setgid bit currently for them. If we think it's a rather low risk then we can simply move the strippping completely out of inode_init_owner(). If we think that that's too risky it might be worth adding a new inode_owner() helper that is called from inode_init_owner() and that filesystem can be switched to that we know are safe in that regard?
On Mon, 2022-03-28 at 17:56 +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 umask with S_IXGRP. > > 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 by using umask but S_ISGID clear isn't related to > SB_POSIXACL flag. > > Suggested-by: Dave Chinner <david@fromorbit.com> > Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> > --- > fs/inode.c | 4 ---- > fs/namei.c | 7 +++++-- > 2 files changed, 5 insertions(+), 6 deletions(-) > > diff --git a/fs/inode.c b/fs/inode.c > index 1f964e7f9698..a2dd71c2437e 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..e68a99e0ac96 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,8 @@ 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); > + > error = dir->i_op->tmpfile(mnt_userns, dir, child, mode); > if (error) > goto out_err; > @@ -3849,14 +3852,14 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode, > error = PTR_ERR(dentry); > 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(); > 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, I haven't gone over this in detail, but have you tested this with NFS at all? IIRC, NFS has to leave setuid/gid stripping to the server, so I wonder if this may end up running afoul of that by forcing the client to try and strip these bits.
On Tue, Mar 29, 2022 at 07:12:11AM -0400, Jeff Layton wrote: > On Mon, 2022-03-28 at 17:56 +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 umask with S_IXGRP. > > > > 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 by using umask but S_ISGID clear isn't related to > > SB_POSIXACL flag. > > > > Suggested-by: Dave Chinner <david@fromorbit.com> > > Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> > > --- > > fs/inode.c | 4 ---- > > fs/namei.c | 7 +++++-- > > 2 files changed, 5 insertions(+), 6 deletions(-) > > > > diff --git a/fs/inode.c b/fs/inode.c > > index 1f964e7f9698..a2dd71c2437e 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..e68a99e0ac96 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,8 @@ 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); > > + > > error = dir->i_op->tmpfile(mnt_userns, dir, child, mode); > > if (error) > > goto out_err; > > @@ -3849,14 +3852,14 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode, > > error = PTR_ERR(dentry); > > 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(); > > 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, > > I haven't gone over this in detail, but have you tested this with NFS at > all? > > IIRC, NFS has to leave setuid/gid stripping to the server, so I wonder > if this may end up running afoul of that by forcing the client to try > and strip these bits. All it means is that the mode passed to the NFS server for the create already has the SGID bit stripped from it. It means the client is no longer reliant on the server behaving correctly to close this security hole. That is, failing to strip the SGID bit appropriately in the local context is a security issue. Hence local machine security requires that the NFS client should try to strip the SGID to defend against buggy/unfixed servers that fail to strip it appropriately and thereby continute to expose the local machine to this SGID security issue. That's the problem here - the SGID stripping in inode_init_owner() is not documented, wasn't reviewed, doesn't work correctly across all filesystems and leaves nasty security landmines when the VFS create mode and the stripped inode mode differ. Various filesystems have workarounds, partial fixes or no fixes for these issues and landmines. Hence we have a situation where we are playing whack-a-mole to discover and slap band-aids over all the places that inode_init_owner() based stripping does not work correctly. In XFS, this meant the problem was not orginally fixed by the silent, unreviewed change to inode_init_owner() in 2018 because it didn't call inode_init_owner() at all. So 4 years after the bug was "fixed" and the CVE released, we are still exposed to the bug because *no filesystem people knew about it* and *nobody wrote a regression test* to check that the probelm was fixed and stayed fixed. And now that XFS does call inode_init_owner(), we've subsequently discovered that XFS still fail when default acls are enabled because we create the ACL from the mode passed from the VFS, not the stripped mode that results from inode_init_owner() being called. See what I mean about landmines? The fact is this: 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. We need the architecture to be *secure by design*, not tacked onto the side like it is now. We need to stop trying to dance around these landmines - it is *not working* and we are blowing our own feet off repeatedly. This hurts a lot (especially in distro land) so we need to take the responsibility for stripping SGID properly away from the filesystems and put it where it belongs: in the VFS. Cheers, Dave.
On Wed, Mar 30, 2022 at 09:10:59AM +1100, Dave Chinner wrote: > On Tue, Mar 29, 2022 at 07:12:11AM -0400, Jeff Layton wrote: > > On Mon, 2022-03-28 at 17:56 +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 umask with S_IXGRP. > > > > > > 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 by using umask but S_ISGID clear isn't related to > > > SB_POSIXACL flag. > > > > > > Suggested-by: Dave Chinner <david@fromorbit.com> > > > Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> > > > --- > > > fs/inode.c | 4 ---- > > > fs/namei.c | 7 +++++-- > > > 2 files changed, 5 insertions(+), 6 deletions(-) > > > > > > diff --git a/fs/inode.c b/fs/inode.c > > > index 1f964e7f9698..a2dd71c2437e 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..e68a99e0ac96 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,8 @@ 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); > > > + > > > error = dir->i_op->tmpfile(mnt_userns, dir, child, mode); > > > if (error) > > > goto out_err; > > > @@ -3849,14 +3852,14 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode, > > > error = PTR_ERR(dentry); > > > 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(); > > > 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, > > > > I haven't gone over this in detail, but have you tested this with NFS at > > all? > > > > IIRC, NFS has to leave setuid/gid stripping to the server, so I wonder > > if this may end up running afoul of that by forcing the client to try > > and strip these bits. > > All it means is that the mode passed to the NFS server for the > create already has the SGID bit stripped from it. It means the > client is no longer reliant on the server behaving correctly to > close this security hole. > > That is, failing to strip the SGID bit appropriately in the local > context is a security issue. Hence local machine security requires > that the NFS client should try to strip the SGID to defend against > buggy/unfixed servers that fail to strip it appropriately and > thereby continute to expose the local machine to this SGID security > issue. > > That's the problem here - the SGID stripping in inode_init_owner() > is not documented, wasn't reviewed, doesn't work correctly > across all filesystems and leaves nasty security landmines when the VFS > create mode and the stripped inode mode differ. > > Various filesystems have workarounds, partial fixes or no fixes for > these issues and landmines. Hence we have a situation where we are > playing whack-a-mole to discover and slap band-aids over all the > places that inode_init_owner() based stripping does not work > correctly. > > In XFS, this meant the problem was not orginally fixed by the > silent, unreviewed change to inode_init_owner() in 2018 > because it didn't call inode_init_owner() at all. So 4 years after > the bug was "fixed" and the CVE released, we are still exposed to > the bug because *no filesystem people knew about it* and *nobody wrote a > regression test* to check that the probelm was fixed and stayed > fixed. > > And now that XFS does call inode_init_owner(), we've subsequently > discovered that XFS still fail when default acls are enabled because > we create the ACL from the mode passed from the VFS, not the > stripped mode that results from inode_init_owner() being called. > > See what I mean about landmines? > > The fact is this: 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. > > We need the architecture to be *secure by design*, not tacked onto > the side like it is now. We need to stop trying to dance around > these landmines - it is *not working* and we are blowing our own > feet off repeatedly. This hurts a lot (especially in distro land) > so we need to take the responsibility for stripping SGID properly > away from the filesystems and put it where it belongs: in the VFS. I agree. When I added tests for set*id stripping to xfstests for the sake of getting complete vfs coverage of idmapped mounts in generic/633 I immediately found bugs. Once I made the testsuite useable by all filesystems we started seeing more. I think we should add and use the new proposed stripping helper in the vfs - albeit with a slightly changed api and also use it in inode_init_owner(). While it is a delicate change in the worst case we end up removing additional privileges that's an acceptable regression risk to take.
On Wed, Mar 30, 2022 at 12:44:19PM +0200, Christian Brauner wrote: > On Wed, Mar 30, 2022 at 09:10:59AM +1100, Dave Chinner wrote: > > On Tue, Mar 29, 2022 at 07:12:11AM -0400, Jeff Layton wrote: > > > On Mon, 2022-03-28 at 17:56 +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 umask with S_IXGRP. > > > > > > > > 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 by using umask but S_ISGID clear isn't related to > > > > SB_POSIXACL flag. > > > > > > > > Suggested-by: Dave Chinner <david@fromorbit.com> > > > > Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> > > > > --- > > > > fs/inode.c | 4 ---- > > > > fs/namei.c | 7 +++++-- > > > > 2 files changed, 5 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/fs/inode.c b/fs/inode.c > > > > index 1f964e7f9698..a2dd71c2437e 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..e68a99e0ac96 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,8 @@ 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); > > > > + > > > > error = dir->i_op->tmpfile(mnt_userns, dir, child, mode); > > > > if (error) > > > > goto out_err; > > > > @@ -3849,14 +3852,14 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode, > > > > error = PTR_ERR(dentry); > > > > 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(); > > > > 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, > > > > > > I haven't gone over this in detail, but have you tested this with NFS at > > > all? > > > > > > IIRC, NFS has to leave setuid/gid stripping to the server, so I wonder > > > if this may end up running afoul of that by forcing the client to try > > > and strip these bits. > > > > All it means is that the mode passed to the NFS server for the > > create already has the SGID bit stripped from it. It means the > > client is no longer reliant on the server behaving correctly to > > close this security hole. > > > > That is, failing to strip the SGID bit appropriately in the local > > context is a security issue. Hence local machine security requires > > that the NFS client should try to strip the SGID to defend against > > buggy/unfixed servers that fail to strip it appropriately and > > thereby continute to expose the local machine to this SGID security > > issue. > > > > That's the problem here - the SGID stripping in inode_init_owner() > > is not documented, wasn't reviewed, doesn't work correctly > > across all filesystems and leaves nasty security landmines when the VFS > > create mode and the stripped inode mode differ. > > > > Various filesystems have workarounds, partial fixes or no fixes for > > these issues and landmines. Hence we have a situation where we are > > playing whack-a-mole to discover and slap band-aids over all the > > places that inode_init_owner() based stripping does not work > > correctly. > > > > In XFS, this meant the problem was not orginally fixed by the > > silent, unreviewed change to inode_init_owner() in 2018 > > because it didn't call inode_init_owner() at all. So 4 years after > > the bug was "fixed" and the CVE released, we are still exposed to > > the bug because *no filesystem people knew about it* and *nobody wrote a > > regression test* to check that the probelm was fixed and stayed > > fixed. > > > > And now that XFS does call inode_init_owner(), we've subsequently > > discovered that XFS still fail when default acls are enabled because > > we create the ACL from the mode passed from the VFS, not the > > stripped mode that results from inode_init_owner() being called. > > > > See what I mean about landmines? > > > > The fact is this: 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. > > > > We need the architecture to be *secure by design*, not tacked onto > > the side like it is now. We need to stop trying to dance around > > these landmines - it is *not working* and we are blowing our own > > feet off repeatedly. This hurts a lot (especially in distro land) > > so we need to take the responsibility for stripping SGID properly > > away from the filesystems and put it where it belongs: in the VFS. > > I agree. When I added tests for set*id stripping to xfstests for the > sake of getting complete vfs coverage of idmapped mounts in generic/633 > I immediately found bugs. Once I made the testsuite useable by all > filesystems we started seeing more. > > I think we should add and use the new proposed stripping helper in the > vfs - albeit with a slightly changed api and also use it in > inode_init_owner(). While it is a delicate change in the worst case we > end up removing additional privileges that's an acceptable regression > risk to take. And if it's not too much trouble, can we add an fstest to encode our current expectations about how setgid inheritance works? I would really like to reduce the need for historic setgid behavior spelunking. ;) --D
on 2022/3/31 0:44, Darrick J. Wong wrote: > On Wed, Mar 30, 2022 at 12:44:19PM +0200, Christian Brauner wrote: >> On Wed, Mar 30, 2022 at 09:10:59AM +1100, Dave Chinner wrote: >>> On Tue, Mar 29, 2022 at 07:12:11AM -0400, Jeff Layton wrote: >>>> On Mon, 2022-03-28 at 17:56 +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 umask with S_IXGRP. >>>>> >>>>> 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 by using umask but S_ISGID clear isn't related to >>>>> SB_POSIXACL flag. >>>>> >>>>> Suggested-by: Dave Chinner<david@fromorbit.com> >>>>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com> >>>>> --- >>>>> fs/inode.c | 4 ---- >>>>> fs/namei.c | 7 +++++-- >>>>> 2 files changed, 5 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/fs/inode.c b/fs/inode.c >>>>> index 1f964e7f9698..a2dd71c2437e 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..e68a99e0ac96 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,8 @@ 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); >>>>> + >>>>> error = dir->i_op->tmpfile(mnt_userns, dir, child, mode); >>>>> if (error) >>>>> goto out_err; >>>>> @@ -3849,14 +3852,14 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode, >>>>> error = PTR_ERR(dentry); >>>>> 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(); >>>>> 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, >>>> >>>> I haven't gone over this in detail, but have you tested this with NFS at >>>> all? >>>> >>>> IIRC, NFS has to leave setuid/gid stripping to the server, so I wonder >>>> if this may end up running afoul of that by forcing the client to try >>>> and strip these bits. >>> >>> All it means is that the mode passed to the NFS server for the >>> create already has the SGID bit stripped from it. It means the >>> client is no longer reliant on the server behaving correctly to >>> close this security hole. >>> >>> That is, failing to strip the SGID bit appropriately in the local >>> context is a security issue. Hence local machine security requires >>> that the NFS client should try to strip the SGID to defend against >>> buggy/unfixed servers that fail to strip it appropriately and >>> thereby continute to expose the local machine to this SGID security >>> issue. >>> >>> That's the problem here - the SGID stripping in inode_init_owner() >>> is not documented, wasn't reviewed, doesn't work correctly >>> across all filesystems and leaves nasty security landmines when the VFS >>> create mode and the stripped inode mode differ. >>> >>> Various filesystems have workarounds, partial fixes or no fixes for >>> these issues and landmines. Hence we have a situation where we are >>> playing whack-a-mole to discover and slap band-aids over all the >>> places that inode_init_owner() based stripping does not work >>> correctly. >>> >>> In XFS, this meant the problem was not orginally fixed by the >>> silent, unreviewed change to inode_init_owner() in 2018 >>> because it didn't call inode_init_owner() at all. So 4 years after >>> the bug was "fixed" and the CVE released, we are still exposed to >>> the bug because *no filesystem people knew about it* and *nobody wrote a >>> regression test* to check that the probelm was fixed and stayed >>> fixed. >>> >>> And now that XFS does call inode_init_owner(), we've subsequently >>> discovered that XFS still fail when default acls are enabled because >>> we create the ACL from the mode passed from the VFS, not the >>> stripped mode that results from inode_init_owner() being called. >>> >>> See what I mean about landmines? >>> >>> The fact is this: 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. >>> >>> We need the architecture to be *secure by design*, not tacked onto >>> the side like it is now. We need to stop trying to dance around >>> these landmines - it is *not working* and we are blowing our own >>> feet off repeatedly. This hurts a lot (especially in distro land) >>> so we need to take the responsibility for stripping SGID properly >>> away from the filesystems and put it where it belongs: in the VFS. >> >> I agree. When I added tests for set*id stripping to xfstests for the >> sake of getting complete vfs coverage of idmapped mounts in generic/633 >> I immediately found bugs. Once I made the testsuite useable by all >> filesystems we started seeing more. >> >> I think we should add and use the new proposed stripping helper in the >> vfs - albeit with a slightly changed api and also use it in >> inode_init_owner(). While it is a delicate change in the worst case we >> end up removing additional privileges that's an acceptable regression >> risk to take. > > And if it's not too much trouble, can we add an fstest to encode our > current expectations about how setgid inheritance works? I would really > like to reduce the need for historic setgid behavior spelunking. ;) I have sent two patches to increase the idmapped mounts coverage for S_ISGID in xfstests. Best Regards Yang Xu > > --D
diff --git a/fs/inode.c b/fs/inode.c index 1f964e7f9698..a2dd71c2437e 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..e68a99e0ac96 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,8 @@ 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); + error = dir->i_op->tmpfile(mnt_userns, dir, child, mode); if (error) goto out_err; @@ -3849,14 +3852,14 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode, error = PTR_ERR(dentry); 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(); 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,
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 umask with S_IXGRP. 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 by using umask but S_ISGID clear isn't related to SB_POSIXACL flag. Suggested-by: Dave Chinner <david@fromorbit.com> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> --- fs/inode.c | 4 ---- fs/namei.c | 7 +++++-- 2 files changed, 5 insertions(+), 6 deletions(-)