[this is a bit long, but it details two ways in which GNU cp now fails to conform to the POSIX specification]
GNU coreutils' cp (copy.c, actually) has a safety feature that happens to conflict with the letter of the POSIX spec for cp: http://www.opengroup.org/susv3xcu/cp.html When cp "knows" that a destination file does not exist, POSIX requires that it open/create that file with the equivalent of the following: open (dst_name, O_WRONLY | O_CREAT) Note that those flags do not include O_EXCL. However, adding O_EXCL protects against a symlink attack, when the destination directory is writable by others. GNU cp has been using O_EXCL in that case for almost a year, since coreutils-6.7. While this is an "obvious" improvement, the initial implementation came with a drawback: it inadvertently made cp fail to copy through a dangling destination symlink. That happened because this call fails with EEXIST when dst_name is a dangling symlink: open (dst_name, O_WRONLY | O_CREAT | O_EXCL) However, copying through dangling symlinks is not generally useful; the problem was not even noticed/reported until months after the coreutils-6.7 release. To address that problem, coreutils added code to resolve the destination to its absolute, symlink-free file name before calling open. But that approach has several of its own problems, not least of which are security and reliability. Here, the cure ended up being worse than the disease, so... I found myself choosing between making cp: 1) perform the open without O_EXCL (thus adhering to the letter of the POSIX spec and making cp, mv, and install vulnerable in some cases) 2) use O_EXCL, and take additional measures to perform the open safely and reliably, even for dangling destination symlinks 3) by default, refuse to perform the potentially risky DDS open, but when the POSIXLY_CORRECT envvar is set, do #1. I refuse to make #1 the default. Too many people and scripts copy into world-writable directories. Security matters. I believe that #2 is not feasible, in general. However, before reaching that conclusion, I did write an implementation that follows symlinks carefully. But it was already convoluted, in spite of not handling the possibility of a symlink value specifying a path /A/B/C, where A is a symlink-to-a-directory, say /E/F/G, where E or F is an other-writable directory (or a symlink to a similarly-vulnerable path). In that case, it is not safe to follow the link. If you are tempted to write such an implementation, consider the task of avoiding cycles, too. I have implemented #3. Patch below. I imagine that changing POSIX to allow a conforming cp to use O_EXCL will not be controversial. Considering the security implications, I hope that a change allowing cp not to copy through a dangling symlink would be welcome, too. A fourth option would be to add a new open flag (or combination of existing flags) that makes the usual O_EXCL+O_CREAT semantics also work for dangling destination symlinks. But that would mean changing the open syscall in the kernel. I've just made a snapshot, too: 3.6M http://meyering.net/cu/coreutils-ss.tar.lzma http://meyering.net/cu/coreutils-ss.tar.lzma.sig or 8.6M http://meyering.net/cu/coreutils-ss.tar.gz http://meyering.net/cu/coreutils-ss.tar.gz.sig ----------------------------- 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. --- ChangeLog | 18 ++++++++++++++++++ NEWS | 3 +++ doc/coreutils.texi | 13 +++++++++---- src/copy.c | 46 ++++++++++++++++++++++++---------------------- src/copy.h | 35 ++++++++++++++++++++--------------- src/cp.c | 7 +++++++ src/install.c | 1 + src/mv.c | 1 + tests/cp/thru-dangling | 13 +++++++++++-- 9 files changed, 94 insertions(+), 43 deletions(-) ... diff --git a/NEWS b/NEWS index 11efa75..a5936f8 100644 --- a/NEWS +++ b/NEWS @@ -15,6 +15,9 @@ GNU coreutils NEWS -*- outline -*- ** Changes in behavior + cp, by default, refuses to copy through a dangling destination symlink + Set POSIXLY_CORRECT if you require the old, risk-prone behavior. + pr -F no longer suppresses the footer or the first two blank lines in the header. This is for compatibility with BSD and POSIX. diff --git a/doc/coreutils.texi b/doc/coreutils.texi index 136a3f7..6ed1171 100644 --- a/doc/coreutils.texi +++ b/doc/coreutils.texi @@ -6920,10 +6920,15 @@ recursively. This default can be overridden with the @option{-H} options. If more than one of these options is specified, the last one silently overrides the others. -When copying to a symbolic link, @command{cp} normally follows the -link when creating or copying to a regular file, even if the link is -dangling. However, @command{cp} does not follow these links when -creating directories or special files. Also, when an option like +When copying to a symbolic link, @command{cp} follows the +link only when it refers to an existing regular file. +However, when copying to a dangling symbolic link, @command{cp} +refuses by default, and fails with a diagnostic, since the operation +is inherently dangerous. This behavior is contrary to historical +practice and to @acronym{POSIX}. +Set @env{POSIXLY_CORRECT} to make @command{cp} attempt to create +the target of a dangling destination symlink, in spite of the possible risk. +Also, when an option like @option{--backup} or @option{--link} acts to rename or remove the destination before copying, @command{cp} renames or removes the symbolic link rather than the file it points to. diff --git a/src/copy.c b/src/copy.c index 1a265e3..31e29b1 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 2ea7c05..47c97f3 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 625376b..5859f8c 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 8e48758..802dfcf 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 8b70d00..3b1ba8e 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; diff --git a/tests/cp/thru-dangling b/tests/cp/thru-dangling index 0503af9..0256a16 100755 --- a/tests/cp/thru-dangling +++ b/tests/cp/thru-dangling @@ -1,5 +1,5 @@ #!/bin/sh -# Ensure that cp works when the destination is a dangling symlink +# Ensure that cp works as documented, when the destination is a dangling symlink # Copyright (C) 2007 Free Software Foundation, Inc. @@ -26,10 +26,19 @@ fi ln -s no-such dangle || framework_failure echo hi > f || framework_failure echo hi > exp || framework_failure +echo "cp: not writing through dangling symlink \`dangle'" \ + > exp-err || framework_failure fail=0 -cp f dangle > out 2>&1 || fail=1 +# Starting with 6.9.90, this usage fails, by default: +cp f dangle > err 2>&1 && fail=1 + +compare err exp-err || fail=1 +test -f no-such && fail=1 + +# But you can set POSIXLY_CORRECT to get the historical behavior. +POSIXLY_CORRECT=1 cp f dangle > out 2>&1 || fail=1 cat no-such >> out || fail=1 compare out exp || fail=1 -- 1.5.3.6.736.gb7f30 _______________________________________________ Bug-coreutils mailing list [email protected] http://lists.gnu.org/mailman/listinfo/bug-coreutils
