bug#17103: regression: cp -al doesn't copy symlinks, but tries to link to them (fail)

[Kees Cook]

On Wed, Mar 26, 2014 at 2:05 PM, Linda Walsh <address@hidden> wrote:
>
>
> Pádraig Brady wrote:
>>
>> That is true, but I confirmed that this is caused by "protected_hardlinks"
>> Perhaps there is a blanket ban on symlinks if you're not the owner,
>> since the symlink could be later changed to point somewhere more
>> sensitive?
>> Kees do you know if this is the case?

Sorry for the delay; I was taking some vacation time.

>
> ---
>         If you have 'write' access to the symlink, I would say
> yes, if not, then no.  however, traditionally, the ownership and permissions
> on symlinks haven't been considered important.

Right, you found the code already, but just to answer it again:
currently the hardlink check rejects anything that isn't a regular
file if owners don't match:

static bool safe_hardlink_source(struct inode *inode)
{
       umode_t mode = inode->i_mode;

       /* Special files should not get pinned to the filesystem. */
       if (!S_ISREG(mode))
               return false;
...

But that only gets called if owners don't match:

       /* Source inode owner (or CAP_FOWNER) can hardlink all they like,
        * otherwise, it must be a safe source.
        */
       if (cred->fsuid == inode->i_uid || safe_hardlink_source(inode) ||
           capable(CAP_FOWNER))
               return 0;

>         Still -- that I can link to a file but not a symlink is an
> obvious flaw in the implementation.  I.e. I have write access to the
> file -- so I should be able to link to it under their new rules,
> but I also have write access to the symlink as the mode bits are 777.
>
>         That's a bit bogus.  They are creating a special case where
> there shouldn't be one.  I'm the directory owner -- I should be able
> to create arbitrary 'entries' in the directory as I own the directory's
> content -- that's been the tradition interpretation.
>
>         Though the traditional rules never applied to symlinks -- and
> now they've come up with an incompatible access method for symlinks...
> If they really wanted to make them non-linkable, they should start
> recognizing the mode bits on the symlink (to change the content of the
> symlink -- which, in this case, is where it points).

Yeah, this is a poor interaction. The trouble with hardlinks is that
they retain the original ownership. This is what leads to the
"dangerous" security concerns for hardlinks. But in the symlink case,
thing are weird because they can only be created, not written.

Pádraig Brady wrote:
> Consider:
>
>  while sleep 1d; do
>    find source/ -links 1 | process_new_files_or_links
>    cp -al source/ snap_$(date +%s)/ || break
>  done
>
> If cp -al silently created symlinks as a fall back, then the above
> would continue to erroneously process existing symlinks.
> This could happen if an existing script is copied to a new system
> with /proc/sys/fs/protected_hardlinks enabled.
> This is my main concern with the fall back.  The inconsistency
> concern (with not also handling setuid files or fifos etc.) is
> valid also, but secondary as it's less likely and shouldn't
> cause a logic issue like above.
>
> Maybe the above case is a bit esoteric, but logic wise it's sound.
> Also this whole issue is a _little_ unusual anyway because usually
> it's the root user doing the cp -al, or the owner of source/

Seems like doing the fallback would serve most users best. Just keep
it documented in the case of really weird stuff like above. Though
this case of hardlink-copying a writable unowned tree is pretty
unusual already! :)

> Ideally the solution to this would be to adjust the symlinks in source/
> in such a way that one could hardlink them, or otherwise relax this
> restriction in the kernel.  That would be preferable as it would
> be available for all applications rather than placing possibly
> problematic workarounds in a single program.

Yeah, it'd be nice if the symlink bits had meaning. However, relaxing
this check on the kernel side results in bad scenarios, especially
when combined with the symlink restrictions. e.g., creating a hardlink
to a symlink that matches the directory owner in /tmp. Now an attacker
controls the destination of a followable symlink. It's just a really
poor interaction between hardlinks, symlinks, and sticky directories.
But if you don't run a system where you have to worry about /tmp races
and such, the protections are much less important.

Linda Walsh wrote:
> The above statement is no longer true on linux with
> the new feature -- which is enabled by default (I find nothing under
> '/etc/' that would change or references 'protected_' other

Regardless of the outcome for "cp", it seems like turning off this
restriction on the system you're doing this on would be the best
short-term solution. It sounds like you're not using a Debian or
Ubuntu system which carries defaults in /etc/sysctl.d/ files. Fedora's
systemd likes to put defaults into /lib/sysctl.d, if that helps you
track it down. I think systemd recognizes /etc/sysctl.d for overriding
settings in /lib/sysctl.d, so you might be able to set it there
instead.

-Kees

-- 
Kees Cook
Chrome OS Security