From 9df2abea53065c88878f00a467811cd635d3c6a8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 4 May 2019 10:31:18 +0200 Subject: [PATCH] auth-chain: don't clone the permission string for AuthChain Out of the 33 callers of nm_auth_chain_add_call(), the permission argument is: - 29 times a C string literal like NM_AUTH_PERMISSION_NETWORK_CONTROL. - 3 times assign a string that is in fact a static string (it's just not a string literal) - only NMManager's device_auth_request_cb() passes a permission of (possibly) non static origin. But it already duplicates the string for it's own purposes and attaches it as user-data to the NMAuthChain. There really is no need to duplicate the string. Replace nm_auth_chain_add_call() by a macro that ensures that the permission string is a C literal. Rename nm_auth_chain_add_call() to nm_auth_chain_add_call_unsafe() to indicate that the lifetime of the permission argument is now the responsibility of the caller. --- src/nm-auth-utils.c | 47 +++++++++++++++++++++++++-------- src/nm-auth-utils.h | 9 ++++--- src/nm-manager.c | 9 ++++--- src/settings/nm-agent-manager.c | 2 +- src/settings/nm-settings.c | 4 +-- 5 files changed, 51 insertions(+), 20 deletions(-) diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index b1b3d7042e..2e61ad2060 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -51,8 +51,8 @@ typedef struct { CList auth_call_lst; NMAuthChain *chain; NMAuthManagerCallId *call_id; + const char *permission; NMAuthCallResult result; - char permission[]; } AuthCall; /*****************************************************************************/ @@ -66,6 +66,7 @@ _ASSERT_call (AuthCall *call) { nm_assert (call); nm_assert (call->chain); + nm_assert (call->permission && strlen (call->permission) > 0); nm_assert (nm_c_list_contains_entry (&call->chain->auth_call_lst_head, call, auth_call_lst)); #if NM_MORE_ASSERTS > 5 { @@ -321,14 +322,30 @@ pk_call_cb (NMAuthManager *auth_manager, } } +/** + * nm_auth_chain_add_call_unsafe: + * @self: the #NMAuthChain + * @permission: the permission string. This string is kept by reference + * and you must make sure that it's lifetime lasts until the NMAuthChain + * gets destroyed. That's why the function is "unsafe". Use + * nm_auth_chain_add_call() instead. + * @allow_interaction: flag + * + * It's "unsafe" because @permission is not copied. It's the callers responsibility + * that the permission string stays valid as long as NMAuthChain. + * + * If you can, use nm_auth_chain_add_call() instead! + * + * If you have a non-static string, you may attach the permission string as + * user-data via nm_auth_chain_set_data(). + */ void -nm_auth_chain_add_call (NMAuthChain *self, - const char *permission, - gboolean allow_interaction) +nm_auth_chain_add_call_unsafe (NMAuthChain *self, + const char *permission, + gboolean allow_interaction) { AuthCall *call; NMAuthManager *auth_manager = nm_auth_manager_get (); - gsize l_p_1; g_return_if_fail (self); g_return_if_fail (self->subject); @@ -342,12 +359,16 @@ nm_auth_chain_add_call (NMAuthChain *self, * can only return one-permission. */ nm_assert (!_find_auth_call (self, permission)); - l_p_1 = strlen (permission) + 1; - call = g_malloc (sizeof (AuthCall) + l_p_1); - call->chain = self; - call->call_id = NULL; - call->result = NM_AUTH_CALL_RESULT_UNKNOWN; - memcpy (call->permission, permission, l_p_1); + call = g_slice_new (AuthCall); + + *call = (AuthCall) { + .chain = self, + .call_id = NULL, + .result = NM_AUTH_CALL_RESULT_UNKNOWN, + + /* we don't clone the permission string. It's the callers responsiblity. */ + .permission = permission, + }; /* above we assert that no duplicate permissions are added. Still, track the * new request to the front of the list so that it would shadow an earlier @@ -463,6 +484,10 @@ _auth_chain_destroy (NMAuthChain *self) nm_clear_g_object (&self->subject); nm_clear_g_object (&self->context); + /* we must first destry all AuthCall instances before ChainData. The reason is + * that AuthData.permission is not cloned and the lifetime of the string must + * be ensured by the caller. A sensible thing to do for the caller is attach the + * permission string via nm_auth_chain_set_data(). Hence, first free the AuthCall. */ while ((call = c_list_first_entry (&self->auth_call_lst_head, AuthCall, auth_call_lst))) auth_call_free (call); diff --git a/src/nm-auth-utils.h b/src/nm-auth-utils.h index 58180bc4b1..e482af7f58 100644 --- a/src/nm-auth-utils.h +++ b/src/nm-auth-utils.h @@ -52,9 +52,12 @@ void nm_auth_chain_set_data (NMAuthChain *chain, NMAuthCallResult nm_auth_chain_get_result (NMAuthChain *chain, const char *permission); -void nm_auth_chain_add_call (NMAuthChain *chain, - const char *permission, - gboolean allow_interaction); +void nm_auth_chain_add_call_unsafe (NMAuthChain *chain, + const char *permission, + gboolean allow_interaction); + +#define nm_auth_chain_add_call(chain, permission, allow_interaction) \ + nm_auth_chain_add_call_unsafe ((chain), ""permission"", (allow_interaction)) void nm_auth_chain_destroy (NMAuthChain *chain); diff --git a/src/nm-manager.c b/src/nm-manager.c index b35761b2b3..0ce15892fa 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -2385,6 +2385,7 @@ device_auth_request_cb (NMDevice *device, GError *error = NULL; NMAuthSubject *subject = NULL; NMAuthChain *chain; + char *permission_dup; /* Validate the caller */ subject = nm_auth_subject_new_unix_process_from_context (context); @@ -2413,12 +2414,14 @@ device_auth_request_cb (NMDevice *device, goto done; } + permission_dup = g_strdup (permission); + priv->auth_chains = g_slist_append (priv->auth_chains, chain); nm_auth_chain_set_data (chain, "device", g_object_ref (device), g_object_unref); - nm_auth_chain_set_data (chain, "requested-permission", g_strdup (permission), g_free); nm_auth_chain_set_data (chain, "callback", callback, NULL); nm_auth_chain_set_data (chain, "user-data", user_data, NULL); - nm_auth_chain_add_call (chain, permission, allow_interaction); + nm_auth_chain_set_data (chain, "requested-permission", permission_dup /* transfer ownership */, g_free); + nm_auth_chain_add_call_unsafe (chain, permission_dup, allow_interaction); done: if (error) @@ -6844,7 +6847,7 @@ nm_manager_dbus_set_property_handle (NMDBusObject *obj, chain = nm_auth_chain_new_subject (subject, invocation, _dbus_set_property_auth_cb, handle_data); priv->auth_chains = g_slist_append (priv->auth_chains, chain); - nm_auth_chain_add_call (chain, property_info->writable.permission, TRUE); + nm_auth_chain_add_call_unsafe (chain, property_info->writable.permission, TRUE); return; err: diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index 73f59c0b3c..a9fc56d3ff 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -1073,7 +1073,7 @@ _con_get_request_start (Request *req) perm = NM_AUTH_PERMISSION_SETTINGS_MODIFY_SYSTEM; nm_auth_chain_set_data (req->con.chain, "perm", (gpointer) perm, NULL); - nm_auth_chain_add_call (req->con.chain, perm, TRUE); + nm_auth_chain_add_call_unsafe (req->con.chain, perm, TRUE); } else { _LOGD (NULL, "("LOG_REQ_FMT") requesting user-owned secrets from agent %s", LOG_REQ_ARG (req), agent_dbus_owner); diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index 23ae87984d..2476caa117 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -1274,7 +1274,7 @@ nm_settings_add_connection_dbus (NMSettings *self, * request affects more than just the caller, require 'modify.system'. */ s_con = nm_connection_get_setting_connection (connection); - g_assert (s_con); + nm_assert (s_con); if (nm_setting_connection_get_num_permissions (s_con) == 1) perm = NM_AUTH_PERMISSION_SETTINGS_MODIFY_OWN; else @@ -1296,7 +1296,7 @@ nm_settings_add_connection_dbus (NMSettings *self, nm_auth_chain_set_data (chain, "callback-data", user_data, NULL); nm_auth_chain_set_data (chain, "subject", g_object_ref (subject), g_object_unref); nm_auth_chain_set_data (chain, "save-to-disk", GUINT_TO_POINTER (save_to_disk), NULL); - nm_auth_chain_add_call (chain, perm, TRUE); + nm_auth_chain_add_call_unsafe (chain, perm, TRUE); return; done: