From 26213aa0e0d8dca5f36cc23f6942525224cbe9f5 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, switches to 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. There 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 bd51aa4cdac380f55d607f4ffdf2ab3c00d08721 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 4c5b12e363410e490e776e4b4a86dcce157a543d 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 |  135 ++++++++++++++++++++++++++++++++++--------------------------

 1 file changed, 76 insertions(+), 59 deletions(-)

diff --git a/src/util.c b/src/util.c

index 1ce375b..adc559a 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);

         }

--

1.7.10.2