cp and dangling destination symlinks: POSIX or safety

[Jim Meyering]

[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