Bug 89225

Created attachment 113660 [details] [review]
[1/2] bus: move shared libaudit code to a new audit.[ch]

This fixes various duplicated libaudit interactions in both
SELinux and AppArmor code paths, including opening two audit sockets
if both SELinux and AppArmor were enabled at compile time.
In particular, audit.c is now the only user of libcap-ng.

This commit is not intended to introduce any functional changes,
except for the de-duplication.

The actual audit_log_user_avc_message() call is still duplicated,
because the SELinux and AppArmor code paths use different mechanisms
to compose the audit message: the SELinux path uses a statically-sized
buffer on the stack which might be subject to truncation, whereas
the AppArmor path uses malloc() (via DBusString) and falls back to
using syslog on a memory allocation failure.

---

"No functional changes" means that, in particular, I have not applied (the equivalent of) Laurent's proposed changes from Bug #83856.

Created attachment 113661 [details] [review]
[2/2] audit: use DBUS_SYSTEM_LOG_WARNING if we cannot open the  audit fd

Comment on attachment 113660 [details] [review]
[1/2] bus: move shared libaudit code to a new audit.[ch]

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

::: bus/audit.c
@@ +60,5 @@
> +      if (errno == EINVAL || errno == EPROTONOSUPPORT || errno == EAFNOSUPPORT)
> +        return;
> +      /* If user bus, bail out */
> +      if (errno == EPERM && getuid() != 0)
> +        return;

In fact getuid() is always nonzero here in practice, because we call this after dropping privileges. Out of scope for this particular bug, though.

Laurent says this doesn't actually fail with EPERM in practice but, again, out of scope for this particular bug.

::: bus/bus.c
@@ +972,4 @@
>  	  goto failed;
>  	}
>  
> +      bus_audit_init ();

I know this is still inside the "if (we just dropped privileges)" block, and I know Laurent wants to move it outside that block so that the session bus can write to the audit log if it happens to have CAP_AUDIT_WRITE, but that's out of scope for this particular bug.

::: bus/selinux.c
@@ +132,5 @@
>    {
> +    /* This should really be a DBusString, but DBusString allocates
> +     * memory dynamically; before switching it, we need to check with
> +     * SELinux people that it would be OK for this to fall back to
> +     * syslog if OOM, like the equivalent AppArmor code does. */

Fixing this is out of scope for this particular bug, but I did add a comment.

@@ +135,5 @@
> +     * SELinux people that it would be OK for this to fall back to
> +     * syslog if OOM, like the equivalent AppArmor code does. */
> +    char buf[PATH_MAX*2];
> +
> +    /* FIXME: need to change this to show real user */

This is a pre-existing FIXME in both SELinux and AppArmor. If someone tells us what "real user" means, we might even be able to fix it. As far as I can see it's been here ever since Havoc imported libaudit patches from Fedora in 2007.

Comment on attachment 113661 [details] [review]
[2/2] audit: use DBUS_SYSTEM_LOG_WARNING if we cannot open the  audit fd

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

::: bus/audit.c
@@ +55,5 @@
>    audit_fd = audit_open ();
>  
>    if (audit_fd < 0)
>      {
> +      int e = errno;

Saved because getuid() could conceivably clobber errno.

I think these are ready for review, but I also need to test them on a system that actively uses audit (Ubuntu with AppArmor seems to be suitable).

Comment on attachment 113660 [details] [review]
[1/2] bus: move shared libaudit code to a new audit.[ch]

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

Ignore the second comment, obviated by the first.

Looks like a straightforward code cleanup to me, I don't see
any issues.

::: bus/audit.c
@@ +80,5 @@
> +    capng_get_caps_process ();
> +
> +    if (!capng_have_capability (CAPNG_EFFECTIVE, CAP_AUDIT_WRITE))
> +      return -1;
> +

The effective caps aren't going to change after process start, so rechecking
them here seems odd, I'd have expected it in bus_audit_init().

Anyways I think this was there originally, and not a big deal.

::: bus/selinux.c
@@ +125,5 @@
>  
>    va_start(ap, fmt);
>  
>  #ifdef HAVE_LIBAUDIT
> +  audit_fd = bus_audit_get_fd ();

This is relatively minor and can be done later, but I'd like to see this fd cached somewhere.

Any thoughts on patch 2/2?

(In reply to Colin Walters from comment #6)
> The effective caps aren't going to change after process start, so rechecking
> them here seems odd, I'd have expected it in bus_audit_init().

This isn't a regression: caps were previously checked in SELinux's log_callback() and AppArmor's log_message(), so, just as expensive (or not) as the current implementation.

Would you like me to move it to bus_audit_init(), a behaviour change relative to the previous code, as well as doing the refactoring that I've already done here?

> >  #ifdef HAVE_LIBAUDIT
> > +  audit_fd = bus_audit_get_fd ();
> 
> This is relatively minor and can be done later, but I'd like to see this fd
> cached somewhere.

If I move the caps check into bus_audit_init(), there's nothing to cache, because the function would become a trivial accessor for a static global; so there would be little point in having a cache for the fd in each LSM's module. Is that what you meant by ignoring your second comment?

Created attachment 116931 [details] [review]
audit: only check for CAP_AUDIT_WRITE once, during  initialization

---

Colin, is this what you wanted?

(This *is* a behaviour change; it makes sense to me, but again, I haven't yet tested it on a properly audit-enabled system.)

Comment on attachment 116931 [details] [review]
audit: only check for CAP_AUDIT_WRITE once, during  initialization

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

Looks reasonable to me, yes.

Fixed in 1.9.20.