Bug in chown -- always kills suid/sgid bits.

Greg Alexander (galexand@sietch.bloomington.in.us)
Sun, 1 Jun 1997 00:44:47 -0500 (EST)

It _always_ kills the sgid/suid bits on a file, despite the fact that the
comments indicated that it should only kill them when the uid or gid
changes. I have included a patch to make this work as the comments would
indicated.
However, I feel _VERY_ strongly that this is an incorrect behaviour.
chown is chown, it changes ownership. chmod is chmod, it changes the
rights. They are seperate system calls because they do seperate jobs. One
of the great things about Unix is that you can make it do exactly what you
tell it without ever having to repeat yourself...usually. But if the only
way to make chown() act _only_ as chown() involves getting the rights,
calling chown(), then restoring the rights, there is something very clearly
wrong here.
So I'm including two patches. The first one makes the code work as
the comments indicate and I think are suitable for inclusion into a real
Linux kernel. The second one makes the code work as it should. I don't
know what POSIX has to say on this and I don't think it matters. Linux
should not sacrifice sanity merely to be compliant. There are times where
the standard is weak and we must be better.
If these were corrupted in the mail, just E-Mail me and I can
uuencode or MIME them.

--- open.c.orig Sat May 31 22:23:47 1997
+++ open.c Sun Jun 1 00:36:32 1997
@@ -402,7 +402,7 @@
/*
* If the owner has been changed, remove the setuid bit
*/
- if (inode->i_mode & S_ISUID) {
+ if ((user != inode->i_uid) && (inode->i_mode & S_ISUID)) {
newattrs.ia_mode &= ~S_ISUID;
newattrs.ia_valid |= ATTR_MODE;
}
@@ -412,7 +412,7 @@
* 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))) {
+ if ((group != inode->i_gid) && (((inode->i_mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)))) {
newattrs.ia_mode &= ~S_ISGID;
newattrs.ia_valid |= ATTR_MODE;
}
-----------------------------------------------------------------------

--- open.c.orig Sat May 31 22:23:47 1997
+++ open.c Sat May 31 22:24:17 1997
@@ -399,23 +399,6 @@
newattrs.ia_uid = user;
newattrs.ia_gid = group;
newattrs.ia_valid = ATTR_UID | ATTR_GID | ATTR_CTIME;
- /*
- * If the owner has been changed, remove the setuid bit
- */
- if (inode->i_mode & S_ISUID) {
- newattrs.ia_mode &= ~S_ISUID;
- newattrs.ia_valid |= ATTR_MODE;
- }
- /*
- * 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;
- }
inode->i_dirt = 1;
if (inode->i_sb && inode->i_sb->dq_op) {
inode->i_sb->dq_op->initialize(inode, -1);
@@ -454,23 +437,6 @@
newattrs.ia_uid = user;
newattrs.ia_gid = group;
newattrs.ia_valid = ATTR_UID | ATTR_GID | ATTR_CTIME;
- /*
- * If the owner has been changed, remove the setuid bit
- */
- if (inode->i_mode & S_ISUID) {
- newattrs.ia_mode &= ~S_ISUID;
- newattrs.ia_valid |= ATTR_MODE;
- }
- /*
- * 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;
- }
inode->i_dirt = 1;
if (inode->i_sb->dq_op) {
inode->i_sb->dq_op->initialize(inode, -1);
-----------------------------------------------------------------------

Greg Alexander
http://www.cia-g.com/~sietch/
----
"I read about monkeys in the encyclopedia as soon as I got home from the
funeral and I wonder if this one throws turds and masturbates all the time
like those monkeys saw it the zoo in San Francisco or if witness monkeys are
more like people."
-- a character in Orson Scott Card and Kathryn H. Kidd's novel,