Horribly Mistimed complaint about fs/open.c:chown_common()

David C Niemi (niemi@tux.org)
Thu, 15 Oct 1998 00:03:47 -0400 (EDT)


I realize this is at a totally stupid time to bring this up, and I will
understand if wants to think about this until the 2.3 days, but there is
what I consider a long-standing bug (and certainly unexpected behavior) in
chown_common:

/*
* If the group has been changed, remove the setgid bit
*
* Don't remove the setgid bit if no group execute bit.
* This is a file marked for mandatory locking.
*/
if (((inode->i_mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP))) {
newattrs.ia_mode &= ~S_ISGID;
newattrs.ia_valid |= ATTR_MODE;
}

In other variants of Unix, this test is not applied to directories, where
the setgid bit does not have the same security implications as it does for
files (rather it signals that BSD group ID behavior is to be used on new
files/dirs created under it). So I believe this test should also exclude
directories, perhaps something like this:

if ((((inode->i_mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP))
&& (! S_ISDIR(inode->i_mode)))

Is there some reason for this unusual behavior that I am missing? I have
been annoyed for literally YEARS on my setgid bits getting lost on my
directories whenever they are chgrp'd.

Hmmmm, by looking at this, I don't think this comment really reflects what
is going on! This looks to me like it would also remove the setgid bit any
time the owner changes as well -- and a quick test verifies this is so.
There is likewise no such logic on the part that removes SUID bits either!
So the code does not at all match the comments, and any change to either
group or user clears both SUID and SGID bits.

The patch to do this is pretty obvious, I'll be happy to generate it if
anyone wants to actually use it but doesn't consider it trivial.

--- David C Niemi ---niemi at tux.org--- Reston, Virginia, USA ---
But only the man who cares about something in itself, who loves
it and does it *con amore*, will do it in all seriousness. The
highest achievement has always been that of such men, and not of
the hacks who serve for pay. -- Arthur Schopenhauer

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/