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 <[EMAIL PROTECTED]>
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
_______________________________________________
Bug-coreutils mailing list
Bug-coreutils@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-coreutils
