[PATCH 0/4] add killattr inode operation to allow filesystems to interpret ATTR_KILL_S*ID bits

[PATCH 0/4] add killattr inode operation to allow filesystems to interpret ATTR_KILL_S*ID bits

[Jeff Layton]

When an unprivileged process attempts to modify a file that has the
setuid or setgid bits set, the VFS will attempt to clear these bits. The
VFS will set the ATTR_KILL_SUID or ATTR_KILL_SGID bits in the ia_valid
mask, and then call notify_change to clear these bits and set the mode
accordingly.

With a networked filesystem (NFS and CIFS in particular but likely
others), the client machine or process may not have credentials that
allow for setting the mode. In some situations, this can lead to file
corruption, an operation failing outright because the setattr fails, or
to races that lead to a mode change being reverted.

In this situation, we'd like to just leave the handling of this to the
server and ignore these bits. The problem is that by the time the
setattr op is called, the VFS has already reinterpreted the ATTR_KILL_*
bits into a mode change. We can't fix this in the filesystems where this
is a problem, as doing so would leave us having to second-guess what the
VFS wants us to do. So we need to change it so that filesystems have
more flexibility in how to interpret the ATTR_KILL_* bits.

The first patch in the following patchset adds a new inode operation
called "killattr" and has notify change call it if it's defined. The
purpose of this inode op is to properly interpret the ATTR_KILL_SUID and
ATTR_KILL_SGID bits. Filesystems that do not declare a killattr inode
operation will keep the existing behavior, converting these bits
into a mode change.

The next two patches add a killattr inode op for NFS and CIFS which just
clears the bits (to allow the server to handle them). The final patch
updates the Documentation dir to describe the new killattr inode
operation.

This patchset should apply cleanly to 2.6.23-rc3-mm1. Comments and
suggestions appreciated...

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

Re: [PATCH 0/4] add killattr inode operation to allow filesystems to interpret ATTR_KILL_S*ID bits

[Christoph Hellwig]

Sorry for not replying to the previsious revisions, but I've been out
for on vacation.

I can't say I like this version.  Now we've got callouts at two rather close
levels which is not very nice from the interface POV.

Maybe preference is for the first scheme where we simply move interpreation
of the ATTR_KILL_SUID/ATTR_KILL_SGID into the setattr routine and provide
a nice helper for the normal filesystem to use.

If people are really concerned about adding two lines of code to the
handfull of setattr operation there's a variant of this scheme that can
avoid it:

 - notify_change is modified to not clear the ATTR_KILL_SUID/ATTR_KILL_SGID
   but update ia_mode and the ia_valid flag to include ATTR_MODE.
 - disk filesystems stay unchanged and never look at
   ATTR_KILL_SUID/ATTR_KILL_SGID, but nfs can check for it and ignore
   the ATTR_MODE flags and ia_valid in this case and do the right thing
   on the server side.

Re: [PATCH 0/4] add killattr inode operation to allow filesystems to interpret ATTR_KILL_S*ID bits

[Josef Sipek]

On Tue, Aug 28, 2007 at 08:11:14PM +0100, Christoph Hellwig wrote:
> 
> Sorry for not replying to the previsious revisions, but I've been out
> for on vacation.
> 
> I can't say I like this version.  Now we've got callouts at two rather close
> levels which is not very nice from the interface POV.
> 
> Maybe preference is for the first scheme where we simply move interpreation
> of the ATTR_KILL_SUID/ATTR_KILL_SGID into the setattr routine and provide
> a nice helper for the normal filesystem to use.
> 
> If people are really concerned about adding two lines of code to the
> handfull of setattr operation there's a variant of this scheme that can
> avoid it:
 
It's not about adding 2 lines of code - it's about adding the requirement
for the fs to call a function.

>  - notify_change is modified to not clear the ATTR_KILL_SUID/ATTR_KILL_SGID
>    but update ia_mode and the ia_valid flag to include ATTR_MODE.
>  - disk filesystems stay unchanged and never look at
>    ATTR_KILL_SUID/ATTR_KILL_SGID, but nfs can check for it and ignore
>    the ATTR_MODE flags and ia_valid in this case and do the right thing
>    on the server side.

Sounds reasonable.

Josef 'Jeff' Sipek.

-- 
I abhor a system designed for the "user", if that word is a coded pejorative
meaning "stupid and unsophisticated."
		- Ken Thompson

Re: [NFS] [PATCH 0/4] add killattr inode operation to allow filesystems to interpret ATTR_KILL_S*ID bits

[Trond Myklebust]

On Tue, 2007-08-28 at 20:11 +0100, Christoph Hellwig wrote:
> Sorry for not replying to the previsious revisions, but I've been out
> for on vacation.
> 
> I can't say I like this version.  Now we've got callouts at two rather close
> levels which is not very nice from the interface POV.

Agreed.

