a few more symlink-related problems in cp/mv/install

[Paul Eggert]

With the recent symlink-related issues in GNU 'cp' in mind, I audited
the symlink-related code in 'cp', 'mv', and 'install' and found three
related problems, with fixes proposed below.  The cp --parents bug is
easily testable and I've added a test case for it.  The others are
minor-vulnerability races and I couldn't think of easy tests.

2007-06-16  Paul Eggert  <address@hidden>

        A few more symlink-related fixes.  Fix a bug triggered by cp
        --parents and symlinks.  Close some race conditions possible when
        the destination replaces a newly-created file with a symlink.
        * NEWS: Document that 'cp --parents' no longer mishandles
        symlinks in file name components of source.
        * src/copy.c (HAVE_LCHOWN): Default to false.
        (lchown) [!defined HAVE_LCHOWN]: Define to chown, for convenience.
        * src/cp.c (lchown) [!HAVE_LCHOWN]: Likewise.
        * src/install.c (lchown [!HAVE_LCHOWN]: Likewise.
        * src/copy.c (set_owner): Use lchown instead of chown, for safety
        in case the file got replaced by a symlink in the meantime.
        * src/cp.c (re_protect): Likewise.
        * src/install.c (change_attributes): Likewise.
        * src/copy.c (copy_internal): Use ordinary C rather than an #if.
        * src/cp.c (lchown) [!HAVE_LCHOWN]: Define to chown, for convenience.
        (struct dir_attr): Cache the entire struct stat of the directory,
        rather than just its mode, so that we needn't stat the directory
        twice (which can lead to races).
        (re_protect): Don't use XSTAT as that's not appropriate in
        this context (symlinks should be followed here).  Instead, use
        the cached stat value.
        (make_dir_parents_private): Save dir's entire struct stat, not
        just its mode.
        * tests/cp/cp-parents: Add test to check against bug with
        cp --parents and symlinks.

diff --git a/NEWS b/NEWS
index f1b81f0..c587fe7 100644
--- a/NEWS
+++ b/NEWS
@@ -18,7 +18,10 @@ GNU coreutils NEWS                                    -*- 
outline -*-
 ** Bug fixes

   cp no longer fails to write through a dangling symlink
-  [bug introduced in coreutils-6.7].  Also, 'cp' no longer considers a
+  [bug introduced in coreutils-6.7].  cp --parents no
+  longer mishandles symlinks to directories in file name
+  components in the source, e.g., "cp --parents symlink/a/b
+  d" no longer fails.  Also, 'cp' no longer considers a
   destination symlink to be the same as the referenced file when
   copying links or making backups.  For example, if SYM is a symlink
   to FILE, "cp -l FILE SYM" now reports an error instead of silently
diff --git a/src/copy.c b/src/copy.c
index 0e9f2d7..2a8d42b 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -62,6 +62,11 @@
 # define fchown(fd, uid, gid) (-1)
 #endif

+#ifndef HAVE_LCHOWN
+# define HAVE_LCHOWN false
+# define lchown(name, uid, gid) chown (name, uid, gid)
+#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))
@@ -172,7 +177,9 @@ copy_dir (char const *src_name_in, char const *dst_name_in, 
bool new_dst,

 /* Set the owner and owning group of DEST_DESC to the st_uid and
    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
+   the owner and owning group of DST_NAME instead; for
+   safety prefer lchown if the system supports it since no
+   symbolic links should be involved.  DEST_DESC must
    refer to the same file as DEST_NAME if defined.
    Return 1 if the syscall succeeds, 0 if it fails but it's OK
    not to preserve ownership, -1 otherwise.  */
@@ -188,7 +195,7 @@ set_owner (const struct cp_options *x, char const 
*dst_name, int dest_desc,
     }
   else
     {
-      if (chown (dst_name, uid, gid) == 0)
+      if (lchown (dst_name, uid, gid) == 0)
        return 1;
     }

@@ -212,6 +219,9 @@ static void
 set_author (const char *dst_name, int dest_desc, const struct stat *src_sb)
 {
 #if HAVE_STRUCT_STAT_ST_AUTHOR
+  /* FIXME: Modify the following code so that it does not
+     follow symbolic links.  */
+
   /* Preserve the st_author field.  */
   file_t file = (dest_desc < 0
                 ? file_name_lookup (dst_name, 0, 0)
@@ -1909,20 +1919,21 @@ copy_internal (char const *src_name, char const 
*dst_name,
        {
          /* Preserve the owner and group of the just-`copied'
             symbolic link, if possible.  */
-#if HAVE_LCHOWN
-         if (lchown (dst_name, src_sb.st_uid, src_sb.st_gid) != 0
+         if (HAVE_LCHOWN
+             && lchown (dst_name, src_sb.st_uid, src_sb.st_gid) != 0
              && ! chown_failure_ok (x))
            {
              error (0, errno, _("failed to preserve ownership for %s"),
                     dst_name);
              goto un_backup;
            }
-#else
-         /* Can't preserve ownership of symlinks.
-            FIXME: maybe give a warning or even error for symlinks
-            in directories with the sticky bit set -- there, not
-            preserving owner/group is a potential security problem.  */
-#endif
+         else
+           {
+             /* Can't preserve ownership of symlinks.
+                FIXME: maybe give a warning or even error for symlinks
+                in directories with the sticky bit set -- there, not
+                preserving owner/group is a potential security problem.  */
+           }
        }
     }
   else
diff --git a/src/cp.c b/src/cp.c
index 1d66bc3..9436be6 100644
--- a/src/cp.c
+++ b/src/cp.c
@@ -36,6 +36,10 @@
 #include "utimens.h"
 #include "acl.h"

+#if ! HAVE_LCHOWN
+# define lchown(name, uid, gid) chown (name, uid, gid)
+#endif
+
 #define ASSIGN_BASENAME_STRDUPA(Dest, File_name)       \
   do                                                   \
     {                                                  \
@@ -56,7 +60,7 @@
    need to be fixed after copying. */
 struct dir_attr
 {
-  mode_t mode;
+  struct stat st;
   bool restore_mode;
   size_t slash_offset;
   struct dir_attr *next;
@@ -290,17 +294,8 @@ re_protect (char const *const_dst_name, size_t src_offset,

   for (p = attr_list; p; p = p->next)
     {
-      struct stat src_sb;
-
       dst_name[p->slash_offset] = '\0';

-      if (XSTAT (x, src_name, &src_sb))
-       {
-         error (0, errno, _("failed to get attributes of %s"),
-                quote (src_name));
-         return false;
-       }
-
       /* Adjust the times (and if possible, ownership) for the copy.
         chown turns off set[ug]id bits for non-root,
         so do the chmod last.  */
@@ -309,8 +304,8 @@ re_protect (char const *const_dst_name, size_t src_offset,
        {
          struct timespec timespec[2];

-         timespec[0] = get_stat_atime (&src_sb);
-         timespec[1] = get_stat_mtime (&src_sb);
+         timespec[0] = get_stat_atime (&p->st);
+         timespec[1] = get_stat_mtime (&p->st);

          if (utimens (dst_name, timespec))
            {
@@ -322,7 +317,7 @@ re_protect (char const *const_dst_name, size_t src_offset,

       if (x->preserve_ownership)
        {
-         if (chown (dst_name, src_sb.st_uid, src_sb.st_gid) != 0
+         if (lchown (dst_name, p->st.st_uid, p->st.st_gid) != 0
              && ! chown_failure_ok (x))
            {
              error (0, errno, _("failed to preserve ownership for %s"),
@@ -333,12 +328,12 @@ re_protect (char const *const_dst_name, size_t src_offset,

       if (x->preserve_mode)
        {
-         if (copy_acl (src_name, -1, dst_name, -1, src_sb.st_mode))
+         if (copy_acl (src_name, -1, dst_name, -1, p->st.st_mode) != 0)
            return false;
        }
       else if (p->restore_mode)
        {
-         if (lchmod (dst_name, p->mode) != 0)
+         if (lchmod (dst_name, p->st.st_mode) != 0)
            {
              error (0, errno, _("failed to preserve permissions for %s"),
                     quote (dst_name));
@@ -421,14 +416,14 @@ make_dir_parents_private (char const *const_dir, size_t 
src_offset,
              int src_errno;

              /* This component does not exist.  We must set
-                *new_dst and new->mode inside this loop because,
+                *new_dst and new->st.st_mode inside this loop because,
                 for example, in the command `cp --parents ../a/../b/c e_dir',
                 make_dir_parents_private creates only e_dir/../a if
                 ./b already exists. */
              *new_dst = true;
-             src_errno = (stat (src, &stats) != 0
+             src_errno = (stat (src, &new->st) != 0
                           ? errno
-                          : S_ISDIR (stats.st_mode)
+                          : S_ISDIR (new->st.st_mode)
                           ? 0
                           : ENOTDIR);
              if (src_errno)
@@ -437,7 +432,7 @@ make_dir_parents_private (char const *const_dir, size_t 
src_offset,
                         quote (src));
                  return false;
                }
-             src_mode = stats.st_mode;
+             src_mode = new->st.st_mode;

              /* If the ownership or special mode bits might change,
                 omit some permissions at first, so unauthorized users
@@ -485,7 +480,7 @@ make_dir_parents_private (char const *const_dir, size_t 
src_offset,
                  if (omitted_permissions & ~stats.st_mode
                      || (stats.st_mode & S_IRWXU) != S_IRWXU)
                    {
-                     new->mode = stats.st_mode | omitted_permissions;
+                     new->st.st_mode = stats.st_mode | omitted_permissions;
                      new->restore_mode = true;
                    }
                }
diff --git a/src/install.c b/src/install.c
index dc7ff01..39d5149 100644
--- a/src/install.c
+++ b/src/install.c
@@ -62,6 +62,10 @@ static bool use_default_selinux_context = true;
 # define endpwent() ((void) 0)
 #endif

+#if ! HAVE_LCHOWN
+# define lchown(name, uid, gid) chown (name, uid, gid)
+#endif
+
 /* Initial number of entries in each hash table entry's table of inodes.  */
 #define INITIAL_HASH_MODULE 100

@@ -606,7 +610,7 @@ change_attributes (char const *name)
      want to know.  */

   if (! (owner_id == (uid_t) -1 && group_id == (gid_t) -1)
-      && chown (name, owner_id, group_id) != 0)
+      && lchown (name, owner_id, group_id) != 0)
     error (0, errno, _("cannot change ownership of %s"), quote (name));
   else if (chmod (name, mode) != 0)
     error (0, errno, _("cannot change permissions of %s"), quote (name));
diff --git a/tests/cp/cp-parents b/tests/cp/cp-parents
index 384f6c9..274f47d 100755
--- a/tests/cp/cp-parents
+++ b/tests/cp/cp-parents
@@ -48,7 +48,8 @@ cd $tmp || framework_failure=1
 . "$abs_srcdir/../setgid-check"

 mkdir foo bar || framework_failure=1
-mkdir -p a/b/c d e || framework_failure=1
+mkdir -p a/b/c d e g || framework_failure=1
+ln -s d/a sym || framework_failure=1
 touch f || framework_failure=1

 if test $framework_failure = 1; then
@@ -75,8 +76,11 @@ test -d d/f && fail=1
 # Check that re_protect works.
 chmod go=w d/a
 cp -a --parents d/a/b/c e || fail=1
+cp -a --parents sym/b/c g || fail=1
 p=`ls -ld e/d|cut -b-10`; case $p in drwxr-xr-x);; *) fail=1;; esac
 p=`ls -ld e/d/a|cut -b-10`; case $p in drwx-w--w-);; *) fail=1;; esac
+p=`ls -ld g/sym|cut -b-10`; case $p in drwx-w--w-);; *) fail=1;; esac
 p=`ls -ld e/d/a/b/c|cut -b-10`; case $p in drwxr-xr-x);; *) fail=1;; esac
+p=`ls -ld g/sym/b/c|cut -b-10`; case $p in drwxr-xr-x);; *) fail=1;; esac

 (exit $fail); exit $fail
M ChangeLog
M NEWS
M src/copy.c
M src/cp.c
M src/install.c
M tests/cp/cp-parents
Committed as d2c7ce100b7cf2b09dffd4e10df63e6e3646b816