|
Hello, Could somebody have a quick look at the two patches that I opened for two dbus bugs: https://bugs.freedesktop.org/show_bug.cgi?id=92831 (stop using
avc_init()) https://bugs.freedesktop.org/attachment.cgi?id=138021 (stop using selinux_set_mapping()) I'm also wondering whether the call to avc_add_callback() shouldn't be replaced by selinux_set_callback(), an opinion on this? Kind regards, Laurent Bigonville |
>From 1299035853924131d40d1033ce367153933d4a84 Mon Sep 17 00:00:00 2001 From: Laurent Bigonville <bigon@xxxxxxxx> Date: Sat, 3 Mar 2018 13:15:17 +0100 Subject: [PATCH 1/2] Stop using avc_init() which is deprecated Stop using avc_init() and use avc_open() instead. With this commit dbus-daemon will stop using a thread to monitor the avc netlink and will poll it instead. https://bugs.freedesktop.org/show_bug.cgi?id=92831 --- bus/bus.c | 2 +- bus/selinux.c | 213 +++++++++++++++++++++++++++----------------------------- bus/selinux.h | 2 +- bus/test-main.c | 6 -- bus/test.c | 9 +++ 5 files changed, 113 insertions(+), 119 deletions(-) diff --git a/bus/bus.c b/bus/bus.c index 9fd9820b..5b59ed45 100644 --- a/bus/bus.c +++ b/bus/bus.c @@ -995,7 +995,7 @@ bus_context_new (const DBusString *config_file, */ bus_audit_init (context); - if (!bus_selinux_full_init ()) + if (!bus_selinux_full_init (context, error)) { bus_context_log (context, DBUS_SYSTEM_LOG_ERROR, "SELinux enabled but D-Bus initialization failed; " diff --git a/bus/selinux.c b/bus/selinux.c index d09afb4b..f0ddfa11 100644 --- a/bus/selinux.c +++ b/bus/selinux.c @@ -49,6 +49,7 @@ #include <stdarg.h> #include <stdio.h> #include <grp.h> +#include <dbus/dbus-watch.h> #endif /* HAVE_SELINUX */ #ifdef HAVE_LIBAUDIT #include <libaudit.h> @@ -64,45 +65,20 @@ static dbus_bool_t selinux_enabled = FALSE; /* Store an avc_entry_ref to speed AVC decisions. */ static struct avc_entry_ref aeref; +/* Store the avc netlink fd. */ +static int avc_netlink_fd = -1; + +/* Watch to listen for SELinux status changes via netlink. */ +static DBusWatch *avc_netlink_watch_obj = NULL; +static DBusLoop *avc_netlink_loop_obj = NULL; + /* Store the SID of the bus itself to use as the default. */ static security_id_t bus_sid = SECSID_WILD; -/* Thread to listen for SELinux status changes via netlink. */ -static pthread_t avc_notify_thread; - /* Prototypes for AVC callback functions. */ -static void log_callback (const char *fmt, ...) _DBUS_GNUC_PRINTF (1, 2); -static void log_audit_callback (void *data, security_class_t class, char *buf, size_t bufleft); -static void *avc_create_thread (void (*run) (void)); -static void avc_stop_thread (void *thread); -static void *avc_alloc_lock (void); -static void avc_get_lock (void *lock); -static void avc_release_lock (void *lock); -static void avc_free_lock (void *lock); - -/* AVC callback structures for use in avc_init. */ -static const struct avc_memory_callback mem_cb = -{ - .func_malloc = dbus_malloc, - .func_free = dbus_free -}; -static const struct avc_log_callback log_cb = -{ - .func_log = log_callback, - .func_audit = log_audit_callback -}; -static const struct avc_thread_callback thread_cb = -{ - .func_create_thread = avc_create_thread, - .func_stop_thread = avc_stop_thread -}; -static const struct avc_lock_callback lock_cb = -{ - .func_alloc_lock = avc_alloc_lock, - .func_get_lock = avc_get_lock, - .func_release_lock = avc_release_lock, - .func_free_lock = avc_free_lock -}; +static int log_callback (int type, const char *fmt, ...) _DBUS_GNUC_PRINTF (2, 3); +static int log_audit_callback (void *data, security_class_t class, char *buf, size_t bufleft); + #endif /* HAVE_SELINUX */ /** @@ -115,8 +91,8 @@ static const struct avc_lock_callback lock_cb = */ #ifdef HAVE_SELINUX -static void -log_callback (const char *fmt, ...) +static int +log_callback (int type, const char *fmt, ...) { va_list ap; #ifdef HAVE_LIBAUDIT @@ -150,6 +126,8 @@ log_callback (const char *fmt, ...) out: #endif va_end(ap); + + return 0; } /** @@ -170,7 +148,7 @@ policy_reload_callback (u_int32_t event, security_id_t ssid, /** * Log any auxiliary data */ -static void +static int log_audit_callback (void *data, security_class_t class, char *buf, size_t bufleft) { DBusString *audmsg = data; @@ -188,73 +166,20 @@ log_audit_callback (void *data, security_class_t class, char *buf, size_t buflef if (bufleft > (size_t) _dbus_string_get_length(&s)) _dbus_string_copy_to_buffer_with_nul (&s, buf, bufleft); } -} - -/** - * Create thread to notify the AVC of enforcing and policy reload - * changes via netlink. - * - * @param run the thread run function - * @return pointer to the thread - */ -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", _dbus_strerror (rc)); - exit (1); - } - return &avc_notify_thread; -} -/* Stop AVC netlink thread. */ -static void -avc_stop_thread (void *thread) -{ - pthread_cancel (*(pthread_t *) thread); + return 0; } -/* Allocate a new AVC lock. */ -static void * -avc_alloc_lock (void) +static dbus_bool_t +handle_avc_netlink_watch (DBusWatch *passed_watch, unsigned int flags, void *data) { - pthread_mutex_t *avc_mutex; - - avc_mutex = dbus_new (pthread_mutex_t, 1); - if (avc_mutex == NULL) + if (avc_netlink_check_nb() < 0) { - _dbus_warn ("Could not create mutex: %s", _dbus_strerror (errno)); - exit (1); + _dbus_warn ("Failed to check the netlink socket for pending messages and process them: %s", _dbus_strerror(errno)); + return FALSE; } - pthread_mutex_init (avc_mutex, NULL); - - return avc_mutex; -} - -/* Acquire an AVC lock. */ -static void -avc_get_lock (void *lock) -{ - pthread_mutex_lock (lock); -} -/* Release an AVC lock. */ -static void -avc_release_lock (void *lock) -{ - pthread_mutex_unlock (lock); -} - -/* Free an AVC lock. */ -static void -avc_free_lock (void *lock) -{ - pthread_mutex_destroy (lock); - dbus_free (lock); + return TRUE; } #endif /* HAVE_SELINUX */ @@ -335,7 +260,7 @@ static struct security_class_mapping dbus_map[] = { * logging callbacks. */ dbus_bool_t -bus_selinux_full_init (void) +bus_selinux_full_init (BusContext *context, DBusError *error) { #ifdef HAVE_SELINUX char *bus_context; @@ -358,9 +283,11 @@ bus_selinux_full_init (void) } avc_entry_ref_init (&aeref); - if (avc_init ("avc", &mem_cb, &log_cb, &thread_cb, &lock_cb) < 0) + if (avc_open (NULL, 0) < 0) { - _dbus_warn ("Failed to start Access Vector Cache (AVC)."); + dbus_set_error (error, DBUS_ERROR_FAILED, + "Failed to start Access Vector Cache (AVC): %s", + _dbus_strerror (errno)); return FALSE; } else @@ -368,34 +295,84 @@ bus_selinux_full_init (void) _dbus_verbose ("Access Vector Cache (AVC) started.\n"); } + avc_netlink_fd = avc_netlink_acquire_fd(); + if (avc_netlink_fd < 0) + { + _dbus_warn ("Cannot acquire avc netlink fd"); + goto error; + } + + _dbus_fd_set_close_on_exec (avc_netlink_fd); + + avc_netlink_loop_obj = bus_context_get_loop (context); + _dbus_loop_ref (avc_netlink_loop_obj); + + avc_netlink_watch_obj = _dbus_watch_new (avc_netlink_fd, DBUS_WATCH_READABLE, TRUE, + handle_avc_netlink_watch, NULL, NULL); + if (avc_netlink_watch_obj == NULL) + { + BUS_SET_OOM (error); + goto error; + } + + if (!_dbus_loop_add_watch (avc_netlink_loop_obj, avc_netlink_watch_obj)) + { + _dbus_warn ("Unable to add reload watch to main loop"); + goto error; + } + if (avc_add_callback (policy_reload_callback, AVC_CALLBACK_RESET, NULL, NULL, 0, 0) < 0) { - _dbus_warn ("Failed to add policy reload callback: %s", - _dbus_strerror (errno)); - avc_destroy (); - return FALSE; + dbus_set_error (error, DBUS_ERROR_FAILED, + "Failed to add policy reload callback: %s", + _dbus_strerror (errno)); + goto error; } + selinux_set_callback (SELINUX_CB_AUDIT, (union selinux_callback) log_audit_callback); + selinux_set_callback (SELINUX_CB_LOG, (union selinux_callback) log_callback); + bus_context = NULL; bus_sid = SECSID_WILD; if (getcon (&bus_context) < 0) { - _dbus_verbose ("Error getting context of bus: %s\n", - _dbus_strerror (errno)); - return FALSE; + dbus_set_error (error, DBUS_ERROR_FAILED, + "Error getting context of bus: %s\n", + _dbus_strerror (errno)); + goto error; } if (avc_context_to_sid (bus_context, &bus_sid) < 0) { - _dbus_verbose ("Error getting SID from bus context: %s\n", - _dbus_strerror (errno)); + dbus_set_error (error, DBUS_ERROR_FAILED, + "Error getting SID from bus context: %s\n", + _dbus_strerror (errno)); freecon (bus_context); - return FALSE; + goto error; } freecon (bus_context); + + return TRUE; + +error: + if (avc_netlink_watch_obj) + { + _dbus_loop_remove_watch (avc_netlink_loop_obj, avc_netlink_watch_obj); + _dbus_watch_invalidate (avc_netlink_watch_obj); + _dbus_clear_watch (&avc_netlink_watch_obj); + } + _dbus_clear_loop (&avc_netlink_loop_obj); + if (avc_netlink_fd >= 0) + { + avc_netlink_release_fd (); + avc_netlink_fd = -1; + } + avc_destroy (); + _DBUS_ASSERT_ERROR_IS_SET (error); + return FALSE; #endif /* HAVE_SELINUX */ return TRUE; @@ -976,6 +953,20 @@ bus_selinux_shutdown (void) _dbus_verbose ("AVC shutdown\n"); + if (avc_netlink_watch_obj) + { + _dbus_loop_remove_watch (avc_netlink_loop_obj, avc_netlink_watch_obj); + _dbus_watch_invalidate (avc_netlink_watch_obj); + _dbus_clear_watch (&avc_netlink_watch_obj); + } + _dbus_clear_loop (&avc_netlink_loop_obj); + + if (avc_netlink_fd >= 0) + { + avc_netlink_release_fd (); + avc_netlink_fd = -1; + } + if (bus_sid != SECSID_WILD) { bus_sid = SECSID_WILD; diff --git a/bus/selinux.h b/bus/selinux.h index a0383cdd..53de1a84 100644 --- a/bus/selinux.h +++ b/bus/selinux.h @@ -28,7 +28,7 @@ #include "services.h" dbus_bool_t bus_selinux_pre_init (void); -dbus_bool_t bus_selinux_full_init(void); +dbus_bool_t bus_selinux_full_init(BusContext *context, DBusError *error); void bus_selinux_shutdown (void); dbus_bool_t bus_selinux_enabled (void); diff --git a/bus/test-main.c b/bus/test-main.c index 400ea423..ba73a1b4 100644 --- a/bus/test-main.c +++ b/bus/test-main.c @@ -47,12 +47,6 @@ static DBusString test_data_dir; static void test_pre_hook (void) { - - if (_dbus_getenv ("DBUS_TEST_SELINUX") - && (!bus_selinux_pre_init () - || !bus_selinux_full_init ())) - _dbus_test_fatal ("Could not init selinux support"); - initial_fds = _dbus_check_fdleaks_enter (); } diff --git a/bus/test.c b/bus/test.c index 76960a30..730cd64a 100644 --- a/bus/test.c +++ b/bus/test.c @@ -28,6 +28,8 @@ #include <dbus/dbus-internals.h> #include <dbus/dbus-list.h> #include <dbus/dbus-sysdeps.h> +#include <dbus/dbus-test-tap.h> +#include "selinux.h" /* The "debug client" watch/timeout handlers don't dispatch messages, * as we manually pull them in order to verify them. This is why they @@ -307,6 +309,13 @@ bus_context_new_test (const DBusString *test_data_dir, return NULL; } + if (_dbus_getenv ("DBUS_TEST_SELINUX") + && (!bus_selinux_pre_init () + || !bus_selinux_full_init (context, &error))) + _dbus_test_fatal ("Could not init selinux support"); + + dbus_error_free (&error); + _dbus_string_free (&config_file); return context; -- 2.16.2
>From 94889d438c81f001d4716d11df8a55a3706e45b0 Mon Sep 17 00:00:00 2001 From: Laurent Bigonville <bigon@xxxxxxxx> Date: Sat, 3 Mar 2018 11:15:23 +0100 Subject: [PATCH 2/2] Stop using selinux_set_mapping() function Currently, if the "dbus" security class or the associated AV doesn't exist, dbus-daemon fails to initialize and exits immediately. Also the security classes or access vector cannot be reordered in the policy. This can be a problem for people developping their own policy or trying to access a machine where, for some reasons, there is not policy defined at all. The code here copy the behaviour of the selinux_check_access() function. We cannot use this function here as it doesn't allow us to define a cache. https://bugs.freedesktop.org/show_bug.cgi?id=105330 --- bus/selinux.c | 75 ++++++++++++++++++++++++++++++++++------------------------- 1 file changed, 43 insertions(+), 32 deletions(-) diff --git a/bus/selinux.c b/bus/selinux.c index f0ddfa11..7cde0537 100644 --- a/bus/selinux.c +++ b/bus/selinux.c @@ -236,24 +236,6 @@ bus_selinux_pre_init (void) #endif } -/* - * Private Flask definitions; the order of these constants must - * exactly match that of the structure array below! - */ -/* security dbus class constants */ -#define SECCLASS_DBUS 1 - -/* dbus's per access vector constants */ -#define DBUS__ACQUIRE_SVC 1 -#define DBUS__SEND_MSG 2 - -#ifdef HAVE_SELINUX -static struct security_class_mapping dbus_map[] = { - { "dbus", { "acquire_svc", "send_msg", NULL } }, - { NULL } -}; -#endif /* HAVE_SELINUX */ - /** * Establish dynamic object class and permission mapping and * initialize the user space access vector cache (AVC) for D-Bus and set up @@ -275,13 +257,6 @@ bus_selinux_full_init (BusContext *context, DBusError *error) _dbus_verbose ("SELinux is enabled in this kernel.\n"); - if (selinux_set_mapping (dbus_map) < 0) - { - _dbus_warn ("Failed to set up security class mapping (selinux_set_mapping():%s).", - strerror (errno)); - return FALSE; - } - avc_entry_ref_init (&aeref); if (avc_open (NULL, 0) < 0) { @@ -398,19 +373,55 @@ error: static dbus_bool_t bus_selinux_check (BusSELinuxID *sender_sid, BusSELinuxID *override_sid, - security_class_t target_class, - access_vector_t requested, + const char *target_class, + const char *requested, DBusString *auxdata) { + int saved_errno; + security_class_t security_class; + access_vector_t requested_access; + if (!selinux_enabled) return TRUE; + security_class = string_to_security_class (target_class); + if (security_class == 0) + { + saved_errno = errno; + _dbus_warn ("Error getting security class \"%s\": %s", + target_class, _dbus_strerror (errno)); + log_callback (SELINUX_ERROR, "Unknown class %s", target_class); + if (security_deny_unknown () == 0) + { + return TRUE; + } + + errno = saved_errno; + return FALSE; + } + + requested_access = string_to_av_perm (security_class, requested); + if (requested_access == 0) + { + saved_errno = errno; + _dbus_warn ("Error getting access vector \"%s\": %s", + requested, _dbus_strerror (errno)); + log_callback (SELINUX_ERROR, "Unknown permission %s for class %s", requested, target_class); + if (security_deny_unknown () == 0) + { + return TRUE; + } + + errno = saved_errno; + return FALSE; + } + /* Make the security check. AVC checks enforcing mode here as well. */ if (avc_has_perm (SELINUX_SID_FROM_BUS (sender_sid), override_sid ? SELINUX_SID_FROM_BUS (override_sid) : bus_sid, - target_class, requested, &aeref, auxdata) < 0) + security_class, requested_access, &aeref, auxdata) < 0) { switch (errno) { @@ -477,8 +488,8 @@ bus_selinux_allows_acquire_service (DBusConnection *connection, ret = bus_selinux_check (connection_sid, service_sid, - SECCLASS_DBUS, - DBUS__ACQUIRE_SVC, + "dbus", + "acquire_svc", &auxdata); _dbus_string_free (&auxdata); @@ -606,8 +617,8 @@ bus_selinux_allows_send (DBusConnection *sender, ret = bus_selinux_check (sender_sid, recipient_sid, - SECCLASS_DBUS, - DBUS__SEND_MSG, + "dbus", + "send_msg", &auxdata); _dbus_string_free (&auxdata); -- 2.16.2
