diff options
author | Colin Walters <walters@redhat.com> | 2015-06-17 13:07:02 -0400 |
---|---|---|
committer | Colin Walters <walters@verbum.org> | 2015-06-17 13:18:17 -0400 |
commit | 493aa5dc1d278ab9097110c1262f5229bbaf1766 (patch) | |
tree | cfb67093a0c2dd23eec6fa5c5954ecd717809b49 /src | |
parent | ea544ffc18405237ccd95d28d7f45afef49aca17 (diff) | |
download | polkit-493aa5dc1d278ab9097110c1262f5229bbaf1766.tar.gz |
CVE-2015-4625: Bind use of cookies to specific uids
http://lists.freedesktop.org/archives/polkit-devel/2015-June/000425.html
The "cookie" value that Polkit hands out is global to all polkit
users. And when `AuthenticationAgentResponse` is invoked, we
previously only received the cookie and *target* identity, and
attempted to find an agent from that.
The problem is that the current cookie is just an integer
counter, and if it overflowed, it would be possible for
an successful authorization in one session to trigger a response
in another session.
The overflow and ability to guess the cookie were fixed by the
previous patch.
This patch is conceptually further hardening on top of that. Polkit
currently treats uids as equivalent from a security domain
perspective; there is no support for
SELinux/AppArmor/etc. differentiation.
We can retrieve the uid from `getuid()` in the setuid helper, which
allows us to ensure the uid invoking `AuthenticationAgentResponse2`
matches that of the agent.
Then the authority only looks at authentication sessions matching the
cookie that were created by a matching uid, thus removing the ability
for different uids to interfere with each other entirely.
Several fixes to this patch were contributed by:
Miloslav Trmač <mitr@redhat.com>
Bug: https://bugs.freedesktop.org/show_bug.cgi?id=90837
CVE: CVE-2015-4625
Reported-by: Tavis Ormandy <taviso@google.com>
Reviewed-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Colin Walters <walters@redhat.com>
Diffstat (limited to 'src')
-rw-r--r-- | src/polkit/polkitauthority.c | 13 | ||||
-rw-r--r-- | src/polkitbackend/polkitbackendauthority.c | 61 | ||||
-rw-r--r-- | src/polkitbackend/polkitbackendauthority.h | 2 | ||||
-rw-r--r-- | src/polkitbackend/polkitbackendinteractiveauthority.c | 39 |
4 files changed, 107 insertions, 8 deletions
diff --git a/src/polkit/polkitauthority.c b/src/polkit/polkitauthority.c index ab6d3cd..6bd684a 100644 --- a/src/polkit/polkitauthority.c +++ b/src/polkit/polkitauthority.c @@ -1492,6 +1492,14 @@ polkit_authority_authentication_agent_response (PolkitAuthority *authority, gpointer user_data) { GVariant *identity_value; + /* Note that in reality, this API is only accessible to root, and + * only called from the setuid helper `polkit-agent-helper-1`. + * + * However, because this is currently public API, we avoid + * triggering warnings from ABI diff type programs by just grabbing + * the real uid of the caller here. + */ + uid_t uid = getuid (); g_return_if_fail (POLKIT_IS_AUTHORITY (authority)); g_return_if_fail (cookie != NULL); @@ -1501,8 +1509,9 @@ polkit_authority_authentication_agent_response (PolkitAuthority *authority, identity_value = polkit_identity_to_gvariant (identity); g_variant_ref_sink (identity_value); g_dbus_proxy_call (authority->proxy, - "AuthenticationAgentResponse", - g_variant_new ("(s@(sa{sv}))", + "AuthenticationAgentResponse2", + g_variant_new ("(us@(sa{sv}))", + (guint32)uid, cookie, identity_value), G_DBUS_CALL_FLAGS_NONE, diff --git a/src/polkitbackend/polkitbackendauthority.c b/src/polkitbackend/polkitbackendauthority.c index 601a974..03a4e84 100644 --- a/src/polkitbackend/polkitbackendauthority.c +++ b/src/polkitbackend/polkitbackendauthority.c @@ -355,6 +355,7 @@ polkit_backend_authority_unregister_authentication_agent (PolkitBackendAuthority gboolean polkit_backend_authority_authentication_agent_response (PolkitBackendAuthority *authority, PolkitSubject *caller, + uid_t uid, const gchar *cookie, PolkitIdentity *identity, GError **error) @@ -373,7 +374,7 @@ polkit_backend_authority_authentication_agent_response (PolkitBackendAuthority } else { - return klass->authentication_agent_response (authority, caller, cookie, identity, error); + return klass->authentication_agent_response (authority, caller, uid, cookie, identity, error); } } @@ -587,6 +588,11 @@ static const gchar *server_introspection_data = " <arg type='s' name='cookie' direction='in'/>" " <arg type='(sa{sv})' name='identity' direction='in'/>" " </method>" + " <method name='AuthenticationAgentResponse2'>" + " <arg type='u' name='uid' direction='in'/>" + " <arg type='s' name='cookie' direction='in'/>" + " <arg type='(sa{sv})' name='identity' direction='in'/>" + " </method>" " <method name='EnumerateTemporaryAuthorizations'>" " <arg type='(sa{sv})' name='subject' direction='in'/>" " <arg type='a(ss(sa{sv})tt)' name='temporary_authorizations' direction='out'/>" @@ -1035,6 +1041,57 @@ server_handle_authentication_agent_response (Server *server, error = NULL; if (!polkit_backend_authority_authentication_agent_response (server->authority, caller, + (uid_t)-1, + cookie, + identity, + &error)) + { + g_dbus_method_invocation_return_gerror (invocation, error); + g_error_free (error); + goto out; + } + + g_dbus_method_invocation_return_value (invocation, g_variant_new ("()")); + + out: + if (identity != NULL) + g_object_unref (identity); +} + +static void +server_handle_authentication_agent_response2 (Server *server, + GVariant *parameters, + PolkitSubject *caller, + GDBusMethodInvocation *invocation) +{ + const gchar *cookie; + GVariant *identity_gvariant; + PolkitIdentity *identity; + GError *error; + guint32 uid; + + identity = NULL; + + g_variant_get (parameters, + "(u&s@(sa{sv}))", + &uid, + &cookie, + &identity_gvariant); + + error = NULL; + identity = polkit_identity_new_for_gvariant (identity_gvariant, &error); + if (identity == NULL) + { + g_prefix_error (&error, "Error getting identity: "); + g_dbus_method_invocation_return_gerror (invocation, error); + g_error_free (error); + goto out; + } + + error = NULL; + if (!polkit_backend_authority_authentication_agent_response (server->authority, + caller, + (uid_t)uid, cookie, identity, &error)) @@ -1222,6 +1279,8 @@ server_handle_method_call (GDBusConnection *connection, server_handle_unregister_authentication_agent (server, parameters, caller, invocation); else if (g_strcmp0 (method_name, "AuthenticationAgentResponse") == 0) server_handle_authentication_agent_response (server, parameters, caller, invocation); + else if (g_strcmp0 (method_name, "AuthenticationAgentResponse2") == 0) + server_handle_authentication_agent_response2 (server, parameters, caller, invocation); else if (g_strcmp0 (method_name, "EnumerateTemporaryAuthorizations") == 0) server_handle_enumerate_temporary_authorizations (server, parameters, caller, invocation); else if (g_strcmp0 (method_name, "RevokeTemporaryAuthorizations") == 0) diff --git a/src/polkitbackend/polkitbackendauthority.h b/src/polkitbackend/polkitbackendauthority.h index f9f7385..88df82e 100644 --- a/src/polkitbackend/polkitbackendauthority.h +++ b/src/polkitbackend/polkitbackendauthority.h @@ -147,6 +147,7 @@ struct _PolkitBackendAuthorityClass gboolean (*authentication_agent_response) (PolkitBackendAuthority *authority, PolkitSubject *caller, + uid_t uid, const gchar *cookie, PolkitIdentity *identity, GError **error); @@ -249,6 +250,7 @@ gboolean polkit_backend_authority_unregister_authentication_agent (PolkitBackend gboolean polkit_backend_authority_authentication_agent_response (PolkitBackendAuthority *authority, PolkitSubject *caller, + uid_t uid, const gchar *cookie, PolkitIdentity *identity, GError **error); diff --git a/src/polkitbackend/polkitbackendinteractiveauthority.c b/src/polkitbackend/polkitbackendinteractiveauthority.c index 15adc6a..96725f7 100644 --- a/src/polkitbackend/polkitbackendinteractiveauthority.c +++ b/src/polkitbackend/polkitbackendinteractiveauthority.c @@ -108,8 +108,9 @@ static AuthenticationAgent *get_authentication_agent_for_subject (PolkitBackendI PolkitSubject *subject); -static AuthenticationSession *get_authentication_session_for_cookie (PolkitBackendInteractiveAuthority *authority, - const gchar *cookie); +static AuthenticationSession *get_authentication_session_for_uid_and_cookie (PolkitBackendInteractiveAuthority *authority, + uid_t uid, + const gchar *cookie); static GList *get_authentication_sessions_initiated_by_system_bus_unique_name (PolkitBackendInteractiveAuthority *authority, const gchar *system_bus_unique_name); @@ -169,6 +170,7 @@ static gboolean polkit_backend_interactive_authority_unregister_authentication_a static gboolean polkit_backend_interactive_authority_authentication_agent_response (PolkitBackendAuthority *authority, PolkitSubject *caller, + uid_t uid, const gchar *cookie, PolkitIdentity *identity, GError **error); @@ -440,6 +442,7 @@ struct AuthenticationAgent { volatile gint ref_count; + uid_t creator_uid; PolkitSubject *scope; guint64 serial; @@ -1603,6 +1606,7 @@ authentication_agent_unref (AuthenticationAgent *agent) static AuthenticationAgent * authentication_agent_new (guint64 serial, PolkitSubject *scope, + PolkitIdentity *creator, const gchar *unique_system_bus_name, const gchar *locale, const gchar *object_path, @@ -1611,6 +1615,10 @@ authentication_agent_new (guint64 serial, { AuthenticationAgent *agent; GDBusProxy *proxy; + PolkitUnixUser *creator_user; + + g_assert (POLKIT_IS_UNIX_USER (creator)); + creator_user = POLKIT_UNIX_USER (creator); if (!g_variant_is_object_path (object_path)) { @@ -1638,6 +1646,7 @@ authentication_agent_new (guint64 serial, agent->ref_count = 1; agent->serial = serial; agent->scope = g_object_ref (scope); + agent->creator_uid = (uid_t)polkit_unix_user_get_uid (creator_user); agent->object_path = g_strdup (object_path); agent->unique_system_bus_name = g_strdup (unique_system_bus_name); agent->locale = g_strdup (locale); @@ -1736,8 +1745,9 @@ get_authentication_agent_for_subject (PolkitBackendInteractiveAuthority *authori } static AuthenticationSession * -get_authentication_session_for_cookie (PolkitBackendInteractiveAuthority *authority, - const gchar *cookie) +get_authentication_session_for_uid_and_cookie (PolkitBackendInteractiveAuthority *authority, + uid_t uid, + const gchar *cookie) { PolkitBackendInteractiveAuthorityPrivate *priv; GHashTableIter hash_iter; @@ -1755,6 +1765,23 @@ get_authentication_session_for_cookie (PolkitBackendInteractiveAuthority *author { GList *l; + /* We need to ensure that if somehow we have duplicate cookies + * due to wrapping, that the cookie used is matched to the user + * who called AuthenticationAgentResponse2. See + * http://lists.freedesktop.org/archives/polkit-devel/2015-June/000425.html + * + * Except if the legacy AuthenticationAgentResponse is invoked, + * we don't know the uid and hence use -1. Continue to support + * the old behavior for backwards compatibility, although everyone + * who is using our own setuid helper will automatically be updated + * to the new API. + */ + if (uid != (uid_t)-1) + { + if (agent->creator_uid != uid) + continue; + } + for (l = agent->active_sessions; l != NULL; l = l->next) { AuthenticationSession *session = l->data; @@ -2544,6 +2571,7 @@ polkit_backend_interactive_authority_register_authentication_agent (PolkitBacken priv->agent_serial++; agent = authentication_agent_new (priv->agent_serial, subject, + user_of_caller, polkit_system_bus_name_get_name (POLKIT_SYSTEM_BUS_NAME (caller)), locale, object_path, @@ -2757,6 +2785,7 @@ polkit_backend_interactive_authority_unregister_authentication_agent (PolkitBack static gboolean polkit_backend_interactive_authority_authentication_agent_response (PolkitBackendAuthority *authority, PolkitSubject *caller, + uid_t uid, const gchar *cookie, PolkitIdentity *identity, GError **error) @@ -2799,7 +2828,7 @@ polkit_backend_interactive_authority_authentication_agent_response (PolkitBacken } /* find the authentication session */ - session = get_authentication_session_for_cookie (interactive_authority, cookie); + session = get_authentication_session_for_uid_and_cookie (interactive_authority, uid, cookie); if (session == NULL) { g_set_error (error, |