FUSE: regression when clearing setuid bits on chown

FUSE: regression when clearing setuid bits on chown

[Jeff Layton]

Hi Miklos,

I think we've found a "regression" that has crept in due to this patch:

commit a09f99eddef44035ec764075a37bace8181bec38
Author: Miklos Szeredi <mszeredi@redhat.com>
Date:   Sat Oct 1 07:32:32 2016 +0200

    fuse: fix killing s[ug]id in setattr
    
Basically, the pjdfstests set the ownership of a file to 06555, and then
chowns it (as root) to a new uid/gid. Prior to the patch above, 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.

So, the issue I think is the use of should_remove_suid, which will
always return 0 when the process has CAP_FSETID. That's appropriate (I
think) for write/truncate, but not chown, where we want to ignore that.

Thoughts on the right fix here? A simple fix would be to add an
"override" bool to should_remove_suid, but maybe there's some more
elegant way to do it?

-- 
Jeff Layton <jlayton@redhat.com>

Re: FUSE: regression when clearing setuid bits on chown

[Miklos Szeredi]

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@redhat.com>
> 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@redhat.com>
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@redhat.com> 
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Fixes: a09f99eddef4 ("fuse: fix killing s[ug]id in setattr")
Cc: <stable@vger.kernel.org>
---
 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;
-			}
+			attr->ia_mode = inode->i_mode & ~(S_ISUID | S_ISGID);
+			attr->ia_valid |= ATTR_MODE;
 		}
 	}
 	if (!attr->ia_valid)

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@redhat.com>
> > 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@redhat.com>
> 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@redhat.com> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> Fixes: a09f99eddef4 ("fuse: fix killing s[ug]id in setattr")
> Cc: <stable@vger.kernel.org>
> ---
>  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@redhat.com>

Re: FUSE: regression when clearing setuid bits on chown

[Miklos Szeredi]

On Tue, Dec 06, 2016 at 07:13:25AM -0500, Jeff Layton wrote:

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

Fixed, see updated patch below.

It also fixes a bug in the previous patch where in case of "-rwsrwSr-x" it would
clear the sgid bit without execute.

> 
> > > +			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.

Yeah, that would be a nice little cleanup.

Thanks,
Miklos
---

From: Miklos Szeredi <mszeredi@redhat.com>
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@redhat.com> 
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Fixes: a09f99eddef4 ("fuse: fix killing s[ug]id in setattr")
Cc: <stable@vger.kernel.org>
---
 fs/fuse/dir.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 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.
