Jim Meyering [mailto:[EMAIL PROTECTED] 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:[EMAIL PROTECTED] Please note: The email address has changed from [EMAIL PROTECTED] to [EMAIL PROTECTED] -- 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 _______________________________________________ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils