Fix a security race with "cp -p A B" when B already exists.

* src/copy.h (struct cp_options): New member owner_privileges.
* src/copy.c (USE_ACL): Define to 0 if not defined, for convenience.
(owner_failure_ok): New function.
(set_owner): Avoid a security-related race by doing an extra chmod
first if it looks like there might be trouble right after a chown.
Accept a source struct stat rather than a uid and gid, and
accept a boolean NEW_DST and destination struct stat.
All callers changed.
* src/copy.h (cp_options_default): New function, replacing the
old chown_privileges.
* src/copy.c (cp_options_default): Likewise.
* src/cp.c (cp_option_init): Use it.
* src/install.c (cp_option_init): Likewise.
* src/mv.c (cp_option_init): Likewise.

diff --git a/ChangeLog b/ChangeLog
index 6b21a5a..f662660 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,21 @@
+2007-11-28  Paul Eggert  <eggert@cs.ucla.edu>
+
+       Fix a security race with "cp -p A B" when B already exists.
+       * src/copy.h (struct cp_options): New member owner_privileges.
+       * src/copy.c (USE_ACL): Define to 0 if not defined, for convenience.
+       (owner_failure_ok): New function.
+       (set_owner): Avoid a security-related race by doing an extra chmod
+       first if it looks like there might be trouble right after a chown.
+       Accept a source struct stat rather than a uid and gid, and
+       accept a boolean NEW_DST and destination struct stat.
+       All callers changed.
+       * src/copy.h (cp_options_default): New function, replacing the
+       old chown_privileges.
+       * src/copy.c (cp_options_default): Likewise.
+       * src/cp.c (cp_option_init): Use it.
+       * src/install.c (cp_option_init): Likewise.
+       * src/mv.c (cp_option_init): Likewise.
+
 2007-11-30  Jim Meyering  <meyering@redhat.com>
 
        Avoid a spurious test failure when build directory is set-GID.

diff --git a/src/copy.c b/src/copy.c
index 9758907..1e803ec 100644 (file)
--- a/src/copy.c
+++ b/src/copy.c
@@ -66,6 +66,10 @@
 # define lchown(name, uid, gid) chown (name, uid, gid)
 #endif
 
+#ifndef USE_ACL
+# define USE_ACL 0
+#endif
+
 #define SAME_OWNER(A, B) ((A).st_uid == (B).st_uid)
 #define SAME_GROUP(A, B) ((A).st_gid == (B).st_gid)
 #define SAME_OWNER_AND_GROUP(A, B) (SAME_OWNER (A, B) && SAME_GROUP (A, B))
@@ -87,6 +91,7 @@ static bool copy_internal (char const *src_name, char const *dst_name,
                           bool command_line_arg,
                           bool *copy_into_self,
                           bool *rename_succeeded);
+static bool owner_failure_ok (struct cp_options const *x);
 
 /* Pointers to the file names:  they're used in the diagnostic that is issued
    when we detect the user is trying to copy a directory into itself.  */
