Attachment 113660 Details for Bug 89225 – [1/2] bus: move shared libaudit code to a new audit.[ch]

[patch] [1/2] bus: move shared libaudit code to a new audit.[ch]

0001-bus-move-shared-libaudit-code-to-a-new-audit.-ch.patch (text/plain), 17.41 KB, created by Simon McVittie on 2015-02-19 12:27:46 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2015-02-19 12:27:46 UTC

Size: 17.41 KB

patch

obsolete

>From 3cc671cd2b245bf8d85e32af46f64e5977665888 Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Thu, 19 Feb 2015 12:04:26 +0000
>Subject: [PATCH 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.
>---
> bus/Makefile.am          |   2 +
> bus/apparmor.c           |  39 ++--------
> bus/apparmor.h           |   1 -
> bus/audit.c              | 180 +++++++++++++++++++++++++++++++++++++++++++++++
> bus/audit.h              |  31 ++++++++
> bus/bus.c                |   9 +--
> bus/main.c               |   2 +
> bus/selinux.c            | 136 +++++------------------------------
> bus/selinux.h            |   3 -
> cmake/bus/CMakeLists.txt |   2 +
> 10 files changed, 243 insertions(+), 162 deletions(-)
> create mode 100644 bus/audit.c
> create mode 100644 bus/audit.h
>
>diff --git a/bus/Makefile.am b/bus/Makefile.am
>index 9d3cb00..9d29c41 100644
>--- a/bus/Makefile.am
>+++ b/bus/Makefile.am
>@@ -73,6 +73,8 @@ BUS_SOURCES=					\
> 	activation-exit-codes.h			\
> 	apparmor.c				\
> 	apparmor.h				\
>+	audit.c					\
>+	audit.h					\
> 	bus.c					\
> 	bus.h					\
> 	config-parser.c				\
>diff --git a/bus/apparmor.c b/bus/apparmor.c
>index a1b3621..3ba84d8 100644
>--- a/bus/apparmor.c
>+++ b/bus/apparmor.c
>@@ -43,10 +43,10 @@
> #include <unistd.h>
> 
> #ifdef HAVE_LIBAUDIT
>-#include <cap-ng.h>
> #include <libaudit.h>
> #endif /* HAVE_LIBAUDIT */
> 
>+#include "audit.h"
> #include "connection.h"
> #include "utils.h"
> 
>@@ -62,10 +62,6 @@ typedef enum {
> /* Store the value of the AppArmor mediation mode in the bus configuration */
> static AppArmorConfigMode apparmor_config_mode = APPARMOR_ENABLED;
> 
>-#ifdef HAVE_LIBAUDIT
>-static int audit_fd = -1;
>-#endif
>-
> /* The AppArmor context, consisting of a label and a mode. */
> struct BusAppArmorConfinement
> {
>@@ -105,25 +101,6 @@ bus_apparmor_confinement_new (char       *label,
>   return confinement;
> }
> 
>-void
>-bus_apparmor_audit_init (void)
>-{
>-#ifdef HAVE_LIBAUDIT
>-  audit_fd = audit_open ();
>-
>-  if (audit_fd < 0)
>-    {
>-      /* If kernel doesn't support audit, bail out */
>-      if (errno == EINVAL || errno == EPROTONOSUPPORT || errno == EAFNOSUPPORT)
>-        return;
>-      /* If user bus, bail out */
>-      if (errno == EPERM && getuid () != 0)
>-        return;
>-      _dbus_warn ("Failed opening connection to the audit subsystem");
>-    }
>-#endif /* HAVE_LIBAUDIT */
>-}
>-
> /*
>  * Return TRUE on successful check, FALSE on OOM.
>  * Set *is_supported to whether AA has D-Bus features.
>@@ -189,6 +166,9 @@ static void
> log_message (dbus_bool_t allow, const char *op, DBusString *data)
> {
>   const char *mstr;
>+#ifdef HAVE_LIBAUDIT
>+  int audit_fd;
>+#endif
> 
>   if (allow)
>     mstr = "ALLOWED";
>@@ -196,14 +176,12 @@ log_message (dbus_bool_t allow, const char *op, DBusString *data)
>     mstr = "DENIED";
> 
> #ifdef HAVE_LIBAUDIT
>+  audit_fd = bus_audit_get_fd ();
>+
>   if (audit_fd >= 0)
>   {
>     DBusString avc;
> 
>-    capng_get_caps_process ();
>-    if (!capng_have_capability (CAPNG_EFFECTIVE, CAP_AUDIT_WRITE))
>-      goto syslog;
>-
>     if (!_dbus_string_init (&avc))
>       goto syslog;
> 
>@@ -510,11 +488,6 @@ bus_apparmor_shutdown (void)
> 
>   bus_apparmor_confinement_unref (bus_con);
>   bus_con = NULL;
>-
>-#ifdef HAVE_LIBAUDIT
>-  audit_close (audit_fd);
>-#endif /* HAVE_LIBAUDIT */
>-
> #endif /* HAVE_APPARMOR */
> }
> 
>diff --git a/bus/apparmor.h b/bus/apparmor.h
>index 4a47aec..18f3ee7 100644
>--- a/bus/apparmor.h
>+++ b/bus/apparmor.h
>@@ -29,7 +29,6 @@
> #include <dbus/dbus.h>
> #include "bus.h"
> 
>-void bus_apparmor_audit_init (void);
> dbus_bool_t bus_apparmor_pre_init (void);
> dbus_bool_t bus_apparmor_set_mode_from_config (const char *mode,
>                                                DBusError *error);
>diff --git a/bus/audit.c b/bus/audit.c
>new file mode 100644
>index 0000000..97f7d1c
>--- /dev/null
>+++ b/bus/audit.c
>@@ -0,0 +1,180 @@
>+/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*-
>+ * audit.c - libaudit integration for SELinux and AppArmor
>+ *
>+ * Based on apparmor.c, selinux.c
>+ *
>+ * Copyright Â© 2014-2015 Canonical, Ltd.
>+ * Copyright Â© 2015 Collabora Ltd.
>+ *
>+ * Licensed under the Academic Free License version 2.1
>+ *
>+ * This program is free software; you can redistribute it and/or modify
>+ * it under the terms of the GNU General Public License as published by
>+ * the Free Software Foundation; either version 2 of the License, or
>+ * (at your option) any later version.
>+ *
>+ * This program is distributed in the hope that it will be useful,
>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>+ * GNU General Public License for more details.
>+ *
>+ * You should have received a copy of the GNU General Public License
>+ * along with this program; if not, write to the Free Software
>+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
>+ *
>+ */
>+
>+#include <config.h>
>+#include "audit.h"
>+
>+#ifdef HAVE_ERRNO_H
>+#include <errno.h>
>+#endif
>+
>+#ifdef HAVE_LIBAUDIT
>+#include <cap-ng.h>
>+#include <libaudit.h>
>+#endif
>+
>+#include <dbus/dbus-internals.h>
>+#ifdef DBUS_UNIX
>+#include <dbus/dbus-userdb.h>
>+#endif
>+
>+#ifdef HAVE_LIBAUDIT
>+static int audit_fd = -1;
>+#endif
>+
>+/**
>+ * Open the libaudit fd if appropriate.
>+ */
>+void
>+bus_audit_init(void)
>+{
>+#ifdef HAVE_LIBAUDIT
>+  audit_fd = audit_open ();
>+
>+  if (audit_fd < 0)
>+    {
>+      /* If kernel doesn't support audit, bail out */
>+      if (errno == EINVAL || errno == EPROTONOSUPPORT || errno == EAFNOSUPPORT)
>+        return;
>+      /* If user bus, bail out */
>+      if (errno == EPERM && getuid() != 0)
>+        return;
>+      _dbus_warn ("Failed opening connection to the audit subsystem");
>+    }
>+#endif /* HAVE_LIBAUDIT */
>+}
>+
>+/**
>+ * If libaudit is in use and it would be appropriate to write audit records,
>+ * return the libaudit fd. Otherwise return -1.
>+ */
>+int
>+bus_audit_get_fd (void)
>+{
>+#ifdef HAVE_LIBAUDIT
>+  if (audit_fd >= 0)
>+  {
>+    capng_get_caps_process ();
>+
>+    if (!capng_have_capability (CAPNG_EFFECTIVE, CAP_AUDIT_WRITE))
>+      return -1;
>+
>+    return audit_fd;
>+  }
>+#endif
>+
>+  return -1;
>+}
>+
>+/**
>+ * Close the libaudit fd.
>+ */
>+void
>+bus_audit_shutdown (void)
>+{
>+#ifdef HAVE_LIBAUDIT
>+  audit_close (audit_fd);
>+#endif /* HAVE_LIBAUDIT */
>+}
>+
>+/* The !HAVE_LIBAUDIT case lives in dbus-sysdeps-util-unix.c */
>+#ifdef HAVE_LIBAUDIT
>+/**
>+ * Changes the user and group the bus is running as.
>+ *
>+ * @param user the user to become
>+ * @param error return location for errors
>+ * @returns #FALSE on failure
>+ */
>+dbus_bool_t
>+_dbus_change_to_daemon_user  (const char    *user,
>+                              DBusError     *error)
>+{
>+  dbus_uid_t uid;
>+  dbus_gid_t gid;
>+  DBusString u;
>+
>+  _dbus_string_init_const (&u, user);
>+
>+  if (!_dbus_get_user_id_and_primary_group (&u, &uid, &gid))
>+    {
>+      dbus_set_error (error, DBUS_ERROR_FAILED,
>+                      "User '%s' does not appear to exist?",
>+                      user);
>+      return FALSE;
>+    }
>+
>+  /* If we were root */
>+  if (_dbus_geteuid () == 0)
>+    {
>+      int rc;
>+      int have_audit_write;
>+
>+      have_audit_write = capng_have_capability (CAPNG_PERMITTED, CAP_AUDIT_WRITE);
>+      capng_clear (CAPNG_SELECT_BOTH);
>+      /* Only attempt to retain CAP_AUDIT_WRITE if we had it when
>+       * starting.  See:
>+       * https://bugs.freedesktop.org/show_bug.cgi?id=49062#c9
>+       */
>+      if (have_audit_write)
>+        capng_update (CAPNG_ADD, CAPNG_EFFECTIVE | CAPNG_PERMITTED,
>+                      CAP_AUDIT_WRITE);
>+      rc = capng_change_id (uid, gid, CAPNG_DROP_SUPP_GRP);
>+      if (rc)
>+        {
>+          switch (rc) {
>+            default:
>+              dbus_set_error (error, DBUS_ERROR_FAILED,
>+                              "Failed to drop capabilities: %s\n",
>+                              _dbus_strerror (errno));
>+              break;
>+            case -4:
>+              dbus_set_error (error, _dbus_error_from_errno (errno),
>+                              "Failed to set GID to %lu: %s", gid,
>+                              _dbus_strerror (errno));
>+              break;
>+            case -5:
>+              _dbus_warn ("Failed to drop supplementary groups: %s\n",
>+                          _dbus_strerror (errno));
>+              break;
>+            case -6:
>+              dbus_set_error (error, _dbus_error_from_errno (errno),
>+                              "Failed to set UID to %lu: %s", uid,
>+                              _dbus_strerror (errno));
>+              break;
>+            case -7:
>+              dbus_set_error (error, _dbus_error_from_errno (errno),
>+                              "Failed to unset keep-capabilities: %s\n",
>+                              _dbus_strerror (errno));
>+              break;
>+          }
>+          return FALSE;
>+        }
>+    }
>+
>+ return TRUE;
>+}
>+#endif
>diff --git a/bus/audit.h b/bus/audit.h
>new file mode 100644
>index 0000000..367d7e7
>--- /dev/null
>+++ b/bus/audit.h
>@@ -0,0 +1,31 @@
>+/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*-
>+ * audit.h - libaudit integration for SELinux and AppArmor
>+ *
>+ * Licensed under the Academic Free License version 2.1
>+ *
>+ * This program is free software; you can redistribute it and/or modify
>+ * it under the terms of the GNU General Public License as published by
>+ * the Free Software Foundation; either version 2 of the License, or
>+ * (at your option) any later version.
>+ *
>+ * This program is distributed in the hope that it will be useful,
>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>+ * GNU General Public License for more details.
>+ *
>+ * You should have received a copy of the GNU General Public License
>+ * along with this program; if not, write to the Free Software
>+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301 USA
>+ *
>+ */
>+
>+#ifndef BUS_AUDIT_H
>+#define BUS_AUDIT_H
>+
>+#include <dbus/dbus.h>
>+
>+void bus_audit_init (void);
>+int bus_audit_get_fd (void);
>+void bus_audit_shutdown (void);
>+
>+#endif
>diff --git a/bus/bus.c b/bus/bus.c
>index 68de1b2..31e4e5e 100644
>--- a/bus/bus.c
>+++ b/bus/bus.c
>@@ -35,6 +35,7 @@
> #include "signals.h"
> #include "selinux.h"
> #include "apparmor.h"
>+#include "audit.h"
> #include "dir-watch.h"
> #include <dbus/dbus-list.h>
> #include <dbus/dbus-hash.h>
>@@ -971,13 +972,7 @@ bus_context_new (const DBusString *config_file,
> 	  goto failed;
> 	}
> 
>-#ifdef HAVE_SELINUX
>-      /* FIXME - why not just put this in full_init() below? */
>-      bus_selinux_audit_init ();
>-#endif
>-#ifdef HAVE_APPARMOR
>-      bus_apparmor_audit_init ();
>-#endif
>+      bus_audit_init ();
>     }
> 
>   dbus_server_free_data_slot (&server_data_slot);
>diff --git a/bus/main.c b/bus/main.c
>index b48f03f..68eaf0c 100644
>--- a/bus/main.c
>+++ b/bus/main.c
>@@ -40,6 +40,7 @@
> #endif
> #include "selinux.h"
> #include "apparmor.h"
>+#include "audit.h"
> 
> static BusContext *context;
> 
>@@ -657,6 +658,7 @@ main (int argc, char **argv)
>   bus_context_unref (context);
>   bus_selinux_shutdown ();
>   bus_apparmor_shutdown ();
>+  bus_audit_shutdown ();
> 
>   return 0;
> }
>diff --git a/bus/selinux.c b/bus/selinux.c
>index 2b8e5db..ef627c8 100644
>--- a/bus/selinux.c
>+++ b/bus/selinux.c
>@@ -28,6 +28,7 @@
> #include <dbus/dbus-userdb.h>
> #endif
> #include "selinux.h"
>+#include "audit.h"
> #include "services.h"
> #include "policy.h"
> #include "utils.h"
>@@ -50,7 +51,6 @@
> #include <grp.h>
> #endif /* HAVE_SELINUX */
> #ifdef HAVE_LIBAUDIT
>-#include <cap-ng.h>
> #include <libaudit.h>
> #endif /* HAVE_LIBAUDIT */
> 
>@@ -115,53 +115,35 @@ static const struct avc_lock_callback lock_cb =
>  */
> #ifdef HAVE_SELINUX
> 
>-#ifdef HAVE_LIBAUDIT
>-static int audit_fd = -1;
>-#endif
>-
>-void
>-bus_selinux_audit_init(void)
>-{
>-#ifdef HAVE_LIBAUDIT  
>-  audit_fd = audit_open ();
>-
>-  if (audit_fd < 0)
>-    {
>-      /* If kernel doesn't support audit, bail out */
>-      if (errno == EINVAL || errno == EPROTONOSUPPORT || errno == EAFNOSUPPORT)
>-        return;
>-      /* If user bus, bail out */
>-      if (errno == EPERM && getuid() != 0)
>-        return;
>-      _dbus_warn ("Failed opening connection to the audit subsystem");
>-    }
>-#endif /* HAVE_LIBAUDIT */
>-}
>-
> static void 
> log_callback (const char *fmt, ...) 
> {
>   va_list ap;
>+#ifdef HAVE_LIBAUDIT
>+  int audit_fd;
>+#endif
> 
>   va_start(ap, fmt);
> 
> #ifdef HAVE_LIBAUDIT
>+  audit_fd = bus_audit_get_fd ();
>+
>   if (audit_fd >= 0)
>   {
>-    capng_get_caps_process();
>-    if (capng_have_capability(CAPNG_EFFECTIVE, CAP_AUDIT_WRITE))
>-    {
>-      char buf[PATH_MAX*2];
>-    
>-      /* FIXME: need to change this to show real user */
>-      vsnprintf(buf, sizeof(buf), fmt, ap);
>-      audit_log_user_avc_message(audit_fd, AUDIT_USER_AVC, buf, NULL, NULL,
>-                               NULL, getuid());
>-      goto out;
>-    }
>+    /* 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. */
>+    char buf[PATH_MAX*2];
>+
>+    /* FIXME: need to change this to show real user */
>+    vsnprintf(buf, sizeof(buf), fmt, ap);
>+    audit_log_user_avc_message(audit_fd, AUDIT_USER_AVC, buf, NULL, NULL,
>+                             NULL, getuid());
>+    goto out;
>   }
> #endif /* HAVE_LIBAUDIT */
>-  
>+
>   vsyslog (LOG_USER | LOG_INFO, fmt, ap);
> 
> out:
>@@ -1031,88 +1013,6 @@ bus_selinux_shutdown (void)
>       bus_avc_print_stats ();
> 
>       avc_destroy ();
>-#ifdef HAVE_LIBAUDIT
>-      audit_close (audit_fd);
>-#endif /* HAVE_LIBAUDIT */
>     }
> #endif /* HAVE_SELINUX */
> }
>-
>-/* The !HAVE_LIBAUDIT case lives in dbus-sysdeps-util-unix.c */
>-#ifdef HAVE_LIBAUDIT
>-/**
>- * Changes the user and group the bus is running as.
>- *
>- * @param user the user to become
>- * @param error return location for errors
>- * @returns #FALSE on failure
>- */
>-dbus_bool_t
>-_dbus_change_to_daemon_user  (const char    *user,
>-                              DBusError     *error)
>-{
>-  dbus_uid_t uid;
>-  dbus_gid_t gid;
>-  DBusString u;
>-
>-  _dbus_string_init_const (&u, user);
>-
>-  if (!_dbus_get_user_id_and_primary_group (&u, &uid, &gid))
>-    {
>-      dbus_set_error (error, DBUS_ERROR_FAILED,
>-                      "User '%s' does not appear to exist?",
>-                      user);
>-      return FALSE;
>-    }
>-
>-  /* If we were root */
>-  if (_dbus_geteuid () == 0)
>-    {
>-      int rc;
>-      int have_audit_write;
>-
>-      have_audit_write = capng_have_capability (CAPNG_PERMITTED, CAP_AUDIT_WRITE);
>-      capng_clear (CAPNG_SELECT_BOTH);
>-      /* Only attempt to retain CAP_AUDIT_WRITE if we had it when
>-       * starting.  See:
>-       * https://bugs.freedesktop.org/show_bug.cgi?id=49062#c9
>-       */
>-      if (have_audit_write)
>-        capng_update (CAPNG_ADD, CAPNG_EFFECTIVE | CAPNG_PERMITTED,
>-                      CAP_AUDIT_WRITE);
>-      rc = capng_change_id (uid, gid, CAPNG_DROP_SUPP_GRP);
>-      if (rc)
>-        {
>-          switch (rc) {
>-            default:
>-              dbus_set_error (error, DBUS_ERROR_FAILED,
>-                              "Failed to drop capabilities: %s\n",
>-                              _dbus_strerror (errno));
>-              break;
>-            case -4:
>-              dbus_set_error (error, _dbus_error_from_errno (errno),
>-                              "Failed to set GID to %lu: %s", gid,
>-                              _dbus_strerror (errno));
>-              break;
>-            case -5:
>-              _dbus_warn ("Failed to drop supplementary groups: %s\n",
>-                          _dbus_strerror (errno));
>-              break;
>-            case -6:
>-              dbus_set_error (error, _dbus_error_from_errno (errno),
>-                              "Failed to set UID to %lu: %s", uid,
>-                              _dbus_strerror (errno));
>-              break;
>-            case -7:
>-              dbus_set_error (error, _dbus_error_from_errno (errno),
>-                              "Failed to unset keep-capabilities: %s\n",
>-                              _dbus_strerror (errno));
>-              break;
>-          }
>-          return FALSE;
>-        }
>-    }
>-
>- return TRUE;
>-}
>-#endif
>diff --git a/bus/selinux.h b/bus/selinux.h
>index 3bab36d..e44c97e 100644
>--- a/bus/selinux.h
>+++ b/bus/selinux.h
>@@ -66,7 +66,4 @@ dbus_bool_t bus_selinux_allows_send            (DBusConnection *sender,
> BusSELinuxID* bus_selinux_init_connection_id (DBusConnection *connection,
>                                               DBusError      *error);
> 
>-
>-void bus_selinux_audit_init(void);
>-
> #endif /* BUS_SELINUX_H */
>diff --git a/cmake/bus/CMakeLists.txt b/cmake/bus/CMakeLists.txt
>index 40f6690..7e8cf57 100644
>--- a/cmake/bus/CMakeLists.txt
>+++ b/cmake/bus/CMakeLists.txt
>@@ -39,6 +39,8 @@ set (BUS_SOURCES
> 	${BUS_DIR}/activation.h				
> 	${BUS_DIR}/apparmor.c
> 	${BUS_DIR}/apparmor.h
>+	${BUS_DIR}/audit.c
>+	${BUS_DIR}/audit.h
> 	${BUS_DIR}/bus.c					
> 	${BUS_DIR}/bus.h					
> 	${BUS_DIR}/config-parser.c				
>-- 
>2.1.4
>

Actions: View | Splinter Review

Attachments on bug 89225: 113660 | 113661 | 116931

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.