* FUSE: regression when clearing setuid bits on chown @ 2016-12-05 18:21 Jeff Layton 2016-12-06 10:02 ` Miklos Szeredi 0 siblings, 1 reply; 7+ messages in thread From: Jeff Layton @ 2016-12-05 18:21 UTC (permalink / raw) To: Miklos Szeredi; +Cc: linux-fsdevel, open list, Al Viro 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: FUSE: regression when clearing setuid bits on chown 2016-12-05 18:21 FUSE: regression when clearing setuid bits on chown Jeff Layton @ 2016-12-06 10:02 ` Miklos Szeredi 2016-12-06 12:13 ` Jeff Layton 0 siblings, 1 reply; 7+ messages in thread From: Miklos Szeredi @ 2016-12-06 10:02 UTC (permalink / raw) To: Jeff Layton; +Cc: linux-fsdevel, open list, Al Viro 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) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: FUSE: regression when clearing setuid bits on chown 2016-12-06 10:02 ` Miklos Szeredi @ 2016-12-06 12:13 ` Jeff Layton 2016-12-06 14:39 ` Miklos Szeredi 0 siblings, 1 reply; 7+ messages in thread From: Jeff Layton @ 2016-12-06 12:13 UTC (permalink / raw) To: Miklos Szeredi; +Cc: linux-fsdevel, open list, Al Viro 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: FUSE: regression when clearing setuid bits on chown 2016-12-06 12:13 ` Jeff Layton @ 2016-12-06 14:39 ` Miklos Szeredi 2016-12-06 14:45 ` Jeff Layton 0 siblings, 1 reply; 7+ messages in thread From: Miklos Szeredi @ 2016-12-06 14:39 UTC (permalink / raw) To: Jeff Layton; +Cc: linux-fsdevel, open list, Al Viro 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; } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: FUSE: regression when clearing setuid bits on chown 2016-12-06 14:39 ` Miklos Szeredi @ 2016-12-06 14:45 ` Jeff Layton 2016-12-06 14:51 ` Miklos Szeredi 0 siblings, 1 reply; 7+ messages in thread From: Jeff Layton @ 2016-12-06 14:45 UTC (permalink / raw) To: Miklos Szeredi; +Cc: linux-fsdevel, open list, Al Viro 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: FUSE: regression when clearing setuid bits on chown 2016-12-06 14:45 ` Jeff Layton @ 2016-12-06 14:51 ` Miklos Szeredi 2016-12-06 14:54 ` Jeff Layton 0 siblings, 1 reply; 7+ messages in thread From: Miklos Szeredi @ 2016-12-06 14:51 UTC (permalink / raw) To: Jeff Layton; +Cc: linux-fsdevel, open list, Al Viro 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: FUSE: regression when clearing setuid bits on chown 2016-12-06 14:51 ` Miklos Szeredi @ 2016-12-06 14:54 ` Jeff Layton 0 siblings, 0 replies; 7+ messages in thread From: Jeff Layton @ 2016-12-06 14:54 UTC (permalink / raw) To: Miklos Szeredi; +Cc: linux-fsdevel, open list, Al Viro 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! ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-12-06 14:54 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-12-05 18:21 FUSE: regression when clearing setuid bits on chown Jeff Layton 2016-12-06 10:02 ` Miklos Szeredi 2016-12-06 12:13 ` Jeff Layton 2016-12-06 14:39 ` Miklos Szeredi 2016-12-06 14:45 ` Jeff Layton 2016-12-06 14:51 ` Miklos Szeredi 2016-12-06 14:54 ` Jeff Layton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).