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>

diff --git a/ChangeLog b/ChangeLog
index e2ee3ca099fef041e18ca3a7ae03165090273d2e..25b01d1b0bc43b218ec1078f0b9cddd9dd26fa60 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -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.

diff --git a/NEWS b/NEWS
index fad4e1c631f0347d27ba32cdd3b00a33f31c3187..1ce50f85a84bcd6de8584b806d4604653afb04e0 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -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

diff --git a/src/copy.c b/src/copy.c
index d49f9b4e60e48562901804083933c0f80208388a..24c7251320e9d907a836125cc2a490f2b7e860e5 100644 (file)
--- a/src/copy.c
+++ b/src/copy.c
@@ -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);

diff --git a/tests/cp/special-bits b/tests/cp/special-bits
index 6a9b0949d9566025b99609848b22e3ba0aef2d25..96dbf3d92e9ab79e6190e803b458b5394f447805 100755 (executable)
--- a/tests/cp/special-bits
+++ b/tests/cp/special-bits
@@ -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