From 3aedc94fa66d5dd98d645de4f6042b92c22b608d Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Wed, 11 Jun 2014 13:24:10 -0500 Subject: [PATCH 1/4] keyfile: fix use-after-free and refcounting of invalid changed connections If a valid connection was updated and still valid, and then was updated and become invalid, the connection would not be properly removed from the keyfile plugin's priv->connections hash, and thus would never be disposed. This was due to using the direct pointer to the connection's UUID as the key for the hash table. When a connection is updated and its settings are replaced, the old UUID is freed and replaced with a new pointer. But the keyfile plugin hash table still uses the old (now freed) UUID pointer as the key. Thus when the connection is updated and becomes invalid, looking up the UUID in the hash table fails to find the connection, and the connection is not removed from the hash. This bug could cause a crash in some cases, if two keys of the GHashTable hashed to the same value, in which case GLib would call g_str_equal() on the freed pointer. Since code other than in the keyfile plugin replaces settings, we cannot be guaranteed that the pointer won't change. Avoid all that and just strdup() the UUID when using it as a key. (also collapses _internal_new_connection() into its only caller) --- src/settings/plugins/keyfile/plugin.c | 54 ++++++++++++--------------- 1 file changed, 23 insertions(+), 31 deletions(-) diff --git a/src/settings/plugins/keyfile/plugin.c b/src/settings/plugins/keyfile/plugin.c index bad1fd75c7..e1b690a82f 100644 --- a/src/settings/plugins/keyfile/plugin.c +++ b/src/settings/plugins/keyfile/plugin.c @@ -77,43 +77,26 @@ connection_removed_cb (NMSettingsConnection *obj, gpointer user_data) nm_connection_get_uuid (NM_CONNECTION (obj))); } -static NMSettingsConnection * -_internal_new_connection (SCPluginKeyfile *self, - const char *full_path, - NMConnection *source, - GError **error) -{ - SCPluginKeyfilePrivate *priv = SC_PLUGIN_KEYFILE_GET_PRIVATE (self); - NMKeyfileConnection *connection; - - connection = nm_keyfile_connection_new (source, full_path, error); - if (connection) { - g_hash_table_insert (priv->connections, - (gpointer) nm_connection_get_uuid (NM_CONNECTION (connection)), - connection); - g_signal_connect (connection, NM_SETTINGS_CONNECTION_REMOVED, - G_CALLBACK (connection_removed_cb), - self); - } - - return (NMSettingsConnection *) connection; -} - /* Monitoring */ static void remove_connection (SCPluginKeyfile *self, NMKeyfileConnection *connection) { + gboolean removed; + g_return_if_fail (connection != NULL); nm_log_info (LOGD_SETTINGS, "removed %s.", nm_keyfile_connection_get_path (connection)); /* Removing from the hash table should drop the last reference */ g_object_ref (connection); - g_hash_table_remove (SC_PLUGIN_KEYFILE_GET_PRIVATE (self)->connections, - nm_connection_get_uuid (NM_CONNECTION (connection))); + g_signal_handlers_disconnect_by_func (connection, connection_removed_cb, self); + removed = g_hash_table_remove (SC_PLUGIN_KEYFILE_GET_PRIVATE (self)->connections, + nm_connection_get_uuid (NM_CONNECTION (connection))); nm_settings_connection_signal_remove (NM_SETTINGS_CONNECTION (connection)); g_object_unref (connection); + + g_return_if_fail (removed); } static void @@ -174,6 +157,7 @@ new_connection (SCPluginKeyfile *self, SCPluginKeyfilePrivate *priv = SC_PLUGIN_KEYFILE_GET_PRIVATE (self); NMKeyfileConnection *tmp, *connection; GError *error = NULL; + const char *uuid; if (out_old_path) *out_old_path = NULL; @@ -187,7 +171,8 @@ new_connection (SCPluginKeyfile *self, } /* Connection renames will show as different paths but same UUID */ - connection = g_hash_table_lookup (priv->connections, nm_connection_get_uuid (NM_CONNECTION (tmp))); + uuid = nm_connection_get_uuid (NM_CONNECTION (tmp)); + connection = g_hash_table_lookup (priv->connections, uuid); if (connection) { nm_log_info (LOGD_SETTINGS, "rename %s -> %s", nm_keyfile_connection_get_path (connection), name); if (!nm_settings_connection_replace_settings (NM_SETTINGS_CONNECTION (connection), @@ -203,9 +188,7 @@ new_connection (SCPluginKeyfile *self, nm_keyfile_connection_set_path (connection, name); } else { nm_log_info (LOGD_SETTINGS, "new connection %s", name); - g_hash_table_insert (priv->connections, - (gpointer) nm_connection_get_uuid (NM_CONNECTION (tmp)), - tmp); + g_hash_table_insert (priv->connections, g_strdup (uuid), tmp); g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_CONNECTION_ADDED, tmp); g_signal_connect (tmp, NM_SETTINGS_CONNECTION_REMOVED, @@ -445,6 +428,7 @@ add_connection (NMSystemConfigInterface *config, GError **error) { SCPluginKeyfile *self = SC_PLUGIN_KEYFILE (config); + SCPluginKeyfilePrivate *priv = SC_PLUGIN_KEYFILE_GET_PRIVATE (self); NMSettingsConnection *added = NULL; char *path = NULL; @@ -452,8 +436,16 @@ add_connection (NMSystemConfigInterface *config, if (!nm_keyfile_plugin_write_connection (connection, NULL, &path, error)) return NULL; } - - added = _internal_new_connection (self, path, connection, error); + + added = (NMSettingsConnection *) nm_keyfile_connection_new (connection, path, error); + if (added) { + g_hash_table_insert (priv->connections, + g_strdup (nm_connection_get_uuid (NM_CONNECTION (added))), + added); + g_signal_connect (added, NM_SETTINGS_CONNECTION_REMOVED, + G_CALLBACK (connection_removed_cb), + self); + } g_free (path); return added; } @@ -615,7 +607,7 @@ sc_plugin_keyfile_init (SCPluginKeyfile *plugin) { SCPluginKeyfilePrivate *priv = SC_PLUGIN_KEYFILE_GET_PRIVATE (plugin); - priv->connections = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, g_object_unref); + priv->connections = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_object_unref); } static void From 95e4b6fc1eda7f5b0019f0abf5b6b69fdf563763 Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Wed, 11 Jun 2014 13:32:00 -0500 Subject: [PATCH 2/4] keyfile: clean up logging connection verify errors Prevents: Connection failed to verify: (unknown) invalid or missing connection property 'blah blah/foo bar' Simply removing the warning in reader.c is fine, because callers that care already log the warning themselves. Also make the warning in update_connection() the same as the warning in new_connection(). --- src/settings/plugins/keyfile/plugin.c | 3 ++- src/settings/plugins/keyfile/reader.c | 2 -- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/settings/plugins/keyfile/plugin.c b/src/settings/plugins/keyfile/plugin.c index e1b690a82f..10c8397180 100644 --- a/src/settings/plugins/keyfile/plugin.c +++ b/src/settings/plugins/keyfile/plugin.c @@ -110,7 +110,8 @@ update_connection (SCPluginKeyfile *self, tmp = nm_keyfile_connection_new (NULL, name, &error); if (!tmp) { /* Error; remove the connection */ - nm_log_warn (LOGD_SETTINGS, " %s", (error && error->message) ? error->message : "(unknown)"); + nm_log_warn (LOGD_SETTINGS, " error in connection %s: %s", name, + (error && error->message) ? error->message : "(unknown)"); g_clear_error (&error); remove_connection (self, connection); return; diff --git a/src/settings/plugins/keyfile/reader.c b/src/settings/plugins/keyfile/reader.c index 7fdfbee964..83090f37aa 100644 --- a/src/settings/plugins/keyfile/reader.c +++ b/src/settings/plugins/keyfile/reader.c @@ -1316,8 +1316,6 @@ nm_keyfile_plugin_connection_from_file (const char *filename, GError **error) g_clear_error (&verify_error); g_object_unref (connection); connection = NULL; - nm_log_warn (LOGD_SETTINGS, "Connection failed to verify: %s", - verify_error ? g_type_name (nm_connection_lookup_setting_type_by_quark (verify_error->domain)) : "(unknown)"); } out: From 72fb1f897fff6cb272fbc7a67d195cffe969f6db Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Wed, 11 Jun 2014 13:37:23 -0500 Subject: [PATCH 3/4] example: fix possible invalid refcounting when changing connections Since the pointer to the connection's path could change any time commit_changes() is called, it's not safe to use it as the hash table key directly. strdup it instead. --- src/settings/plugins/example/plugin.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/settings/plugins/example/plugin.c b/src/settings/plugins/example/plugin.c index 15c7bfe281..eea9494608 100644 --- a/src/settings/plugins/example/plugin.c +++ b/src/settings/plugins/example/plugin.c @@ -137,7 +137,7 @@ _internal_new_connection (SCPluginExample *self, connection = nm_example_connection_new (full_path, source, error); if (connection) { g_hash_table_insert (priv->connections, - (gpointer) nm_example_connection_get_path (connection), + g_strdup (nm_example_connection_get_path (connection)), connection); } @@ -356,7 +356,7 @@ dir_changed (GFileMonitor *monitor, /* Re-insert the connection back into the hash with the new filename */ g_hash_table_insert (priv->connections, - (gpointer) nm_example_connection_get_path (found), + g_strdup (nm_example_connection_get_path (found)), found); /* Get rid of the temporary connection */ @@ -364,7 +364,7 @@ dir_changed (GFileMonitor *monitor, } else { /* Completely new connection, not a rename. */ g_hash_table_insert (priv->connections, - (gpointer) nm_example_connection_get_path (connection), + g_strdup (nm_example_connection_get_path (connection)), connection); /* Tell NM we found a new connection */ g_signal_emit_by_name (config, NM_SYSTEM_CONFIG_INTERFACE_CONNECTION_ADDED, connection); @@ -435,7 +435,7 @@ setup_monitoring (NMSystemConfigInterface *config) /* Initialize connection hash here; we use the connection hash as the * "are we initialized yet" variable. */ - priv->connections = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, g_object_unref); + priv->connections = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_object_unref); if (nm_config_get_monitor_connection_files (nm_config_get ())) { /* Set up the watch for our config directory */ From c44556a1d5998d3210fbb44686bb594122c523eb Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Wed, 11 Jun 2014 13:40:19 -0500 Subject: [PATCH 4/4] ifnet: fix use-after-free and refcounting of invalid changed connections If a valid connection was updated and still valid, and then was updated and become invalid, the connection would not be properly removed from the ifnet plugin's priv->connections hash, and thus would never be disposed. This was due to using the direct pointer to the connection's UUID as the key for the hash table. When a connection is updated and its settings are replaced, the old UUID is freed and replaced with a new pointer. But the ifnet plugin hash table still uses the old (now freed) UUID pointer as the key. Thus when the connection is updated and becomes invalid, looking up the UUID in the hash table fails to find the connection, and the connection is not removed from the hash. This bug could cause a crash in some cases, if two keys of the GHashTable hashed to the same value, in which case GLib would call g_str_equal() on the freed pointer. Since code other than in the ifnet plugin replaces settings, we cannot be guaranteed that the pointer won't change. Avoid all that and just strdup() the UUID when using it as a key. --- src/settings/plugins/ifnet/plugin.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/settings/plugins/ifnet/plugin.c b/src/settings/plugins/ifnet/plugin.c index bc7b35c22c..ab07b8d627 100644 --- a/src/settings/plugins/ifnet/plugin.c +++ b/src/settings/plugins/ifnet/plugin.c @@ -231,7 +231,7 @@ static void track_new_connection (SCPluginIfnet *self, NMIfnetConnection *connection) { g_hash_table_insert (SC_PLUGIN_IFNET_GET_PRIVATE (self)->connections, - (gpointer) nm_connection_get_uuid (NM_CONNECTION (connection)), + g_strdup (nm_connection_get_uuid (NM_CONNECTION (connection))), g_object_ref (connection)); g_signal_connect (connection, NM_SETTINGS_CONNECTION_REMOVED, G_CALLBACK (connection_removed_cb), @@ -432,7 +432,7 @@ init (NMSystemConfigInterface *config) nm_log_info (LOGD_SETTINGS, "Initializing!"); - priv->connections = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, g_object_unref); + priv->connections = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_object_unref); priv->unmanaged_well_known = !is_managed_plugin (); nm_log_info (LOGD_SETTINGS, "management mode: %s", priv->unmanaged_well_known ? "unmanaged" : "managed");