From 2bdc48121916ab0d7bb7d0cb5cee25549c3705c9 Mon Sep 17 00:00:00 2001 From: Jim Meyering Date: Fri, 16 Nov 2007 09:31:15 +0100 Subject: cp: by default, refuse to copy through a dangling destination symlink * NEWS: Mention this change. * doc/coreutils.texi (cp invocation): Describe the new behavior. * src/copy.c: No longer include "canonicalize.h". (copy_reg): Upon failure to open a dangling destination symlink, don't canonicalize the name, but rather fail (default) or, with POSIXLY_CORRECT, repeat the open call without O_EXCL (potentially dangerous). * src/copy.h (struct cp_options) [open_dangling_dest_symlink]: New member. Reorder the others, grouping "bool" and "enum" members together. * tests/cp/thru-dangling: Test for changed and new behavior. * src/cp.c (cp_option_init): Initialize new member. * src/install.c (cp_option_init): Likewise. * src/mv.c (cp_option_init): Likewise. Signed-off-by: Jim Meyering --- src/copy.c | 46 ++++++++++++++++++++++++---------------------- src/copy.h | 35 ++++++++++++++++++++--------------- src/cp.c | 7 +++++++ src/install.c | 1 + src/mv.c | 1 + 5 files changed, 53 insertions(+), 37 deletions(-) (limited to 'src') diff --git a/src/copy.c b/src/copy.c index 1a265e3c2..31e29b1bf 100644 --- a/src/copy.c +++ b/src/copy.c @@ -33,7 +33,6 @@ #include "acl.h" #include "backupfile.h" #include "buffer-lcm.h" -#include "canonicalize.h" #include "copy.h" #include "cp-hash.h" #include "euidaccess.h" @@ -265,7 +264,6 @@ copy_reg (char const *src_name, char const *dst_name, char *buf; char *buf_alloc = NULL; char *name_alloc = NULL; - char const *followed_dest_name = dst_name; int dest_desc; int dest_errno; int source_desc; @@ -362,35 +360,39 @@ copy_reg (char const *src_name, char const *dst_name, if (*new_dst) { - int open_flags = O_WRONLY | O_CREAT | O_EXCL | O_BINARY; - dest_desc = open (dst_name, open_flags, + int open_flags = O_WRONLY | O_CREAT | O_BINARY; + dest_desc = open (dst_name, open_flags | O_EXCL , dst_mode & ~omitted_permissions); dest_errno = errno; /* When trying to copy through a dangling destination symlink, the above open fails with EEXIST. If that happens, and - lstat'ing the DST_NAME shows that it is a symlink, repeat - the open call, but this time with the name of the final, - missing directory entry. All of this is relevant only for - cp, i.e., not in move_mode. */ + lstat'ing the DST_NAME shows that it is a symlink, then we + have a problem: trying to resolve this dangling symlink to + a directory/destination-entry pair is fundamentally racy, + so punt. If POSIXLY_CORRECT is set, simply call open again, + but without O_EXCL (potentially dangerous). If not, fail + with a diagnostic. These shenanigans are necessary only + when copying, i.e., not in move_mode. */ if (dest_desc < 0 && dest_errno == EEXIST && ! x->move_mode) { struct stat dangling_link_sb; if (lstat (dst_name, &dangling_link_sb) == 0 && S_ISLNK (dangling_link_sb.st_mode)) { - /* FIXME: This is way overkill, since all that's needed - is to follow the symlink that is the last file name - component. */ - name_alloc = - canonicalize_filename_mode (dst_name, CAN_MISSING); - if (name_alloc) + if (x->open_dangling_dest_symlink) { - followed_dest_name = name_alloc; - dest_desc = open (followed_dest_name, open_flags, + dest_desc = open (dst_name, open_flags, dst_mode & ~omitted_permissions); dest_errno = errno; } + else + { + error (0, 0, _("not writing through dangling symlink %s"), + quote (dst_name)); + return_val = false; + goto close_src_desc; + } } } } @@ -591,7 +593,7 @@ copy_reg (char const *src_name, char const *dst_name, timespec[0] = get_stat_atime (src_sb); timespec[1] = get_stat_mtime (src_sb); - if (gl_futimens (dest_desc, followed_dest_name, timespec) != 0) + if (gl_futimens (dest_desc, dst_name, timespec) != 0) { error (0, errno, _("preserving times for %s"), quote (dst_name)); if (x->require_preserve) @@ -604,7 +606,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, followed_dest_name, dest_desc, + switch (set_owner (x, dst_name, dest_desc, src_sb->st_uid, src_sb->st_gid)) { case -1: @@ -617,24 +619,24 @@ copy_reg (char const *src_name, char const *dst_name, } } - set_author (followed_dest_name, dest_desc, src_sb); + set_author (dst_name, dest_desc, src_sb); if (x->preserve_mode || x->move_mode) { - if (copy_acl (src_name, source_desc, followed_dest_name, dest_desc, src_mode) != 0 + if (copy_acl (src_name, source_desc, dst_name, dest_desc, src_mode) != 0 && x->require_preserve) return_val = false; } else if (x->set_mode) { - if (set_acl (followed_dest_name, dest_desc, x->mode) != 0) + 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, followed_dest_name, dst_mode) != 0) + && fchmod_or_lchmod (dest_desc, dst_name, dst_mode) != 0) { error (0, errno, _("preserving permissions for %s"), quote (dst_name)); diff --git a/src/copy.h b/src/copy.h index 2ea7c0549..47c97f389 100644 --- a/src/copy.h +++ b/src/copy.h @@ -82,13 +82,25 @@ struct cp_options { enum backup_type backup_type; + /* How to handle symlinks in the source. */ + enum Dereference_symlink dereference; + + /* This value is used to determine whether to prompt before removing + each existing destination file. It works differently depending on + whether move_mode is set. See code/comments in copy.c. */ + enum Interactive interactive; + + /* Control creation of sparse files. */ + enum Sparse_type sparse_mode; + + /* Set the mode of the destination file to exactly this value + if SET_MODE is nonzero. */ + mode_t mode; + /* If true, copy all files except (directories and, if not dereferencing them, symbolic links,) as if they were regular files. */ bool copy_as_regular; - /* How to handle symlinks in the source. */ - enum Dereference_symlink dereference; - /* If true, remove each existing destination nondirectory before trying to open it. */ bool unlink_dest_before_opening; @@ -104,11 +116,6 @@ struct cp_options Create destination directories as usual. */ bool hard_link; - /* This value is used to determine whether to prompt before removing - each existing destination file. It works differently depending on - whether move_mode is set. See code/comments in copy.c. */ - enum Interactive interactive; - /* If true, rather than copying, first attempt to use rename. If that fails, then resort to copying. */ bool move_mode; @@ -168,13 +175,6 @@ struct cp_options set it based on current umask modified by UMASK_KILL. */ bool set_mode; - /* Set the mode of the destination file to exactly this value - if SET_MODE is nonzero. */ - mode_t mode; - - /* Control creation of sparse files. */ - enum Sparse_type sparse_mode; - /* If true, create symbolic links instead of copying files. Create destination directories as usual. */ bool symbolic_link; @@ -189,6 +189,11 @@ struct cp_options /* If true, stdin is a tty. */ bool stdin_tty; + /* If true, open a dangling destination symlink when not in move_mode. + Otherwise, copy_reg gives a diagnostic (it refuses to write through + such a symlink) and returns false. */ + bool open_dangling_dest_symlink; + /* This is a set of destination name/inode/dev triples. Each such triple represents a file we have created corresponding to a source file name that was specified on the command line. Use it to avoid clobbering diff --git a/src/cp.c b/src/cp.c index 625376b53..5859f8c1d 100644 --- a/src/cp.c +++ b/src/cp.c @@ -761,6 +761,13 @@ cp_option_init (struct cp_options *x) x->update = false; x->verbose = false; + + /* By default, refuse to open a dangling destination symlink, because + in general one cannot do that safely, give the current semantics of + open's O_EXCL flag, (which POSIX doesn't even allow cp to use, btw). + But POSIX requires it. */ + x->open_dangling_dest_symlink = getenv ("POSIXLY_CORRECT") != NULL; + x->dest_info = NULL; x->src_info = NULL; } diff --git a/src/install.c b/src/install.c index 8e4875844..802dfcf77 100644 --- a/src/install.c +++ b/src/install.c @@ -190,6 +190,7 @@ cp_option_init (struct cp_options *x) x->mode = S_IRUSR | S_IWUSR; x->stdin_tty = false; + x->open_dangling_dest_symlink = false; x->update = false; x->preserve_security_context = false; x->verbose = false; diff --git a/src/mv.c b/src/mv.c index 8b70d0048..3b1ba8e9e 100644 --- a/src/mv.c +++ b/src/mv.c @@ -146,6 +146,7 @@ cp_option_init (struct cp_options *x) x->mode = 0; x->stdin_tty = isatty (STDIN_FILENO); + x->open_dangling_dest_symlink = false; x->update = false; x->verbose = false; x->dest_info = NULL; -- cgit v1.2.3-54-g00ecf