NFSD SGID permission problem

So, I sent the message below on 12/10/04, and got no response. So, I'm resending it again. I'd like some feedback on this as I'm not sure my fix doesn't create a security hole.

Thanks!

Ken Sumrall
[email protected]

=======================================================================

Hello,

I've been running the Linux Test Package on a system with NFS root, and one of the errors it found I think is a bug in the kernel NFSD.

The test is chown02, and it tests two things. Quoting from the comments in the test case, it tests:

* Test Description:

  *  Verify that, when chown(2) invoked by super-user to change the owner and
  *  group of a file specified by path to any numeric owner(uid)/group(gid)
  *  values,

* - clears setuid and setgid bits set on an executable file.
* - preserves setgid bit set on a non-group-executable file.

The second test case is the one that fails. In the example, if a file has these permissions and owners:
-rwx--S--- 1 root root 0 Dec 10 17:39 /tmp/testfile2

and chown(2) is called to change the owner/group to 700:701, the resulting file should look like this:
-rwx--S--- 1 700 701 0 Dec 10 17:39 /tmp/testfile2

On an NFS mounted filesystem (with no_root_squash enabled, so I'm still root across the mount) I get this:
-rwx------ 1 700 701 0 Dec 10 17:39 /tmp/testfile2

After a brief search for SGID in the nfsd code, I removed a few lines of code from linux/fs/nfsd/vfs.c (see attached patch) and I started to get the right results. The underlying filesystem appears to properly enforce clearing of the SGID bit when the group execute bit is set, and leave the SGID alone when the group execute bit is not set. I did this work on my Redhat 9.0 workstation which was the NFS server for these tests, so the patch is against a 2.4.20 kernel, but the same code is in 2.4.28, and similar code is in 2.6.9. The underlying filesystem NFSD was exporting was ext3.

So, the question is, is this patch the proper fix? The code to keep the SGID bit if group execute is not present is in linux/fs/open.c, and the comment says:
/*
* Likewise, if the user or group of a non-directory has been changed
* by a non-root user, remove the setgid bit UNLESS there is no group
* execute bit (this would be a file marked for mandatory locking).
* 19981026 David C Niemi <[email protected]>
*

so it appears to be common code, and not dependent on the underlying filesystem.

I'm no NFS expert, so I'm looking for comments and feedback.

Thanks!

Ken Sumrall
[email protected]

Index: linux/fs/nfsd/vfs.c
===================================================================
RCS file: /cvsdev/mvl-kernel/linux/fs/nfsd/vfs.c,v retrieving revision 1.3

diff -u -r1.3 vfs.c
@@ -277,18 +277,6 @@
 		imode = iap->ia_mode |= (imode & ~S_IALLUGO);
 	}
 
-	/* Revoke setuid/setgid bit on chown/chgrp */
-	if ((iap->ia_valid & ATTR_UID) && (imode & S_ISUID)
-	 && iap->ia_uid != inode->i_uid) {
-		iap->ia_valid |= ATTR_MODE;
-		iap->ia_mode = imode &= ~S_ISUID;
-	}
-	if ((iap->ia_valid & ATTR_GID) && (imode & S_ISGID)
-	 && iap->ia_gid != inode->i_gid) {
-		iap->ia_valid |= ATTR_MODE;
-		iap->ia_mode = imode &= ~S_ISGID;
-	}
-
 	/* Change the attributes. */
 
 

Thanks for being persistent.
You have identified a real issue, but you are right that your fix isn't really safe.

The following mimics the logic in chown_common() in open.c. Note that it also ignores the setXid bits on directories.

I would appreciate it if you could double check that this fixes your problem.

This is against 2.4.30-rc1 (and later). The same problem does not exist in 2.6.

NeilBrown

### Diffstat output

 ./fs/nfsd/vfs.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff ./fs/nfsd/vfs.c~current~ ./fs/nfsd/vfs.c
@@ -280,13 +280,17 @@ nfsd_setattr(struct svc_rqst *rqstp, str
 	}
 
 	/* Revoke setuid/setgid bit on chown/chgrp */
-	if ((iap->ia_valid & ATTR_UID) && (imode & S_ISUID)
-	 && iap->ia_uid != inode->i_uid) {
+	if ((iap->ia_valid & ATTR_UID)
+	    && (imode & S_ISUID)
+	    && !S_ISDIR(imode)
+	    && iap->ia_uid != inode->i_uid) {
 		iap->ia_valid |= ATTR_MODE;
 		iap->ia_mode = imode &= ~S_ISUID;
 	}
-	if ((iap->ia_valid & ATTR_GID) && (imode & S_ISGID)
-	 && iap->ia_gid != inode->i_gid) {
+	if ((iap->ia_valid & ATTR_GID)
+	    && (imode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)
+	    && !S_ISDIR(imode)
+	    && iap->ia_gid != inode->i_gid) {
 		iap->ia_valid |= ATTR_MODE;
 		iap->ia_mode = imode &= ~S_ISGID;
 	}

-------------------------------------------------------
This SF.net email is sponsored by Demarc:
A global provider of Threat Management Solutions. Download our HomeAdmin security software for free today!
http://www.demarc.com/Info/Sentarus/hamr30
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Hi Neil,

Thanks for the reply. I'll test your patch tomorrow.

I took a closer look at the 2.6 code, and I see that it uses common code in notify_change() in fs/attr.c to actually clear the bit. So that seems OK. However, shouldn't the code in nfsd/vfs.c also make sure it's not a directory before it sets the ATTR_KILL_SGID flag? That's what the code in open.c does, and there doesn't appear to be a check for it not being a directory in fs/attr.c. Or am I missing something?

Also, is it possible to write up a quick little blurb about why my fix isn't really safe? It seemed to get handled by the underlying file system just fine when I tested it. I'm just trying to learn for the future. If it's extremely complicated, you can just say "It's technical." :-)

Thanks.

Ken Sumrall
[email protected]