* NEWS: Document fix for cp --preserve=mode.

* src/copy.c (copy_internal): Omit the group- or other-writeable
permissions when creating a directory, to avoid a race condition
if the special mode bits aren't right just after the directory is
created.
* src/cp.c (make_dir_parents_private): Likewise.
* tests/cp/parent-perm-race: Test for the "cp --preserve=mode"
race fix in copy.c.

5 files changed, 81 insertions, 46 deletions

diff --git a/ChangeLog b/ChangeLog
index c2ad0b5e2..dbd305992 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,14 @@
 2007-02-02  Paul Eggert  <eggert@cs.ucla.edu>
 
+	* NEWS: Document fix for cp --preserve=mode.
+	* src/copy.c (copy_internal): Omit the group- or other-writeable
+	permissions when creating a directory, to avoid a race condition
+	if the special mode bits aren't right just after the directory is
+	created.
+	* src/cp.c (make_dir_parents_private): Likewise.
+	* tests/cp/parent-perm-race: Test for the "cp --preserve=mode"
+	race fix in copy.c.
+
 	* NEWS: Document fix for cp --parents.
 	* src/cp.c (make_dir_parents_private): Report the error sooner with
 	"cp --parents DIR/FILE DEST" when DIR is a non-directory, thus not
diff --git a/NEWS b/NEWS
index c90a1b3c5..bd307c14e 100644
--- a/NEWS
+++ b/NEWS
@@ -13,6 +13,13 @@ GNU coreutils NEWS                                    -*- outline -*-
   "cp --parents F/G D" no longer creates a directory D/F when F is not
   a directory (and F/G is therefore invalid).
 
+  "cp --preserve=mode" would create directories that briefly had
+  too-generous permissions in some cases.  For example, when copying a
+  directory with permissions 777 the destination directory might
+  temporarily be setgid on some file systems, which would allow other
+  users to create subfiles with the same group as the directory.  Fix
+  similar problems with 'install' and 'mv'.
+
   cut no longer dumps core for usage like "cut -f2- f1 f2" with two or
   more file arguments.  This was due to a double-free bug, introduced
   in coreutils-5.3.0.
diff --git a/src/copy.c b/src/copy.c
index d9ad254a9..5ec5a921c 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1,5 +1,5 @@
 /* copy.c -- core functions for copying files and directories
-   Copyright (C) 89, 90, 91, 1995-2006 Free Software Foundation.
+   Copyright (C) 89, 90, 91, 1995-2007 Free Software Foundation.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -1005,6 +1005,7 @@ copy_internal (char const *src_name, char const *dst_name,
   struct stat dst_sb;
   mode_t src_mode;
   mode_t dst_mode IF_LINT (= 0);
+  mode_t dst_mode_bits;
   mode_t omitted_permissions;
   bool restore_dst_mode = false;
   char *earlier_file = NULL;
@@ -1504,13 +1505,16 @@ copy_internal (char const *src_name, char const *dst_name,
       new_dst = true;
     }
 
-  /* If the ownership might change, omit some permissions at first, so
-     unauthorized users cannot nip in before the file has the right
-     ownership.  */
+  /* If the ownership might change, or if it is a directory (whose
+     special mode bits may change after the directory is created),
+     omit some permissions at first, so unauthorized users cannot nip
+     in before the file is ready.  */
+  dst_mode_bits = (x->set_mode ? x->mode : src_mode) & CHMOD_MODE_BITS;
   omitted_permissions =
-    (x->preserve_ownership
-     ? (x->set_mode ? x->mode : src_mode) & (S_IRWXG | S_IRWXO)
-     : 0);
+    (dst_mode_bits
+     & (x->preserve_ownership ? S_IRWXG | S_IRWXO
+	: S_ISDIR (src_mode) ? S_IWGRP | S_IWOTH
+	: 0));
 
   delayed_ok = true;
 
