AW: cp -p does not work if normal users are allowed to chown files

[PHILIPP, Axel, Dr.]

Jim Meyering [mailto:address@hidden wrote:
>  ....  This is relatively
>  low priority, since very few people care about systems configured to
>  allow regular users to use chown.  If you don't hear back 
> from someone
>  after a couple weeks, please ping the mailing list. ]
> 

I agree. Otherwise the problem should have been detected earlier. So I
tried to produce a patch by myself. It does pass some simple tests, but
I am not sure whether it is wellbehaved in all possible situations.
Anyhow, I hope it can help as starting point.

Changes are:
(i) change mode before ownership if non-root user specifies preserve
ownership.
(ii) do not transfer special bits if preserve mode is specified without
preserve owner. Since this is a deviation from current behaviour I made
it dependent on (the new variable) force_suid_transfer=false
Rationale: let cp --preserve=mode,time behave for non-root users on
systems with unrestricted chown behave like cp -p on systems with
restricted chown.
(iii) added some more caution with respect to temporary permissions.

--- coreutils-6.10/src/copy.h.ori       2008-01-05 23:58:25.000000000
+0100
+++ coreutils-6.10/src/copy.h   2008-03-10 09:24:09.413606000 +0100
@@ -140,6 +140,10 @@
   bool preserve_ownership;
   bool preserve_mode;
   bool preserve_timestamps;
+  /* additional entries needed for "unrestricted chown" fix */
+  uid_t cp_uid;
+  gid_t cp_gid;
+  bool force_suid_transfer; /* used for sgid as well */
 
   /* Enabled for mv, and for cp by the --preserve=links option.
      If true, attempt to preserve in the destination files any
--- coreutils-6.10/src/cp.c.ori 2008-01-11 12:19:53.000000000 +0100
+++ coreutils-6.10/src/cp.c     2008-03-10 14:29:28.540002000 +0100
@@ -287,7 +287,11 @@
   struct dir_attr *p;
   char *dst_name;              /* A copy of CONST_DST_NAME we can
change. */
   char *src_name;              /* The source name in `dst_name'. */
+  bool mode_already_changed;    /* if mode has to be changed before
ownership
+       in case of unrestricted chown */
+  mode_t src_mode;
 
+  mode_already_changed=false;
   ASSIGN_STRDUPA (dst_name, const_dst_name);
   src_name = dst_name + src_offset;
 
@@ -314,8 +318,29 @@
            }
        }
 
