[syslog-ng] [Bug 42] capabilities, chown, chmod

[bugzilla at bugzilla.balabit.com]

https://bugzilla.balabit.com/show_bug.cgi?id=42





--- Comment #5 from Balazs Scheidler <bazsi at balabit.hu>  2009-05-07 16:58:21 ---
(In reply to comment #4)
> (In reply to comment #2)
> > (In reply to comment #0)
> > > Let's take a look at syslog-ng-3.0.1/src/affile.c lines 60-83
> > >
> > > 1. CAP_SYS_ADMIN is needed only for /proc/kmsg, it is added w/o check
> > > 2. CAP_DAC_READ_SEARCH should be added only if open fail with errno 13
> > > 2a. CAP_DAC_OVERRIDE should be added only if open fail with errno 13 and with CAP_DAC_READ_SEARCH set
> >
> > well, I wouldn't want to complicate enabling those capabilities too much.
> > Currently those capabilities are only enabled for /proc/kmsg and nothing else.
> > (see the check for AFFILE_PRIVILEGED in affile_sd_new)
> 
> Ok, agreed, maybe it's better to keep it simple. But CAP_SYS_ADMIN is enabled always (in src/main.c).

Well, the problem is that you need CAP_SYS_ADMIN even for reading from /proc/kmsg not just to open it. And the 
read() call is so heavily buried in various layers, that I can't get propagate the fact that it is 
actually reading /proc/kmsg. Sure it must be possible, but I find it odd that I need that cap for reading 
in the first place.

I'd definitely rather fix that in the kernel, but this solution was rejected by kernel gods in the past. 
I'd recommend using a separate dd process to read /proc/kmsg and feed that into a pipe that syslog-ng 
is configured to read. This is similar to what ubuntu does for instance. 

Then you can turn off that capability. The problem is that you need to be in a position 
of controlling the whole system (e.g. distribution) to do that, I really can't require all 
syslog-ng users to read /proc/kmsg with the dd process, especially that earlier syslog-ng 
required all-root priviliges to start with. The current situation is still much better.

> 
> > > 3. fchown needs CAP_CHOWN unconditionaly
> > > 4. fchmod needs CAP_FOWNER if file owner != euid (root here)
> > 
> > I didn't know those. If these are needed for fchown/fchmod, do I need 
> > CAP_DAC_OVERRIDE at all? I was enabling DAC_OVERRIDE to be able to 
> > change owner/mode information, but as it seems that is not needed, right?
> 
> Yes, it is needed to write in log file w/o permissions ie owner(bla) group(ble) perm(0660). And for create dirs if
> parent has no perm too.

I didn't want to enable writing to any files on the system. I'd guess that if you are running
syslog-ng non-root, you'd create your /var/log directory with the appropriate permissions. 
(e.g. user "syslog-ng")

Changing the permissions is useful even in that case though, therefore I allowed it explicitly.

But you can convince me with this one.

> 
> > > 5. all caps should be restored
> > 
> > this was done:
> > 
> >   if (privileged)
> >     {
> >       g_process_cap_restore(saved_caps);
> >     }
> 
> Ah, it simply sholud be restored without this condition.

Yes. And that I did in my patch. ah. where's that hunk? I'm positive that I did this, 
but probably lost it during my commits, just the same that I did with saving the caps 
in the beginning of the function.

> 
> > > summary:
> > > - CAP_SYS_ADMIN and CAP_DAC_OVERRIDE are set always even if unnecessary, and permanently
> >
> > no, this is not true.
> 
> It is. Run getpcaps `pidof syslog-ng`

without my hunk to restore the caps, this is true, I was talking with that in the context.

> 
> > this should be fixed by this patch:
> [...]
> 
> Yes, but now have CAP_CHOWN and CAP_FOWNER permanently (run getpcaps).
> 

this is because of the missing hunk, but here it goes this time:

commit 691c7a3a49eb3f5d019bfa753b80aabd875af1fd
Author: Balazs Scheidler <bazsi at balabit.hu>
Date:   Thu May 7 16:57:55 2009 +0200

    [affile] restore changed caps unconditionally (fixes: pub#42)


-- 
Configure bugmail: https://bugzilla.balabit.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching all bug changes.