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>

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,