* NEWS: Document the cp --preserve=ownership fix.

* m4/jm-macros.m4 (coreutils_MACROS): Check for fchmod.
* src/copy.c (fchmod_or_lchmod): New function.
(copy_reg): New arg OMITTED_PERMISSIONS.  All uses changed.
Omit confusing and unused ", dst_mode" arg to 'open' without O_CREAT.
When creating a file, use O_EXCL, so we're more likely to detect
funny business by other processes.  At the end, if permissions
were omitted, chmod them back in.
(copy_internal): If the ownership might change, omit some permissions
at first, then restore them after chowning the file.
* src/cp.c (make_dir_parents_private): Likewise.
* src/copy.c (cached_umask): New function.
* src/copy.h (cached_umask): New decl.

6 files changed, 151 insertions, 33 deletions

diff --git a/ChangeLog b/ChangeLog
index ec4e3ecde..e2ee3ca09 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,19 @@
+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.
+	(copy_reg): New arg OMITTED_PERMISSIONS.  All uses changed.
+	Omit confusing and unused ", dst_mode" arg to 'open' without O_CREAT.
+	When creating a file, use O_EXCL, so we're more likely to detect
+	funny business by other processes.  At the end, if permissions
+	were omitted, chmod them back in.
+	(copy_internal): If the ownership might change, omit some permissions
+	at first, then restore them after chowning the file.
+	* src/cp.c (make_dir_parents_private): Likewise.
+	* src/copy.c (cached_umask): New function.
+	* src/copy.h (cached_umask): New decl.
+
 2006-12-06  Jim Meyering  <jim@meyering.net>
 
 	Make the output of "make check" more reproducible.
diff --git a/NEWS b/NEWS
index 50eb623b4..fad4e1c63 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,15 @@ GNU coreutils NEWS                                    -*- outline -*-
 
 ** Bug fixes
 
+  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
+  directory, the output file was briefly readable by group B.
+  Fix similar problems with cp options like -p that imply
+  --preserve=ownership, with install -d when combined with either -o
+  or -g, and with mv when copying across file system boundaries.
+  This bug affects coreutils 6.0 through 6.6.
+
   du --one-file-system (-x) would skip subdirectories of any directory
   listed as second or subsequent command line argument.  This bug affects
   coreutils-6.4, 6.5 and 6.6.
diff --git a/m4/jm-macros.m4 b/m4/jm-macros.m4
index d65f1086f..5bf46a9e3 100644
--- a/m4/jm-macros.m4
+++ b/m4/jm-macros.m4
@@ -62,6 +62,7 @@ AC_DEFUN([coreutils_MACROS],
     endgrent \
     endpwent \
     fchown \
+    fchmod \
     ftruncate \
     iswspace \
     mkfifo \
diff --git a/src/copy.c b/src/copy.c
index 5b66b281d..d49f9b4e6 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -230,11 +230,26 @@ set_author (const char *dst_name, int dest_desc, const struct stat *src_sb)
 #endif
 }
 
