Bug 83856

Summary: audit never initialized on session bus
Product: dbus Reporter: Laurent Bigonville <bigon>
Component: coreAssignee: D-Bus Maintainers <dbus>
Status: RESOLVED FIXED QA Contact: D-Bus Maintainers <dbus>
Severity: normal    
Priority: medium CC: bigon, msniko14, walters
Version: 1.5Keywords: 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

Description Laurent Bigonville 2014-09-14 19:39:28 UTC
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?
Comment 1 Laurent Bigonville 2014-09-15 09:43:14 UTC
Created attachment 106306 [details] [review]
0001-Initialize-audit-subsystem-even-for-the-session-bus.patch
Comment 2 Laurent Bigonville 2014-09-15 09:44:25 UTC
Created attachment 106308 [details] [review]
0002-Throw-a-warning-if-we-cannot-open-the-audit-socket-a.patch
Comment 3 Simon McVittie 2014-09-15 10:03:12 UTC
(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.
Comment 4 Laurent Bigonville 2014-09-15 11:06:16 UTC
(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?
Comment 5 Colin Walters 2014-09-15 13:07:48 UTC
(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.
Comment 6 Colin Walters 2014-09-15 13:09:24 UTC
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.
Comment 7 Laurent Bigonville 2014-09-15 13:30:46 UTC
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..
Comment 8 Simon McVittie 2015-02-19 12:21:41 UTC
(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.
Comment 9 Simon McVittie 2015-02-19 12:53:53 UTC
See also Bug #89225.
Comment 10 Laurent Bigonville 2015-06-11 14:32:28 UTC
Created attachment 116435 [details] [review]
0001-Initialize-audit-subsystem-even-for-the-session-bus.patch
Comment 11 Laurent Bigonville 2015-06-11 14:32:47 UTC
Created attachment 116436 [details] [review]
0002-Throw-a-warning-if-we-cannot-open-the-audit-socket-a.patch
Comment 12 Laurent Bigonville 2015-06-11 14:40:05 UTC
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.
Comment 13 Laurent Bigonville 2015-06-15 13:39:15 UTC
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
Comment 14 Laurent Bigonville 2015-06-15 13:39:43 UTC
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
Comment 15 Simon McVittie 2015-06-15 15:51:38 UTC
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.
Comment 16 Laurent Bigonville 2015-06-19 11:40:23 UTC
I just rebased my patches against yours, I didn't really tested them, sorry
Comment 17 Simon McVittie 2015-06-19 13:08:52 UTC
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!
Comment 18 Simon McVittie 2015-07-03 16:04:37 UTC
> 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().
Comment 19 Simon McVittie 2015-08-07 11:57:38 UTC
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.