@@ -1549,10 +1553,7 @@ copy_internal (char const *src_name, char const *dst_name,
 	     (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.  */
-	  mode_t mkdir_mode =
-	    ((x->set_mode ? x->mode : src_mode)
-	     & CHMOD_MODE_BITS & ~omitted_permissions);
-	  if (mkdir (dst_name, mkdir_mode) != 0)
+	  if (mkdir (dst_name, dst_mode_bits & ~omitted_permissions) != 0)
 	    {
 	      error (0, errno, _("cannot create directory %s"),
 		     quote (dst_name));
diff --git a/src/cp.c b/src/cp.c
index aea63dce7..5759e0d07 100644
--- a/src/cp.c
+++ b/src/cp.c
@@ -435,8 +435,16 @@ make_dir_parents_private (char const *const_dir, size_t src_offset,
 		  return false;
 		}
 	      src_mode = stats.st_mode;
-	      omitted_permissions =
-		x->preserve_ownership ? src_mode & (S_IRWXG | S_IRWXO) : 0;
+
+	      /* If the ownership or special mode bits might change,
+		 omit some permissions at first, so unauthorized users
+		 cannot nip in before the file is ready.  */
+	      omitted_permissions = (src_mode
+				     & (x->preserve_ownership
+					? S_IRWXG | S_IRWXO
+					: x->preserve_mode
+					? S_IWGRP | S_IWOTH
+					: 0));
 
 	      /* POSIX says mkdir's behavior is implementation-defined when
 		 (src_mode & ~S_IRWXUGO) != 0.  However, common practice is
diff --git a/tests/cp/parent-perm-race b/tests/cp/parent-perm-race
index 80c95a69b..d2870bcea 100755
--- a/tests/cp/parent-perm-race
+++ b/tests/cp/parent-perm-race
@@ -1,7 +1,7 @@
 #!/bin/sh
 # Make sure cp -pR --parents isn't too generous with parent permissions.
 
-# Copyright (C) 2006 Free Software Foundation, Inc.
+# Copyright (C) 2006, 2007 Free Software Foundation, Inc.
 
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -32,37 +32,47 @@ framework_failure=0
 mkdir -p $tmp || framework_failure=1
 cd $tmp || framework_failure=1
 
-umask 022
-mkdir -p a d || framework_failure=1
-mkfifo a/fifo || {
-  echo "$0: fifos not supported; skipping this test." 1>&2
-  (exit 77); exit 77
-}
-
-# Copy a fifo's contents.  That way, we can examine d/a's
-# state while cp is running.
-cp -p -R --copy-contents --parents a d &
-cp_pid=$!
-
-(
-  # Now 'cp' is reading the fifo.
-  # Check the permissions of the temporary destination
-  # directory that 'cp' has made.
-  ls -ld d/a >d/ls.out
-
-  # Close the fifo so that "cp" can continue.  But output first,
-  # before exiting, otherwise some shells would optimize away the file
-  # descriptor that holds the fifo open.
-  echo foo
-) >a/fifo
-
-case `cat d/ls.out` in
-d???--[-S]--[-S]*)
-  fail=0;;
-*)
-  fail=1;;
-esac
-
-wait $cp_pid || fail=1
+umask 002
+mkdir mode ownership d || framework_failure=1
+chmod g+s d 2>/dev/null # The cp test is valid either way.
+
+fail=0
+
+for attr in mode ownership
+do
+  mkfifo $attr/fifo || {
+    echo "$0: fifos not supported; skipping this test." 1>&2
+    (exit 77); exit 77
+  }
+
+  # Copy a fifo's contents.  That way, we can examine d/$attr's
+  # state while cp is running.
+  cp --preserve=$attr -R --copy-contents --parents $attr d &
+  cp_pid=$!
+
+  (
+    # Now 'cp' is reading the fifo.
+    # Check the permissions of the temporary destination
+    # directory that 'cp' has made.
+    ls -ld d/$attr >d/$attr.ls
+
+    # Close the fifo so that "cp" can continue.  But output first,
+    # before exiting, otherwise some shells would optimize away the file
+    # descriptor that holds the fifo open.
+    echo foo
+  ) >$attr/fifo
+
+  ls_output=`cat d/$attr.ls` || fail=1
+  case $attr,$ls_output in
+  ownership,d???--[-S]--[-S]* | \
+  mode,d????-??-?* | \
+  mode,d??[-x]?w[-x]?-[-x]* )
+    ;;
+  *)
+    fail=1;;
+  esac
+
+  wait $cp_pid || fail=1
+done
 
 (exit $fail); exit $fail