+/* Change the file mode bits of the file identified by DESC or NAME to MODE.
+   Use DESC if DESC is valid and fchmod is available, NAME otherwise.  */
+
+static int
+fchmod_or_lchmod (int desc, char const *name, mode_t mode)
+{
+#if HAVE_FCHMOD
+  if (0 <= desc)
+    return fchmod (desc, mode);
+#endif
+  return lchmod (name, mode);
+}
+
 /* Copy a regular file from SRC_NAME to DST_NAME.
    If the source file contains holes, copies holes and blocks of zeros
    in the source file as holes in the destination file.
    (Holes are read as zeroes by the `read' system call.)
-   Use DST_MODE as the 3rd argument in the call to open.
+   When creating the destination, use DST_MODE & ~OMITTED_PERMISSIONS
+   as the third argument in the call to open, adding
+   OMITTED_PERMISSIONS after copying as needed.
    X provides many option settings.
    Return true if successful.
    *NEW_DST is as in copy_internal.
@@ -242,7 +257,8 @@ set_author (const char *dst_name, int dest_desc, const struct stat *src_sb)
 
 static bool
 copy_reg (char const *src_name, char const *dst_name,
-	  const struct cp_options *x, mode_t dst_mode, bool *new_dst,
+	  const struct cp_options *x,
+	  mode_t dst_mode, mode_t omitted_permissions, bool *new_dst,
 	  struct stat const *src_sb)
 {
   char *buf;
@@ -282,7 +298,7 @@ copy_reg (char const *src_name, char const *dst_name,
      The if-block will be taken in move_mode.  */
   if (! *new_dst)
     {
-      dest_desc = open (dst_name, O_WRONLY | O_TRUNC | O_BINARY, dst_mode);
+      dest_desc = open (dst_name, O_WRONLY | O_TRUNC | O_BINARY);
 
       if (dest_desc < 0 && x->unlink_dest_after_failed_open)
 	{
@@ -301,7 +317,10 @@ copy_reg (char const *src_name, char const *dst_name,
     }
 
   if (*new_dst)
-    dest_desc = open (dst_name, O_WRONLY | O_CREAT | O_BINARY, dst_mode);
+    dest_desc = open (dst_name, O_WRONLY | O_CREAT | O_EXCL | O_BINARY,
+		      dst_mode & ~omitted_permissions);
+  else
+    omitted_permissions = 0;
 
   if (dest_desc < 0)
     {
@@ -520,6 +539,18 @@ copy_reg (char const *src_name, char const *dst_name,
       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;
+	}
+    }
 
 close_src_and_dst_desc:
   if (close (dest_desc) < 0)
@@ -967,6 +998,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 omitted_permissions;
   bool restore_dst_mode = false;
   char *earlier_file = NULL;
   char *dst_backup = NULL;
@@ -1465,6 +1497,14 @@ 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.  */
+  omitted_permissions =
+    (x->preserve_ownership
+     ? (x->set_mode ? x->mode : src_mode) & (S_IRWXG | S_IRWXO)
+     : 0);
+
   delayed_ok = true;
 
   /* In certain modes (cp's --symbolic-link), and for certain file types
@@ -1502,7 +1542,10 @@ 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.  */
-	  if (mkdir (dst_name, src_mode & CHMOD_MODE_BITS) != 0)
+	  mode_t mkdir_mode =
+	    ((x->set_mode ? x->mode : src_mode)
+	     & CHMOD_MODE_BITS & ~omitted_permissions);
+	  if (mkdir (dst_name, mkdir_mode) != 0)
 	    {
 	      error (0, errno, _("cannot create directory %s"),
 		     quote (dst_name));
@@ -1629,7 +1672,7 @@ copy_internal (char const *src_name, char const *dst_name,
 	 practice passed all the source mode bits to 'open', but the extra
 	 bits were ignored, so it should be the same either way.  */
       if (! copy_reg (src_name, dst_name, x, src_mode & S_IRWXUGO,
-		      &new_dst, &src_sb))
+		      omitted_permissions, &new_dst, &src_sb))
 	goto un_backup;
     }
   else if (S_ISFIFO (src_mode))
@@ -1637,7 +1680,7 @@ copy_internal (char const *src_name, char const *dst_name,
       /* Use mknod, rather than mkfifo, because the former preserves
 	 the special mode bits of a fifo on Solaris 10, while mkfifo
 	 does not.  */
-      if (mknod (dst_name, src_mode, 0) != 0)
+      if (mknod (dst_name, src_mode & ~omitted_permissions, 0) != 0)
 	{
 	  error (0, errno, _("cannot create fifo %s"), quote (dst_name));
 	  goto un_backup;
@@ -1645,7 +1688,8 @@ copy_internal (char const *src_name, char const *dst_name,
     }
   else if (S_ISBLK (src_mode) || S_ISCHR (src_mode) || S_ISSOCK (src_mode))
     {
-      if (mknod (dst_name, src_mode, src_sb.st_rdev) != 0)
+      if (mknod (dst_name, src_mode & ~omitted_permissions, src_sb.st_rdev)
+	  != 0)
 	{
 	  error (0, errno, _("cannot create special file %s"),
 		 quote (dst_name));
@@ -1774,14 +1818,40 @@ copy_internal (char const *src_name, char const *dst_name,
       if (set_acl (dst_name, -1, x->mode) != 0)
 	return false;
     }
-  else if (restore_dst_mode)
+  else
     {
-      if (lchmod (dst_name, dst_mode) != 0)
+      if (omitted_permissions)
 	{
-	  error (0, errno, _("preserving permissions for %s"),
-		 quote (dst_name));
-	  if (x->require_preserve)
-	    return false;
+	  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;
+	    }
 	}
     }
 
@@ -1885,3 +1955,17 @@ chown_failure_ok (struct cp_options const *x)
 
   return ((errno == EPERM || errno == EINVAL) && !x->chown_privileges);
 }
+
+/* Return the user's umask, caching the result.  */
+
+extern mode_t
+cached_umask (void)
+{
+  static mode_t mask = -1;
+  if (mask == (mode_t) -1)
+    {
+      mask = umask (0);
+      umask (mask);
+    }
+  return mask;
+}
diff --git a/src/copy.h b/src/copy.h
index f08b62520..c815baf64 100644
--- a/src/copy.h
+++ b/src/copy.h
@@ -213,5 +213,6 @@ void src_info_init (struct cp_options *);
 
 bool chown_privileges (void);
 bool chown_failure_ok (struct cp_options const *);
+mode_t cached_umask (void);
 
 #endif
diff --git a/src/cp.c b/src/cp.c
index 9ba334255..8fe11a1e7 100644
--- a/src/cp.c
+++ b/src/cp.c
@@ -413,6 +413,8 @@ make_dir_parents_private (char const *const_dir, size_t src_offset,
 	  if (XSTAT (x, dir, &stats))
 	    {
 	      mode_t src_mode;
+	      mode_t omitted_permissions;
+	      mode_t mkdir_mode;
 
 	      /* This component does not exist.  We must set
 		 *new_dst and new->mode inside this loop because,
@@ -427,12 +429,15 @@ 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;
 
 	      /* POSIX says mkdir's behavior is implementation-defined when
 		 (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.  */
-	      if (mkdir (dir, src_mode & CHMOD_MODE_BITS) != 0)
+	      mkdir_mode = src_mode & CHMOD_MODE_BITS & ~omitted_permissions;
+	      if (mkdir (dir, mkdir_mode) != 0)
 		{
 		  error (0, errno, _("cannot make directory %s"),
 			 quote (dir));
@@ -454,28 +459,30 @@ make_dir_parents_private (char const *const_dir, size_t src_offset,
 			 quote (dir));
 		  return false;
 		}
-	      else
-	        {
-		  if (x->preserve_mode)
-		    {
-		      new->mode = src_mode;
-		      new->restore_mode = (src_mode != stats.st_mode);
-		    }
 
-		  if ((stats.st_mode & S_IRWXU) != S_IRWXU)
-		    {
-		      /* Make the new directory searchable and writable. The
-			 original permissions will be restored later.  */
 
-		      new->mode = stats.st_mode;
+	      if (! x->preserve_mode)
+		{
+		  if (omitted_permissions & ~stats.st_mode)
+		    omitted_permissions &= ~ cached_umask ();
+		  if (omitted_permissions & ~stats.st_mode
+		      || (stats.st_mode & S_IRWXU) != S_IRWXU)
+		    {
+		      new->mode = stats.st_mode | omitted_permissions;
 		      new->restore_mode = true;
+		    }
+		}
 
-		      if (lchmod (dir, stats.st_mode | S_IRWXU) != 0)
-			{
-			  error (0, errno, _("setting permissions for %s"),
-				 quote (dir));
-			  return false;
-			}
+	      if ((stats.st_mode & S_IRWXU) != S_IRWXU)
+		{
+		  /* Make the new directory searchable and writable.
+		     The original permissions will be restored later.  */
+
+		  if (lchmod (dir, stats.st_mode | S_IRWXU) != 0)
+		    {
+		      error (0, errno, _("setting permissions for %s"),
+			     quote (dir));
+		      return false;
 		    }
 		}
 	    }