From 87b2290c03f28841594451c7276e0ca44970c1fe Mon Sep 17 00:00:00 2001 From: Colin Walters 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 Signed-off-by: Colin Walters --- .../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