Hi, > > One possible solution would be to always create files with S_IRUSR|S_IWUSR > > as the mode parameter to open and do a chmod to sb.st_mode after the > > chown. I'm not sure, though, whether that would still leave problems > > in some cases where an attacker could create the to-be-created file > > beforehand and thus cause it to just be opened and truncated without > > re-creation. The most-easily constructed scenario (directory writeable > > by a user/group not allowed to read the log file) should be prevented by > > the chown failing for a non-root logrotate process when trying to change > > the owner of the destination file. > > Surely in that case, the file couldn't be created in the first place.
Why not? Directory owned by the user running logrotate and the group that's not supposed to read the log, mode 0770, logfile owned by the same user and mode 0700 - so, any member of said group could create a file there and logrotate would open it and write all the valuable data to it - well, were it not for the chown that would fail because the user running logrotate would not be allowed to change the owner of a file not belonging to him ... > > Unless anyone is sure that there are no problems left with this > > solution (and for the sake of clarity maybe even then), I'd suggest > > to instead unlink() the filename beforehand, then (re-)create it with > > O_EXCL and mode of 0 (shouldn't cause any problems anymore, since > > logrotate never will need to open it unless the later chmod has been > > executed), then chown and then chmod the file to set the "cloned > > permissions" (possibly obeying the current umask for backwards > > compatibility, even though I do consider it rather counter-intuitive > > that the umask is obeyed while cloning the permissions of one file onto > > another). > > So, you're suggesting that the sequence should be: > > unlink(file) > open(file,O_CREAT|O_EXCL|O_NOFOLLOW,0) -- fails if file exists I don't see any need for the O_NOFOLLOW!? O_EXCL already causes open() to fail on a symbolic link ... > close(file) > chown(file,user,group) > chmod(file,mode) Erm, no, of course, fchown() and fchmod() should still be used in order to avoid race conditions there. I find it a bit difficult to judge whether this would be "secure", as the intended usage scenarios aren't completely clear to me. If you consider, for example, a destination directory that is below a globally writable directory, any user could rename that former directory and then re-create one with the original name in which he would be able to delete rotated files. Is that a problem? *shrug* :-) At least the "usual" setups where the destination directory itself is accessible by more than just the user running logrotate and root, but where the object named by that directory's path cannot be changed by anyone "untrusted", and where this is all on a local filesystem, and where there are no ACLs involved, this should prevent any log contents to be leaked or to be manipulated. Oh, and BTW, I'd be willing to create a patch, given that I do know what the patch should fix ;-) In particular, one question that still would be needed to be answered is whether the current umask should have any effect on the permissions of created files, as it currently does (partially). Florian -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
