Re: FYI: bug-fix: cp would fail to write through a dangling symlink

[Paul Eggert]

Jim Meyering <address@hidden> writes:

> +     * src/copy.c (copy_reg): When open fails with EEXIST, the destination
> +     is lstat'able, and a symlink, call open again, but now without O_EXCL.

This patch fixes the bug, but it seems to me that it introduces other
bugs.  The rest of that code assumes that if 'cp' created the file
successfully, then it is not a symbolic link.  'cp' then might go on
to apply (say) lchmod to the file, if the current host doesn't have
fchmod.  But lchmod is incorrect in this case, since we want to set
the permissions of the newly-created file, not on the
previously-dangling symlink.  It seems to me that the rest of the code
needs to be told about this special case, and to use ordinary chmod
instead of lchmod.  Similarly for ACLs, perhaps (I haven't checked for
the other metadata; all I've checked is chmod).

One other thing that's not a bug per se, but a security issue:

> +      if (dest_desc < 0 && errno == EEXIST)

There is a race condition here in which an attacker can insert a
symlink between the two 'open' calls.  It's not a big-time enormous
race (since if the attacker can write to the destination directory the
user is already in trouble) and I don't see any way to avoid the race
in general, but coreutils should avoid it if it's easy.  We can avoid
it in many important cases (e.g., for 'mv') as follows:

      if (dest_desc < 0 && errno == EEXIST && x->dereference != DEREF_NEVER)

I think there are more race conditions that could be closed fairly
easily.  For example, if 'cp' creates a destination directory, 'cp'
should never attempt to follow dangling symlinks in that directory
since they must have been created by an attacker.  It shouldn't
require any extra system calls to add this protection either, but most
likely another flag needs to be passed around internally.