2011/6/13 Jan Stary <h...@stare.cz>
> On Jun 13 11:50:04, Jesus Sanchez wrote:
> > Hi misc,
> >
> > First of all sorry for my crappy english, i'm trying my best:
> >
> > I found a posible issue with cp(1) when the source file and target file
> > match
> > with the process uid and gid and source file have S_ISUID or S_ISGID
> active.
> > It
> > overrides the target file permission mode with the source file mode. This
> > issue
> > also involves the '-p' behaviour. Before filling a bugreport I would like
> to
> > discuss this in misc@
> >
> > In the form 'cp src dst', src and dst are existing regular files with
> same
> > uid
> > and gid with the calling process (i.e. user 'foo' copies in a file in
> his
> > home), the mode of 'dst' will be the mode of 'src' with ~umask and
> > RETAINBITS.
> >
> > the manpage says that a existing file will preserve mode
>
> yes:
>
> For each destination file that already exists, its contents are
> overwritten if permissions allow, but its mode, user ID, and group ID
> are
> unchanged.
>
>
> > and if source file
> > have set-user id bit or set-group id bit actives and both source file and
> > target file share uid and gid with the calling process S_ISUID or S_ISGID
> > bits
> > will be preserved.
>
> Actually, the manpage doesn't explicitly say "the set[ug]id bits will be
> preserved if permissions allow"; it carefully describes the situations
> when the source's setuid bit is *removed* from the copy (not the same
> owner,
> or the owner cannot be preserved).
>
> Anyway, the setuid/setgid bits are parts of the mode (right?), so they
> should not be altered for *existing* destination files, according to the
> quote above; which I suppose is what you are trying to say is broken.
>
that's the part not clear to me also, the design could have two options:
- in case of the file exists not touch the mode at all (not even setuid
setgid)
- in case of the file exists preserve the first 9 bits (rwxrwxrwx) and add
the setuid and/or setgid of the source file.
this isn't assuming the '-p' option , only the clean 'cp src target' way.
the problem here is other, even if cp(1) add or not the source setuid
or setgid bits to the target the rest of the mode( the first 9 bits
(rwxrwxrwx) )should not be changed at all, and it changes it in the case of
both source
and target file having the UID and GID of the process (i.e. the user
copying a file in his home).
>
> > assuming user umask 0000
> >
> > if file 'src' exists with mode 4666 (rwSrw-rw-) and 'dst' exist with mode
> > 0600
> > (rw-------) the result of 'cp src dst' (if the process uid and gid is the
> > same
> > as the src and dst files) will be dst with 4666 instead 4600 as expected.
> >
> > In case of 'dst' beign an existing file it should preserve his original
> mode
> > (0600) and add the S_ISUID or S_ISGID from the source file (4000)
> resulting
> > in
> > (4600), not (4666) as result.
> >
> > I wrote a dirty diff for src/bin/cp/util.c to illustrate the possible
> patch.
> >
> > to reproduce the issue:
> >
> > umask 000
> > mkdir tmp
> > cd tmp
> > touch src
> > touch dst
> > chmod 4666 src
> > cp src dst
> > ls -l
> > --- dst have 4666 mode (rwSrw-rw-) ---
>
>
> I think I know what you mean, but there is at least the 'chmod 600 dst'
>
that's right!, i missed that one, exactly, my 'reproduce code' needs
a chmod 600 dst before the copy to illustrate the issue.
> missing to show the behaviour you are describing. I will try to rephrase,
> in context:
>
> hans@stary:tmp$ cd `mktemp -d -p.`
> hans@stary:tmp.7RPD6gvp99$ umask 000
>
> hans@stary:tmp.7RPD6gvp99$ touch src
> hans@stary:tmp.7RPD6gvp99$ ls -l
> total 0
> -rw-rw-rw- 1 hans wheel 0 Jun 13 17:19 src
> hans@stary:tmp.7RPD6gvp99$ cp src dst
> hans@stary:tmp.7RPD6gvp99$ ls -l
> total 0
> -rw-rw-rw- 1 hans wheel 0 Jun 13 17:19 dst
> -rw-rw-rw- 1 hans wheel 0 Jun 13 17:19 src
>
> Here, the destination file did not exist, so it was created
> with the (umasked) mode of the source.
>
> hans@stary:tmp.7RPD6gvp99$ chmod 600 dst
> hans@stary:tmp.7RPD6gvp99$ ls -l
> total 0
> -rw------- 1 hans wheel 0 Jun 13 17:19 dst
> -rw-rw-rw- 1 hans wheel 0 Jun 13 17:19 src
>
> Now the destination file exists, so if we cp(1) again,
> its mode should stay the same ...
>
> hans@stary:tmp.7RPD6gvp99$ cp src dst
> hans@stary:tmp.7RPD6gvp99$ ls -l
> total 0
> -rw------- 1 hans wheel 0 Jun 13 17:20 dst
> -rw-rw-rw- 1 hans wheel 0 Jun 13 17:19 src
>
> ... and it does.
>
> Now this is exactly what breaks if the src file has the setuid bit set:
>
> hans@stary:tmp.7RPD6gvp99$ chmod 4666 src
> hans@stary:tmp.7RPD6gvp99$ ls -l
> total 0
> -rw------- 1 hans wheel 0 Jun 13 17:20 dst
> -rwSrw-rw- 1 hans wheel 0 Jun 13 17:19 src
> hans@stary:tmp.7RPD6gvp99$ cp src dst
> hans@stary:tmp.7RPD6gvp99$ ls -l
> total 0
> -rwSrw-rw- 1 hans wheel 0 Jun 13 17:20 dst
> -rwSrw-rw- 1 hans wheel 0 Jun 13 17:19 src
>
> Here, cp(1) did *not* preserve the mode of an existing destination file.
> Is that what you meant?
>
you got it!, that's my point, even if cp(1) adds or not the setuid
or setgid bits of the source file, the rwxrwxrwx bits of the target
should be preserved.
>
> (With regard to the '-p' option: with 'cp -p', the destination file's mode
> is always overwritten by the (umasked) mode of the source, which is
> what -p is supposed to do; so the above does not manifest with 'cp -p'.)
>
>
in that case my diff it's not right for that part, the first added
lines should be ignored.
> In short, it seems that "copying a setuid file implies -p".
>
> -p Preserve in the copy as many of the modification time, access
> time, file flags, file mode, user ID, and group ID as allowed
> by
> permissions.
>
> > [demime 1.01d removed an attachment of type application/octet-stream
> which had a name of 20110613_bin_cp_util_c.diff]
>
> This maling list strips attachments; put your diff inline.
>
>
> Jan
>
>
thanks for your time and effort in this issue.
here is the diff (was mailed before to Otto).
-----------------------------------------------------------
--- utils.c.orig Tue Jun 14 03:18:56 2011
+++ utils.c Tue Jun 14 03:18:51 2011
@@ -170,20 +170,26 @@
return (1);
}
- if (pflag && setfile(fs, to_fd))
- rval = 1;
+ if (pflag){ /* In case of pflag we don't try to retain S_ISUID or
S_ISGID bits and ignore the umask values */
+ if( setfile(fs, to_fd))
+ rval = 1;
+#define RETAINBITS \
+ (S_ISUID | S_ISGID | S_ISVTX | S_IRWXU | S_IRWXG | S_IRWXO)
+ }
/*
* 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)) {
+ fchmod(to_fd, (((S_ISUID | S_ISGID) & fs->st_mode ) |
to_stat->st_mode) & RETAINBITS & ~myumask)) {
+ /*
+ * We preserve the target file mode and add only the
+ * S_ISUID and S_ISGID bits from the source file mode
+ */
warn("%s", to.p_path);
rval = 1;
}
