From eef6c3f88ef275c834e372cf736e307af004c079 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 24 Sep 2020 19:11:40 +0200 Subject: [PATCH 1/2] core: add nm_auth_is_invocation_in_acl_set_error() helper --- src/nm-auth-utils.c | 27 +++++++++++++++++++++++++++ src/nm-auth-utils.h | 7 +++++++ 2 files changed, 34 insertions(+) diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index 9d10d56510..fb54952068 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -13,6 +13,7 @@ #include "nm-auth-manager.h" #include "nm-session-monitor.h" #include "nm-dbus-manager.h" +#include "nm-core-utils.h" /*****************************************************************************/ @@ -667,3 +668,29 @@ nm_auth_is_subject_in_acl_set_error(NMConnection * connection, g_free(error_desc); return FALSE; } + +gboolean +nm_auth_is_invocation_in_acl_set_error(NMConnection * connection, + GDBusMethodInvocation *invocation, + GQuark err_domain, + int err_code, + NMAuthSubject ** out_subject, + GError ** error) +{ + gs_unref_object NMAuthSubject *subject = NULL; + gboolean success; + + nm_assert(!out_subject || !*out_subject); + + subject = nm_dbus_manager_new_auth_subject_from_context(invocation); + if (!subject) { + g_set_error_literal(error, err_domain, err_code, NM_UTILS_ERROR_MSG_REQ_UID_UKNOWN); + return FALSE; + } + + success = nm_auth_is_subject_in_acl_set_error(connection, subject, err_domain, err_code, error); + + NM_SET_OUT(out_subject, g_steal_pointer(&subject)); + + return success; +} diff --git a/src/nm-auth-utils.h b/src/nm-auth-utils.h index b9aa90ce54..8f44692982 100644 --- a/src/nm-auth-utils.h +++ b/src/nm-auth-utils.h @@ -85,4 +85,11 @@ gboolean nm_auth_is_subject_in_acl_set_error(NMConnection * connection, int err_code, GError ** error); +gboolean nm_auth_is_invocation_in_acl_set_error(NMConnection * connection, + GDBusMethodInvocation *invocation, + GQuark err_domain, + int err_code, + NMAuthSubject ** out_subject, + GError ** error); + #endif /* __NETWORKMANAGER_MANAGER_AUTH_H__ */ From 549b126a5c964f9e3b3e9e12a4b66c139cb46044 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 24 Sep 2020 18:34:21 +0200 Subject: [PATCH 2/2] device: allow non-privileged users to call device.GetAppliedConnection() Compare to the connection's GetSettings() call, which is not protected by policykit permissions. It only checks that the requesting user is allowed according to "connection.permission". Previously, device's GetAppliedConnection() requires "network-control" permissions. This although it only reads a profile, without modifying anything. That seems unnecessary, also because in the common case the applied connection is identical to the current settings connection, and the latter can be read without special permissions. Don't require a special policykit permission to read the applied connection. https://bugzilla.redhat.com/show_bug.cgi?id=1882380 --- NEWS | 2 + src/devices/nm-device.c | 92 +++++++++++------------------------------ 2 files changed, 27 insertions(+), 67 deletions(-) diff --git a/NEWS b/NEWS index 4b00eeba53..e0eca35c4d 100644 --- a/NEWS +++ b/NEWS @@ -17,6 +17,8 @@ USE AT YOUR OWN RISK. NOT RECOMMENDED FOR PRODUCTION USE! was built with the respective options enabled. * The long deprecated D-Bus property "Ip4Address" on "org.freedesktop.NetworkManager.Device" interface is not defunct and always returns zero. +* dbus: don't require policykit permission for GetAppliedConnection of + a device. ============================================= NetworkManager-1.26 diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 04331bd3a3..883dee3b8d 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -12876,62 +12876,6 @@ impl_device_reapply(NMDBusObject * obj, /*****************************************************************************/ -static void -get_applied_connection_cb(NMDevice * self, - GDBusMethodInvocation *context, - NMAuthSubject * subject, - GError * error, - gpointer user_data /* possibly dangling pointer */) -{ - NMDevicePrivate *priv; - NMConnection * applied_connection; - GVariant * settings; - - g_return_if_fail(NM_IS_DEVICE(self)); - - if (error) { - g_dbus_method_invocation_return_gerror(context, error); - return; - } - - priv = NM_DEVICE_GET_PRIVATE(self); - - applied_connection = nm_device_get_applied_connection(self); - - if (!applied_connection) { - error = g_error_new_literal(NM_DEVICE_ERROR, - NM_DEVICE_ERROR_NOT_ACTIVE, - "Device is not activated"); - g_dbus_method_invocation_take_error(context, error); - return; - } - - if (applied_connection != user_data) { - /* The applied connection changed due to a race. Reauthenticate. */ - nm_device_auth_request( - self, - context, - applied_connection, - NM_AUTH_PERMISSION_NETWORK_CONTROL, - TRUE, - NULL, - get_applied_connection_cb, - applied_connection /* no need take a ref. We will not dereference this pointer. */); - return; - } - - settings = nm_connection_to_dbus(applied_connection, NM_CONNECTION_SERIALIZE_NO_SECRETS); - if (!settings) - settings = g_variant_new_array(G_VARIANT_TYPE("{sa{sv}}"), NULL, 0); - - g_dbus_method_invocation_return_value( - context, - g_variant_new( - "(@a{sa{sv}}t)", - settings, - nm_active_connection_version_id_get((NMActiveConnection *) priv->act_request.obj))); -} - static void impl_device_get_applied_connection(NMDBusObject * obj, const NMDBusInterfaceInfoExtended *interface_info, @@ -12941,9 +12885,12 @@ impl_device_get_applied_connection(NMDBusObject * obj, GDBusMethodInvocation * invocation, GVariant * parameters) { - NMDevice * self = NM_DEVICE(obj); - NMConnection *applied_connection; - guint32 flags; + NMDevice * self = NM_DEVICE(obj); + NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE(self); + gs_free_error GError *error = NULL; + NMConnection * applied_connection; + guint32 flags; + GVariant * var_settings; g_variant_get(parameters, "(u)", &flags); @@ -12965,15 +12912,26 @@ impl_device_get_applied_connection(NMDBusObject * obj, return; } - nm_device_auth_request( - self, + if (!nm_auth_is_invocation_in_acl_set_error(applied_connection, + invocation, + NM_MANAGER_ERROR, + NM_MANAGER_ERROR_PERMISSION_DENIED, + NULL, + &error)) { + g_dbus_method_invocation_take_error(invocation, g_steal_pointer(&error)); + return; + } + + var_settings = nm_connection_to_dbus(applied_connection, NM_CONNECTION_SERIALIZE_NO_SECRETS); + if (!var_settings) + var_settings = g_variant_new_array(G_VARIANT_TYPE("{sa{sv}}"), NULL, 0); + + g_dbus_method_invocation_return_value( invocation, - applied_connection, - NM_AUTH_PERMISSION_NETWORK_CONTROL, - TRUE, - NULL, - get_applied_connection_cb, - applied_connection /* no need take a ref. We will not dereference this pointer. */); + g_variant_new( + "(@a{sa{sv}}t)", + var_settings, + nm_active_connection_version_id_get((NMActiveConnection *) priv->act_request.obj))); } /*****************************************************************************/