[Toybox] [PATCH] cp: fix copying xattrs on directories.

[Rob Landley]

On 2/19/19 7:16 PM, enh via Toybox wrote:
> Tested manually:
> 
>   mkdir x
>   setfattr -n user.key -v value x
>   ./toybox cp -r --preserve=a x y
>   getfattr -d y
> 
> I didn't want to be the first to add a test that uses setfattr/getfattr,
> but if you say it's okay I'll send another patch with that.

Except this:

> -        // We only copy xattrs for files because there's no flistxattrat()

Is why I didn't do that:

> +    // POSIX extended attributes
> +    if (TT.pflags&(_CP_xattr|_CP_context)) {
> +      int fdin = openat(tfd, try->name, O_RDONLY);

How do you know the filehandle you just opened points to the same object you
were working on a moment ago? In the filehandle case I have a filehandle to what
I was working on and I _know_ it's the same thing. With the new code, there's a
race where someone can potentially hardlink a file into place to get arbitrary
xattrs applied to it by a privileged process.

The original code was written not to have that race condition.

If you want to factor out the xattr() application body into a function so it can
be called securely on the filehandle(s) we already have from the first place and
called less securely for the directory case (but I suppose we can at least
confirm the filehandle we just opened _is_ a directory on that codepath), maybe
that's reasonable?

But this patch makes me nervous. (Unless I'm misunderstanding what you've done?)

Sigh. I suppose could could fstat() the earlier filehandle and store the
dev:inode and then fstat() the new filehandle and compare them... If you don't
get to this by the weekend I might take a swing at it, but I've actually got a
user of the route command who just sent me a patch to it so I'm trying to get my
rewrite in while I've got a tester other than me for it...

Rob