Re: [PATCH] cp, mv: do preserve extended attributes even for read-only source files

[Pádraig Brady]

Ondřej Vašík wrote:
> Pádraig Brady wrote:
>> Ondřej Vašík wrote:
>>> Pádraig Brady wrote:
>>>> To minimize side affects perhaps we should only do the chmod(600)
>>>> if (geteuid () != 0 && !access (src_name, W_OK)) ?
>>> Good idea, it would reduce possibility of security leak, playing with
>>> access rights is always a bit dangerous (although here we play with
>>> rights on destination descriptor, which is imho much more safe).
>>>
>>> Additionally - Jim is correct that for different owner 0600 rights are
>>> not sufficient for different owner of the file - and 0666 is too much
>>> devil-like ;) . Any idea?
>> preserve_xattr before preserve_ownership ?
> 
> Good idea, moved there and used that (geteuid () != 0 && access
> (src_name, W_OK)) construction - additionally I tried to reduce those
> chmod calls (call for returning permissions only when the write_access 
> granting call was used) - so it should be safer now.
> 
> Anyway, added comment that real problem is in libattr and this is just
> workaround and added FIXME. Better now?

That looks much better, thanks.
Since we're only doing u+rw, and we've already stat'd it's
probably better to just (sb.mode & S_IWUSR) rather than access(...).
Also a couple of the  if statements are indented too far.

This should now be safer but as Jim says it
only effects file systems mounted user_xattr.
Perhaps we should wait until coreutils-7.7 and
also feedback from libattr devs so as we can put
an accurate comment in the code.

cheers,
Pádraig.