Random cookie generation not cryptographically secure
Submitted by Andy Wingo
Assigned to David Zeuthen @david
Description
I am having a hard time understanding polkit's cookie generation policy. It seems to me to be either too much or not enough. Following bug 90832, polkit attempts to avoid cookie collisions by using randomness. Avoiding collision appears to have some security properties. However the way polkit avoids collisions is via GRand, which is not well-suited to this purpose.
Each authentication agent gets its own cookie prefix:
{ 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 ();
}
But this is both too much and not enough. Too much, if the security model is really per-user and not per-connection, following bug 90837. Too much also in that the prefix can be known to the user, so it just needs to be unique within the server and not globally. Too much also in that, why a prefix? Why not just a proper random number for each request?
But not enough if the security model needs non-colliding cookies. GRand is not a CSPRNG, and it could end up seeding itself from the current time. Better to just read 256 bits out of /dev/urandom and be done with it.
Then, authentication_agent_generate_cookie generates cookies for individual authentication conversations:
{ 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); }
But if it's important that cookies be globally unique, we shouldn't be using a not-CSPRNG. If creating a collision is a security problem -- and it would seem that given the reopened 90837 that it's at least possible to spoof polkit auth attempts from root -- then we should be using cryptographically strong random values.
Suggestion: instead of all this prefix and counter business, just read 8 4-byte ints from /dev/urandom and be done with it.