diff options
| -rw-r--r-- | ChangeLog | 9 | ||||
| -rw-r--r-- | NEWS | 8 | ||||
| -rw-r--r-- | src/copy.c | 40 | ||||
| -rwxr-xr-x | tests/cp/special-bits | 10 |
4 files changed, 53 insertions, 14 deletions
@@ -1,5 +1,14 @@ 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. * m4/jm-macros.m4 (coreutils_MACROS): Check for fchmod. * src/copy.c (fchmod_or_lchmod): New function. @@ -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 d49f9b4e6..24c725132 100644 --- 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 6a9b0949d..96dbf3d92 100755 --- 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 |