Re: [RFC:PATCH] How best to handle implicit clearing of setuid/setgid bits on NFS?

Re: [RFC:PATCH] How best to handle implicit clearing of setuid/setgid bits on NFS?

[Jeff Layton]

On Wed, 27 Jun 2007 18:15:55 -0400
Trond Myklebust <trond.myklebust@fys.uio.no> wrote:

> On Tue, 2007-05-29 at 12:47 -0400, Jeff Layton wrote:
> > I've been looking at issue of clearing setuid/setgid bits when a file
> > is written to on NFS. Here's the problem in a nutshell:
> > 
> > We have 2 users. test1 and test2. Both are members of the group
> > "testgrp":
> > 
> > test2@host$ ls -l f1
> > -rwxrwsr-x 1 test1 testgrp 2 2007-05-29 12:23 f1
> > test2@host$ echo foo > f1
> > -bash: f1: Permission denied
> > 
> > ...and f1 is unchanged. The problem is that the VFS calls remove_suid
> > to wipe the setgid bit. This ends up causing a SETATTR call, which
> > fails on NFS because we're attempting to remove these bits as user
> > "test2".
> > 
> > Until recently, the situation here was worse. The VFS would truncate
> > the file first and then try to clear the setgid bit. The truncate would
> > succeed, but the perm change would fail. You'd end up with a zero-length
> > file. This was fixed my making the size change and bit-clearing go via
> > the same setattr call, so the whole operation just errors out now.
> > 
> > My question is -- Is there anything we can do to make this work as it
> > does on a local filesystem? Ideally there would be some way to tell the
> > server "clear the setuid/gid bits", without actually modifying the
> > contents of the file. Is there a NFS call we can use that would do this?
> > 
> > The only thing I can think of is to read the first byte of the file and
> > then overwrite it with the same data, but that seems racy and may have
> > other problems (and what do you do with a zero-length, setuid file?).
> > 
> > Any suggestions appreciated...
> 
> The answer should be simple: leave it to the server. All servers I know
> (except possibly Hummingbird) will clear the setuid/setgid bit whenever
> the file is modified. Anything else would be _extremely_ racy anyway:
> Consider the scenario where you are preparing to write to the file, when
> suddenly a user on another client makes the file setuid after you have
> open()ed the file, but before you actually issue the write().
> 
> IOW: calling remove_suid() on the client is completely redundant, and
> should be suppressed.
> 
> Trond
> 

Ok. This is a bit more complex now since we remove suid bits on
truncate, but don't set ATTR_FORCE.

Here's a patch that should do this. I know there's a general
aversion to adding new flags to vfs structures, but I couldn't think of
a way to cleanly do this without adding one.

Note that I've not tested this patch at all so this is just a RFC.

CC'ing Al here since he's expressed interest in this problem as well.

