From: Jeff Layton <jlayton@redhat.com>
Date: Fri, 2 May 2008 12:15:15 -0400
Subject: [nfs] knfsd: revoke setuid/setgid when uid/gid changes
Message-id: 1209744915-4635-1-git-send-email-jlayton@redhat.com
O-Subject: [RHEL5.3 PATCH] BZ#443043: knfsd: revoke both setuid and setgid when uid or gid changes
Bugzilla: 443043
RH-Acked-by: Steve Dickson <SteveD@redhat.com>
RH-Acked-by: Peter Staubach <staubach@redhat.com>

Currently, knfsd only clears the setuid bit if the owner of a file is
changed on a SETATTR call, and only clears the setgid bit if the group
is changed. POSIX says this in the spec for chown():

    "If the specified file is a regular file, one or more of the
     S_IXUSR, S_IXGRP, or S_IXOTH bits of the file mode are set, and the
     process does not have appropriate privileges, the set-user-ID
     (S_ISUID) and set-group-ID (S_ISGID) bits of the file mode shall
     be cleared upon successful return from chown()."

If I'm reading this correctly, then knfsd is doing this wrong. It should
be clearing both the setuid and setgid bit on any SETATTR that changes
the uid or gid. This wasn't really as noticable before, but now that the
ATTR_KILL_S*ID bits are a no-op for the NFS client, it's more evident.

This patch recently went upstream and should be in 2.6.26. Tested by
doing chown/chgrp on a NFS client against a file with the setuid and
setgid bits set.

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index ebd8bf6..e3bae15 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -352,10 +352,9 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
 	}
 
 	/* Revoke setuid/setgid bit on chown/chgrp */
-	if ((iap->ia_valid & ATTR_UID) && iap->ia_uid != inode->i_uid)
-		iap->ia_valid |= ATTR_KILL_SUID;
-	if ((iap->ia_valid & ATTR_GID) && iap->ia_gid != inode->i_gid)
-		iap->ia_valid |= ATTR_KILL_SGID;
+	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);
 
 	/* Change the attributes. */