Attachment 593044 Details for Bug 832532 – Drop all uses of polkit_unix_process_get_uid

[patch] Drop all uses of polkit_unix_process_get_uid

CVE-2012-2737.patch (text/plain), 17.04 KB, created by Ray Strode [halfline] on 2012-06-19 19:05:24 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Ray Strode [halfline]

Created: 2012-06-19 19:05:24 UTC

Size: 17.04 KB

patch

obsolete

>From 4da994eeab9a6b04938d84dd398d5781eb1fc94f Mon Sep 17 00:00:00 2001
>From: Ray Strode <rstrode@redhat.com>
>Date: Tue, 19 Jun 2012 12:02:24 -0400
>Subject: [PATCH 1/3] util: CVE-2012-2737: Validate SetIconFile caller over
> bus
>
>The AccountsService SetIconFile call associates an icon
>with a user.
>
>SetIconFile allows users to have icons visible at the login
>screen that don't necessarily originate in globally
>readable or always available locations. This is accomplished
>by copying the originating icon to the local disk in /var.
>
>Since AccountsService runs with with root privileges, the
>implemention of the SetIconFile method queries the uid of
>the method caller, forks, assumes that uid and performs
>the image copy as if it were the user.
>
>Unfortunately, the UID lookup peformed is done "just in time"
>instead of looking at peer credentials from the time the call
>was initiated. This is a race condition that means a caller
>could invoke the method call, quickly exec a setuid binary, and
>then cause the copy to be performed as the uid of the setuid
>process.
>
>This commit changes the uid lookup logic to query the system
>bus daemon for the peer credentials that were cached from the
>caller at the time of the initial connection.
>---
> src/util.c |   37 ++++++++++++++++++++++++++-----------
> 1 file changed, 26 insertions(+), 11 deletions(-)
>
>diff --git a/src/util.c b/src/util.c
>index 66ddd98..1ce375b 100644
>--- a/src/util.c
>+++ b/src/util.c
>@@ -251,22 +251,37 @@ get_user_groups (const gchar  *user,
> 
> 
> gboolean
>-get_caller_uid (GDBusMethodInvocation *context, gint *uid)
>+get_caller_uid (GDBusMethodInvocation *context,
>+                gint                  *uid)
> {
>-        PolkitSubject *subject;
>-        PolkitSubject *process;
>+        GVariant      *reply;
>+        GError        *error;
>+
>+        error = NULL;
>+        reply = g_dbus_connection_call_sync (g_dbus_method_invocation_get_connection (context),
>+                                             "org.freedesktop.DBus",
>+                                             "/org/freedesktop/DBus",
>+                                             "org.freedesktop.DBus",
>+                                             "GetConnectionUnixUser",
>+                                             g_variant_new ("(s)",
>+                                                            g_dbus_method_invocation_get_sender (context)),
>+                                             G_VARIANT_TYPE ("(u)"),
>+                                             G_DBUS_CALL_FLAGS_NONE,
>+                                             -1,
>+                                             NULL,
>+                                             &error);
>+
>+        if (reply == NULL) {
>+                g_warning ("Could not talk to message bus to find uid of sender %s: %s",
>+                           g_dbus_method_invocation_get_sender (context),
>+                           error->message);
>+                g_error_free (error);
> 
>-        subject = polkit_system_bus_name_new (g_dbus_method_invocation_get_sender (context));
>-        process = polkit_system_bus_name_get_process_sync (POLKIT_SYSTEM_BUS_NAME (subject), NULL, NULL);
>-        if (!process) {
>-                g_object_unref (subject);
>                 return FALSE;
>         }
> 
>-        *uid = polkit_unix_process_get_uid (POLKIT_UNIX_PROCESS (process));
>-
>-        g_object_unref (subject);
>-        g_object_unref (process);
>+        g_variant_get (reply, "(u)", uid);
>+        g_variant_unref (reply);
> 
>         return TRUE;
> }
>-- 
>1.7.10.2
>
>
>From b9eb2a51d7eda658ad104e0af0c8b41c3745f37a Mon Sep 17 00:00:00 2001
>From: Ray Strode <rstrode@redhat.com>
>Date: Tue, 19 Jun 2012 14:02:42 -0400
>Subject: [PATCH 2/3] user: CVE-2012-2737: verify caller through bus in more
> cases
>
>The previous commit changed the SetIconFile call to identify
>the uid of the calling process via cached peer credentials
>stored by the bus daemon.
>
>This commit fixes other similar cases where we try to figure
>out process identity on our own instead of through the bus
>daemon.
>---
> src/user.c |   78 ++++++++++++++++++++++++++++++++----------------------------
> 1 file changed, 42 insertions(+), 36 deletions(-)
>
>diff --git a/src/user.c b/src/user.c
>index 55c238d..9713ecd 100644
>--- a/src/user.c
>+++ b/src/user.c
>@@ -552,35 +552,21 @@ user_change_real_name_authorized_cb (Daemon                *daemon,
>         accounts_user_complete_set_real_name (ACCOUNTS_USER (user), context);
> }
> 
>-static uid_t
>-method_invocation_get_uid (GDBusMethodInvocation *context)
>-{
>-  const gchar *sender;
>-  PolkitSubject *busname;
>-  PolkitSubject *process;
>-  uid_t uid;
>-
>-  sender = g_dbus_method_invocation_get_sender (context);
>-  busname = polkit_system_bus_name_new (sender);
>-  process = polkit_system_bus_name_get_process_sync (POLKIT_SYSTEM_BUS_NAME (busname), NULL, NULL);
>-  uid = polkit_unix_process_get_uid (POLKIT_UNIX_PROCESS (process));
>-  g_object_unref (busname);
>-  g_object_unref (process);
>-
>-  return uid;
>-}
>-
> static gboolean
> user_set_real_name (AccountsUser          *auser,
>                     GDBusMethodInvocation *context,
>                     const gchar           *real_name)
> {
>         User *user = (User*)auser;
>-        uid_t uid;
>+        int uid;
>         const gchar *action_id;
> 
>-        uid = method_invocation_get_uid (context);
>-        if (user->uid == uid)
>+        if (!get_caller_uid (context, &uid)) {
>+                throw_error (context, ERROR_FAILED, "identifying caller failed");
>+                return FALSE;
>+        }
>+
>+        if (user->uid == (uid_t) uid)
>                 action_id = "org.freedesktop.accounts.change-own-user-data";
>         else
>                 action_id = "org.freedesktop.accounts.user-administration";
>@@ -692,11 +678,15 @@ user_set_email (AccountsUser          *auser,
>                 const gchar           *email)
> {
>         User *user = (User*)auser;
>-        uid_t uid;
>+        int uid;
>         const gchar *action_id;
> 
>-        uid = method_invocation_get_uid (context);
>-        if (user->uid == uid)
>+        if (!get_caller_uid (context, &uid)) {
>+                throw_error (context, ERROR_FAILED, "identifying caller failed");
>+                return FALSE;
>+        }
>+
>+        if (user->uid == (uid_t) uid)
>                 action_id = "org.freedesktop.accounts.change-own-user-data";
>         else
>                 action_id = "org.freedesktop.accounts.user-administration";
>@@ -744,11 +734,15 @@ user_set_language (AccountsUser          *auser,
>                    const gchar           *language)
> {
>         User *user = (User*)auser;
>-        uid_t uid;
>+        int uid;
>         const gchar *action_id;
> 
>-        uid = method_invocation_get_uid (context);
>-        if (user->uid == uid)
>+        if (!get_caller_uid (context, &uid)) {
>+                throw_error (context, ERROR_FAILED, "identifying caller failed");
>+                return FALSE;
>+        }
>+
>+        if (user->uid == (uid_t) uid)
>                 action_id = "org.freedesktop.accounts.change-own-user-data";
>         else
>                 action_id = "org.freedesktop.accounts.user-administration";
>@@ -794,11 +788,15 @@ user_set_x_session (AccountsUser          *auser,
>                     const gchar           *x_session)
> {
>         User *user = (User*)auser;
>-        uid_t uid;
>+        int uid;
>         const gchar *action_id;
> 
>-        uid = method_invocation_get_uid (context);
>-        if (user->uid == uid)
>+        if (!get_caller_uid (context, &uid)) {
>+                throw_error (context, ERROR_FAILED, "identifying caller failed");
>+                return FALSE;
>+        }
>+
>+        if (user->uid == (uid_t) uid)
>                 action_id = "org.freedesktop.accounts.change-own-user-data";
>         else
>                 action_id = "org.freedesktop.accounts.user-administration";
>@@ -844,11 +842,15 @@ user_set_location (AccountsUser          *auser,
>                    const gchar           *location)
> {
>         User *user = (User*)auser;
>-        uid_t uid;
>+        int uid;
>         const gchar *action_id;
> 
>-        uid = method_invocation_get_uid (context);
>-        if (user->uid == uid)
>+        if (!get_caller_uid (context, &uid)) {
>+                throw_error (context, ERROR_FAILED, "identifying caller failed");
>+                return FALSE;
>+        }
>+
>+        if (user->uid == (uid_t) uid)
>                 action_id = "org.freedesktop.accounts.change-own-user-data";
>         else
>                 action_id = "org.freedesktop.accounts.user-administration";
>@@ -1163,11 +1165,15 @@ user_set_icon_file (AccountsUser          *auser,
>                     const gchar           *filename)
> {
>         User *user = (User*)auser;
>-        uid_t uid;
>+        int uid;
>         const gchar *action_id;
> 
>-        uid = method_invocation_get_uid (context);
>-        if (user->uid == uid)
>+        if (!get_caller_uid (context, &uid)) {
>+                throw_error (context, ERROR_FAILED, "identifying caller failed");
>+                return FALSE;
>+        }
>+
>+        if (user->uid == (uid_t) uid)
>                 action_id = "org.freedesktop.accounts.change-own-user-data";
>         else
>                 action_id = "org.freedesktop.accounts.user-administration";
>-- 
>1.7.10.2
>
>
>From 02631411131c48f0616c94dc330f4146c3c25a8a Mon Sep 17 00:00:00 2001
>From: Ray Strode <rstrode@redhat.com>
>Date: Tue, 19 Jun 2012 14:34:18 -0400
>Subject: [PATCH 3/3] util: CVE-2012-2737: drop _polkit_subject_get_cmdline
>
>_polkit_subject_get_cmdline is a function copy and pasted
>from the polkit code that returns the command line, uid, and
>pid of a particular polkit subject.  It's used for helping to
>generate log entries that detail what processes are invoking methods
>on the accounts service.
>
>It's also used, on older kernels, for setting up the the loginuid
>of subprocesses that are run on behalf of AccountsService clients,
>so the audit trail leads back to the user initiating a request.
>
>_polkit_subject_get_cmdline directly looks up the uid of the caller,
>instead of querying the system bus.  As such, it's vulnerable to
>the same race condition discussed in the previous two commits.
>
>This commit guts _polkit_subject_get_cmdline, keeping only the part
>that reads /proc/pid/cmdline. We now get the uid and pid from the
>bus daemon.
>---
> src/util.c |  136 ++++++++++++++++++++++++++++++++++--------------------------
> 1 file changed, 77 insertions(+), 59 deletions(-)
>
>diff --git a/src/util.c b/src/util.c
>index 1ce375b..8f023cf 100644
>--- a/src/util.c
>+++ b/src/util.c
>@@ -34,11 +34,9 @@
> 
> #include "util.h"
> 
>-
> static gchar *
>-_polkit_subject_get_cmdline (PolkitSubject *subject, gint *pid, gint *uid)
>+get_cmdline_of_pid (GPid pid)
> {
>-  PolkitSubject *process;
>   gchar *ret;
>   gchar *filename;
>   gchar *contents;
>@@ -46,43 +44,7 @@ _polkit_subject_get_cmdline (PolkitSubject *subject, gint *pid, gint *uid)
>   GError *error;
>   guint n;
> 
>-  g_return_val_if_fail (subject != NULL, NULL);
>-
>-  error = NULL;
>-
>-  ret = NULL;
>-  process = NULL;
>-  filename = NULL;
>-  contents = NULL;
>-
>-  if (POLKIT_IS_UNIX_PROCESS (subject))
>-   {
>-      process = g_object_ref (subject);
>-    }
>-  else if (POLKIT_IS_SYSTEM_BUS_NAME (subject))
>-    {
>-      process = polkit_system_bus_name_get_process_sync (POLKIT_SYSTEM_BUS_NAME (subject),
>-                                                         NULL,
>-                                                         &error);
>-      if (process == NULL)
>-        {
>-          g_warning ("Error getting process for system bus name `%s': %s",
>-                     polkit_system_bus_name_get_name (POLKIT_SYSTEM_BUS_NAME (subject)),
>-                     error->message);
>-          g_error_free (error);
>-          goto out;
>-        }
>-    }
>-  else
>-    {
>-      g_warning ("Unknown subject type passed to guess_program_name()");
>-      goto out;
>-    }
>-
>-  *pid = polkit_unix_process_get_pid (POLKIT_UNIX_PROCESS (process));
>-  *uid = polkit_unix_process_get_uid (POLKIT_UNIX_PROCESS (process));
>-
>-  filename = g_strdup_printf ("/proc/%d/cmdline", *pid);
>+  filename = g_strdup_printf ("/proc/%d/cmdline", (int) pid);
> 
>   if (!g_file_get_contents (filename,
>                             &contents,
>@@ -108,11 +70,49 @@ _polkit_subject_get_cmdline (PolkitSubject *subject, gint *pid, gint *uid)
>  out:
>   g_free (filename);
>   g_free (contents);
>-  if (process != NULL)
>-    g_object_unref (process);
>+
>   return ret;
> }
> 
>+static gboolean
>+get_caller_pid (GDBusMethodInvocation *context,
>+                GPid                  *pid)
>+{
>+        GVariant      *reply;
>+        GError        *error;
>+        guint32        pid_as_int;
>+
>+        error = NULL;
>+        reply = g_dbus_connection_call_sync (g_dbus_method_invocation_get_connection (context),
>+                                             "org.freedesktop.DBus",
>+                                             "/org/freedesktop/DBus",
>+                                             "org.freedesktop.DBus",
>+                                             "GetConnectionUnixProcessID",
>+                                             g_variant_new ("(s)",
>+                                                            g_dbus_method_invocation_get_sender (context)),
>+                                             G_VARIANT_TYPE ("(u)"),
>+                                             G_DBUS_CALL_FLAGS_NONE,
>+                                             -1,
>+                                             NULL,
>+                                             &error);
>+
>+        if (reply == NULL) {
>+                g_warning ("Could not talk to message bus to find uid of sender %s: %s",
>+                           g_dbus_method_invocation_get_sender (context),
>+                           error->message);
>+                g_error_free (error);
>+
>+                return FALSE;
>+        }
>+
>+        g_variant_get (reply, "(u)", &pid_as_int);
>+        *pid = pid_as_int;
>+
>+        g_variant_unref (reply);
>+
>+        return TRUE;
>+}
>+
> void
> sys_log (GDBusMethodInvocation *context,
>          const gchar           *format,
>@@ -127,21 +127,36 @@ sys_log (GDBusMethodInvocation *context,
> 
>         if (context) {
>                 PolkitSubject *subject;
>-                gchar *cmdline;
>+                gchar *cmdline = NULL;
>                 gchar *id;
>-                gint pid = 0;
>-                gint uid = 0;
>+                GPid pid = 0;
>+                gint uid = -1;
>                 gchar *tmp;
> 
>                 subject = polkit_system_bus_name_new (g_dbus_method_invocation_get_sender (context));
>                 id = polkit_subject_to_string (subject);
>-                cmdline = _polkit_subject_get_cmdline (subject, &pid, &uid);
> 
>-                if (cmdline == NULL) {
>-                        tmp = g_strdup_printf ("request by %s: %s", id, msg);
>+                if (get_caller_pid (context, &pid)) {
>+                        cmdline = get_cmdline_of_pid (pid);
>+                } else {
>+                        pid = 0;
>+                        cmdline = NULL;
>                 }
>-                else {
>-                        tmp = g_strdup_printf ("request by %s [%s pid:%d uid:%d]: %s", id, cmdline, pid, uid, msg);
>+
>+                if (cmdline != NULL) {
>+                        if (get_caller_uid (context, &uid)) {
>+                                tmp = g_strdup_printf ("request by %s [%s pid:%d uid:%d]: %s", id, cmdline, (int) pid, uid, msg);
>+                        } else {
>+                                tmp = g_strdup_printf ("request by %s [%s pid:%d]: %s", id, cmdline, (int) pid, msg);
>+                        }
>+                } else {
>+                        if (get_caller_uid (context, &uid) && pid != 0) {
>+                                tmp = g_strdup_printf ("request by %s [pid:%d uid:%d]: %s", id, (int) pid, uid, msg);
>+                        } else if (pid != 0) {
>+                                tmp = g_strdup_printf ("request by %s [pid:%d]: %s", id, (int) pid, msg);
>+                        } else {
>+                                tmp = g_strdup_printf ("request by %s: %s", id, msg);
>+                        }
>                 }
> 
>                 g_free (msg);
>@@ -160,20 +175,22 @@ sys_log (GDBusMethodInvocation *context,
> static void
> get_caller_loginuid (GDBusMethodInvocation *context, gchar *loginuid, gint size)
> {
>-        PolkitSubject *subject;
>-        gchar *cmdline;
>-        gint pid;
>+        GPid pid;
>         gint uid;
>         gchar *path;
>         gchar *buf;
> 
>-        subject = polkit_system_bus_name_new (g_dbus_method_invocation_get_sender (context));
>-        cmdline = _polkit_subject_get_cmdline (subject, &pid, &uid);
>-        g_free (cmdline);
>-        g_object_unref (subject);
>+        if (!get_caller_uid (context, &uid)) {
>+                uid = getuid ();
>+        }
>+
>+        if (get_caller_pid (context, &pid)) {
>+                path = g_strdup_printf ("/proc/%d/loginuid", (int) pid);
>+        } else {
>+                path = NULL;
>+        }
> 
>-        path = g_strdup_printf ("/proc/%d/loginuid", pid);
>-        if (g_file_get_contents (path, &buf, NULL, NULL)) {
>+        if (path != NULL && g_file_get_contents (path, &buf, NULL, NULL)) {
>                 strncpy (loginuid, buf, size);
>                 g_free (buf);
>         }
>@@ -285,3 +302,4 @@ get_caller_uid (GDBusMethodInvocation *context,
> 
>         return TRUE;
> }
>+
>-- 
>1.7.10.2
>

Actions: View | Diff

Attachments on bug 832532: 593003 | 593044