@@ -1750,12 +1748,11 @@ static int fuse_setattr(struct dentry *e
 				return ret;
 
 			attr->ia_mode = inode->i_mode;
-			kill = should_remove_suid(entry);
-			if (kill & ATTR_KILL_SUID) {
+			if (inode->i_mode & S_ISUID) {
 				attr->ia_valid |= ATTR_MODE;
 				attr->ia_mode &= ~S_ISUID;
 			}
-			if (kill & ATTR_KILL_SGID) {
+			if ((inode->i_mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
 				attr->ia_valid |= ATTR_MODE;
 				attr->ia_mode &= ~S_ISGID;
 			}

Re: FUSE: regression when clearing setuid bits on chown

[Jeff Layton]

On Tue, 2016-12-06 at 15:39 +0100, Miklos Szeredi wrote:
> On Tue, Dec 06, 2016 at 07:13:25AM -0500, Jeff Layton wrote:
> 
> > 
> > Should we be checking that the latest i_mode even has these bits before
> > sending down the mode change?
> 
> Fixed, see updated patch below.
> 
> It also fixes a bug in the previous patch where in case of "-rwsrwSr-x" it would
> clear the sgid bit without execute.
> 
> > 
> > 
> > > 
> > > > 
> > > > +			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.
> 
> Yeah, that would be a nice little cleanup.
> 
> Thanks,
> Miklos
> ---
> 
> From: Miklos Szeredi <mszeredi@redhat.com>
> 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@redhat.com> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> Fixes: a09f99eddef4 ("fuse: fix killing s[ug]id in setattr")
> Cc: <stable@vger.kernel.org>
> ---
>  fs/fuse/dir.c |    7 ++-----
>  1 file changed, 2 insertions(+), 5 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) {

One more thing too. I don't think we really want to monkey with the mode
at all if there is a request to set the mode already in the request. So
maybe this should be:

    if (!fc->handle_killpriv && !(attr->ia_mode & ATTR_MODE))

Granted that won't generally happen from normal process context, but we
could have knfsd in here too and I think that's possible from there.

> -			int kill;
> -
>  			/*
>  			 * ia_mode calculation may have used stale i_mode.
>  			 * Refresh and recalculate.
> @@ -1750,12 +1748,11 @@ static int fuse_setattr(struct dentry *e
>  				return ret;
>  
>  			attr->ia_mode = inode->i_mode;
> -			kill = should_remove_suid(entry);
> -			if (kill & ATTR_KILL_SUID) {
> +			if (inode->i_mode & S_ISUID) {
>  				attr->ia_valid |= ATTR_MODE;
>  				attr->ia_mode &= ~S_ISUID;
>  			}
> -			if (kill & ATTR_KILL_SGID) {
> +			if ((inode->i_mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
>  				attr->ia_valid |= ATTR_MODE;
>  				attr->ia_mode &= ~S_ISGID;
>  			}

Looks good otherwise!
-- 
Jeff Layton <jlayton@redhat.com>

Re: FUSE: regression when clearing setuid bits on chown

[Miklos Szeredi]

On Tue, Dec 6, 2016 at 3:45 PM, Jeff Layton <jlayton@redhat.com> wrote:

>> @@ -1739,8 +1739,6 @@ static int fuse_setattr(struct dentry *e
>>                * This should be done on write(), truncate() and chown().
>>                */
>>               if (!fc->handle_killpriv) {
>
> One more thing too. I don't think we really want to monkey with the mode
> at all if there is a request to set the mode already in the request. So
> maybe this should be:
>
>     if (!fc->handle_killpriv && !(attr->ia_mode & ATTR_MODE))
>
> Granted that won't generally happen from normal process context, but we
> could have knfsd in here too and I think that's possible from there.

Apparently this can't happen even from knfsd; notify_change() has this comment:

    /*
     * We now pass ATTR_KILL_S*ID to the lower level setattr function so
     * that the function has the ability to reinterpret a mode change
     * that's due to these bits. This adds an implicit restriction that
     * no function will ever call notify_change with both ATTR_MODE and
     * ATTR_KILL_S*ID set.
     */
    if ((ia_valid & (ATTR_KILL_SUID|ATTR_KILL_SGID)) &&
        (ia_valid & ATTR_MODE))
        BUG();

Thanks,
Miklos

Re: FUSE: regression when clearing setuid bits on chown

[Jeff Layton]

On Tue, 2016-12-06 at 15:51 +0100, Miklos Szeredi wrote:
> On Tue, Dec 6, 2016 at 3:45 PM, Jeff Layton <jlayton@redhat.com> wrote:
> 
> > 
> > > 
> > > @@ -1739,8 +1739,6 @@ static int fuse_setattr(struct dentry *e
> > >                * This should be done on write(), truncate() and chown().
> > >                */
> > >               if (!fc->handle_killpriv) {
> > 
> > One more thing too. I don't think we really want to monkey with the mode
> > at all if there is a request to set the mode already in the request. So
> > maybe this should be:
> > 
> >     if (!fc->handle_killpriv && !(attr->ia_mode & ATTR_MODE))
> > 
> > Granted that won't generally happen from normal process context, but we
> > could have knfsd in here too and I think that's possible from there.
> 
> Apparently this can't happen even from knfsd; notify_change() has this comment:
> 
>     /*
>      * We now pass ATTR_KILL_S*ID to the lower level setattr function so
>      * that the function has the ability to reinterpret a mode change
>      * that's due to these bits. This adds an implicit restriction that
>      * no function will ever call notify_change with both ATTR_MODE and
>      * ATTR_KILL_S*ID set.
>      */
>     if ((ia_valid & (ATTR_KILL_SUID|ATTR_KILL_SGID)) &&
>         (ia_valid & ATTR_MODE))
>         BUG();
> 
> 

Ahh right, I had forgotten about that. Eventually we may want to lift
that restriction, but you can add this to the current patch:

Reviewed-by: Jeff Layton <jlayton@redhat.com>

Thanks for fixing it quickly!