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

[Jim Meyering]

Paul Eggert <address@hidden> wrote:

> 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

Good catch!
I'm glad it's only a problem in a very unusual situation
(copying through a dangling symlink) and on a system that is
so deficient as to lack fchmod.

If this is the only problem (I doubt it),
then I'd be tempted not to bother trying to fix it,
on the principle that anyone using a system without
fchmod cannot possibly be concerned about security.

> 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).

Yes, this deserves some investigation...

> 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

Yes, I noticed that, and even considered going back to the use of the
POSIX-mandated open flags (without O_EXCL), but that seemed too much
like a regression.

Now that you mention this, for starters, I will add O_NOFOLLOW.
That will solve the problem on systems that support that open option
(Linux and FreeBSD, at least).

> 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:

Sounds like it's worth doing.
Are you interested?

>       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.