PolicyKit: Branch 'master' - 2 commits
Colin Walters
walters at kemper.freedesktop.org
Wed Jun 3 12:57:51 PDT 2015
src/polkitbackend/polkitbackendinteractiveauthority.c | 82 +++++++++++-------
1 file changed, 52 insertions(+), 30 deletions(-)
New commits:
commit 48e646918efb2bf0b3b505747655726d7869f31c
Author: Colin Walters <walters at redhat.com>
Date: Sat May 30 09:06:23 2015 -0400
CVE-2015-3218: backend: Handle invalid object paths in RegisterAuthenticationAgent
Properly propagate the error, otherwise we dereference a `NULL`
pointer. This is a local, authenticated DoS.
`RegisterAuthenticationAgentWithOptions` and
`UnregisterAuthentication` have been validated to not need changes for
this.
http://lists.freedesktop.org/archives/polkit-devel/2015-May/000420.html
https://bugs.freedesktop.org/show_bug.cgi?id=90829
Reported-by: Tavis Ormandy <taviso at google.com>
Reviewed-by: Philip Withnall <philip at tecnocode.co.uk>
Reviewed-by: Miloslav Trmač <mitr at redhat.com>
Signed-off-by: Colin Walters <walters at redhat.com>
diff --git a/src/polkitbackend/polkitbackendinteractiveauthority.c b/src/polkitbackend/polkitbackendinteractiveauthority.c
index f6ea0fc..587f954 100644
--- a/src/polkitbackend/polkitbackendinteractiveauthority.c
+++ b/src/polkitbackend/polkitbackendinteractiveauthority.c
@@ -1566,36 +1566,42 @@ authentication_agent_new (PolkitSubject *scope,
const gchar *unique_system_bus_name,
const gchar *locale,
const gchar *object_path,
- GVariant *registration_options)
+ GVariant *registration_options,
+ GError **error)
{
AuthenticationAgent *agent;
- GError *error;
+ GDBusProxy *proxy;
- agent = g_new0 (AuthenticationAgent, 1);
+ if (!g_variant_is_object_path (object_path))
+ {
+ g_set_error (error, POLKIT_ERROR, POLKIT_ERROR_FAILED,
+ "Invalid object path '%s'", object_path);
+ return NULL;
+ }
+
+ proxy = g_dbus_proxy_new_for_bus_sync (G_BUS_TYPE_SYSTEM,
+ G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES |
+ G_DBUS_PROXY_FLAGS_DO_NOT_CONNECT_SIGNALS,
+ NULL, /* GDBusInterfaceInfo* */
+ unique_system_bus_name,
+ object_path,
+ "org.freedesktop.PolicyKit1.AuthenticationAgent",
+ NULL, /* GCancellable* */
+ error);
+ if (proxy == NULL)
+ {
+ g_prefix_error (error, "Failed to construct proxy for agent: " );
+ return NULL;
+ }
+ agent = g_new0 (AuthenticationAgent, 1);
agent->ref_count = 1;
agent->scope = g_object_ref (scope);
agent->object_path = g_strdup (object_path);
agent->unique_system_bus_name = g_strdup (unique_system_bus_name);
agent->locale = g_strdup (locale);
agent->registration_options = registration_options != NULL ? g_variant_ref (registration_options) : NULL;
-
- error = NULL;
- agent->proxy = g_dbus_proxy_new_for_bus_sync (G_BUS_TYPE_SYSTEM,
- G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES |
- G_DBUS_PROXY_FLAGS_DO_NOT_CONNECT_SIGNALS,
- NULL, /* GDBusInterfaceInfo* */
- agent->unique_system_bus_name,
- agent->object_path,
- "org.freedesktop.PolicyKit1.AuthenticationAgent",
- NULL, /* GCancellable* */
- &error);
- if (agent->proxy == NULL)
- {
- g_warning ("Error constructing proxy for agent: %s", error->message);
- g_error_free (error);
- /* TODO: Make authentication_agent_new() return NULL and set a GError */
- }
+ agent->proxy = proxy;
return agent;
}
@@ -2398,8 +2404,6 @@ polkit_backend_interactive_authority_register_authentication_agent (PolkitBacken
caller_cmdline = NULL;
agent = NULL;
- /* TODO: validate that object path is well-formed */
-
interactive_authority = POLKIT_BACKEND_INTERACTIVE_AUTHORITY (authority);
priv = POLKIT_BACKEND_INTERACTIVE_AUTHORITY_GET_PRIVATE (interactive_authority);
@@ -2486,7 +2490,10 @@ polkit_backend_interactive_authority_register_authentication_agent (PolkitBacken
polkit_system_bus_name_get_name (POLKIT_SYSTEM_BUS_NAME (caller)),
locale,
object_path,
- options);
+ options,
+ error);
+ if (!agent)
+ goto out;
g_hash_table_insert (priv->hash_scope_to_authentication_agent,
g_object_ref (subject),
commit 87b2290c03f28841594451c7276e0ca44970c1fe
Author: Colin Walters <walters at redhat.com>
Date: Wed Jun 3 14:35:42 2015 -0400
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 at google.com>
Signed-off-by: Colin Walters <walters at redhat.com>
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;
More information about the hal-commit
mailing list