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

[enh]

On Wed, Feb 20, 2019 at 6:31 AM Rob Landley <rob at landley.net> wrote:
>
> 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.

my understanding is that the security folks think that if you're doing
this and assuming it's secure, you've already lost. best effort is
fine. (which is why it's only one of many things i'm looking at.)

they did point out that we can probably do a bit better by using the
fd stashed in `try->extra`.

they also ask whether this (in the existing code) would be better as
just `|O_DIRECTORY` in the openat?

          if (-1 != (try->extra = openat(cfd, catch, O_NOFOLLOW)))
            if (!fstat(try->extra, &st2) && S_ISDIR(st2.st_mode))

they also point out that with my patch we open regular files twice; we
might want to move the regular content copying via sendfile down to
where it can share the same fd as the attributes.

they ask asked whether the code works for fifos and devices and so on
(all of which can have xattrs). i think the answer is "yes", but with
the special case of open failing if:

       ENXIO  The  file  is  a device special file and no corresponding device
              exists.

they wonder whether we should use O_PATH, though obviously as someone
who wants to have toybox on macos eventually (at least for stuff like
cp that isn't linux-specific) i'd personally like to avoid adding a
new linux dependency if possible.

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

yeah, i'm still hoping to have a look but didn't get chance yet. i'm
still trying to breadth-first so anything that turns out to be hard
goes back on my to-do list (which i am at least keeping in good
order), which is why i've been sending you unrelated stuff in the
meantime. (although the dd patch turned out to be bigger than i
expected, it still seems like it's moving dd in a slightly tidier
direction.)

> Rob