+       /* "unrestricted chown" fix. chmod before chown if necessary */ 
       if (x->preserve_ownership)
        {
+         if ( x->cp_uid != 0 && p->st.st_uid != x->cp_uid )
+           {
+             src_mode = p->st.st_mode & ~(S_ISUID | S_ISGID );
+             /* the following chown will clear these bits anyway */
+              if (x->preserve_mode)
+               {
+                 if (copy_acl (src_name, -1, dst_name, -1, src_mode) !=
0)
+                    return false;
+               }
+              else if (p->restore_mode)
+               {
+                 if (lchmod (dst_name, src_mode) != 0)
+                   {
+                     error (0, errno, _("failed to preserve permissions
for %s"),
+                            quote (dst_name));
+                     return false;
+                   }
+               }
+             mode_already_changed=true;
+           }
           if (lchown (dst_name, p->st.st_uid, p->st.st_gid) != 0)
             {
               if (! chown_failure_ok (x))
@@ -330,21 +355,25 @@
             }
        }
 
+     if ( !mode_already_changed) {
+      src_mode=p->st.st_mode;
+      if ( ! x->preserve_ownership && x->cp_uid != p->st.st_uid &&
!x->force_suid_transfer )
+        src_mode &= ~(S_ISUID | S_ISGID );
       if (x->preserve_mode)
        {
-         if (copy_acl (src_name, -1, dst_name, -1, p->st.st_mode) != 0)
+         if (copy_acl (src_name, -1, dst_name, -1, src_mode) != 0)
            return false;
        }
       else if (p->restore_mode)
        {
-         if (lchmod (dst_name, p->st.st_mode) != 0)
+         if (lchmod (dst_name, src_mode) != 0)
            {
              error (0, errno, _("failed to preserve permissions for
%s"),
                     quote (dst_name));
              return false;
            }
        }
-
+      }
       dst_name[p->slash_offset] = '/';
     }
   return true;
@@ -463,7 +492,12 @@
                 (src_mode & ~S_IRWXUGO) != 0.  However, common practice
is
                 to ask mkdir to copy all the CHMOD_MODE_BITS, letting
mkdir
                 decide what to do with S_ISUID | S_ISGID | S_ISVTX.  */
+             /* i prefer to be on the save side, so clear S_ISGID */
              mkdir_mode = src_mode & CHMOD_MODE_BITS &
~omitted_permissions;
+             if ( !x->preserve_ownership && !x->force_suid_transfer ||
+               !x->preserve_mode )
+               mkdir_mode &= ~S_ISGID;
+
              if (mkdir (dir, mkdir_mode) != 0)
                {
                  error (0, errno, _("cannot make directory %s"),
@@ -781,6 +815,11 @@
   x->symbolic_link = false;
   x->set_mode = false;
   x->mode = 0;
+  /* "unrestricted chown" fix */
+  x->cp_uid=geteuid();
+  x->cp_gid=getegid();
+  x->force_suid_transfer=false;
+  /* may introduce long option --force-suid-transfer to set this var to
true */
 
   /* Not used.  */
   x->stdin_tty = false;
--- coreutils-6.10/src/copy.c.ori       2008-01-05 23:59:11.000000000
+0100
+++ coreutils-6.10/src/copy.c   2008-03-10 14:21:56.767086000 +0100
@@ -195,8 +195,12 @@
      the file's old permissions are too generous for the new owner and
      group.  Avoid the window by first changing to a restrictive
      temporary mode if necessary.  */
+  /* this applies as well if an already existing file is overwritten
+     with different contents. therefore the mode restriction is moved
to
+     before copying. it is also not compatible with the "unrestricted
chown"
+     fix */
 
-  if (!new_dst & (x->preserve_mode | x->move_mode | x->set_mode))
+ /* if (!new_dst & (x->preserve_mode | x->move_mode | x->set_mode))
     {
       mode_t old_mode = dst_sb->st_mode;
       mode_t new_mode =
@@ -212,7 +216,7 @@
            error (0, errno, _("clearing permissions for %s"), quote
(dst_name));
          return -x->require_preserve;
        }
-    }
+    } */
 
   if (HAVE_FCHOWN && dest_desc != -1)
     {
@@ -315,6 +319,7 @@
          const struct cp_options *x,
          mode_t dst_mode, mode_t omitted_permissions, bool *new_dst,
          struct stat const *src_sb)
+       /* dst_mode is src_mode without special bits */
 {
   char *buf;
   char *buf_alloc = NULL;
@@ -326,6 +331,8 @@
   struct stat sb;
   struct stat src_open_sb;
   bool return_val = true;
+  bool mode_already_changed=false;    /* if mode has to be changed
before ownership
+       in case of unrestricted chown */
 
   source_desc = open (src_name,
                      (O_RDONLY | O_BINARY
@@ -469,6 +476,29 @@
       goto close_src_and_dst_desc;
     }
 
+  /* trim permissions of already existing files. Moved here from
set_owner */
+  if ( !*new_dst)
+    {
+      mode_t temp_perm=sb.st_mode;
+      if ( x->preserve_mode | x->move_mode | x->set_mode )
+        temp_perm=S_IRWXU;
+      else
+        {
+         if ( src_sb->st_uid != sb.st_uid )
+           temp_perm=sb.st_mode & ~(S_ISUID | S_ISGID);
+       }
+      if ( temp_perm != sb.st_mode ) 
+        {
+         if (lchmod (dst_name, temp_perm) != 0)
+           {
+             error (0, errno, _("resetting permissions for %s"),
+                 quote (dst_name));
+             return_val = false;
+             goto close_src_and_dst_desc;
+           }
+       }
+    }
+
   {
     typedef uintptr_t word;
     off_t n_read_total = 0;
@@ -659,8 +689,37 @@
        }
     }
 
+       /* "unrestricted chown" fix. chmod before chown if necessary */ 
   if (x->preserve_ownership && ! SAME_OWNER_AND_GROUP (*src_sb, sb))
     {
+      if ( x->cp_uid != 0 && src_sb->st_uid != x->cp_uid )
+        {
+         src_mode &= ~ (S_ISUID | S_ISGID);
+         if (x->preserve_mode || x->move_mode)
+           {
+             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)
+           {
+             if (set_acl (dst_name, dest_desc, x->mode) != 0)
+               return_val = false;
+           }
+         else if (omitted_permissions)
+           {
+             omitted_permissions &= ~ cached_umask ();
+             if (omitted_permissions
+                 && fchmod_or_lchmod (dest_desc, dst_name, dst_mode) !=
0)
+               {
+                 error (0, errno, _("preserving permissions for %s"),
+                        quote (dst_name));
+                 if (x->require_preserve)
+                   return_val = false;
+               }
+           }
+         mode_already_changed=true;
+       }
       switch (set_owner (x, dst_name, dest_desc, src_sb, *new_dst,
&sb))
        {
        case -1:
@@ -675,8 +734,11 @@
 
   set_author (dst_name, dest_desc, src_sb);
 
+ if ( !mode_already_changed) {
   if (x->preserve_mode || x->move_mode)
     {
+      if ( ! x->preserve_ownership && x->cp_uid != src_sb->st_uid &&
!x->force_suid_transfer )
+        src_mode &= ~(S_ISUID | S_ISGID);
       if (copy_acl (src_name, source_desc, dst_name, dest_desc,
src_mode) != 0
          && x->require_preserve)
        return_val = false;
@@ -691,6 +753,7 @@
       omitted_permissions &= ~ cached_umask ();
       if (omitted_permissions
          && fchmod_or_lchmod (dest_desc, dst_name, dst_mode) != 0)
+         /* dst_mode has the special bits cleared */
        {
          error (0, errno, _("preserving permissions for %s"),
                 quote (dst_name));
@@ -698,6 +761,7 @@
            return_val = false;
        }
     }
+   }
 
 close_src_and_dst_desc:
   if (close (dest_desc) < 0)
@@ -1066,6 +1130,8 @@
   bool copied_as_regular = false;
   bool preserve_metadata;
   bool have_dst_lstat = false;
+  bool mode_already_changed;    /* if mode has to be changed before
ownership
+       in case of unrestricted chown */
 
   if (x->move_mode && rename_succeeded)
     *rename_succeeded = false;
@@ -1689,6 +1755,10 @@
             (src_mode & ~S_IRWXUGO) != 0.  However, common practice is
             to ask mkdir to copy all the CHMOD_MODE_BITS, letting mkdir
             decide what to do with S_ISUID | S_ISGID | S_ISVTX.  */
+             /* i prefer to be on the save side, so clear S_ISGID */
+             if ( !x->preserve_ownership && !x->force_suid_transfer ||
+               !x->preserve_mode )
+               dst_mode_bits &= ~S_ISGID;
          if (mkdir (dst_name, dst_mode_bits & ~omitted_permissions) !=
0)
            {
              error (0, errno, _("cannot create directory %s"),
@@ -1943,6 +2013,7 @@
      chown turns off set[ug]id bits for non-root,
      so do the chmod last.  */
 
+  mode_already_changed=false;
   if (x->preserve_timestamps)
     {
       struct timespec timespec[2];
@@ -1957,10 +2028,65 @@
        }
     }
 
+       /* "unrestricted chown" fix. chmod before chown if necessary */ 
+  mode_already_changed=false;
   /* Avoid calling chown if we know it's not necessary.  */
   if (x->preserve_ownership
       && (new_dst || !SAME_OWNER_AND_GROUP (src_sb, dst_sb)))
     {
+      if ( x->cp_uid != 0 && src_sb.st_uid != x->cp_uid )
+        {
+         src_mode &= ~ (S_ISUID | S_ISGID);
+         if (x->preserve_mode || x->move_mode)
+           {
+             if (copy_acl (src_name, -1, dst_name, -1, src_mode) != 0
+                 && x->require_preserve)
+               return false;
+           }
+         else if (x->set_mode)
+           {
+             if (set_acl (dst_name, -1, x->mode) != 0)
+               return false;
+           }
+         else
+           {
+             if (omitted_permissions)
+               {
+                 omitted_permissions &= ~ cached_umask ();
+
+                 if (omitted_permissions && !restore_dst_mode)
+                   {
+             /* Permissions were deliberately omitted when the file
+                was created due to security concerns.  See whether
+                they need to be re-added now.  It'd be faster to omit
+                the lstat, but deducing the current destination mode
+                is tricky in the presence of implementation-defined
+                rules for special mode bits.  */
+                     if (new_dst && lstat (dst_name, &dst_sb) != 0)
+                       {
+                         error (0, errno, _("cannot stat %s"), quote
(dst_name));
+                         return false;
+                       }
+                     dst_mode = dst_sb.st_mode;
+                     if (omitted_permissions & ~dst_mode)
+                       restore_dst_mode = true;
+                   }
+               }
+
+             if (restore_dst_mode)
+               {
+                 if (lchmod (dst_name, dst_mode | omitted_permissions)
!= 0)
+                   {
+                     error (0, errno, _("preserving permissions for
%s"),
+                            quote (dst_name));
+                     if (x->require_preserve)
+                       return false;
+                   }
+               }
+         }
+
+         mode_already_changed=true;
+       }
       switch (set_owner (x, dst_name, -1, &src_sb, new_dst, &dst_sb))
        {
        case -1:
@@ -1974,8 +2100,11 @@
 
   set_author (dst_name, -1, &src_sb);
 
+ if ( !mode_already_changed) {
   if (x->preserve_mode || x->move_mode)
     {
+      if ( ! x->preserve_ownership && x->cp_uid != src_sb.st_uid &&
!x->force_suid_transfer )
+        src_mode &= ~(S_ISUID | S_ISGID);
       if (copy_acl (src_name, -1, dst_name, -1, src_mode) != 0
          && x->require_preserve)
        return false;
@@ -2021,6 +2150,7 @@
            }
        }
     }
+   }
 
   return delayed_ok;
 



Mit freundlichen Gruessen / Best Regards

Axel PHILIPP   Geb. 044/557
Dr. rer. nat., Dipl. Phys.

MTU Aero Engines GmbH
Informationswirtschaft/Entwicklungssysteme (FIE)
Information Management/Engineering Systems (FIE)
Dachauer Str. 665
80995 Muenchen
Germany

Tel  +49 (0)89 1489-4715
Fax +49 (0)89 1489-97533
mailto:address@hidden

Please note: The email address has changed from address@hidden to address@hidden

-- 
MTU Aero Engines GmbH
Geschaeftsfuehrung/Board of Management: Egon W. Behle, Vorsitzender/CEO; Dr. 
Rainer Martens, Dr. Stefan Weingartner, Reiner Winkler
Vorsitzender des Aufsichtsrats/Chairman of the Supervisory Board: Klaus 
Eberhardt
Sitz der Gesellschaft/Registered Office: Muenchen
Handelsregister/Commercial Register: Muenchen HRB 154230