Bug 92832

Downstream bug https://bugzilla.redhat.com/show_bug.cgi?id=1278602

Apparently one of the threads doens't have the proper capability

Wait, when did dbus-daemon get threads?

Does libselinux or libaudit use threads behind our back?

You define a thread_cb function that spawn a pthread:

if (avc_init ("avc", &mem_cb, &log_cb, &thread_cb, &lock_cb) < 0)

[...]

static void *
avc_create_thread (void (*run) (void))
{
  int rc;

  rc = pthread_create (&avc_notify_thread, NULL, (void *(*) (void *)) run, NULL);
  if (rc != 0)
    {
      _dbus_warn ("Failed to start AVC thread: %s\n", _dbus_strerror (rc));
      exit (1);
    }
  return &avc_notify_thread;
}

capng_apply(3) manpage states:

       If you are doing multi-threaded programming, calling this function will only set capabilities on the calling thread. All other threads are unaffected. If you want to set overall capabilities  for  a
       multi-threaded process, you will need to do that before creating any threads. See the capset syscall for more information on this topic.


So I guess that the part that set the capabilities should be placed before the avc_init() function is called

Created attachment 119480 [details] [review]
0001-Initialize-SELinux-and-Apparmor-after-capabilities-a.patch

OK, moving bus_selinux_full_init() after _dbus_change_to_daemon_user() call seems to fix this.

I tested the SELinux case and everything seems to work, hope there are no regressions

Comment on attachment 119480 [details] [review]
0001-Initialize-SELinux-and-Apparmor-after-capabilities-a.patch

Review of attachment 119480 [details] [review]:
-----------------------------------------------------------------

We'll need to test this on an AppArmor system (e.g. Ubuntu) before merging.

I would be interested to know when this regressed.

I'd appreciate review/testing from LSM/audit experts here. The audit refactor between 1.8 and 1.10 merged duplicate code from apparmor.c and selinux.c into a new audit.c, but did not re-order initialization, so I'm unsure why this would have regressed.

The proposed change is essentially a revert of 1f3bcd2 "Initialize AVC earlier so we can look up service security contexts", <http://lists.freedesktop.org/archives/dbus/2008-October/010493.html>, which was presumably done for a reason...

Created attachment 119756 [details] [review]
0001-Initialize-SELinux-and-Apparmor-after-capabilities-a.patch

Well the important point in my patch is to set the capabilities (capng_update()) before the call to avc_init()

So this new patch seems to be OK too (tested for SELinux code path again)

Created attachment 120082 [details] [review]
Initialize SELinux and Apparmor after capabilities are set

From: Laurent Bigonville

avc_init() in the SELinux code path is creating a new thread, we need to
set to capabilities before it gets created so it has the permission to
send audit messages.

It also make more sense to open the audit netlink before the different
logging callbacks are set.

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=92832
[smcv: add comments explaining why initialization must happen in this
specific order]

---

No functional changes, but the new comments hopefully mean this is more permanently fixed.

(In reply to Simon McVittie from comment #9)
> attachment #120082 [details] [review]
> Initialize SELinux and Apparmor after capabilities are set

I've applied this one to master, but not dbus-1.10 yet. I would appreciate testing and review on systems that actively use SELinux.

Comment on attachment 120082 [details] [review]
Initialize SELinux and Apparmor after capabilities are set

Review of attachment 120082 [details] [review]:
-----------------------------------------------------------------

(In reply to Simon McVittie from comment #10) 
> I've applied this one to master, but not dbus-1.10 yet. I would appreciate
> testing and review on systems that actively use SELinux.

Fedora has dbus 1.11 on Rawhide, 25 and 24, and I can see USER_AVC messages in audit.log:

type=USER_AVC msg=audit(1473394973.221:243): pid=873 uid=81 auid=4294967295 ses=4294967295 subj=system_u:system_r:system_dbusd_t:s0-s0:c0.c1023 msg='avc:  received policyload notice (seqno=2)  exe="/usr/bin/dbus-daemon" sauid=81 hostname=? addr=? terminal=?'

From the functional point of view, the patch seems to work fine.

Also reviewed by SELinux developer Stephen Smalley. Fixed in 1.11.0, and in git for 1.10.18.