This service will be undergoing maintenance at 00:00 UTC, 2017-10-23 It is expected to last about 30 minutes
Bug 471463 - selinux problem with logrotate
selinux problem with logrotate
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: logrotate (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Daniel Novotny
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-11-13 15:18 EST by Michael J. Chudobiak
Modified: 2008-11-20 09:50 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-11-20 09:50:21 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Patch, so logrotate can co-exist with selinux (330 bytes, patch)
2008-11-14 09:40 EST, Michael J. Chudobiak
no flags Details | Diff
Patch, so logrotate can co-exist with selinux, version2 (467 bytes, patch)
2008-11-18 09:25 EST, Michael J. Chudobiak
no flags Details | Diff
323902: Patch, so logrotate can co-exist with selinux, version3 (2.07 KB, patch)
2008-11-20 08:30 EST, Daniel Novotny
no flags Details | Diff

  None (edit)
Description Michael J. Chudobiak 2008-11-13 15:18:30 EST
For some reason, logrotate tries to read "." here:

https://fedorahosted.org/logrotate/browser/trunk/config.c#L305

"." = "/root" when run from cron.

This is a problem on selinux systems. I get these selinux denials:

 type=AVC msg=audit(1226489667.211:371): avc:  denied  { read } for
 pid=2291 comm="logrotate" name="root" dev=dm-0 ino=2162689
 scontext=system_u:system_r:logrotate_t:s0-s0:c0.c1023
 tcontext=system_u:object_r:admin_home_t:s0 tclass=dir
 type=SYSCALL msg=audit(1226489667.211:371): arch=40000003 syscall=5
 success=no exit=-13 a0=80525d3 a1=8000 a2=0 a3=8000 items=0 ppid=2289
 pid=2291 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0
 tty=(none) ses=39 comm="logrotate" exe="/usr/sbin/logrotate"
 subj=system_u:system_r:logrotate_t:s0-s0:c0.c1023 key=(null)

The selinux developers can add a policy exception for this, but it seems unnecessary - why should logrotate require access to /root?

Could that be removed?

This bug report was also posted at https://fedorahosted.org/logrotate/ticket/1, but it's ticket #1, which seems a little... odd.


- Mike
Comment 1 Michael J. Chudobiak 2008-11-14 09:40:46 EST
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
Comment 2 Daniel Walsh 2008-11-14 11:02:11 EST
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?
Comment 3 Michael J. Chudobiak 2008-11-14 11:16:07 EST
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
Comment 4 Daniel Walsh 2008-11-14 14:22:23 EST
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.
Comment 5 Michael J. Chudobiak 2008-11-18 09:25:08 EST
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
Comment 6 Daniel Novotny 2008-11-20 08:30:49 EST
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,...
Comment 7 Michael J. Chudobiak 2008-11-20 08:37:08 EST
What purpose does the chdir(".") serve anyway?

- Mike
Comment 8 Daniel Novotny 2008-11-20 09:14:17 EST
(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
Comment 9 Michael J. Chudobiak 2008-11-20 09:23:49 EST
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
Comment 10 Daniel Novotny 2008-11-20 09:50:21 EST
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

Note You need to log in before you can comment on or make changes to this bug.