Thoughts?

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

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index ca20d3c..afdd82e 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -71,7 +71,7 @@ static struct file_system_type nfs_fs_type = {
 	.name		= "nfs",
 	.get_sb		= nfs_get_sb,
 	.kill_sb	= nfs_kill_super,
-	.fs_flags	= FS_RENAME_DOES_D_MOVE|FS_REVAL_DOT|FS_BINARY_MOUNTDATA,
+	.fs_flags	= FS_RENAME_DOES_D_MOVE|FS_REVAL_DOT|FS_BINARY_MOUNTDATA|FS_DONT_REMOVE_SUID,
 };
 
 struct file_system_type nfs_xdev_fs_type = {
@@ -79,7 +79,7 @@ struct file_system_type nfs_xdev_fs_type = {
 	.name		= "nfs",
 	.get_sb		= nfs_xdev_get_sb,
 	.kill_sb	= nfs_kill_super,
-	.fs_flags	= FS_RENAME_DOES_D_MOVE|FS_REVAL_DOT|FS_BINARY_MOUNTDATA,
+	.fs_flags	= FS_RENAME_DOES_D_MOVE|FS_REVAL_DOT|FS_BINARY_MOUNTDATA|FS_DONT_REMOVE_SUID,
 };
 
 static const struct super_operations nfs_sops = {
@@ -107,7 +107,7 @@ static struct file_system_type nfs4_fs_type = {
 	.name		= "nfs4",
 	.get_sb		= nfs4_get_sb,
 	.kill_sb	= nfs4_kill_super,
-	.fs_flags	= FS_RENAME_DOES_D_MOVE|FS_REVAL_DOT|FS_BINARY_MOUNTDATA,
+	.fs_flags	= FS_RENAME_DOES_D_MOVE|FS_REVAL_DOT|FS_BINARY_MOUNTDATA|FS_DONT_REMOVE_SUID,
 };
 
 struct file_system_type nfs4_xdev_fs_type = {
@@ -115,7 +115,7 @@ struct file_system_type nfs4_xdev_fs_type = {
 	.name		= "nfs4",
 	.get_sb		= nfs4_xdev_get_sb,
 	.kill_sb	= nfs4_kill_super,
-	.fs_flags	= FS_RENAME_DOES_D_MOVE|FS_REVAL_DOT|FS_BINARY_MOUNTDATA,
+	.fs_flags	= FS_RENAME_DOES_D_MOVE|FS_REVAL_DOT|FS_BINARY_MOUNTDATA|FS_DONT_REMOVE_SUID,
 };
 
 struct file_system_type nfs4_referral_fs_type = {
@@ -123,7 +123,7 @@ struct file_system_type nfs4_referral_fs_type = {
 	.name		= "nfs4",
 	.get_sb		= nfs4_referral_get_sb,
 	.kill_sb	= nfs4_kill_super,
-	.fs_flags	= FS_RENAME_DOES_D_MOVE|FS_REVAL_DOT|FS_BINARY_MOUNTDATA,
+	.fs_flags	= FS_RENAME_DOES_D_MOVE|FS_REVAL_DOT|FS_BINARY_MOUNTDATA|FS_DONT_REMOVE_SUID,
 };
 
 static const struct super_operations nfs4_sops = {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 6a41f4c..e6e18dd 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -93,6 +93,7 @@ extern int dir_notify_enable;
 #define FS_REQUIRES_DEV 1 
 #define FS_BINARY_MOUNTDATA 2
 #define FS_HAS_SUBTYPE 4
+#define FS_DONT_REMOVE_SUID 8	/* don't try to remove_suid */
 #define FS_REVAL_DOT	16384	/* Check the paths ".", ".." for staleness */
 #define FS_RENAME_DOES_D_MOVE	32768	/* FS will handle d_move()
 					 * during rename() internally.
diff --git a/mm/filemap.c b/mm/filemap.c
index edb1b0b..27febc8 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1888,6 +1888,14 @@ int should_remove_suid(struct dentry *dentry)
 	mode_t mode = dentry->d_inode->i_mode;
 	int kill = 0;
 
+	/*
+	 * We want to supress this on some filesystems -- particularly NFS
+	 * as we expect the server to handle it, and we may not have
+	 * permission to remove these bits.
+	 */
+	if (dentry->d_sb->s_type->fs_flags & FS_DONT_REMOVE_SUID)
+		return 0;
+
 	/* suid always must be killed */
 	if (unlikely(mode & S_ISUID))
 		kill = ATTR_KILL_SUID;

Re: [RFC:PATCH] How best to handle implicit clearing of setuid/setgid bits on NFS?

[Trond Myklebust]

On Wed, 2007-06-27 at 22:13 -0400, Jeff Layton wrote:
> Ok. This is a bit more complex now since we remove suid bits on
> truncate, but don't set ATTR_FORCE.
> 
> Here's a patch that should do this. I know there's a general
> aversion to adding new flags to vfs structures, but I couldn't think of
> a way to cleanly do this without adding one.
> 
> Note that I've not tested this patch at all so this is just a RFC.
> 
> CC'ing Al here since he's expressed interest in this problem as well.
> 
> Thoughts?

We don't really need to do this with extra VFS flags. Here is my
preferred approach for dealing with this problem.

        http://article.gmane.org/gmane.linux.nfs/8511/match=attr%5fkill%5fsuid

As you can see, that still allows the filesystem to determine how it
should deal with the ATTR_KILL_SUID/ATTR_KILL_SGID flags. The default
behaviour is provided by inode_setattr(), and is the same as today. Only
filesystems that don't use inode_setattr() will need to be audited for
whether or not they need a fix.

Cheers,
  Trond

Re: [RFC:PATCH] How best to handle implicit clearing of setuid/setgid bits on NFS?

[Jeff Layton]

On Thu, 28 Jun 2007 09:38:22 -0400
Trond Myklebust <trond.myklebust@fys.uio.no> wrote:

> On Wed, 2007-06-27 at 22:13 -0400, Jeff Layton wrote:
> > Ok. This is a bit more complex now since we remove suid bits on
> > truncate, but don't set ATTR_FORCE.
> > 
> > Here's a patch that should do this. I know there's a general
> > aversion to adding new flags to vfs structures, but I couldn't think of
> > a way to cleanly do this without adding one.
> > 
> > Note that I've not tested this patch at all so this is just a RFC.
> > 
> > CC'ing Al here since he's expressed interest in this problem as well.
> > 
> > Thoughts?
> 
> We don't really need to do this with extra VFS flags. Here is my
> preferred approach for dealing with this problem.
> 
>         http://article.gmane.org/gmane.linux.nfs/8511/match=attr%5fkill%5fsuid
> 
> As you can see, that still allows the filesystem to determine how it
> should deal with the ATTR_KILL_SUID/ATTR_KILL_SGID flags. The default
> behaviour is provided by inode_setattr(), and is the same as today. Only
> filesystems that don't use inode_setattr() will need to be audited for
> whether or not they need a fix.
> 
> Cheers,
>   Trond


I had a look at this today, and I have some reservations about this
approach. Quite a few filesystems define a .setattr inode op, but don't
call inode_setattr. As a first pass through the current git tree, the
following setattr ops never seem to call inode_setattr:

adfs_notify_change
afs_setattr
coda_setattr
ecryptfs_setattr
fuse_setattr
jffs2_setattr
nfs_setattr (expected)
ntfs_setattr
smb_notify_change
xfs_vn_setattr

...some of these won't matter, but some will need to be fixed. There
may be other situations we'll need to fix as well.

My concern here is that we'll be moving from a "default safe" model for
filesystems that define a .setattr op to one where those filesystems
are expected to make sure that they clear setuid/gid bits. They can do
this with a simple call to inode_setattr, or on their own, but they
must do it. Is this really the right thing to do here?

It seems like an "opt-out" approach when clearing these bits might be
safer. i.e., make it so that filesystems have to explicitly disable
clearing of setuid/setgid bits.

Thoughts?

-- 
Jeff Layton <jlayton@redhat.com>

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [NFS] [RFC:PATCH] How best to handle implicit clearing of setuid/setgid bits on NFS?

[Trond Myklebust]

On Mon, 2007-07-23 at 15:05 -0400, Jeff Layton wrote:
> On Thu, 28 Jun 2007 09:38:22 -0400
> Trond Myklebust <trond.myklebust@fys.uio.no> wrote:
> 
> > On Wed, 2007-06-27 at 22:13 -0400, Jeff Layton wrote:
> > > Ok. This is a bit more complex now since we remove suid bits on
> > > truncate, but don't set ATTR_FORCE.
> > > 
> > > Here's a patch that should do this. I know there's a general
> > > aversion to adding new flags to vfs structures, but I couldn't think of
> > > a way to cleanly do this without adding one.
> > > 
> > > Note that I've not tested this patch at all so this is just a RFC.
> > > 
> > > CC'ing Al here since he's expressed interest in this problem as well.
> > > 
> > > Thoughts?
> > 
> > We don't really need to do this with extra VFS flags. Here is my
> > preferred approach for dealing with this problem.
> > 
> >         http://article.gmane.org/gmane.linux.nfs/8511/match=attr%5fkill%5fsuid
> > 
> > As you can see, that still allows the filesystem to determine how it
> > should deal with the ATTR_KILL_SUID/ATTR_KILL_SGID flags. The default
> > behaviour is provided by inode_setattr(), and is the same as today. Only
> > filesystems that don't use inode_setattr() will need to be audited for
> > whether or not they need a fix.
> > 
> > Cheers,
> >   Trond
> 
> 
> I had a look at this today, and I have some reservations about this
> approach. Quite a few filesystems define a .setattr inode op, but don't
> call inode_setattr. As a first pass through the current git tree, the
> following setattr ops never seem to call inode_setattr:

> adfs_notify_change
    ^^^ doesn't support setuid/setgid mode bits in the first place.

> afs_setattr
    networked filesystem: may need a solution like nfs. Can band aid
over the problem by calling a helper to translate.
ATTR_KILL_SUID/ATTR_KILL_SGID into the appropriate ATTR_MODE.

> coda_setattr
    ditto

> ecryptfs_setattr
    this is a layered filesystem. The underlying filesystem needs to
deal with the mode.

> fuse_setattr
    fix iattr_to_fattr to call helper to translate
ATTR_KILL_SUID/ATTR_KILL_SGID into ATTR_MODE

> jffs2_setattr
    add call to helper to translate ATTR_KILL_SUID/ATTR_KILL_SGID to
ATTR_MODE.

> nfs_setattr (expected)

> ntfs_setattr
    Returns EOPNOTSUPP if you try to set ATTR_MODE.

> smb_notify_change
    Returns EPERM if you try to set ATTR_MODE

> xfs_vn_setattr
    add call to helper to translate ATTR_KILL_SUID/ATTR_KILL_SGID

> ...some of these won't matter, but some will need to be fixed. There
> may be other situations we'll need to fix as well.

> My concern here is that we'll be moving from a "default safe" model for
> filesystems that define a .setattr op to one where those filesystems
> are expected to make sure that they clear setuid/gid bits. They can do
> this with a simple call to inode_setattr, or on their own, but they
> must do it. Is this really the right thing to do here?

What is so bloody difficult about remembering to support ATTR_KILL_SUID
ATTR_KILL_SGID vs all the other ATTR_* flags if you are choosing to
implement your own .setattr?
As long as there exists a simple VFS helper to do the translation into
an ATTR_MODE request, so that those filesystems that rely on the current
translation by 'notify_change' can easily migrate, then I can't see why
this is such a problem.

Trond

Re: [RFC:PATCH] How best to handle implicit clearing of setuid/setgid bits on NFS?

[Jeff Layton]

On Mon, 23 Jul 2007 16:33:23 -0400 Trond Myklebust
<trond.myklebust@fys.uio.no> wrote:
> What is so bloody difficult about remembering to support ATTR_KILL_SUID
> ATTR_KILL_SGID vs all the other ATTR_* flags if you are choosing to
> implement your own .setattr?
> As long as there exists a simple VFS helper to do the translation into
> an ATTR_MODE request, so that those filesystems that rely on the current
> translation by 'notify_change' can easily migrate, then I can't see why
> this is such a problem.
> 

You're quite right. If you declare your own .setattr, then you ought to
know what you're doing. My concern was more about the breadth of the
change and how best to make sure we don't open security holes with this.

On the technical side, I don't think we can just move this into
inode_setattr. The ia_mode really needs to be set before inode_change_ok
is called. So I think we'll have to make sure that all .setattr ops
call the helper explicitly.

I'm working on a patchset now and hope to have something together in a
few days.

-- 
Jeff Layton <jlayton@redhat.com>

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs