How best to handle implicit clearing of setuid/setgid bits on NFS?

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

Quoted 27 lines · Jeff Layton

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?

Quoted 5 lines

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?

Quoted 7 lines · Jeff Layton

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.

Quoted 5 lines

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

Quoted 19 lines · On Thu, 28 Jun 2007 09:38:22 -0400 Trond Myklebust…

Quoted 7 lines · Jeff Layton

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.

Quoted 5 lines

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

Quoted 8 lines

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?

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.