From 9fe46cd24f6899f50f13c7c3d7d7d7e4ea30583a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 20 Aug 2015 14:26:39 +0200 Subject: [PATCH 1/6] manager: take a reference to NMBusManager --- src/nm-manager.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index c04a910870..a196b72d9c 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -4857,7 +4857,7 @@ nm_manager_init (NMManager *manager) priv->state = NM_STATE_DISCONNECTED; priv->startup = TRUE; - priv->dbus_mgr = nm_bus_manager_get (); + priv->dbus_mgr = g_object_ref (nm_bus_manager_get ()); g_signal_connect (priv->dbus_mgr, NM_BUS_MANAGER_DBUS_CONNECTION_CHANGED, G_CALLBACK (dbus_connection_changed_cb), @@ -5068,7 +5068,7 @@ dispose (GObject *object) priv->prop_filter = 0; } g_signal_handlers_disconnect_by_func (priv->dbus_mgr, dbus_connection_changed_cb, manager); - priv->dbus_mgr = NULL; + g_clear_object (&priv->dbus_mgr); } if (priv->sleep_monitor) { From d188dbe9c30396022f0b0286503172dcde346315 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 20 Aug 2015 13:32:34 +0200 Subject: [PATCH 2/6] manager: use slice allocator for PropertyFilterData --- src/nm-manager.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index a196b72d9c..7ea640357d 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -4391,7 +4391,7 @@ free_property_filter_data (PropertyFilterData *pfd) g_object_unref (pfd->message); g_object_unref (pfd->subject); g_object_unref (pfd->object); - g_free (pfd); + g_slice_free (PropertyFilterData, pfd); } static void @@ -4547,7 +4547,7 @@ prop_filter (GDBusConnection *connection, * make other D-Bus calls from. In particular, we cannot call * org.freedesktop.DBus.GetConnectionUnixUser to find the remote UID. */ - pfd = g_new0 (PropertyFilterData, 1); + pfd = g_slice_new0 (PropertyFilterData); pfd->self = g_object_ref (self); pfd->connection = g_object_ref (connection); pfd->message = message; From eca0d1230673b67bfeffc0de196fe9b6f1147d12 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 20 Aug 2015 13:42:00 +0200 Subject: [PATCH 3/6] manager: also audit-log early failure to setting property --- src/nm-manager.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index 7ea640357d..79ab398361 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -4378,6 +4378,7 @@ typedef struct { NMAuthSubject *subject; const char *permission; const char *audit_op; + char *audit_prop_value; GObject *object; const char *property; gboolean set_enable; @@ -4391,6 +4392,7 @@ free_property_filter_data (PropertyFilterData *pfd) g_object_unref (pfd->message); g_object_unref (pfd->subject); g_object_unref (pfd->object); + g_free (pfd->audit_prop_value); g_slice_free (PropertyFilterData, pfd); } @@ -4403,23 +4405,20 @@ prop_set_auth_done_cb (NMAuthChain *chain, PropertyFilterData *pfd = user_data; NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (pfd->self); NMAuthCallResult result; - gs_free char *prop_value = NULL; GDBusMessage *reply; - prop_value = g_strdup_printf ("%s:%d", pfd->property, pfd->set_enable); - priv->auth_chains = g_slist_remove (priv->auth_chains, chain); result = nm_auth_chain_get_result (chain, pfd->permission); if (error || (result != NM_AUTH_CALL_RESULT_YES)) { reply = g_dbus_message_new_method_error (pfd->message, NM_PERM_DENIED_ERROR, "Not authorized to perform this operation"); - nm_audit_log_control_op (pfd->audit_op, prop_value, FALSE, pfd->subject, error ? error->message : NULL); + nm_audit_log_control_op (pfd->audit_op, pfd->audit_prop_value, FALSE, pfd->subject, error ? error->message : NULL); } else { g_object_set (pfd->object, pfd->property, pfd->set_enable, NULL); reply = g_dbus_message_new_method_reply (pfd->message); g_dbus_message_set_body (reply, g_variant_new_tuple (NULL, 0)); - nm_audit_log_control_op (pfd->audit_op, prop_value, TRUE, pfd->subject, NULL); + nm_audit_log_control_op (pfd->audit_op, pfd->audit_prop_value, TRUE, pfd->subject, NULL); } g_dbus_connection_send_message (pfd->connection, reply, @@ -4438,12 +4437,13 @@ do_set_property_check (gpointer user_data) NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (pfd->self); GDBusMessage *reply = NULL; NMAuthChain *chain; + const char *error_message = NULL; pfd->subject = nm_auth_subject_new_unix_process_from_message (pfd->connection, pfd->message); if (!pfd->subject) { reply = g_dbus_message_new_method_error (pfd->message, NM_PERM_DENIED_ERROR, - "Could not determine request UID."); + (error_message = "Could not determine request UID.")); goto out; } @@ -4452,7 +4452,7 @@ do_set_property_check (gpointer user_data) if (!chain) { reply = g_dbus_message_new_method_error (pfd->message, NM_PERM_DENIED_ERROR, - "Could not authenticate request."); + (error_message = "Could not authenticate request.")); goto out; } @@ -4461,6 +4461,7 @@ do_set_property_check (gpointer user_data) out: if (reply) { + nm_audit_log_control_op (pfd->audit_op, pfd->audit_prop_value, FALSE, pfd->subject, error_message); g_dbus_connection_send_message (pfd->connection, reply, G_DBUS_SEND_MESSAGE_FLAGS_NONE, NULL, NULL); @@ -4556,6 +4557,7 @@ prop_filter (GDBusConnection *connection, pfd->property = glib_propname; pfd->set_enable = set_enable; pfd->audit_op = audit_op; + pfd->audit_prop_value = g_strdup_printf ("%s:%d", pfd->property, pfd->set_enable); g_idle_add (do_set_property_check, pfd); return NULL; From c79d6dfc1b13087fbf2fc7ff36bc8ce10b980b3d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 20 Aug 2015 13:51:56 +0200 Subject: [PATCH 4/6] manager: don't invoke non-thread-safe operations during prop_filter() prop_filter() is run on another thread. It cannot make use of the non-thread-safe object NMBusManager. --- src/nm-manager.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index 79ab398361..0c4fccfec9 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -4391,7 +4391,8 @@ free_property_filter_data (PropertyFilterData *pfd) g_object_unref (pfd->connection); g_object_unref (pfd->message); g_object_unref (pfd->subject); - g_object_unref (pfd->object); + if (pfd->object) + g_object_unref (pfd->object); g_free (pfd->audit_prop_value); g_slice_free (PropertyFilterData, pfd); } @@ -4439,6 +4440,18 @@ do_set_property_check (gpointer user_data) NMAuthChain *chain; const char *error_message = NULL; + if (!pfd->object) { + pfd->object = nm_bus_manager_get_registered_object (nm_bus_manager_get (), + g_dbus_message_get_path (pfd->message)); + if (!pfd->object) { + reply = g_dbus_message_new_method_error (pfd->message, + "org.freedesktop.DBus.Error.UnknownObject", + (error_message = "Object doesn't exist.")); + goto out; + } + g_object_ref (pfd->object); + } + pfd->subject = nm_auth_subject_new_unix_process_from_message (pfd->connection, pfd->message); if (!pfd->subject) { reply = g_dbus_message_new_method_error (pfd->message, @@ -4485,7 +4498,7 @@ prop_filter (GDBusConnection *connection, const char *glib_propname = NULL, *permission = NULL; const char *audit_op = NULL; gboolean set_enable; - gpointer obj; + GObject *object = NULL; PropertyFilterData *pfd; /* The sole purpose of this function is to validate property accesses on the @@ -4528,7 +4541,7 @@ prop_filter (GDBusConnection *connection, } else return message; - obj = self; + object = g_object_ref (self); } else if (!strcmp (propiface, NM_DBUS_INTERFACE_DEVICE)) { if (!strcmp (propname, "Autoconnect")) { glib_propname = NM_DEVICE_AUTOCONNECT; @@ -4536,11 +4549,6 @@ prop_filter (GDBusConnection *connection, audit_op = NM_AUDIT_OP_DEVICE_AUTOCONNECT; } else return message; - - obj = nm_bus_manager_get_registered_object (nm_bus_manager_get (), - g_dbus_message_get_path (message)); - if (!obj) - return message; } else return message; @@ -4553,7 +4561,7 @@ prop_filter (GDBusConnection *connection, pfd->connection = g_object_ref (connection); pfd->message = message; pfd->permission = permission; - pfd->object = g_object_ref (obj); + pfd->object = object; pfd->property = glib_propname; pfd->set_enable = set_enable; pfd->audit_op = audit_op; From 52ed6a8e5c37d32f1d0bc1b559bc602e00d9d61f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 20 Aug 2015 14:20:16 +0200 Subject: [PATCH 5/6] manager: check for object type in do_set_property_check() --- src/nm-manager.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/nm-manager.c b/src/nm-manager.c index 0c4fccfec9..e8272cd1b6 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -4449,6 +4449,14 @@ do_set_property_check (gpointer user_data) (error_message = "Object doesn't exist.")); goto out; } + /* If we lookup the object, we expect the object to be of a certain type. + * Only NMDevice type have settable properties. */ + if (!NM_IS_DEVICE (pfd->object)) { + reply = g_dbus_message_new_method_error (pfd->message, + "org.freedesktop.DBus.Error.InvalidArgs", + (error_message = "Object is of unexpected type.")); + goto out; + } g_object_ref (pfd->object); } From 751c67464387f1bfef2a18b257d75a3679ce73d9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 20 Aug 2015 14:25:46 +0200 Subject: [PATCH 6/6] manager: fix race subscribing prop_filter() prop_filter() gets invoked on another thread and we don't take a strong reference to the manager when subscribing the filter. Fix the race, by passing on a weak reference. --- src/nm-manager.c | 81 +++++++++++++++++++++++++++++++++++++----------- 1 file changed, 63 insertions(+), 18 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index e8272cd1b6..5744a8cdc9 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -112,7 +112,10 @@ typedef struct { NMPolicy *policy; NMBusManager *dbus_mgr; - guint prop_filter; + struct { + GDBusConnection *connection; + guint id; + } prop_filter; NMRfkillManager *rfkill_mgr; NMSettings *settings; @@ -4499,7 +4502,7 @@ prop_filter (GDBusConnection *connection, gboolean incoming, gpointer user_data) { - NMManager *self = NM_MANAGER (user_data); + gs_unref_object NMManager *self = NULL; GVariant *args, *value = NULL; const char *propiface = NULL; const char *propname = NULL; @@ -4509,6 +4512,10 @@ prop_filter (GDBusConnection *connection, GObject *object = NULL; PropertyFilterData *pfd; + self = g_weak_ref_get (user_data); + if (!self) + return message; + /* The sole purpose of this function is to validate property accesses on the * NMManager object since gdbus doesn't give us this functionality. */ @@ -4565,7 +4572,8 @@ prop_filter (GDBusConnection *connection, * org.freedesktop.DBus.GetConnectionUnixUser to find the remote UID. */ pfd = g_slice_new0 (PropertyFilterData); - pfd->self = g_object_ref (self); + pfd->self = self; + self = NULL; pfd->connection = g_object_ref (connection); pfd->message = message; pfd->permission = permission; @@ -4579,6 +4587,55 @@ prop_filter (GDBusConnection *connection, return NULL; } +/******************************************************************************/ + +static int +_set_prop_filter_free2 (gpointer user_data) +{ + g_slice_free (GWeakRef, user_data); + return G_SOURCE_REMOVE; +} + +static void +_set_prop_filter_free (gpointer user_data) +{ + g_weak_ref_clear (user_data); + + /* Delay the final deletion of the user_data. There is a race when + * calling g_dbus_connection_remove_filter() that the callback and user_data + * might have been copied and being executed after the destroy function + * runs (bgo #704568). + * This doesn't really fix the race, but it should work well enough. */ + g_timeout_add_seconds (2, _set_prop_filter_free2, user_data); +} + +static void +_set_prop_filter (NMManager *self, GDBusConnection *connection) +{ + NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); + + nm_assert ((!priv->prop_filter.connection) == (!priv->prop_filter.id)); + + if (priv->prop_filter.connection == connection) + return; + + if (priv->prop_filter.connection) { + g_dbus_connection_remove_filter (priv->prop_filter.connection, priv->prop_filter.id); + priv->prop_filter.id = 0; + g_clear_object (&priv->prop_filter.connection); + } + if (connection) { + GWeakRef *wptr; + + wptr = g_slice_new (GWeakRef); + g_weak_ref_init (wptr, self); + priv->prop_filter.id = g_dbus_connection_add_filter (connection, prop_filter, wptr, _set_prop_filter_free); + priv->prop_filter.connection = g_object_ref (connection); + } +} + +/******************************************************************************/ + static void authority_changed_cb (NMAuthManager *auth_manager, gpointer user_data) { @@ -4728,11 +4785,7 @@ dbus_connection_changed_cb (NMBusManager *dbus_mgr, GDBusConnection *connection, gpointer user_data) { - NMManager *self = NM_MANAGER (user_data); - NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); - - if (connection) - priv->prop_filter = g_dbus_connection_add_filter (connection, prop_filter, self, NULL); + _set_prop_filter (NM_MANAGER (user_data), connection); } /**********************************************************************/ @@ -4762,7 +4815,6 @@ nm_manager_setup (const char *state_file, { NMManager *self; NMManagerPrivate *priv; - GDBusConnection *bus; NMConfigData *config_data; /* Can only be called once */ @@ -4772,9 +4824,7 @@ nm_manager_setup (const char *state_file, priv = NM_MANAGER_GET_PRIVATE (self); - bus = nm_bus_manager_get_connection (priv->dbus_mgr); - if (bus) - priv->prop_filter = g_dbus_connection_add_filter (bus, prop_filter, self, NULL); + _set_prop_filter (self, nm_bus_manager_get_connection (priv->dbus_mgr)); priv->settings = nm_settings_new (); g_signal_connect (priv->settings, "notify::" NM_SETTINGS_STARTUP_COMPLETE, @@ -5035,7 +5085,6 @@ dispose (GObject *object) { NMManager *manager = NM_MANAGER (object); NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (manager); - GDBusConnection *bus; g_slist_free_full (priv->auth_chains, (GDestroyNotify) nm_auth_chain_unref); priv->auth_chains = NULL; @@ -5080,14 +5129,10 @@ dispose (GObject *object) /* Unregister property filter */ if (priv->dbus_mgr) { - bus = nm_bus_manager_get_connection (priv->dbus_mgr); - if (bus && priv->prop_filter) { - g_dbus_connection_remove_filter (bus, priv->prop_filter); - priv->prop_filter = 0; - } g_signal_handlers_disconnect_by_func (priv->dbus_mgr, dbus_connection_changed_cb, manager); g_clear_object (&priv->dbus_mgr); } + _set_prop_filter (manager, NULL); if (priv->sleep_monitor) { g_signal_handlers_disconnect_by_func (priv->sleep_monitor, sleeping_cb, manager);