Bug 92832 - SELinux policy reload not logged in audit
Summary: SELinux policy reload not logged in audit
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.10
Hardware: Other All
: medium normal
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2015-11-05 09:35 UTC by Laurent Bigonville
Modified: 2017-04-05 15:18 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
0001-Initialize-SELinux-and-Apparmor-after-capabilities-a.patch (2.27 KB, patch)
2015-11-08 13:54 UTC, Laurent Bigonville
Details | Splinter Review
0001-Initialize-SELinux-and-Apparmor-after-capabilities-a.patch (1.80 KB, patch)
2015-11-18 09:38 UTC, Laurent Bigonville
Details | Splinter Review
Initialize SELinux and Apparmor after capabilities are set (2.85 KB, patch)
2015-11-24 11:52 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Laurent Bigonville 2015-11-05 09:35:07 UTC
Hi,

When running semodule -B (which triggers a SELinux policy reload), I get the following message from the system dbus daemon:

Can't send to audit system: USER_AVC avc:  received policyload notice (seqno=2) exe="/usr/bin/dbus-daemon" sauid=103 hostname=? addr=? terminal=?

And indeed the message is not logged in audit.

I've started a thread on the audit-linux ML: https://www.redhat.com/archives/linux-audit/2015-November/msg00002.html
Comment 1 Laurent Bigonville 2015-11-05 23:17:37 UTC
Downstream bug https://bugzilla.redhat.com/show_bug.cgi?id=1278602

Apparently one of the threads doens't have the proper capability
Comment 2 Simon McVittie 2015-11-06 08:53:33 UTC
Wait, when did dbus-daemon get threads?

Does libselinux or libaudit use threads behind our back?
Comment 3 Laurent Bigonville 2015-11-06 09:08:27 UTC
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;
}
Comment 4 Laurent Bigonville 2015-11-08 11:40:40 UTC
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
Comment 5 Laurent Bigonville 2015-11-08 13:54:13 UTC
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 6 Simon McVittie 2015-11-11 11:04:59 UTC
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.
Comment 7 Simon McVittie 2015-11-17 14:14:04 UTC
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...
Comment 8 Laurent Bigonville 2015-11-18 09:38:45 UTC
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)
Comment 9 Simon McVittie 2015-11-24 11:52:24 UTC
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.
Comment 10 Simon McVittie 2015-12-01 22:13:36 UTC
(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 11 David King 2016-11-29 14:08:18 UTC
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.
Comment 12 Simon McVittie 2017-04-05 15:18:13 UTC
Also reviewed by SELinux developer Stephen Smalley. Fixed in 1.11.0, and in git for 1.10.18.


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.