Re: NFSD SGID permission problem

[Kenneth Sumrall <ksumrall@pacbell.net>]

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