Attachment 116398 Details for Bug 90832 – 0001-CVE-XXX-XXXX-Use-unpredictable-cookie-values-keep-th.patch

[patch] 0001-CVE-XXX-XXXX-Use-unpredictable-cookie-values-keep-th.patch

0001-CVE-XXX-XXXX-Use-unpredictable-cookie-values-keep-th.patch (text/plain), 18.00 KB, created by Miloslav Trmac on 2015-06-09 18:02:10 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Miloslav Trmac

Created: 2015-06-09 18:02:10 UTC

Size: 18.00 KB

patch

obsolete

>From 46d1a01694268a86aa155a66c9cc757e8d136661 Mon Sep 17 00:00:00 2001
>From: Colin Walters <walters@redhat.com>
>Date: Thu, 4 Jun 2015 12:15:18 -0400
>Subject: [PATCH] CVE-XXX-XXXX: Use unpredictable cookie values, keep them
> secret
>
>Tavis noted that it'd be possible with a 32 bit counter for someone to
>cause the cookie to wrap by creating Authentication requests in a
>loop.
>
>Something important to note here is that wrapping of signed integers
>is undefined behavior in C, so we definitely want to fix that.  All
>counter integers used in this patch are unsigned.
>
>See the comment above `authentication_agent_generate_cookie` for
>details, but basically we're now using a cookie of the form:
>
>        <agent serial> - <agent random id> - <session serial> - <session
>random id>
>
>Which has multiple 64 bit counters, plus unpredictable random 128 bit
>integer ids (effectively UUIDs, but we're not calling them that
>because we don't need to be globally unique.
>
>We further ensure that the cookies are not visible to other processes
>by changing the setuid helper to accept them over standard input.  This
>means that an attacker would have to guess both ids.
>
>In any case, the security hole here is better fixed with the other
>change to bind user id (uid) of the agent with cookie lookups, making
>cookie guessing worthless.
>
>Nevertheless, I think it's worth doing this change too, for defense in
>depth.
>
>Bug: https://bugs.freedesktop.org/show_bug.cgi?id=90832
>Reported-by: Tavis Ormandy <taviso@google.com>
>Signed-off-by: Colin Walters <walters@redhat.com>
>---
> configure.ac                                       |  2 +-
> src/polkitagent/polkitagenthelper-pam.c            | 12 ++-
> src/polkitagent/polkitagenthelper-shadow.c         | 12 ++-
> src/polkitagent/polkitagenthelperprivate.c         | 33 ++++++++
> src/polkitagent/polkitagenthelperprivate.h         |  2 +
> src/polkitagent/polkitagentsession.c               | 30 ++++---
> .../polkitbackendinteractiveauthority.c            | 99 +++++++++++++++++-----
> 7 files changed, 150 insertions(+), 40 deletions(-)
>
>diff --git a/configure.ac b/configure.ac
>index d9a97bb..91cc627 100644
>--- a/configure.ac
>+++ b/configure.ac
>@@ -121,7 +121,7 @@ if test "x$GCC" = "xyes"; then
>   changequote([,])dnl
> fi
> 
>-PKG_CHECK_MODULES(GLIB, [gmodule-2.0 gio-2.0 >= 2.30.0])
>+PKG_CHECK_MODULES(GLIB, [gmodule-2.0 gio-unix-2.0 >= 2.30.0])
> AC_SUBST(GLIB_CFLAGS)
> AC_SUBST(GLIB_LIBS)
> AC_DEFINE([GLIB_VERSION_MIN_REQUIRED], [GLIB_VERSION_2_30],
>diff --git a/src/polkitagent/polkitagenthelper-pam.c b/src/polkitagent/polkitagenthelper-pam.c
>index 937386e..19062aa 100644
>--- a/src/polkitagent/polkitagenthelper-pam.c
>+++ b/src/polkitagent/polkitagenthelper-pam.c
>@@ -65,7 +65,7 @@ main (int argc, char *argv[])
> {
>   int rc;
>   const char *user_to_auth;
>-  const char *cookie;
>+  char *cookie = NULL;
>   struct pam_conv pam_conversation;
>   pam_handle_t *pam_h;
>   const void *authed_user;
>@@ -97,7 +97,7 @@ main (int argc, char *argv[])
>   openlog ("polkit-agent-helper-1", LOG_CONS | LOG_PID, LOG_AUTHPRIV);
> 
>   /* check for correct invocation */
>-  if (argc != 3)
>+  if (!(argc == 2 || argc == 3))
>     {
>       syslog (LOG_NOTICE, "inappropriate use of helper, wrong number of arguments [uid=%d]", getuid ());
>       fprintf (stderr, "polkit-agent-helper-1: wrong number of arguments. This incident has been logged.\n");
>@@ -105,7 +105,10 @@ main (int argc, char *argv[])
>     }
> 
>   user_to_auth = argv[1];
>-  cookie = argv[2];
>+
>+  cookie = read_cookie (argc, argv);
>+  if (!cookie)
>+    goto error;
> 
>   if (getuid () != 0)
>     {
>@@ -203,6 +206,8 @@ main (int argc, char *argv[])
>       goto error;
>     }
> 
>+  free (cookie);
>+
> #ifdef PAH_DEBUG
>   fprintf (stderr, "polkit-agent-helper-1: successfully sent D-Bus message to PolicyKit daemon\n");
> #endif /* PAH_DEBUG */
>@@ -212,6 +217,7 @@ main (int argc, char *argv[])
>   return 0;
> 
> error:
>+  free (cookie);
>   if (pam_h != NULL)
>     pam_end (pam_h, rc);
> 
>diff --git a/src/polkitagent/polkitagenthelper-shadow.c b/src/polkitagent/polkitagenthelper-shadow.c
>index a4f73ac..e877915 100644
>--- a/src/polkitagent/polkitagenthelper-shadow.c
>+++ b/src/polkitagent/polkitagenthelper-shadow.c
>@@ -46,7 +46,7 @@ main (int argc, char *argv[])
> {
>   struct spwd *shadow;
>   const char *user_to_auth;
>-  const char *cookie;
>+  char *cookie = NULL;
>   time_t now;
> 
>   /* clear the entire environment to avoid attacks with
>@@ -67,7 +67,7 @@ main (int argc, char *argv[])
>   openlog ("polkit-agent-helper-1", LOG_CONS | LOG_PID, LOG_AUTHPRIV);
> 
>   /* check for correct invocation */
>-  if (argc != 3)
>+  if (!(argc == 2 || argc == 3))
>     {
>       syslog (LOG_NOTICE, "inappropriate use of helper, wrong number of arguments [uid=%d]", getuid ());
>       fprintf (stderr, "polkit-agent-helper-1: wrong number of arguments. This incident has been logged.\n");
>@@ -86,7 +86,10 @@ main (int argc, char *argv[])
>     }
> 
>   user_to_auth = argv[1];
>-  cookie = argv[2];
>+
>+  cookie = read_cookie (argc, argv);
>+  if (!cookie)
>+    goto error;
> 
> #ifdef PAH_DEBUG
>   fprintf (stderr, "polkit-agent-helper-1: user to auth is '%s'.\n", user_to_auth);
>@@ -153,6 +156,8 @@ main (int argc, char *argv[])
>       goto error;
>     }
> 
>+  free (cookie);
>+
> #ifdef PAH_DEBUG
>   fprintf (stderr, "polkit-agent-helper-1: successfully sent D-Bus message to PolicyKit daemon\n");
> #endif /* PAH_DEBUG */
>@@ -162,6 +167,7 @@ main (int argc, char *argv[])
>   return 0;
> 
> error:
>+  free (cookie);
>   fprintf (stdout, "FAILURE\n");
>   flush_and_wait ();
>   return 1;
>diff --git a/src/polkitagent/polkitagenthelperprivate.c b/src/polkitagent/polkitagenthelperprivate.c
>index cfa77fc..6c881cc 100644
>--- a/src/polkitagent/polkitagenthelperprivate.c
>+++ b/src/polkitagent/polkitagenthelperprivate.c
>@@ -23,6 +23,7 @@
> #include "config.h"
> #include "polkitagenthelperprivate.h"
> #include <stdio.h>
>+#include <string.h>
> #include <stdlib.h>
> #include <unistd.h>
> 
>@@ -45,6 +46,38 @@ _polkit_clearenv (void)
> #endif
> 
> 
>+char *
>+read_cookie (int argc, char **argv)
>+{
>+  /* As part of CVE-XXX-XXXX, we started passing the cookie
>+   * on standard input, to ensure it's not visible to other
>+   * processes.  However, to ensure that things continue
>+   * to work if the setuid binary is upgraded while old
>+   * agents are still running (this will be common with
>+   * package managers), we support both modes.
>+   */
>+  if (argc == 3)
>+    return strdup (argv[2]);
>+  else
>+    {
>+      char *ret = NULL;
>+      size_t n = 0;
>+      ssize_t r = getline (&ret, &n, stdin);
>+      if (r == -1)
>+        {
>+          if (!feof (stdin))
>+            perror ("getline");
>+          free (ret);
>+          return NULL;
>+        }
>+      else
>+        {
>+          g_strchomp (ret);
>+          return ret;
>+        }
>+    }
>+}
>+
> gboolean
> send_dbus_message (const char *cookie, const char *user)
> {
>diff --git a/src/polkitagent/polkitagenthelperprivate.h b/src/polkitagent/polkitagenthelperprivate.h
>index aeca2c7..547fdcc 100644
>--- a/src/polkitagent/polkitagenthelperprivate.h
>+++ b/src/polkitagent/polkitagenthelperprivate.h
>@@ -38,6 +38,8 @@
> 
> int _polkit_clearenv (void);
> 
>+char *read_cookie (int argc, char **argv);
>+
> gboolean send_dbus_message (const char *cookie, const char *user);
> 
> void flush_and_wait ();
>diff --git a/src/polkitagent/polkitagentsession.c b/src/polkitagent/polkitagentsession.c
>index f014773..8b93ad0 100644
>--- a/src/polkitagent/polkitagentsession.c
>+++ b/src/polkitagent/polkitagentsession.c
>@@ -55,6 +55,7 @@
> #include <stdio.h>
> #include <sys/types.h>
> #include <sys/wait.h>
>+#include <gio/gunixoutputstream.h>
> #include <pwd.h>
> 
> #include "polkitagentmarshal.h"
>@@ -88,7 +89,7 @@ struct _PolkitAgentSession
>   gchar *cookie;
>   PolkitIdentity *identity;
> 
>-  int child_stdin;
>+  GOutputStream *child_stdin;
>   int child_stdout;
>   GPid child_pid;
> 
>@@ -129,7 +130,6 @@ G_DEFINE_TYPE (PolkitAgentSession, polkit_agent_session, G_TYPE_OBJECT);
> static void
> polkit_agent_session_init (PolkitAgentSession *session)
> {
>-  session->child_stdin = -1;
>   session->child_stdout = -1;
> }
> 
>@@ -395,11 +395,7 @@ kill_helper (PolkitAgentSession *session)
>       session->child_stdout = -1;
>     }
> 
>-  if (session->child_stdin != -1)
>-    {
>-      g_warn_if_fail (close (session->child_stdin) == 0);
>-      session->child_stdin = -1;
>-    }
>+  g_clear_object (&session->child_stdin);
> 
>   session->helper_is_running = FALSE;
> 
>@@ -545,9 +541,9 @@ polkit_agent_session_response (PolkitAgentSession *session,
> 
>   add_newline = (response[response_len] != '\n');
> 
>-  write (session->child_stdin, response, response_len);
>+  (void) g_output_stream_write_all (session->child_stdin, response, response_len, NULL, NULL, NULL);
>   if (add_newline)
>-    write (session->child_stdin, newline, 1);
>+    (void) g_output_stream_write_all (session->child_stdin, newline, 1, NULL, NULL, NULL);
> }
> 
> /**
>@@ -567,8 +563,9 @@ polkit_agent_session_initiate (PolkitAgentSession *session)
> {
>   uid_t uid;
>   GError *error;
>-  gchar *helper_argv[4];
>+  gchar *helper_argv[3];
>   struct passwd *passwd;
>+  int stdin_fd = -1;
> 
>   g_return_if_fail (POLKIT_AGENT_IS_SESSION (session));
> 
>@@ -600,10 +597,8 @@ polkit_agent_session_initiate (PolkitAgentSession *session)
> 
>   helper_argv[0] = PACKAGE_PREFIX "/lib/polkit-1/polkit-agent-helper-1";
>   helper_argv[1] = passwd->pw_name;
>-  helper_argv[2] = session->cookie;
>-  helper_argv[3] = NULL;
>+  helper_argv[2] = NULL;
> 
>-  session->child_stdin = -1;
>   session->child_stdout = -1;
> 
>   error = NULL;
>@@ -615,7 +610,7 @@ polkit_agent_session_initiate (PolkitAgentSession *session)
>                                  NULL,
>                                  NULL,
>                                  &session->child_pid,
>-                                 &session->child_stdin,
>+                                 &stdin_fd,
>                                  &session->child_stdout,
>                                  NULL,
>                                  &error))
>@@ -628,6 +623,13 @@ polkit_agent_session_initiate (PolkitAgentSession *session)
>   if (G_UNLIKELY (_show_debug ()))
>     g_print ("PolkitAgentSession: spawned helper with pid %d\n", (gint) session->child_pid);
> 
>+  session->child_stdin = (GOutputStream*)g_unix_output_stream_new (stdin_fd, TRUE);
>+
>+  /* Write the cookie on stdin so it can't be seen by other processes */
>+  (void) g_output_stream_write_all (session->child_stdin, session->cookie, strlen (session->cookie),
>+                                    NULL, NULL, NULL);
>+  (void) g_output_stream_write_all (session->child_stdin, "\n", 1, NULL, NULL, NULL);
>+
>   session->child_stdout_channel = g_io_channel_unix_new (session->child_stdout);
>   session->child_stdout_watch_source = g_io_create_watch (session->child_stdout_channel,
>                                                           G_IO_IN | G_IO_ERR | G_IO_HUP);
>diff --git a/src/polkitbackend/polkitbackendinteractiveauthority.c b/src/polkitbackend/polkitbackendinteractiveauthority.c
>index dab2756..ee0e27b 100644
>--- a/src/polkitbackend/polkitbackendinteractiveauthority.c
>+++ b/src/polkitbackend/polkitbackendinteractiveauthority.c
>@@ -214,6 +214,8 @@ typedef struct
> 
>   GDBusConnection *system_bus_connection;
>   guint name_owner_changed_signal_id;
>+
>+  guint64 agent_serial;
> } PolkitBackendInteractiveAuthorityPrivate;
> 
> /* ---------------------------------------------------------------------------------------------------- */
>@@ -431,11 +433,15 @@ struct AuthenticationAgent
>   volatile gint ref_count;
> 
>   PolkitSubject *scope;
>+  guint64 serial;
> 
>   gchar *locale;
>   GVariant *registration_options;
>   gchar *object_path;
>   gchar *unique_system_bus_name;
>+  GRand *cookie_pool;
>+  gchar *cookie_prefix;
>+  guint64  cookie_serial;
> 
>   GDBusProxy *proxy;
> 
>@@ -1419,9 +1425,54 @@ authentication_session_cancelled_cb (GCancellable *cancellable,
>   authentication_session_cancel (session);
> }
> 
>+/* We're not calling this a UUID, but it's basically
>+ * the same thing, just not formatted that way because:
>+ *
>+ *  - I'm too lazy to do it
>+ *  - If we did, people might think it was actually
>+ *    generated from /dev/random, which we're not doing
>+ *    because this value doesn't actually need to be
>+ *    globally unique.
>+ */
>+static void
>+append_rand_u128_str (GString *buf,
>+                      GRand   *pool)
>+{
>+  g_string_append_printf (buf, "%08x%08x%08x%08x",
>+                          g_rand_int (pool),
>+                          g_rand_int (pool),
>+                          g_rand_int (pool),
>+                          g_rand_int (pool));
>+}
>+
>+/* A value that should be unique to the (AuthenticationAgent, AuthenticationSession)
>+ * pair, and not guessable by other agents.
>+ *
>+ * <agent serial> - <agent uuid> - <session serial> - <session uuid>
>+ *
>+ * See http://lists.freedesktop.org/archives/polkit-devel/2015-June/000425.html
>+ *
>+ */
>+static gchar *
>+authentication_agent_generate_cookie (AuthenticationAgent *agent)
>+{
>+  GString *buf = g_string_new ("");
>+
>+  g_string_append (buf, agent->cookie_prefix);
>+  
>+  g_string_append_c (buf, '-');
>+  agent->cookie_serial++;
>+  g_string_append_printf (buf, "%" G_GUINT64_FORMAT, 
>+                          agent->cookie_serial);
>+  g_string_append_c (buf, '-');
>+  append_rand_u128_str (buf, agent->cookie_pool);
>+
>+  return g_string_free (buf, FALSE);
>+}
>+
>+
> static AuthenticationSession *
> authentication_session_new (AuthenticationAgent         *agent,
>-                            const gchar                 *cookie,
>                             PolkitSubject               *subject,
>                             PolkitIdentity              *user_of_subject,
>                             PolkitSubject               *caller,
>@@ -1439,7 +1490,7 @@ authentication_session_new (AuthenticationAgent         *agent,
> 
>   session = g_new0 (AuthenticationSession, 1);
>   session->agent = authentication_agent_ref (agent);
>-  session->cookie = g_strdup (cookie);
>+  session->cookie = authentication_agent_generate_cookie (agent);
>   session->subject = g_object_ref (subject);
>   session->user_of_subject = g_object_ref (user_of_subject);
>   session->caller = g_object_ref (caller);
>@@ -1488,16 +1539,6 @@ authentication_session_free (AuthenticationSession *session)
>   g_free (session);
> }
> 
>-static gchar *
>-authentication_agent_new_cookie (AuthenticationAgent *agent)
>-{
>-  static gint counter = 0;
>-
>-  /* TODO: use a more random-looking cookie */
>-
>-  return g_strdup_printf ("cookie%d", counter++);
>-}
>-
> static PolkitSubject *
> authentication_agent_get_scope (AuthenticationAgent *agent)
> {
>@@ -1545,12 +1586,15 @@ authentication_agent_unref (AuthenticationAgent *agent)
>       g_free (agent->unique_system_bus_name);
>       if (agent->registration_options != NULL)
>         g_variant_unref (agent->registration_options);
>+      g_rand_free (agent->cookie_pool);
>+      g_free (agent->cookie_prefix);
>       g_free (agent);
>     }
> }
> 
> static AuthenticationAgent *
>-authentication_agent_new (PolkitSubject *scope,
>+authentication_agent_new (guint64      serial,
>+                          PolkitSubject *scope,
>                           const gchar *unique_system_bus_name,
>                           const gchar *locale,
>                           const gchar *object_path,
>@@ -1584,6 +1628,7 @@ authentication_agent_new (PolkitSubject *scope,
> 
>   agent = g_new0 (AuthenticationAgent, 1);
>   agent->ref_count = 1;
>+  agent->serial = serial;
>   agent->scope = g_object_ref (scope);
>   agent->object_path = g_strdup (object_path);
>   agent->unique_system_bus_name = g_strdup (unique_system_bus_name);
>@@ -1591,6 +1636,25 @@ authentication_agent_new (PolkitSubject *scope,
>   agent->registration_options = registration_options != NULL ? g_variant_ref (registration_options) : NULL;
>   agent->proxy = proxy;
> 
>+  {
>+    GString *cookie_prefix = g_string_new ("");
>+    GRand *agent_private_rand = g_rand_new ();
>+
>+    g_string_append_printf (cookie_prefix, "%" G_GUINT64_FORMAT "-", agent->serial);
>+
>+    /* Use a uniquely seeded PRNG to get a prefix cookie for this agent,
>+     * whose sequence will not correlate with the per-authentication session
>+     * cookies.
>+     */
>+    append_rand_u128_str (cookie_prefix, agent_private_rand);
>+    g_rand_free (agent_private_rand);
>+
>+    agent->cookie_prefix = g_string_free (cookie_prefix, FALSE);
>+    
>+    /* And a newly seeded pool for per-session cookies */
>+    agent->cookie_pool = g_rand_new ();
>+  }
>+
>   return agent;
> }
> 
>@@ -2185,7 +2249,6 @@ authentication_agent_initiate_challenge (AuthenticationAgent         *agent,
> {
>   PolkitBackendInteractiveAuthorityPrivate *priv = POLKIT_BACKEND_INTERACTIVE_AUTHORITY_GET_PRIVATE (authority);
>   AuthenticationSession *session;
>-  gchar *cookie;
>   GList *l;
>   GList *identities;
>   gchar *localized_message;
>@@ -2207,8 +2270,6 @@ authentication_agent_initiate_challenge (AuthenticationAgent         *agent,
>                                     &localized_icon_name,
>                                     &localized_details);
> 
>-  cookie = authentication_agent_new_cookie (agent);
>-
>   identities = NULL;
> 
>   /* select admin user if required by the implicit authorization */
>@@ -2271,7 +2332,6 @@ authentication_agent_initiate_challenge (AuthenticationAgent         *agent,
>     user_identities = g_list_prepend (NULL, polkit_unix_user_new (0));
> 
>   session = authentication_session_new (agent,
>-                                        cookie,
>                                         subject,
>                                         user_of_subject,
>                                         caller,
>@@ -2327,7 +2387,6 @@ authentication_agent_initiate_challenge (AuthenticationAgent         *agent,
>   g_list_free_full (user_identities, g_object_unref);
>   g_list_foreach (identities, (GFunc) g_object_unref, NULL);
>   g_list_free (identities);
>-  g_free (cookie);
> 
>   g_free (localized_message);
>   g_free (localized_icon_name);
>@@ -2474,7 +2533,9 @@ polkit_backend_interactive_authority_register_authentication_agent (PolkitBacken
>       goto out;
>     }
> 
>-  agent = authentication_agent_new (subject,
>+  priv->agent_serial++;
>+  agent = authentication_agent_new (priv->agent_serial,
>+                                    subject,
>                                     polkit_system_bus_name_get_name (POLKIT_SYSTEM_BUS_NAME (caller)),
>                                     locale,
>                                     object_path,
>-- 
>2.4.2
>

Actions: View | Splinter Review

Attachments on bug 90832: 116268 | 116293 | 116294 | 116372 | 116398

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.