Re: FUSE: regression when clearing setuid bits on chown

[Jeff Layton]

On Tue, 2016-12-06 at 11:02 +0100, Miklos Szeredi wrote:
> On Mon, Dec 05, 2016 at 01:21:15PM -0500, Jeff Layton wrote:
> > 
> > Hi Miklos,
> > 
> > I think we've found a "regression" that has crept in due to this patch:
> > 
> > commit a09f99eddef44035ec764075a37bace8181bec38
> > Author: Miklos Szeredi <mszeredi@xxxxxxxxxx>
> > Date:ÂÂÂSat Oct 1 07:32:32 2016 +0200
> > 
> > ÂÂÂÂfuse: fix killing s[ug]id in setattr
> > Â ÂÂ
> 
> Thanks for the report.  Below patch should fix it.  Not sure what I've been
> thinking when adding all that complexity, when the issue here was not whether to
> clear the suid or not, but how stale the mode used is.
> 
> Thanks,
> Miklos
> 
> ---
> From: Miklos Szeredi <mszeredi@xxxxxxxxxx>
> Subject: fuse: fix clearing suid, sgid for chown()
> 
> Basically, the pjdfstests set the ownership of a file to 06555, and then
> chowns it (as root) to a new uid/gid. Prior to commit a09f99eddef4 ("fuse:
> fix killing s[ug]id in setattr"), fuse would send down a setattr with both
> the uid/gid change and a new mode.  Now, it just sends down the uid/gid
> change.
> 
> Technically this is NOTABUG, since POSIX doesn't _require_ that we clear
> these bits for a privileged process, but Linux (wisely) has done that and I
> think we don't want to change that behavior here.
> 
> This is caused by the use of should_remove_suid(), which will always return
> 0 when the process has CAP_FSETID.
> 
> In fact we really don't need to be calling should_remove_suid() at all,
> since we've already been indicated that we should remove the suid, we just
> don't want to use a (very) stale mode for that.
> 
> This patch should fix the above as well as simplify the logic.
> 
> Reported-by: Jeff Layton <jlayton@xxxxxxxxxx> 
> Signed-off-by: Miklos Szeredi <mszeredi@xxxxxxxxxx>
> Fixes: a09f99eddef4 ("fuse: fix killing s[ug]id in setattr")
> Cc: <stable@xxxxxxxxxxxxxxx>
> ---
>  fs/fuse/dir.c |   14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -1739,8 +1739,6 @@ static int fuse_setattr(struct dentry *e
>  		 * This should be done on write(), truncate() and chown().
>  		 */
>  		if (!fc->handle_killpriv) {
> -			int kill;
> -
>  			/*
>  			 * ia_mode calculation may have used stale i_mode.
>  			 * Refresh and recalculate.
> @@ -1749,16 +1747,8 @@ static int fuse_setattr(struct dentry *e
>  			if (ret)
>  				return ret;
>  
> -			attr->ia_mode = inode->i_mode;
> -			kill = should_remove_suid(entry);
> -			if (kill & ATTR_KILL_SUID) {
> -				attr->ia_valid |= ATTR_MODE;
> -				attr->ia_mode &= ~S_ISUID;
> -			}
> -			if (kill & ATTR_KILL_SGID) {
> -				attr->ia_valid |= ATTR_MODE;
> -				attr->ia_mode &= ~S_ISGID;
> -			}

Should we be checking that the latest i_mode even has these bits before
sending down the mode change?

> > +			attr->ia_mode = inode->i_mode & ~(S_ISUID | S_ISGID);
> +			attr->ia_valid |= ATTR_MODE;
>  		}
>  	}
>  	if (!attr->ia_valid)

Yeah that is quite a bit simpler.

That said...if either ATTR_KILL flag is set, then we're going to end up
clearing both bits in the new mode. I guess that's ok since we always
want to clear them both, and we'll only have one set and not the other
if one of the mode bits was set and not the other.

But...I'm starting to wonder if we really need two flags for this. Would
be be better served with a single ATTR_KILL_SUID_SGID flag? I wonder if
that would simplify some of the logic in the whole setuid clearing
morass.

--Â
Jeff Layton <jlayton@xxxxxxxxxx>