From cea2885aa733ef86d0889e478886d410f9f95a98 Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Sat, 4 Apr 2009 11:37:11 -0400 Subject: [PATCH] system-settings: protect system connection secrets with PolicyKit So that normal users who have PolicyKit authorization to edit system connections can read secrets, move system connection secrets logic into the system connection service from libnm-glib, and protect it with PolicyKit checks. Convert the ifcfg-rh plugin over to using NMSysconfigConnection so that it can take advantage of the new PolicyKit protection. --- libnm-glib/nm-settings.c | 112 +------- .../plugins/ifcfg-rh/nm-ifcfg-connection.c | 2 +- .../plugins/ifcfg-rh/nm-ifcfg-connection.h | 7 +- system-settings/plugins/ifcfg-rh/reader.c | 4 +- system-settings/src/dbus-settings.c | 8 +- system-settings/src/nm-polkit-helpers.c | 19 +- system-settings/src/nm-polkit-helpers.h | 2 +- system-settings/src/nm-sysconfig-connection.c | 250 +++++++++++++++++- system-settings/src/nm-system-settings.conf | 6 +- 9 files changed, 284 insertions(+), 126 deletions(-) diff --git a/libnm-glib/nm-settings.c b/libnm-glib/nm-settings.c index 00cb3b8bca..cbe0f8875e 100644 --- a/libnm-glib/nm-settings.c +++ b/libnm-glib/nm-settings.c @@ -375,103 +375,6 @@ impl_exported_connection_delete (NMExportedConnection *connection, return success; } -static GValue * -string_to_gvalue (const char *str) -{ - GValue *val; - - val = g_slice_new0 (GValue); - g_value_init (val, G_TYPE_STRING); - g_value_set_string (val, str); - - return val; -} - -static void -copy_one_secret (gpointer key, gpointer value, gpointer user_data) -{ - const char *value_str = (const char *) value; - - if (value_str) { - g_hash_table_insert ((GHashTable *) user_data, - g_strdup ((char *) key), - string_to_gvalue (value_str)); - } -} - -static void -add_secrets (NMSetting *setting, - const char *key, - const GValue *value, - GParamFlags flags, - gpointer user_data) -{ - GHashTable *secrets = user_data; - - if (!(flags & NM_SETTING_PARAM_SECRET)) - return; - - if (G_VALUE_HOLDS_STRING (value)) { - const char *tmp; - - tmp = g_value_get_string (value); - if (tmp) - g_hash_table_insert (secrets, g_strdup (key), string_to_gvalue (tmp)); - } else if (G_VALUE_HOLDS (value, DBUS_TYPE_G_MAP_OF_STRING)) { - /* Flatten the string hash by pulling its keys/values out */ - g_hash_table_foreach (g_value_get_boxed (value), copy_one_secret, secrets); - } -} - -static void -destroy_gvalue (gpointer data) -{ - GValue *value = (GValue *) data; - - g_value_unset (value); - g_slice_free (GValue, value); -} - -static void -real_get_secrets (NMExportedConnection *exported, - const gchar *setting_name, - const gchar **hints, - gboolean request_new, - DBusGMethodInvocation *context) -{ - NMConnection *connection; - GError *error = NULL; - GHashTable *settings = NULL; - GHashTable *secrets = NULL; - NMSetting *setting; - - connection = nm_exported_connection_get_connection (exported); - setting = nm_connection_get_setting_by_name (connection, setting_name); - if (!setting) { - g_set_error (&error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, - "%s.%d - Connection didn't have requested setting '%s'.", - __FILE__, __LINE__, setting_name); - dbus_g_method_return_error (context, error); - g_error_free (error); - return; - } - - /* Returned secrets are a{sa{sv}}; this is the outer a{s...} hash that - * will contain all the individual settings hashes. - */ - settings = g_hash_table_new_full (g_str_hash, g_str_equal, - g_free, (GDestroyNotify) g_hash_table_destroy); - - /* Add the secrets from this setting to the inner secrets hash for this setting */ - secrets = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, destroy_gvalue); - nm_setting_enumerate_values (setting, add_secrets, secrets); - - g_hash_table_insert (settings, g_strdup (setting_name), secrets); - - dbus_g_method_return (context, settings); - g_hash_table_destroy (settings); -} - static void impl_exported_connection_get_secrets (NMExportedConnection *connection, const gchar *setting_name, @@ -490,10 +393,16 @@ impl_exported_connection_get_secrets (NMExportedConnection *connection, return; } - if (!EXPORTED_CONNECTION_CLASS (connection)->service_get_secrets) - real_get_secrets (connection, setting_name, hints, request_new, context); - else - EXPORTED_CONNECTION_CLASS (connection)->service_get_secrets (connection, setting_name, hints, request_new, context); + if (!EXPORTED_CONNECTION_CLASS (connection)->service_get_secrets) { + g_set_error (&error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_SECRETS_UNAVAILABLE, + "%s.%d - Missing implementation for ConnectionSettings::GetSecrets.", + __FILE__, __LINE__); + dbus_g_method_return_error (context, error); + g_error_free (error); + return; + } + + EXPORTED_CONNECTION_CLASS (connection)->service_get_secrets (connection, setting_name, hints, request_new, context); } static void @@ -567,7 +476,6 @@ nm_exported_connection_class_init (NMExportedConnectionClass *exported_connectio object_class->dispose = nm_exported_connection_dispose; exported_connection_class->get_settings = real_get_settings; - exported_connection_class->service_get_secrets = real_get_secrets; /* Properties */ g_object_class_install_property diff --git a/system-settings/plugins/ifcfg-rh/nm-ifcfg-connection.c b/system-settings/plugins/ifcfg-rh/nm-ifcfg-connection.c index 6926318e16..b14da49996 100644 --- a/system-settings/plugins/ifcfg-rh/nm-ifcfg-connection.c +++ b/system-settings/plugins/ifcfg-rh/nm-ifcfg-connection.c @@ -40,7 +40,7 @@ #include "reader.h" #include "nm-inotify-helper.h" -G_DEFINE_TYPE (NMIfcfgConnection, nm_ifcfg_connection, NM_TYPE_EXPORTED_CONNECTION) +G_DEFINE_TYPE (NMIfcfgConnection, nm_ifcfg_connection, NM_TYPE_SYSCONFIG_CONNECTION) #define NM_IFCFG_CONNECTION_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), NM_TYPE_IFCFG_CONNECTION, NMIfcfgConnectionPrivate)) diff --git a/system-settings/plugins/ifcfg-rh/nm-ifcfg-connection.h b/system-settings/plugins/ifcfg-rh/nm-ifcfg-connection.h index 9a6e7c5e5d..f2a8c6d6d1 100644 --- a/system-settings/plugins/ifcfg-rh/nm-ifcfg-connection.h +++ b/system-settings/plugins/ifcfg-rh/nm-ifcfg-connection.h @@ -23,7 +23,8 @@ G_BEGIN_DECLS -#include +#include +#include #include "nm-system-config-hal-manager.h" #define NM_TYPE_IFCFG_CONNECTION (nm_ifcfg_connection_get_type ()) @@ -38,11 +39,11 @@ G_BEGIN_DECLS #define NM_IFCFG_CONNECTION_UDI "udi" typedef struct { - NMExportedConnection parent; + NMSysconfigConnection parent; } NMIfcfgConnection; typedef struct { - NMExportedConnectionClass parent; + NMSysconfigConnectionClass parent; } NMIfcfgConnectionClass; GType nm_ifcfg_connection_get_type (void); diff --git a/system-settings/plugins/ifcfg-rh/reader.c b/system-settings/plugins/ifcfg-rh/reader.c index 9a217b6fa6..c610159226 100644 --- a/system-settings/plugins/ifcfg-rh/reader.c +++ b/system-settings/plugins/ifcfg-rh/reader.c @@ -1337,8 +1337,8 @@ connection_from_file (const char *filename, /* We don't write connections yet */ if (connection) { s_con = (NMSettingConnection *) nm_connection_get_setting (connection, NM_TYPE_SETTING_CONNECTION); - if (s_con) - g_object_set (s_con, NM_SETTING_CONNECTION_READ_ONLY, TRUE, NULL); +// if (s_con) +// g_object_set (s_con, NM_SETTING_CONNECTION_READ_ONLY, TRUE, NULL); } /* Don't bother reading the connection fully if it's unmanaged */ diff --git a/system-settings/src/dbus-settings.c b/system-settings/src/dbus-settings.c index d098652919..8119f38e16 100644 --- a/system-settings/src/dbus-settings.c +++ b/system-settings/src/dbus-settings.c @@ -355,11 +355,17 @@ nm_sysconfig_settings_init (NMSysconfigSettings *self) { NMSysconfigSettingsPrivate *priv = NM_SYSCONFIG_SETTINGS_GET_PRIVATE (self); char hostname[HOST_NAME_MAX + 2]; + GError *error = NULL; priv->connections = g_hash_table_new_full (g_direct_hash, g_direct_equal, g_object_unref, NULL); priv->unmanaged_devices = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL); - priv->pol_ctx = create_polkit_context (); + priv->pol_ctx = create_polkit_context (&error); + if (!priv->pol_ctx) { + g_warning ("%s: failed to create PolicyKit context: %s", + __func__, + (error && error->message) ? error->message : "(unknown)"); + } /* Grab hostname on startup and use that if no plugins provide one */ memset (hostname, 0, sizeof (hostname)); diff --git a/system-settings/src/nm-polkit-helpers.c b/system-settings/src/nm-polkit-helpers.c index 3ef010a46f..496a6f7ed1 100644 --- a/system-settings/src/nm-polkit-helpers.c +++ b/system-settings/src/nm-polkit-helpers.c @@ -62,22 +62,25 @@ pk_io_remove_watch (PolKitContext *pk_context, int watch_id) } PolKitContext * -create_polkit_context (void) +create_polkit_context (GError **error) { static PolKitContext *global_context = NULL; - PolKitError *err; + PolKitError *pk_err = NULL; if (G_LIKELY (global_context)) return polkit_context_ref (global_context); global_context = polkit_context_new (); polkit_context_set_io_watch_functions (global_context, pk_io_add_watch, pk_io_remove_watch); - err = NULL; - if (!polkit_context_init (global_context, &err)) { - g_warning ("Cannot initialize libpolkit: %s", - err ? polkit_error_get_error_message (err) : "unknown error"); - if (err) - polkit_error_free (err); + if (!polkit_context_init (global_context, &pk_err)) { + g_set_error (error, NM_SYSCONFIG_SETTINGS_ERROR, + NM_SYSCONFIG_SETTINGS_ERROR_GENERAL, + "%s (%d): %s", + pk_err ? polkit_error_get_error_name (pk_err) : "(unknown)", + pk_err ? polkit_error_get_error_code (pk_err) : -1, + pk_err ? polkit_error_get_error_message (pk_err) : "(unknown)"); + if (pk_err) + polkit_error_free (pk_err); /* PK 0.6's polkit_context_init() unrefs the global_context on failure */ #if (POLKIT_VERSION_MAJOR == 0) && (POLKIT_VERSION_MINOR >= 7) diff --git a/system-settings/src/nm-polkit-helpers.h b/system-settings/src/nm-polkit-helpers.h index cbdaede176..1382648c13 100644 --- a/system-settings/src/nm-polkit-helpers.h +++ b/system-settings/src/nm-polkit-helpers.h @@ -28,7 +28,7 @@ #define NM_SYSCONFIG_POLICY_ACTION "org.freedesktop.network-manager-settings.system.modify" -PolKitContext *create_polkit_context (void); +PolKitContext *create_polkit_context (GError **error); gboolean check_polkit_privileges (DBusGConnection *dbus_connection, PolKitContext *pol_ctx, DBusGMethodInvocation *context, diff --git a/system-settings/src/nm-sysconfig-connection.c b/system-settings/src/nm-sysconfig-connection.c index 09a3d9db48..c98c89ab1a 100644 --- a/system-settings/src/nm-sysconfig-connection.c +++ b/system-settings/src/nm-sysconfig-connection.c @@ -20,7 +20,9 @@ #include #include "nm-sysconfig-connection.h" +#include "nm-system-config-error.h" #include "nm-polkit-helpers.h" +#include "nm-dbus-glib-types.h" G_DEFINE_ABSTRACT_TYPE (NMSysconfigConnection, nm_sysconfig_connection, NM_TYPE_EXPORTED_CONNECTION) @@ -29,6 +31,8 @@ G_DEFINE_ABSTRACT_TYPE (NMSysconfigConnection, nm_sysconfig_connection, NM_TYPE_ typedef struct { DBusGConnection *dbus_connection; PolKitContext *pol_ctx; + + DBusGProxy *proxy; } NMSysconfigConnectionPrivate; static gboolean @@ -57,6 +61,231 @@ do_delete (NMExportedConnection *exported, GError **err) return check_polkit_privileges (priv->dbus_connection, priv->pol_ctx, context, err); } +static GValue * +string_to_gvalue (const char *str) +{ + GValue *val = g_slice_new0 (GValue); + + g_value_init (val, G_TYPE_STRING); + g_value_set_string (val, str); + return val; +} + +static void +copy_one_secret (gpointer key, gpointer value, gpointer user_data) +{ + const char *value_str = (const char *) value; + + if (value_str) { + g_hash_table_insert ((GHashTable *) user_data, + g_strdup ((char *) key), + string_to_gvalue (value_str)); + } +} + +static void +add_secrets (NMSetting *setting, + const char *key, + const GValue *value, + GParamFlags flags, + gpointer user_data) +{ + GHashTable *secrets = user_data; + + if (!(flags & NM_SETTING_PARAM_SECRET)) + return; + + if (G_VALUE_HOLDS_STRING (value)) { + const char *tmp; + + tmp = g_value_get_string (value); + if (tmp) + g_hash_table_insert (secrets, g_strdup (key), string_to_gvalue (tmp)); + } else if (G_VALUE_HOLDS (value, DBUS_TYPE_G_MAP_OF_STRING)) { + /* Flatten the string hash by pulling its keys/values out */ + g_hash_table_foreach (g_value_get_boxed (value), copy_one_secret, secrets); + } +} + +static void +destroy_gvalue (gpointer data) +{ + GValue *value = (GValue *) data; + + g_value_unset (value); + g_slice_free (GValue, value); +} + +static GHashTable * +real_get_secrets (NMSysconfigConnection *self, + const gchar *setting_name, + const gchar **hints, + gboolean request_new, + GError **error) +{ + NMConnection *connection; + GHashTable *settings = NULL; + GHashTable *secrets = NULL; + NMSetting *setting; + + connection = nm_exported_connection_get_connection (NM_EXPORTED_CONNECTION (self)); + setting = nm_connection_get_setting_by_name (connection, setting_name); + if (!setting) { + g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, + "%s.%d - Connection didn't have requested setting '%s'.", + __FILE__, __LINE__, setting_name); + return NULL; + } + + /* Returned secrets are a{sa{sv}}; this is the outer a{s...} hash that + * will contain all the individual settings hashes. + */ + settings = g_hash_table_new_full (g_str_hash, g_str_equal, + g_free, (GDestroyNotify) g_hash_table_destroy); + + /* Add the secrets from this setting to the inner secrets hash for this setting */ + secrets = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, destroy_gvalue); + nm_setting_enumerate_values (setting, add_secrets, secrets); + + g_hash_table_insert (settings, g_strdup (setting_name), secrets); + return settings; +} + +typedef struct { + NMSysconfigConnection *self; + char *setting_name; + DBusGMethodInvocation *context; +} GetUnixUserInfo; + +static GetUnixUserInfo * +get_unix_user_info_new (NMSysconfigConnection *self, + const char *setting_name, + DBusGMethodInvocation *context) +{ + GetUnixUserInfo *info; + + g_return_val_if_fail (self != NULL, NULL); + g_return_val_if_fail (setting_name != NULL, NULL); + g_return_val_if_fail (context != NULL, NULL); + + info = g_malloc0 (sizeof (GetUnixUserInfo)); + info->self = self; + info->setting_name = g_strdup (setting_name); + info->context = context; + return info; +} + +static void +get_unix_user_info_free (gpointer user_data) +{ + GetUnixUserInfo *info = user_data; + + g_free (info->setting_name); + g_free (info); +} + +static void +get_unix_user_cb (DBusGProxy *proxy, DBusGProxyCall *call, gpointer user_data) +{ + GetUnixUserInfo *info = user_data; + NMSysconfigConnection *self; + NMSysconfigConnectionPrivate *priv; + GError *error = NULL; + guint32 requestor_uid = G_MAXUINT32; + GHashTable *secrets; + + g_return_if_fail (info != NULL); + + self = info->self; + priv = NM_SYSCONFIG_CONNECTION_GET_PRIVATE (self); + + if (!dbus_g_proxy_end_call (proxy, call, &error, G_TYPE_UINT, &requestor_uid, G_TYPE_INVALID)) + goto error; + + /* Non-root users need PolicyKit authorization */ + if (requestor_uid != 0) { + if (!check_polkit_privileges (priv->dbus_connection, priv->pol_ctx, info->context, &error)) + goto error; + } + + secrets = real_get_secrets (self, info->setting_name, NULL, FALSE, &error); + if (secrets) { + /* success; return secrets to caller */ + dbus_g_method_return (info->context, secrets); + g_hash_table_destroy (secrets); + return; + } + + if (!error) { + /* Shouldn't happen, but... */ + g_set_error (&error, NM_SYSCONFIG_SETTINGS_ERROR, + NM_SYSCONFIG_SETTINGS_ERROR_GENERAL, + "%s", "Could not get secrets from connection (unknown error ocurred)"); + } + +error: + dbus_g_method_return_error (info->context, error); + g_clear_error (&error); +} + +static void +service_get_secrets (NMExportedConnection *exported, + const gchar *setting_name, + const gchar **hints, + gboolean request_new, + DBusGMethodInvocation *context) +{ + NMSysconfigConnection *self = NM_SYSCONFIG_CONNECTION (exported); + NMSysconfigConnectionPrivate *priv = NM_SYSCONFIG_CONNECTION_GET_PRIVATE (self); + GetUnixUserInfo *info; + GError *error = NULL; + char *sender = NULL; + + sender = dbus_g_method_get_sender (context); + if (!sender) { + g_set_error (&error, NM_SYSCONFIG_SETTINGS_ERROR, + NM_SYSCONFIG_SETTINGS_ERROR_GENERAL, + "%s", "Could not determine D-Bus requestor to authorize GetSecrets request"); + goto out; + } + + if (priv->proxy) + g_object_unref (priv->proxy); + + priv->proxy = dbus_g_proxy_new_for_name (priv->dbus_connection, + DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, + DBUS_INTERFACE_DBUS); + if (!priv->proxy) { + g_set_error (&error, NM_SYSCONFIG_SETTINGS_ERROR, + NM_SYSCONFIG_SETTINGS_ERROR_GENERAL, + "%s", "Could not connect to D-Bus to authorize GetSecrets request"); + goto out; + } + + info = get_unix_user_info_new (self, setting_name, context); + if (!info) { + g_set_error (&error, NM_SYSCONFIG_SETTINGS_ERROR, + NM_SYSCONFIG_SETTINGS_ERROR_GENERAL, + "%s", "Not enough memory to authorize GetSecrets request"); + goto out; + } + + dbus_g_proxy_begin_call_with_timeout (priv->proxy, "GetConnectionUnixUser", + get_unix_user_cb, + info, + get_unix_user_info_free, + 5000, + G_TYPE_STRING, sender, + G_TYPE_INVALID); + +out: + if (error) { + dbus_g_method_return_error (context, error); + g_error_free (error); + } +} + /* GObject */ static void @@ -67,25 +296,35 @@ nm_sysconfig_connection_init (NMSysconfigConnection *self) priv->dbus_connection = dbus_g_bus_get (DBUS_BUS_SYSTEM, &err); if (err) { - g_warning ("Could not get DBus connection: %s", err->message); + g_warning ("%s: error getting D-Bus connection: %s", + __func__, + (err && err->message) ? err->message : "(unknown)"); g_error_free (err); } - priv->pol_ctx = create_polkit_context (); + priv->pol_ctx = create_polkit_context (&err); + if (!priv->pol_ctx) { + g_warning ("%s: error creating PolicyKit context: %s", + __func__, + (err && err->message) ? err->message : "(unknown)"); + } } static void -finalize (GObject *object) +dispose (GObject *object) { NMSysconfigConnectionPrivate *priv = NM_SYSCONFIG_CONNECTION_GET_PRIVATE (object); + if (priv->proxy) + g_object_unref (priv->proxy); + if (priv->pol_ctx) polkit_context_unref (priv->pol_ctx); if (priv->dbus_connection) dbus_g_connection_unref (priv->dbus_connection); - G_OBJECT_CLASS (nm_sysconfig_connection_parent_class)->finalize (object); + G_OBJECT_CLASS (nm_sysconfig_connection_parent_class)->dispose (object); } static void @@ -97,8 +336,9 @@ nm_sysconfig_connection_class_init (NMSysconfigConnectionClass *sysconfig_connec g_type_class_add_private (sysconfig_connection_class, sizeof (NMSysconfigConnectionPrivate)); /* Virtual methods */ - object_class->finalize = finalize; + object_class->dispose = dispose; connection_class->update = update; connection_class->do_delete = do_delete; + connection_class->service_get_secrets = service_get_secrets; } diff --git a/system-settings/src/nm-system-settings.conf b/system-settings/src/nm-system-settings.conf index d09bc48c05..2f9a505434 100644 --- a/system-settings/src/nm-system-settings.conf +++ b/system-settings/src/nm-system-settings.conf @@ -12,9 +12,9 @@ - - + 512