Attachment 116268 Details for Bug 90832 – 0001-authority-Avoid-cookie-wrapping-by-using-u64-counter.patch

[patch] 0001-authority-Avoid-cookie-wrapping-by-using-u64-counter.patch

0001-authority-Avoid-cookie-wrapping-by-using-u64-counter.patch (text/plain), 3.63 KB, created by Colin Walters on 2015-06-03 19:06:17 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Colin Walters

Created: 2015-06-03 19:06:17 UTC

Size: 3.63 KB

patch

obsolete

>From 87b2290c03f28841594451c7276e0ca44970c1fe Mon Sep 17 00:00:00 2001
>From: Colin Walters <walters@redhat.com>
>Date: Wed, 3 Jun 2015 14:35:42 -0400
>Subject: [PATCH] authority: Avoid cookie wrapping by using u64 counter
>
>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.
>
>My current belief is that the most effect this could have would be
>that in the case where there are two active sessions, Mallory could
>cause Alice's authorization checks to spuriously fail if she managed
>to create a request with a duplicate cookie.
>
>Something important to note here is that wrapping of signed integers
>is undefined behavior in C, so we definitely want to fix that.  The
>cookie counter is now unsigned.
>
>I chose to extend the counter to 64 bits, add the polkit process start
>time as well as an additional random 32 bits.  At this point we're not
>too far from just having a UUID, but one thing I do like about the
>counter approach is that it is process scoped, we don't actually need
>something "universally" unique.
>
>Reported-by: Tavis Ormandy <taviso@google.com>
>Signed-off-by: Colin Walters <walters@redhat.com>
>---
> .../polkitbackendinteractiveauthority.c            | 29 ++++++++++++++++------
> 1 file changed, 22 insertions(+), 7 deletions(-)
>
>diff --git a/src/polkitbackend/polkitbackendinteractiveauthority.c b/src/polkitbackend/polkitbackendinteractiveauthority.c
>index 59028d5..f6ea0fc 100644
>--- a/src/polkitbackend/polkitbackendinteractiveauthority.c
>+++ b/src/polkitbackend/polkitbackendinteractiveauthority.c
>@@ -214,6 +214,9 @@ typedef struct
> 
>   GDBusConnection *system_bus_connection;
>   guint name_owner_changed_signal_id;
>+
>+  guint64 start_time;
>+  guint64 cookie;
> } PolkitBackendInteractiveAuthorityPrivate;
> 
> /* ---------------------------------------------------------------------------------------------------- */
>@@ -328,6 +331,8 @@ polkit_backend_interactive_authority_init (PolkitBackendInteractiveAuthority *au
>                                             authority,
>                                             NULL); /* GDestroyNotify */
>     }
>+
>+  priv->start_time = g_get_monotonic_time ();
> }
> 
> static void
>@@ -1485,14 +1490,24 @@ authentication_session_free (AuthenticationSession *session)
>   g_free (session);
> }
> 
>+/*
>+ * Generate a value that is used to identify authentication requests.
>+ * This doesn't need to be protected against active forgery - callers
>+ * will have to also match the agent identity.
>+ *
>+ * It'd probably make sense to just use a UUID, we're just not doing
>+ * that for lack of a convenient API.  This code is an evolution
>+ * of older code which used a single process-local 32 bit counter.
>+ */
> static gchar *
>-authentication_agent_new_cookie (AuthenticationAgent *agent)
>+get_new_cookie (PolkitBackendInteractiveAuthority *authority)
> {
>-  static gint counter = 0;
>-
>-  /* TODO: use a more random-looking cookie */
>-
>-  return g_strdup_printf ("cookie%d", counter++);
>+  PolkitBackendInteractiveAuthorityPrivate *priv =
>+    POLKIT_BACKEND_INTERACTIVE_AUTHORITY_GET_PRIVATE (authority);
>+  guint32 rv = g_random_int ();
>+  priv->cookie++;
>+  return g_strdup_printf ("cookie-%" G_GUINT64_FORMAT "-%" G_GUINT64_FORMAT "-%u",
>+                          priv->start_time, priv->cookie, rv);
> }
> 
> static PolkitSubject *
>@@ -2198,7 +2213,7 @@ authentication_agent_initiate_challenge (AuthenticationAgent         *agent,
>                                     &localized_icon_name,
>                                     &localized_details);
> 
>-  cookie = authentication_agent_new_cookie (agent);
>+  cookie = get_new_cookie (authority);
> 
>   identities = NULL;
> 
>-- 
>1.8.3.1
>

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.