| Summary: | audit never initialized on session bus | ||
|---|---|---|---|
| Product: | dbus | Reporter: | Laurent Bigonville <bigon> |
| Component: | core | Assignee: | D-Bus Maintainers <dbus> |
| Status: | RESOLVED FIXED | QA Contact: | D-Bus Maintainers <dbus> |
| Severity: | normal | ||
| Priority: | medium | CC: | bigon, msniko14, walters |
| Version: | 1.5 | Keywords: | patch |
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | review? | ||
| i915 platform: | i915 features: | ||
| Attachments: |
0001-Initialize-audit-subsystem-even-for-the-session-bus.patch
0002-Throw-a-warning-if-we-cannot-open-the-audit-socket-a.patch 0001-Initialize-audit-subsystem-even-for-the-session-bus.patch 0002-Throw-a-warning-if-we-cannot-open-the-audit-socket-a.patch 0001-Initialize-audit-subsystem-even-for-the-session-bus.patch 0002-Throw-a-warning-if-we-cannot-open-the-audit-socket-a.patch |
||
Created attachment 106306 [details] [review] 0001-Initialize-audit-subsystem-even-for-the-session-bus.patch Created attachment 106308 [details] [review] 0002-Throw-a-warning-if-we-cannot-open-the-audit-socket-a.patch (In reply to comment #0) > I'm not an expert here, but I think that the following check is not needed: > > if (errno == EPERM && getuid() != 0) > return; > > Testing on my machine, as a user, audit_open() always returns > 0 Sorry, I know virtually nothing about SELinux. If you are not a SELinux expert, you'll need to find one to review this. Colin Walters is a maintainer and has sometimes reviewed SELinux stuff (or if he can't, perhaps he can find a colleague in Red Hat who can give an informed opinion); adding him to Cc. > Also what's your opinion about setting the file capability CAP_AUDIT_WRITE > to the daemon? Again, I have no informed opinion on this, you'll need to find someone who has one. (In reply to comment #3) > (In reply to comment #0) > > I'm not an expert here, but I think that the following check is not needed: > > > > if (errno == EPERM && getuid() != 0) > > return; > > > > Testing on my machine, as a user, audit_open() always returns > 0 > > Sorry, I know virtually nothing about SELinux. If you are not a SELinux > expert, you'll need to find one to review this. > > Colin Walters is a maintainer and has sometimes reviewed SELinux stuff (or > if he can't, perhaps he can find a colleague in Red Hat who can give an > informed opinion); adding him to Cc. Regarding this precise check, isn't getuid() always != 0 as we are dropping the root privileges just before calling bus_selinux_audit_init() anyway? (In reply to comment #0) It does seem clearly desirable to have the session bus be able to log audit messages if it's acting as a userspace object manager, but... > Also what's your opinion about setting the file capability CAP_AUDIT_WRITE > to the daemon? That's equivalent to a restricted form of setuid. I don't think anyone has audited the dbus-daemon binary for that. (dbus-daemon-launch-helper, yes. dbus-daemon, no) It would be better than pure setuid in that an exploit should only get you CAP_AUDIT_WRITE. As far as solutions...the audit people have always hated dbus becuase it acts as a userspace middleman, which basically breaks auditing. One of several things kdbus fixes. FTR I've send a mail to linux-audit mailing list about granting CAP_AUDIT_WRITE to different applications: https://www.redhat.com/archives/linux-audit/2014-September/msg00029.html Let's see if I get an answer.. (In reply to Laurent Bigonville from comment #4) > Regarding this precise check, isn't getuid() always != 0 as we are dropping > the root privileges just before calling bus_selinux_audit_init() anyway? Yes, so in effect we have always ignored EPERM here :-( I suppose this is what you get if you commit things with confidence-inspiring commit messages like this: * configure.ac, bus/selinux.c, dbus/dbus-sysdeps-unix-util.c: add libaudit support, no clue what this means really but now we have it. Patches from Fedora package. * bus/bus.c (bus_context_new): move selinux initialization after changing to daemon user, patch from Fedora package On the other hand, if audit_open() never actually fails in practice, little harm is done. See also Bug #89225. Created attachment 116435 [details] [review] 0001-Initialize-audit-subsystem-even-for-the-session-bus.patch Created attachment 116436 [details] [review] 0002-Throw-a-warning-if-we-cannot-open-the-audit-socket-a.patch Hey, Any chances this got merged? I tried again and audit_open() is definitely succeeding when called from my non-root user, and anyway the worst that could happen is a warning in the logs. At least it would allow someone to debug his selinux policy without the need of recompiling dbus but just add the CAP_AUDIT_WRITE file capability to the executable. The next part of the discussion is do we want the file capability to be set by default. Created attachment 116510 [details] [review] 0001-Initialize-audit-subsystem-even-for-the-session-bus.patch Same patch as before but with patch from Bug #89225 applied Created attachment 116511 [details] [review] 0002-Throw-a-warning-if-we-cannot-open-the-audit-socket-a.patch Same patch as before but with patch from Bug #89225 applied Do you consider my patches on Bug #89225 to be good? I'd like to land those before altering this bit, really. I'll try to get those patches tested on Ubuntu. I just rebased my patches against yours, I didn't really tested them, sorry I'm not really asking for testing - I can do that myself (when I get round to it). The thing I cannot do myself is to review my patches for correctness - I wrote them, so I would be a poor choice of reviewer! > The call to audit_open() should succeed even if the dbus-daemon doesn't > have the CAP_AUDIT_WRITE capability. On Bug #89225, Colin suggested checking CAP_AUDIT_WRITE once, during initialization, instead of once per message logged. If we do that, the natural place to do it is before calling audit_open(), in which case we wouldn't try this without having CAP_AUDIT_WRITE in any case (there'd be no point). So this change certainly makes sense if I move the CAP_AUDIT_WRITE check to the beginning of bus_audit_init(). Fixed in 1.9.20, thanks for the patches. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
Hello, Looking at the audit code in dbus, it seems that bus_selinux_audit_init() is never called when running as a session bus. This prevents any AVC denials to be logged on the session bus (while SELinux permissions are being checked as bus_selinux_check() is called) Of course to allow dbus to be able to write to audit, the process needs the CAP_AUDIT_WRITE capability set on the executable. I'm not an expert here, but I think that the following check is not needed: if (errno == EPERM && getuid() != 0) return; Testing on my machine, as a user, audit_open() always returns > 0 Also what's your opinion about setting the file capability CAP_AUDIT_WRITE to the daemon?