Re: cp(1) extended attributes bug + [PATCH]: fix the leak in copy_reg()

[Ondřej Vašík]

Ernest N. Mamikonyan wrote:
[returning mailing list to CC, please keep it there]

> On Tue, 01 Sep 2009 02:16:28 -0400, Ondřej Vašík <address@hidden> wrote:
> > Ernest N. Mamikonyan wrote:
> >> Cp(1) doesn't correctly copy extended attributes for read-only files:
> >>
> >> touch foo
> >> setfattr -n user.key -v value foo
> >> chmod a-w foo
> >> cp --preserve=xattr foo bar
> >> cp: setting attribute `user.key' for `user.key': Permission denied
> >
> > This error message is not produced by coreutils sources, but by libattr
> > - all messages about extended attributes are generated there. I'm not
> > sure why they are trying to set attributes for source file - maybe they
> > are not and access rights for destination file are more relevant.
> > Strace/ltrace of the failure could be helpful as well.
> The problem is quite simple! Cp(1) tries to change the xattrs of a file
> with mode 0400 (see attached trace). Is opening the destination file with
> initial an mode of 0600 a security (or some other) risk? I suppose that's
> what needs to be done.

Sorry, I got confused ... it's obvious - not sure about the risk, so not
trying to fix this now. What do you think, Jim/Pádraig?

Additionally it looks like there is a leak (about 36k per file for
failing xattr preserve) in copy_reg() - as the return false; should be
changed to return_val=false; - sending patch for this... but it's not
fixing the reported issue.

> >> If one uses "cp -a" instead, it simply strips the metadata and doesn't
> >> complain.
> >
> > cp -a shouldn't complain, but the xattrs are likely not preserved in
> > this case - just error message from xattr preservation is suppressed and
> > not preserving extended attributes is not considered as error in that
> > case.
> Well, the manpage says that "cp -a" is the same as "cp -dR --preserve=all."

But the manpage also says that relevant source of information is info
documentation. And info documentation mention that little difference in
behaviour. In fact cp -a behaves like "cp -dR --preserve=all", but is
silent about the failures of preserving SELinux context and/or extended
attributes.

> PS. I forgot to mention the version; it's Coreutils 7.5.
> 
> Thanks
> Ernest N. Mamikonyan     


Greetings,
          Ondřej Vašík

cp.log
Description: Text Data

extended-attributes-leak.patch
Description: Text Data

signature.asc
Description: Toto je digitálně podepsaná část zprávy