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. */
On Thursday March 31, [email protected] 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 - [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]
Neil Brown wrote:
> On Thursday March 31, [email protected] 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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs