Messages in this thread Patch in this message | | | Date | Sun, 1 Jun 1997 11:56:49 +0300 (IDT) | From | Andi Gutmans <> | Subject | Re: Bug in chown -- always kills suid/sgid bits. |
| |
Hi,
When changing uid/gid taking off the s-bit is definetaly the RIGHT thing to do. It's much safer this way. ie. I'm against that second patch Andi
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-= Andi Gutmans - Computer Science, Israel Institute of Technology ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Web: http://andi.il.eu.org PGP: finger andi@vipe.technion.ac.il KeyID 62F5661D fingerprint = 1A 87 A2 10 2F EF EF AB 47 E0 4D 42 F5 B5 49 AD
On Sun, 1 Jun 1997, Greg Alexander wrote:
> 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, > >
| |