cp -p now clears special bits if it fails to preserve owner or group

* NEWS: Document the cp -p fix for special bits.
* src/copy.c (set_owner): Now returns a three-way result, so
that the caller can clear the special bits.  All callers changed.
(copy_reg): Don't set the special bits if chown failed.
(copy_internal): Likewise.
* tests/cp/special-bits: Test this fix.
Signed-off-by: Jim Meyering <jim@meyering.net>
This commit is contained in:
Paul Eggert 2006-12-07 08:10:35 +01:00 committed by Jim Meyering
parent a4f7b723f0
commit fc92148eac
4 changed files with 53 additions and 14 deletions

View File

@ -1,3 +1,12 @@
2006-12-06 Paul Eggert <eggert@cs.ucla.edu>
* NEWS: Document the cp -p fix for special bits.
* src/copy.c (set_owner): Now returns a three-way result, so
that the caller can clear the special bits. All callers changed.
(copy_reg): Don't set the special bits if chown failed.
(copy_internal): Likewise.
* tests/cp/special-bits: Test this fix.
2006-12-06 Paul Eggert <eggert@cs.ucla.edu>
* NEWS: Document the cp --preserve=ownership fix.

8
NEWS
View File

@ -4,6 +4,14 @@ GNU coreutils NEWS -*- outline -*-
** Bug fixes
When cp -p copied a file with special mode bits set, the same bits
were set on the copy even when ownership could not be preserved.
This could result in files that were setuid to the wrong user.
To fix this, special mode bits are now set in the copy only if its
ownership is successfully preserved. Similar problems were fixed
with mv when copying across file system boundaries. This problem
affects all versions of coreutils through 6.6.
cp --preserve=ownership would create output files that temporarily
had too-generous permissions in some cases. For example, when
copying a file with group A and mode 644 into a group-B sticky

View File

@ -175,22 +175,22 @@ copy_dir (char const *src_name_in, char const *dst_name_in, bool new_dst,
st_gid fields of SRC_SB. If DEST_DESC is undefined (-1), set
the owner and owning group of DST_NAME instead. DEST_DESC must
refer to the same file as DEST_NAME if defined.
Return true if the syscall succeeds, or if it's ok not to
preserve ownership. */
Return 1 if the syscall succeeds, 0 if it fails but it's OK
not to preserve ownership, -1 otherwise. */
static bool
static int
set_owner (const struct cp_options *x, char const *dst_name, int dest_desc,
uid_t uid, gid_t gid)
{
if (HAVE_FCHOWN && dest_desc != -1)
{
if (fchown (dest_desc, uid, gid) == 0)
return true;
return 1;
}
else
{
if (chown (dst_name, uid, gid) == 0)
return true;
return 1;
}
if (! chown_failure_ok (x))
@ -198,10 +198,10 @@ set_owner (const struct cp_options *x, char const *dst_name, int dest_desc,
error (0, errno, _("failed to preserve ownership for %s"),
quote (dst_name));
if (x->require_preserve)
return false;
return -1;
}
return true;
return 0;
}
/* Set the st_author field of DEST_DESC to the st_author field of
@ -265,6 +265,7 @@ copy_reg (char const *src_name, char const *dst_name,
char *buf_alloc = NULL;
int dest_desc;
int source_desc;
mode_t src_mode = src_sb->st_mode;
struct stat sb;
struct stat src_open_sb;
bool return_val = true;
@ -519,10 +520,16 @@ copy_reg (char const *src_name, char const *dst_name,
if (x->preserve_ownership && ! SAME_OWNER_AND_GROUP (*src_sb, sb))
{
if (! set_owner (x, dst_name, dest_desc, src_sb->st_uid, src_sb->st_gid))
{
switch (set_owner (x, dst_name, dest_desc,
src_sb->st_uid, src_sb->st_gid))
{
case -1:
return_val = false;
goto close_src_and_dst_desc;
case 0:
src_mode &= ~ (S_ISUID | S_ISGID | S_ISVTX);
break;
}
}
@ -530,8 +537,8 @@ copy_reg (char const *src_name, char const *dst_name,
if (x->preserve_mode || x->move_mode)
{
if (copy_acl (src_name, source_desc, dst_name, dest_desc,
src_sb->st_mode) != 0 && x->require_preserve)
if (copy_acl (src_name, source_desc, dst_name, dest_desc, src_mode) != 0
&& x->require_preserve)
return_val = false;
}
else if (x->set_mode)
@ -1801,8 +1808,15 @@ copy_internal (char const *src_name, char const *dst_name,
if (x->preserve_ownership
&& (new_dst || !SAME_OWNER_AND_GROUP (src_sb, dst_sb)))
{
if (! set_owner (x, dst_name, -1, src_sb.st_uid, src_sb.st_gid))
return false;
switch (set_owner (x, dst_name, -1, src_sb.st_uid, src_sb.st_gid))
{
case -1:
return false;
case 0:
src_mode &= ~ (S_ISUID | S_ISGID | S_ISVTX);
break;
}
}
set_author (dst_name, -1, &src_sb);

View File

@ -38,9 +38,12 @@ framework_failure=0
mkdir -p $tmp || framework_failure=1
cd $tmp || framework_failure=1
touch a b || framework_failure=1
touch a b c || framework_failure=1
chmod u+sx,go= a || framework_failure=1
chmod u=rwx,g=sx,o= b || framework_failure=1
chmod a=r,ug+sx c || framework_failure=1
chown $NON_ROOT_USERNAME . || framework_failure=1
chmod u=rwx,g=rx,o=rx . || framework_failure=1
if test $framework_failure = 1; then
echo 'failure in testing framework'
@ -59,4 +62,9 @@ set _ `ls -l b`; shift; p1=$1
set _ `ls -l b2`; shift; p2=$1
test $p1 = $p2 || fail=1
setuidgid $NON_ROOT_USERNAME env PATH="$PATH" cp -p c c2 || fail=1
set _ `ls -l c`; shift; p1=$1
set _ `ls -l c2`; shift; p2=$1
test $p1 = $p2 && fail=1
(exit $fail); exit $fail