Bug 471463

Created attachment 323581 [details]
Patch, so logrotate can co-exist with selinux

I think this one-line patch should fix logrotate + selinux.

logrotate checks to see if "." is readable. If not, logrotate used to print an error message and stop execution.

The patch keeps the error message, but continues execution, since access to "." is not normally needed (unless the config files are sloppy and do not include full paths - but that would be unusual - and the program will now simply report errors when it tries to access ".").

- Mike

But why not report the error only if the config files are sloppy and try to read a file via relative paths?  Since there is no error, here why report anything?

I'm not sure I understand all of the use cases of logrotate, so I tried to keep it simple.

Are you suggesting that relative path names be prohibited in config files? (Does anybody run logrotate from their home directory? Relative names might be handy in a scenario like that - one config file handles all users. Dunno.)

Or do you mean just delete the error messages in config.c at the open and fchdir functions (proceed silently if they don't work)?

- Mike

06 	        if (here < 0) {
307 	            message(MESS_ERROR, "cannot open current directory: %s\n",
308 	                    strerror(errno));
309 	            return 1;
310 	        }

What is the purpose of this code at all.  I don't know why you are checking?  I would say don't check.  Just try to read the path and if this fails report it.

Created attachment 323902 [details]
Patch, so logrotate can co-exist with selinux, version2

Yes, stripping out that section seems to work fine on my systems. Can someone review/commit? (Is this the right upstream place?)

- Mike

Created attachment 324170 [details]
323902: Patch, so logrotate can co-exist with selinux, version3

Hello Daniel and Michael,
this can be tweaked a bit more:
it seems the variable "here" does not make sense at all:
apart from the useless check, it is used only in "fchdir(here)" 
which can be changed to "chdir(".")" thus, the occasional "close(here)"
can be omitted, too,...

What purpose does the chdir(".") serve anyway?

- Mike

(In reply to comment #7)
> What purpose does the chdir(".") serve anyway?
> 
> - Mike

oh, now I see the purpose: you open the directory, got the filehandle, then chdir somewhere else and after that chdir back to the previous place

the question is what can you do, if selinux prevents you to get the filehandle for "here"... question is if there is a better way (selinux-proof) to do this

I think that this situation only arises if the config file(s) point to log files using relative paths.

Is that a real scenario? I think any normal config file would always have absolute paths ("/var/log/messages"). Using relative paths seems like a bad idea, since it relies on ".". But I might not understand all use cases for logwatch.

I would suggest considering modifying basenames.c:ourDirName to abort with an error if a relative path is specified, instead of using "."

But I'm OK with the current patch too...

- Mike

OK... I released logrotate-3.7.7-3.fc11 which removes the check and "return 1", but keeps the filehandle "here", so it serves its previous purpose (remembering
your position in the directory tree) when it works -i.e. I used your patch instead of mine after all...

if there are any other ideas, feel free to reopen the bug