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.
