Date: Sun, 19 Jan 1997 04:27:13 +1100 From: Bruce Evans <bde@zeta.org.au> To: babkin@hq.icb.chel.su, jkh@time.cdrom.com Cc: hackers@freebsd.org Subject: Re: Patch for cp Message-ID: <199701181727.EAA24510@godzilla.zeta.org.au>
next in thread | raw e-mail | index | archive | help
>This patch fixes bug in /bin/cp that made it returning error "Operation
>not permitted" when copying files with schg flags with -p key
>like:
>
> cp -p /usr/bin/passwd /tmp/x
>...
>*** utils.c 1997/01/18 06:36:56 1.1
>--- utils.c 1997/01/18 06:46:47
>***************
>*** 159,181 ****
>...
>--- 159,184 ----
> * to remove it if we created it and its length is 0.
> */
>
>! if (pflag) {
>! if (setfile(fs, to_fd))
>! rval = 1;
> /*
> * If the source was setuid or setgid, lose the bits unless the
> * copy is owned by the same user and group.
> */
> #define RETAINBITS \
> (S_ISUID | S_ISGID | S_ISVTX | S_IRWXU | S_IRWXG | S_IRWXO)
>! } else {
>! if (fs->st_mode & (S_ISUID | S_ISGID) && fs->st_uid == myuid)
>! if (fstat(to_fd, &to_stat)) {
>! warn("%s", to.p_path);
>! rval = 1;
>! } else if (fs->st_gid == to_stat.st_gid &&
>! fchmod(to_fd, fs->st_mode & RETAINBITS & ~myumask)) {
>! warn("%s", to.p_path);
>! rval = 1;
>! }
>! }
> (void)close(from_fd);
> if (close(to_fd)) {
> warn("%s", to.p_path);
>
This patch obscures the important change by reformatting the large `else'
clause. Here is a simpler equivalent version:
---
diff -c2 utils.c~ utils.c
*** utils.c~ Sun Jan 19 02:58:06 1997
--- utils.c Sun Jan 19 02:57:22 1997
***************
*** 160,165 ****
*/
! if (pflag && setfile(fs, to_fd))
! rval = 1;
/*
* If the source was setuid or setgid, lose the bits unless the
--- 160,165 ----
*/
! if (pflag)
! rval = setfile(fs, to_fd);
/*
* If the source was setuid or setgid, lose the bits unless the
---
There seem to be a lot more bugs in this area:
1. chflags(2) silently ignores attempts by non-root to set the schg bit.
This is convenient for cp(1), but doesn't seem right for chflags(2)
or chflags(1). chflags.2 says that only root can set the schg bit,
but it doesn't say that attempts by non-root are silently ignored.
2. The following code (original version) doesn't match either the
comment or the man page:
> /*
> * If the source was setuid or setgid, lose the bits unless the
> * copy is owned by the same user and group.
> */
> #define RETAINBITS \
> (S_ISUID | S_ISGID | S_ISVTX | S_IRWXU | S_IRWXG | S_IRWXO)
>! else if (fs->st_mode & (S_ISUID | S_ISGID) && fs->st_uid == myuid)
>! if (fstat(to_fd, &to_stat)) {
>! warn("%s", to.p_path);
>! rval = 1;
>! } else if (fs->st_gid == to_stat.st_gid &&
>! fchmod(to_fd, fs->st_mode & RETAINBITS & ~myumask)) {
>! warn("%s", to.p_path);
>! rval = 1;
>! }
2a. myuid is the real uid. It seems to be used as a cheap equivalent for
the uid of the target file. This is wrong in two ways: first, if
the file was created by cp, then its uid is the effective uid which
may be different. Second, if the file already existed, its uid may
be different from the process ids. I think the myuid test should just
be removed.
2b. There is no test for fs->st_uid == to_stat.st_uid, probably because
of bug 2a.
2c. The chmod handling seems to be completely broken:
2ca. Only the uid and gid of the target should be touched. The umask
and the other bits in RETAINMASK should not be used here (cp doesn't
normally change the mode of an existing target, and this should not
change apart from the set*id bits if the source or target has set*id
bits set).
2cb. If the target already has some set*id bits set, then they should be
cleared unless they are inherited from the source. This isn't
usually done, since the fchmod() isn't reached unless the source
has some set*id bits set.
2cc. The mode shouldn't be (mostly) copied from the source to the target
just because the source has some set*id bits set.
The fchmod() makes a little more sense if it is done for the cp -p case,
but the original fix stops the fchmod() being reached in this case.
Previously it was only reached when setfile() succeeded. I think that
case is handled correctly now - setfile() doesn't copy the set*id bits if
the chown() failed. I think the case where setfile() fails is OK too -
it always attempts to copy the mode with the set*id bits turned off if
necessary, and if that fchmod() somehow fails then another one would
probably also fail.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199701181727.EAA24510>