[PATCH v1 1/3] vfs: Add inode_sgid_strip() api

[PATCH v1 1/3] vfs: Add inode_sgid_strip() api

[Yang Xu]

inode_sgid_strip() function is used to strip S_ISGID mode
when creat/open/mknod file.

Suggested-by: Dave Chinner <david@fromorbit.com>
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 fs/inode.c         | 12 ++++++++++++
 include/linux/fs.h |  3 +++
 2 files changed, 15 insertions(+)

diff --git a/fs/inode.c b/fs/inode.c
index 63324df6fa27..1f964e7f9698 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2405,3 +2405,15 @@ struct timespec64 current_time(struct inode *inode)
 	return timestamp_truncate(now, inode);
 }
 EXPORT_SYMBOL(current_time);
+
+void inode_sgid_strip(struct user_namespace *mnt_userns, struct inode *dir,
+		      umode_t *mode)
+{
+	if ((dir && dir->i_mode & S_ISGID) &&
+		(*mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP) &&
+		!S_ISDIR(*mode) &&
+		!in_group_p(i_gid_into_mnt(mnt_userns, dir)) &&
+		!capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
+		*mode &= ~S_ISGID;
+}
+EXPORT_SYMBOL(inode_sgid_strip);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e2d892b201b0..639c830ad797 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1921,6 +1921,9 @@ extern long compat_ptr_ioctl(struct file *file, unsigned int cmd,
 void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
 		      const struct inode *dir, umode_t mode);
 extern bool may_open_dev(const struct path *path);
+void inode_sgid_strip(struct user_namespace *mnt_userns, struct inode *dir,
+		      umode_t *mode);
+
 
 /*
  * This is the "filldir" function type, used by readdir() to let
-- 
2.27.0

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

[Yang Xu]

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,
-- 
2.27.0

Re: [PATCH v1 1/3] vfs: Add inode_sgid_strip() api

[Christian Brauner]

On Mon, Mar 28, 2022 at 05:56:27PM +0800, Yang Xu wrote:
> inode_sgid_strip() function is used to strip S_ISGID mode
> when creat/open/mknod file.
> 
> Suggested-by: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---

I would've personally gone for returning umode_t but this work for me
too.
Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>

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

[Christian Brauner]

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?

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

[Jeff Layton]

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.

-- 
Jeff Layton <jlayton@kernel.org>

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

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v1 1/3] vfs: Add inode_sgid_strip() api

[Dave Chinner]

On Tue, Mar 29, 2022 at 12:45:16PM +0200, Christian Brauner wrote:
> On Mon, Mar 28, 2022 at 05:56:27PM +0800, Yang Xu wrote:
> > inode_sgid_strip() function is used to strip S_ISGID mode
> > when creat/open/mknod file.
> > 
> > Suggested-by: Dave Chinner <david@fromorbit.com>
> > Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> > ---
> 
> I would've personally gone for returning umode_t but this work for me
> too.

Agreed, that's a much nicer API for this function - it makes it
clear that it can modifying the mode that is passed in.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Christian Brauner]

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.

Re: [PATCH v1 1/3] vfs: Add inode_sgid_strip() api

[Darrick J. Wong]

On Mon, Mar 28, 2022 at 05:56:27PM +0800, Yang Xu wrote:
> inode_sgid_strip() function is used to strip S_ISGID mode
> when creat/open/mknod file.
> 
> Suggested-by: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  fs/inode.c         | 12 ++++++++++++
>  include/linux/fs.h |  3 +++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 63324df6fa27..1f964e7f9698 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2405,3 +2405,15 @@ struct timespec64 current_time(struct inode *inode)
>  	return timestamp_truncate(now, inode);
>  }
>  EXPORT_SYMBOL(current_time);
> +
> +void inode_sgid_strip(struct user_namespace *mnt_userns, struct inode *dir,
> +		      umode_t *mode)
> +{
> +	if ((dir && dir->i_mode & S_ISGID) &&
> +		(*mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP) &&
> +		!S_ISDIR(*mode) &&
> +		!in_group_p(i_gid_into_mnt(mnt_userns, dir)) &&
> +		!capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
> +		*mode &= ~S_ISGID;

A couple of style nits here:

The secondary if test clauses have the same indentation level as the
code that actually gets executed, which makes this harder to scan
visually.

	if ((dir && dir->i_mode & S_ISGID) &&
	    (*mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP) &&
	    !S_ISDIR(*mode) &&
	    !in_group_p(i_gid_into_mnt(mnt_userns, dir)) &&
	    !capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
		*mode &= ~S_ISGID;

Alternately, you could use inverse logic to bail out early:

void inode_sgid_strip(struct user_namespace *mnt_userns, struct inode *dir,
		      umode_t *mode)
{
	if (!dir || !(dir->i_mode & S_ISGID))
		return;
	if (S_ISDIR(*mode))
		return;
	if (in_group_p(...))
		return;
	if (capable_wrt_inode_uidgid(...))
		return;

	*mode &= ~S_ISGID;
}

Though I suppose that /is/ much longer.

The bigger thing here is that I'd like to see this patch hoist the ISGID
stripping code out of init_inode_owner so that it's easier to verify
that the new helper does exactly the same thing as the old code.  The
second patch would then add callsites around the VFS as necessary to
prevent this problem from happening again.

--D

> +}
> +EXPORT_SYMBOL(inode_sgid_strip);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index e2d892b201b0..639c830ad797 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1921,6 +1921,9 @@ extern long compat_ptr_ioctl(struct file *file, unsigned int cmd,
>  void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
>  		      const struct inode *dir, umode_t mode);
>  extern bool may_open_dev(const struct path *path);
> +void inode_sgid_strip(struct user_namespace *mnt_userns, struct inode *dir,
> +		      umode_t *mode);
> +
>  
>  /*
>   * This is the "filldir" function type, used by readdir() to let
> -- 
> 2.27.0
> 

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

[Darrick J. Wong]

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

Re: [PATCH v1 1/3] vfs: Add inode_sgid_strip() api

[xuyang2018.jy]

on 2022/3/31 0:34, Darrick J. Wong wrote:
> On Mon, Mar 28, 2022 at 05:56:27PM +0800, Yang Xu wrote:
>> inode_sgid_strip() function is used to strip S_ISGID mode
>> when creat/open/mknod file.
>>
>> Suggested-by: Dave Chinner<david@fromorbit.com>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   fs/inode.c         | 12 ++++++++++++
>>   include/linux/fs.h |  3 +++
>>   2 files changed, 15 insertions(+)
>>
>> diff --git a/fs/inode.c b/fs/inode.c
>> index 63324df6fa27..1f964e7f9698 100644
>> --- a/fs/inode.c
>> +++ b/fs/inode.c
>> @@ -2405,3 +2405,15 @@ struct timespec64 current_time(struct inode *inode)
>>   	return timestamp_truncate(now, inode);
>>   }
>>   EXPORT_SYMBOL(current_time);
>> +
>> +void inode_sgid_strip(struct user_namespace *mnt_userns, struct inode *dir,
>> +		      umode_t *mode)
>> +{
>> +	if ((dir&&  dir->i_mode&  S_ISGID)&&
>> +		(*mode&  (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)&&
>> +		!S_ISDIR(*mode)&&
>> +		!in_group_p(i_gid_into_mnt(mnt_userns, dir))&&
>> +		!capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
>> +		*mode&= ~S_ISGID;
>
> A couple of style nits here:
>
> The secondary if test clauses have the same indentation level as the
> code that actually gets executed, which makes this harder to scan
> visually.
>
> 	if ((dir&&  dir->i_mode&  S_ISGID)&&
> 	(*mode&  (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)&&
> 	!S_ISDIR(*mode)&&
> 	!in_group_p(i_gid_into_mnt(mnt_userns, dir))&&
> 	!capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
> 		*mode&= ~S_ISGID;
>
> Alternately, you could use inverse logic to bail out early:
>
> void inode_sgid_strip(struct user_namespace *mnt_userns, struct inode *dir,
> 		      umode_t *mode)
> {
> 	if (!dir || !(dir->i_mode&  S_ISGID))
> 		return;
> 	if (S_ISDIR(*mode))
> 		return;
> 	if (in_group_p(...))
> 		return;
> 	if (capable_wrt_inode_uidgid(...))
> 		return;
>
> 	*mode&= ~S_ISGID;
> }
>
> Though I suppose that /is/ much longer.
>
> The bigger thing here is that I'd like to see this patch hoist the ISGID
> stripping code out of init_inode_owner so that it's easier to verify
> that the new helper does exactly the same thing as the old code.  The
> second patch would then add callsites around the VFS as necessary to
> prevent this problem from happening again.

Sounds reasonable. Will do it in v2.

Best Regards
Yang Xu
>
> --D
>
>> +}
>> +EXPORT_SYMBOL(inode_sgid_strip);
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index e2d892b201b0..639c830ad797 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -1921,6 +1921,9 @@ extern long compat_ptr_ioctl(struct file *file, unsigned int cmd,
>>   void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
>>   		      const struct inode *dir, umode_t mode);
>>   extern bool may_open_dev(const struct path *path);
>> +void inode_sgid_strip(struct user_namespace *mnt_userns, struct inode *dir,
>> +		      umode_t *mode);
>> +
>>
>>   /*
>>    * This is the "filldir" function type, used by readdir() to let
>> --
>> 2.27.0
>>

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

[xuyang2018.jy]

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