@@ -173,13 +178,43 @@ copy_dir (char const *src_name_in, char const *dst_name_in, bool new_dst,
    symbolic links should be involved.  DEST_DESC must
    refer to the same file as DEST_NAME if defined.
    Upon failure to set both UID and GID, try to set only the GID.
+   NEW_DST is true if the file was newly created; otherwise,
+   DST_SB is the status of the destination.
    Return 1 if the initial syscall succeeds, 0 if it fails but it's OK
    not to preserve ownership, -1 otherwise.  */
 
 static int
 set_owner (const struct cp_options *x, char const *dst_name, int dest_desc,
-          uid_t uid, gid_t gid)
+          struct stat const *src_sb, bool new_dst,
+          struct stat const *dst_sb)
 {
+  uid_t uid = src_sb->st_uid;
+  gid_t gid = src_sb->st_gid;
+
+  /* Naively changing the ownership of an already-existing file before
+     changing its permissions would create a window of vulnerability if
+     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.  */
+
+  if (!new_dst & (x->preserve_mode | x->move_mode | x->set_mode))
+    {
+      mode_t old_mode = dst_sb->st_mode;
+      mode_t new_mode =
+       (x->preserve_mode | x->move_mode ? src_sb->st_mode : x->mode);
+      mode_t restrictive_temp_mode = old_mode & new_mode & S_IRWXU;
+
+      if ((USE_ACL
+          || (old_mode & CHMOD_MODE_BITS
+              & (~new_mode | S_ISUID | S_ISGID | S_ISVTX)))
+         && qset_acl (dst_name, dest_desc, restrictive_temp_mode) != 0)
+       {
+         if (! owner_failure_ok (x))
+           error (0, errno, _("clearing permissions for %s"), quote (dst_name));
+         return -x->require_preserve;
+       }
+    }
+
   if (HAVE_FCHOWN && dest_desc != -1)
     {
       if (fchown (dest_desc, uid, gid) == 0)
@@ -627,8 +662,7 @@ copy_reg (char const *src_name, char const *dst_name,
 
   if (x->preserve_ownership && ! SAME_OWNER_AND_GROUP (*src_sb, sb))
     {
-      switch (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, *new_dst, &sb))
        {
        case -1:
          return_val = false;
@@ -1928,7 +1962,7 @@ copy_internal (char const *src_name, char const *dst_name,
   if (x->preserve_ownership
       && (new_dst || !SAME_OWNER_AND_GROUP (src_sb, dst_sb)))
     {
-      switch (set_owner (x, dst_name, -1, src_sb.st_uid, src_sb.st_gid))
+      switch (set_owner (x, dst_name, -1, &src_sb, new_dst, &dst_sb))
        {
        case -1:
          return false;
@@ -2059,23 +2093,26 @@ copy (char const *src_name, char const *dst_name,
                        options, true, copy_into_self, rename_succeeded);
 }
 
-/* Return true if this process has appropriate privileges to chown a
-   file whose owner is not the effective user ID.  */
+/* Set *X to the default options for a value of type struct cp_options.  */
 
-extern bool
-chown_privileges (void)
+void
+cp_options_default (struct cp_options *x)
 {
+  memset (x, 0, sizeof *x);
 #ifdef PRIV_FILE_CHOWN
-  bool result;
-  priv_set_t *pset = priv_allocset ();
-  if (!pset)
-    xalloc_die ();
-  result = (getppriv (PRIV_EFFECTIVE, pset) == 0
-           && priv_ismember (pset, PRIV_FILE_CHOWN));
-  priv_freeset (pset);
-  return result;
+  {
+    priv_set_t *pset = priv_allocset ();
+    if (!pset)
+      xalloc_die ();
+    if (getppriv (PRIV_EFFECTIVE, pset) == 0)
+      {
+       x->chown_privileges = priv_ismember (pset, PRIV_FILE_CHOWN);
+       x->owner_privileges = priv_ismember (pset, PRIV_FILE_OWNER);
+      }
+    priv_freeset (pset);
+  }
 #else
-  return (geteuid () == 0);
+  x->chown_privileges = x->owner_privileges = (geteuid () == 0);
 #endif
 }
 
@@ -2093,6 +2130,16 @@ chown_failure_ok (struct cp_options const *x)
   return ((errno == EPERM || errno == EINVAL) && !x->chown_privileges);
 }
 
+/* Similarly, return true if it's OK for chmod and similar operations
+   to fail, where errno is the error number that chmod failed with and
+   X is the copying option set.  */
+
+static bool
+owner_failure_ok (struct cp_options const *x)
+{
+  return ((errno == EPERM || errno == EINVAL) && !x->owner_privileges);
+}
+
 /* Return the user's umask, caching the result.  */
 
 extern mode_t

diff --git a/src/copy.h b/src/copy.h
index 47c97f3..14104e6 100644 (file)
--- a/src/copy.h
+++ b/src/copy.h
@@ -124,6 +124,13 @@ struct cp_options
      whose owner is not the effective user ID.  */
   bool chown_privileges;
 
+  /* Whether this process has appropriate privileges to do the
+     following operations on a file even when it is owned by some
+     other user: set the file's atime, mtime, mode, or ACL; remove or
+     rename an entry in the file even though it is a sticky directory,
+     or to mount on the file.  */
+  bool owner_privileges;
+
   /* If true, when copying recursively, skip any subdirectories that are
      on different file systems from the one we started on.  */
   bool one_file_system;
@@ -230,7 +237,7 @@ bool copy (char const *src_name, char const *dst_name,
 void dest_info_init (struct cp_options *);
 void src_info_init (struct cp_options *);
 
-bool chown_privileges (void);
+void cp_options_default (struct cp_options *);
 bool chown_failure_ok (struct cp_options const *);
 mode_t cached_umask (void);
 

diff --git a/src/cp.c b/src/cp.c
index 599498d..99e1f73 100644 (file)
--- a/src/cp.c
+++ b/src/cp.c
@@ -737,13 +737,13 @@ do_copy (int n_files, char **file, const char *target_directory,
 static void
 cp_option_init (struct cp_options *x)
 {
+  cp_options_default (x);
   x->copy_as_regular = true;
   x->dereference = DEREF_UNDEFINED;
   x->unlink_dest_before_opening = false;
   x->unlink_dest_after_failed_open = false;
   x->hard_link = false;
   x->interactive = I_UNSPECIFIED;
-  x->chown_privileges = chown_privileges ();
   x->move_mode = false;
   x->one_file_system = false;
 

diff --git a/src/install.c b/src/install.c
index 802dfcf..db08751 100644 (file)
--- a/src/install.c
+++ b/src/install.c
@@ -163,6 +163,7 @@ static struct option const long_options[] =
 static void
 cp_option_init (struct cp_options *x)
 {
+  cp_options_default (x);
   x->copy_as_regular = true;
   x->dereference = DEREF_ALWAYS;
   x->unlink_dest_before_opening = true;
@@ -170,7 +171,6 @@ cp_option_init (struct cp_options *x)
   x->hard_link = false;
   x->interactive = I_UNSPECIFIED;
   x->move_mode = false;
-  x->chown_privileges = chown_privileges ();
   x->one_file_system = false;
   x->preserve_ownership = false;
   x->preserve_links = false;

diff --git a/src/mv.c b/src/mv.c
index 3b1ba8e..44f5bfc 100644 (file)
--- a/src/mv.c
+++ b/src/mv.c
@@ -123,6 +123,7 @@ cp_option_init (struct cp_options *x)
 {
   bool selinux_enabled = (0 < is_selinux_enabled ());
 
+  cp_options_default (x);
   x->copy_as_regular = false;  /* FIXME: maybe make this an option */
   x->dereference = DEREF_NEVER;
   x->unlink_dest_before_opening = false;
@@ -130,7 +131,6 @@ cp_option_init (struct cp_options *x)
   x->hard_link = false;
   x->interactive = I_UNSPECIFIED;
   x->move_mode = true;
-  x->chown_privileges = chown_privileges ();
   x->one_file_system = false;
   x->preserve_ownership = true;
   x->preserve_links = true;