From: Peter Staubach <staubach@redhat.com>
Date: Mon, 1 Jun 2009 11:40:13 -0400
Subject: [nfs] fix stripping SUID/SGID flags when chmod/chgrp dir
Message-id: 4A23F65D.5060108@redhat.com
O-Subject: [PATCH RHEL-5.5] BZ485099 Inconsistent behaviour in stripping SUID/SGID flags when chmod/chgrp directories
Bugzilla: 485099
RH-Acked-by: Jeff Layton <jlayton@redhat.com>

Hi.

Attached is a patch to address bz485099, "Inconsistent behaviour
in stripping SUID/SGID flags when chmod/chgrp directories".  This
bugzilla describe the situation that the NFS server does not
handle change owner requests in the same fashion as do local file
systems such as ext3.

File systems such as ext3 handle requests to change the owner of
a regular file by ensuring that the setuid and/or setgid bits are
cleared appropriately.  Leaving the bits set may allow a security
breach.  The setuid and setgid bits are left alone when an attempt
is made to change the ownership of any other file type, ie.
directories, symbolic links, etc.

The NFS server was unilaterally turning off the setuid and setgid
bits on all change owner requests.  This made the semantics
visible through an NFS client different than they would be locally.

This was addressed by checking the file type before arranging to
have the setuid and setgid bits cleared.

Also, support was added so that setting the mode at the same time
as changing the owner would not allow the NFS client to end up
with a file, changed in ownership, but retaining the setuid or
setgid bit set.

A tiny bit of cleanup was done while I was checking the mode
handling in the routine to ensure that it was correct.

    Thanx...

       ps

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 773b280..b19a125 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -268,7 +268,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
 	struct inode	*inode;
 	int		accmode = MAY_SATTR;
 	int		ftype = 0;
-	int		imode;
 	int		err;
 	int		size_change = 0;
 
@@ -354,16 +353,26 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
 		DQUOT_INIT(inode);
 	}
 
-	imode = inode->i_mode;
+	/* sanitize the mode change */
 	if (iap->ia_valid & ATTR_MODE) {
 		iap->ia_mode &= S_IALLUGO;
-		imode = iap->ia_mode |= (imode & ~S_IALLUGO);
+		iap->ia_mode |= (inode->i_mode & ~S_IALLUGO);
 	}
 
 	/* Revoke setuid/setgid bit on chown/chgrp */
-	if (((iap->ia_valid & ATTR_UID) && iap->ia_uid != inode->i_uid) ||
-	    ((iap->ia_valid & ATTR_GID) && iap->ia_gid != inode->i_gid))
-		iap->ia_valid |= (ATTR_KILL_SGID | ATTR_KILL_SUID);
+	if (!S_ISDIR(inode->i_mode) &&
+	   (((iap->ia_valid & ATTR_UID) && iap->ia_uid != inode->i_uid) ||
+	    ((iap->ia_valid & ATTR_GID) && iap->ia_gid != inode->i_gid))) {
+		if (iap->ia_valid & ATTR_MODE) {
+			/* we're setting mode too, just clear the s*id bits */
+			iap->ia_mode &= ~S_ISUID;
+			if (iap->ia_mode & S_IXGRP)
+				iap->ia_mode &= ~S_ISGID;
+		} else {
+			/* set ATTR_KILL_* bits and let VFS handle it */
+			iap->ia_valid |= (ATTR_KILL_SGID | ATTR_KILL_SUID);
+		}
+	}
 
 	/* Change the attributes. */