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

[PHILIPP, Axel, Dr.]

Jim Meyering [mailto:address@hidden wrote:
> "PHILIPP, Axel, Dr." <address@hidden> wrote:
> > 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.
> 
> Thanks for the patch.
> However my first reaction (not even reading it) is that it looks
> far too big to be acceptable, considering it would change an already

If your main concern is the size of the patch I have 2 comments.
1. The first attempt followed a strict "never use gotos" philisophy,
which led to code doubling.I have changed that, the new patch is much
smaller.
2. A nearly identical change has to be applied in 3 functions:
re_protect,
copy_reg and copy_internal. This puts a lower bound on the size of the
patch.
The modified patch is appended.

> tricky and sensitive part of cp -- and all that, solely for 
> the benefit
> of the very few systems on which regular users can chown files.
> That seems to put the risk/benefit ratio off the scale.

And I want to draw your attention to the other two changes proposed,
which are not limited to the unrestricted chown case.
1. The permissions of already existing destinations are trimmed before
the overwriting begins, not just before a chown.
2. POSIX cp will never transfer suid/sgid bits to another user.
coreutils cp currently does it with --preserve=mode. I believe that is
dangerous, especially in a recursive copy, and therefore i propose
to change that behaviour - at least unless the transfer of suid/sgid
bits is explicitely requested.

> 
> Here's a possibility:
> You can maintain this as a separate patch (or better, as a 
> git branch --
I do not believe that a public code fork is a good solution.

> [...]
> Jim
> 

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

--- coreutils-6.10/src/copy.h.ori       2008-01-05 23:58:25.000000000
+0100
+++ coreutils-6.10/src/copy.h   2008-03-11 15:08:22.491546400 +0100
@@ -142,2 +142,5 @@
   bool preserve_timestamps;
+  /* additional entries needed for "unrestricted chown" fix */
+  uid_t cp_uid;
+  bool force_suid_transfer; /* used for sgid as well */
 
--- coreutils-6.10/src/cp.c.ori 2008-01-11 12:19:53.000000000 +0100
+++ coreutils-6.10/src/cp.c     2008-03-11 15:24:15.434264400 +0100
@@ -289,2 +289,5 @@
   char *src_name;              /* The source name in `dst_name'. */
+  bool mode_changed_first;    /* if mode has to be changed before
ownership
+       in case of unrestricted chown */
+  mode_t src_mode;
 
@@ -316,4 +319,14 @@
 
+       /* "unrestricted chown" fix. chmod before chown if necessary */ 
+      mode_changed_first=false;
       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 */
+             mode_changed_first=true;
+             goto change_mode_first;
+           }
+change_owner:
           if (lchown (dst_name, p->st.st_uid, p->st.st_gid) != 0)
@@ -332,5 +345,10 @@
 
+     if ( !mode_changed_first) {
+      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 );
+change_mode_first:
       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;
@@ -339,3 +357,3 @@
        {
-         if (lchmod (dst_name, p->st.st_mode) != 0)
+         if (lchmod (dst_name, src_mode) != 0)
            {
@@ -346,3 +364,4 @@
        }
-
+        if (mode_changed_first) goto change_owner;
+      }
       dst_name[p->slash_offset] = '/';
@@ -465,3 +484,8 @@
                 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)
@@ -783,2 +807,6 @@
   x->mode = 0;
+  /* "unrestricted chown" fix */
+  x->cp_uid=geteuid();
+  x->force_suid_transfer=false;
+  /* may introduce long option --force-suid-transfer to set this var to
true */
 
--- coreutils-6.10/src/copy.c.ori       2008-01-05 23:59:11.000000000
+0100
+++ coreutils-6.10/src/copy.c   2008-03-11 15:07:10.146997800 +0100
@@ -197,4 +197,8 @@
      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))
     {
@@ -214,3 +218,3 @@
        }
-    }
+    } */
 
@@ -317,2 +321,3 @@
          struct stat const *src_sb)
+       /* dst_mode is src_mode without special bits */
 {
@@ -328,2 +333,4 @@
   bool return_val = true;
+  bool mode_changed_first=false;    /* if mode has to be changed before
ownership
+       in case of unrestricted chown */
 
@@ -471,2 +478,25 @@
 
+  /* 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;
+           }
+       }
+    }
+
   {
@@ -661,4 +691,12 @@
 
+       /* "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);
+         mode_changed_first=true;
+         goto change_mode_first;
+       }
+  change_owner:
       switch (set_owner (x, dst_name, dest_desc, src_sb, *new_dst,
&sb))
@@ -677,2 +715,6 @@
 
+ if ( !mode_changed_first) {
+   if ( ! x->preserve_ownership && x->cp_uid != src_sb->st_uid &&
!x->force_suid_transfer )
+     src_mode &= ~(S_ISUID | S_ISGID);
+change_mode_first:
   if (x->preserve_mode || x->move_mode)
@@ -700,2 +742,4 @@
     }
+    if (mode_changed_first) goto change_owner;
+   }
 
@@ -1068,2 +1112,4 @@
   bool have_dst_lstat = false;
+  bool mode_changed_first;    /* if mode has to be changed before
ownership
+       in case of unrestricted chown */
 
@@ -1691,2 +1737,6 @@
             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)
@@ -1959,2 +2009,4 @@
 
+       /* "unrestricted chown" fix. chmod before chown if necessary */ 
+  mode_changed_first=false;
   /* Avoid calling chown if we know it's not necessary.  */
@@ -1963,2 +2015,9 @@
     {
+      if ( x->cp_uid != 0 && src_sb.st_uid != x->cp_uid )
+        {
+         src_mode &= ~ (S_ISUID | S_ISGID);
+         mode_changed_first=true;
+         goto change_mode_first;
+       }
+  change_owner:
       switch (set_owner (x, dst_name, -1, &src_sb, new_dst, &dst_sb))
@@ -1976,2 +2035,6 @@
 
+ if ( !mode_changed_first) {
+  if ( ! x->preserve_ownership && x->cp_uid != src_sb.st_uid &&
!x->force_suid_transfer )
+     src_mode &= ~(S_ISUID | S_ISGID);
+change_mode_first:
   if (x->preserve_mode || x->move_mode)
@@ -2023,2 +2086,4 @@
     }
+    if (mode_changed_first) goto change_owner;
+   }
 
-- 
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