> Maybe preference is for the first scheme where we simply move interpreation
> of the ATTR_KILL_SUID/ATTR_KILL_SGID into the setattr routine and provide
> a nice helper for the normal filesystem to use.
> 
> If people are really concerned about adding two lines of code to the
> handfull of setattr operation there's a variant of this scheme that can
> avoid it:
> 
>  - notify_change is modified to not clear the ATTR_KILL_SUID/ATTR_KILL_SGID
>    but update ia_mode and the ia_valid flag to include ATTR_MODE.
>  - disk filesystems stay unchanged and never look at
>    ATTR_KILL_SUID/ATTR_KILL_SGID, but nfs can check for it and ignore
>    the ATTR_MODE flags and ia_valid in this case and do the right thing
>    on the server side.

Hmm... There has to be an implicit promise here that nobody else will
ever try to set ATTR_KILL_SUID/ATTR_KILL_SGID and ATTR_MODE at the same
time. Currently, that assumption is not there:


> 	if (ia_valid & ATTR_KILL_SGID) {
> 		attr->ia_valid &= ~ ATTR_KILL_SGID;
> 		if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
> 			if (!(ia_valid & ATTR_MODE)) {
> 				ia_valid = attr->ia_valid |= ATTR_MODE;
> 				attr->ia_mode = inode->i_mode;
> 			}
> 			attr->ia_mode &= ~S_ISGID;
> 		}
> 	}

Should we perhaps just convert the above 'if (!(ia_valid & ATTR_MODE))'
into a 'BUG_ON(ia_valid & ATTR_MODE)'?

Trond

Re: [NFS] [PATCH 0/4] add killattr inode operation to allow filesystems to interpret ATTR_KILL_S*ID bits

[Christoph Hellwig]

On Tue, Aug 28, 2007 at 03:49:51PM -0400, Trond Myklebust wrote:
> Hmm... There has to be an implicit promise here that nobody else will
> ever try to set ATTR_KILL_SUID/ATTR_KILL_SGID and ATTR_MODE at the same
> time. Currently, that assumption is not there:
> 
> 
> > 	if (ia_valid & ATTR_KILL_SGID) {
> > 		attr->ia_valid &= ~ ATTR_KILL_SGID;
> > 		if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
> > 			if (!(ia_valid & ATTR_MODE)) {
> > 				ia_valid = attr->ia_valid |= ATTR_MODE;
> > 				attr->ia_mode = inode->i_mode;
> > 			}
> > 			attr->ia_mode &= ~S_ISGID;
> > 		}
> > 	}
> 
> Should we perhaps just convert the above 'if (!(ia_valid & ATTR_MODE))'
> into a 'BUG_ON(ia_valid & ATTR_MODE)'?

Yes, sounds fine to me.

Re: [NFS] [PATCH 0/4] add killattr inode operation to allow filesystems to interpret ATTR_KILL_S*ID bits

[Jeff Layton]

On Tue, 28 Aug 2007 15:49:51 -0400
Trond Myklebust <trond.myklebust@fys.uio.no> wrote:

> On Tue, 2007-08-28 at 20:11 +0100, Christoph Hellwig wrote:
> > Sorry for not replying to the previsious revisions, but I've been out
> > for on vacation.
> > 
> > I can't say I like this version.  Now we've got callouts at two rather close
> > levels which is not very nice from the interface POV.
> 
> Agreed.
> 
> > Maybe preference is for the first scheme where we simply move interpreation
> > of the ATTR_KILL_SUID/ATTR_KILL_SGID into the setattr routine and provide
> > a nice helper for the normal filesystem to use.
> > 
> > If people are really concerned about adding two lines of code to the
> > handfull of setattr operation there's a variant of this scheme that can
> > avoid it:
> > 
> >  - notify_change is modified to not clear the ATTR_KILL_SUID/ATTR_KILL_SGID
> >    but update ia_mode and the ia_valid flag to include ATTR_MODE.
> >  - disk filesystems stay unchanged and never look at
> >    ATTR_KILL_SUID/ATTR_KILL_SGID, but nfs can check for it and ignore
> >    the ATTR_MODE flags and ia_valid in this case and do the right thing
> >    on the server side.
> 
> Hmm... There has to be an implicit promise here that nobody else will
> ever try to set ATTR_KILL_SUID/ATTR_KILL_SGID and ATTR_MODE at the same
> time. Currently, that assumption is not there:
> 

That was my concern with this scheme as well...

> 
> > 	if (ia_valid & ATTR_KILL_SGID) {
> > 		attr->ia_valid &= ~ ATTR_KILL_SGID;
> > 		if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
> > 			if (!(ia_valid & ATTR_MODE)) {
> > 				ia_valid = attr->ia_valid |= ATTR_MODE;
> > 				attr->ia_mode = inode->i_mode;
> > 			}
> > 			attr->ia_mode &= ~S_ISGID;
> > 		}
> > 	}
> 
> Should we perhaps just convert the above 'if (!(ia_valid & ATTR_MODE))'
> into a 'BUG_ON(ia_valid & ATTR_MODE)'?
> 

Sounds reasonable. I'll also throw in a comment that explains this
reasoning...

-- 
Jeff Layton <jlayton@redhat.com>