Created attachment 60468 [details] [review] the patch When D-Bus is used in containers it will usually be executed without the CAP_AUDIT_WRITE capability. Currently when it tries to drop capabilities it tries to keep CAP_AUDIT_WRITE which will fail with EPERM if it didn't have i in the first place. The attached patch changes D-Bus to keep CAP_AUDIT_WRITE only if it actually has the capability.
Comment on attachment 60468 [details] [review] the patch Review of attachment 60468 [details] [review]: ----------------------------------------------------------------- Patch looks good, but I don't know the API to be completely sure.
Comment on attachment 60468 [details] [review] the patch Review of attachment 60468 [details] [review]: ----------------------------------------------------------------- Please ask a SELinux expert to check this before applying (I assume Red Hat can find such a person), but it looks reasonable.
(In reply to comment #2) > Please ask a SELinux expert to check this before applying (I assume Red Hat > can find such a person) Ping?
Re-ping. Lennart, if you want this patch to land, you will have to find someone with SELinux expertise (not necessarily a D-Bus expert) to check it - perhaps Red Hat's SELinux maintainer? Otherwise, it'll remain unmerged.
Well, let's just say that discussing audit with the audit guy seldom yields any results. I did this patch initially so that containers could turn off auditing by dropping caps, as the kernel audit layer is too broken to deal with containers. In nspawn (systemd's container tool) I recently decided to just grant the audit caps by default. This will fuck up the audit logs on the host, but then again, who uses audit anyway, so nobody will notice... Ultimately for my specific problem the best solution if somebody would just teach the kernel audit layer to handle containers properly. So my interest in getting this fixed is not that big anymore, but it still should be fixed, as what D-Bus is currently doing is simply wrong. That all said, I don't think we'll get a review from any audit folks for this anytime soon...
Colin, do you know enough about SELinux/audit to review this?
(In reply to comment #6) > Colin, do you know enough about SELinux/audit to review this? It won't affect the normal case where we have all caps; what the semantics should be between audit and containers is still...up in the air, let's say. So there's no harm from the patch, but on the other hand, if systemd doesn't actually need it anymore, then...tougher call. I'd say we should take it.
Applied for 1.7.6
Hi, I might be wrong here, but with this patch applied, and dbus running outside of a container, I have no capabilities at all associated with the system bus. If I'm reverting this patch, getpcaps shows me the process has the following capability: "cap_audit_write+ep" Am I missing something here?
(In reply to comment #9) > Hi, > > I might be wrong here, but with this patch applied, and dbus running outside > of a container, I have no capabilities at all associated with the system bus. > > If I'm reverting this patch, getpcaps shows me the process has the following > capability: "cap_audit_write+ep" Blah, ok from a quick read of the libcap-ng source I bet we need this patch (not tested locally yet):
Created attachment 88198 [details] [review] 0001-bus-selinux-Fix-previous-commit-for-CAP_AUDIT_WRITE-.patch
Laurent (or anyone else), if you have a chance to test that'd be great, otherwise I should be able to do it sometime next week after my office is reconstructed and I have a testing environment again.
OK, just tested and now the system bus has the "cap_audit_write+ep" capability Thanks for the patch
Comment on attachment 88198 [details] [review] 0001-bus-selinux-Fix-previous-commit-for-CAP_AUDIT_WRITE-.patch Review of attachment 88198 [details] [review]: ----------------------------------------------------------------- Seems sane to me, so with Reviewed-by and Tested-by Laurent, it seems OK. If you can get a SELinux expert to have a look at it (I hear Red Hat/Fedora like SELinux, so maybe you can find one there?) so much the better.
(In reply to comment #14) > Comment on attachment 88198 [details] [review] [review] > 0001-bus-selinux-Fix-previous-commit-for-CAP_AUDIT_WRITE-.patch > > Review of attachment 88198 [details] [review] [review]: > ----------------------------------------------------------------- > > Seems sane to me, so with Reviewed-by and Tested-by Laurent, it seems OK. > > If you can get a SELinux expert to have a look at it (I hear Red Hat/Fedora > like SELinux, so maybe you can find one there?) so much the better. I currently maintain DBus in Red Hat Enterprise Linux, and in the past did extensive work with SELinux: http://www.nsa.gov/research/selinux/contrib.shtml (Including porting to Debian) What I *didn't* know well before was the libcap API. If you want another reviewer though, Lennart should be OK. Lennart?
Patch looks good afaics, but I didn't test it. Thanks!
Good to merge whenever convenient then, thanks! I'll try this out in Debian experimental, since Laurent wanted audit support there.
Fixed in git for 1.7.8
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.