NFSD SGID permission problem

NFSD SGID permission problem

[Kenneth Sumrall]

[-- Attachment #1: Type: text/plain, Size: 2365 bytes --]

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 <niemi@tux.org>
          *

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
ksumrall@pacbell.net


[-- Attachment #2: nfsd.chown.patch --]
[-- Type: text/plain, Size: 783 bytes --]

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
--- linux/fs/nfsd/vfs.c	17 Dec 2002 18:21:16 -0000	1.3
+++ linux/fs/nfsd/vfs.c	11 Dec 2004 01:56:26 -0000
@@ -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. */
 
 

NFSD SGID permission problem

[Kenneth Sumrall]

[-- Attachment #1: Type: text/plain, Size: 2677 bytes --]

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
ksumrall@pacbell.net

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

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 <niemi@tux.org>
           *

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
ksumrall@pacbell.net



[-- Attachment #2: nfsd.chown.patch --]
[-- Type: text/plain, Size: 784 bytes --]

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
--- linux/fs/nfsd/vfs.c	17 Dec 2002 18:21:16 -0000	1.3
+++ linux/fs/nfsd/vfs.c	11 Dec 2004 01:56:26 -0000
@@ -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. */
 
 

Re: NFSD SGID permission problem

[Neil Brown]

On Thursday March 31, ksumrall@pacbell.net wrote:
> 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 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
--- ./fs/nfsd/vfs.c~current~	2005-04-01 12:00:29.000000000 +1000
+++ ./fs/nfsd/vfs.c	2005-04-01 12:04:44.000000000 +1000
@@ -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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: NFSD SGID permission problem

[Kenneth Sumrall]

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
ksumrall@pacbell.net


Neil Brown wrote:
> On Thursday March 31, ksumrall@pacbell.net wrote:
> 
>>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 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
> --- ./fs/nfsd/vfs.c~current~	2005-04-01 12:00:29.000000000 +1000
> +++ ./fs/nfsd/vfs.c	2005-04-01 12:04:44.000000000 +1000
> @@ -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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs