search

vmihalko / t2_polkit

Other
0 stars 0 forks source link

Random cookie generation not cryptographically secure #7

Open vmihalko opened 8 years ago

vmihalko commented 8 years ago

In GitLab by @bugzilla-migration on Feb 23, 2016, 22:23

Submitted by Andy Wingo

Assigned to David Zeuthen @david

Link to original bug (#94269)

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, "'Reference to deleted milestone  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.

vmihalko commented 8 years ago

In GitLab by @bugzilla-migration on Feb 24, 2016, 16:28

:hammer_and_wrench: Andy Wingo submitted a patch:

Patch from https://github.com/wingo/polkit

Patch 121949, "Patch from https://github.com/wingo/polkit against master":
4c9a813f3fc1ada4fcce508d286e95f965a3002a.patch

vmihalko commented 8 years ago

In GitLab by @bugzilla-migration on Feb 24, 2016, 18:38

:speech_balloon: Miloslav Trmac said:

Thanks for the patch.

Sorry, some of the conversations at the time of #90832 were off-list. This should probably be better commented in *interactiveauthority.c — after we agree on what the behavior is, see below ☺

The critical property of the cookies is that they must not collide (to prevent user A’s agent authentication from colliding with user B’s authentication session and incorrectly giving user B privilege). This is already sufficiently guaranteed by the (agent serial, cookie serial) pair, and no randomness is necessary at all.

The randomness, and keeping the cookie private, was there to mitigate a possible DoS, where a malicious user knowing or being able to predict a cookie value used by a victim user would supposedly be able to prevent the victim's authentication from succeeding.

But, looking at this now, I can’t see how that DoS would work:

  1. (polkit-agent-helper-1 only sends something on successful authentication; there is no AuthenticationAgentResponse*(… auth has failed) kind of call.)

  2. polkit_backend_interactive_authority_authentication_agent_response does nothing to the state of the session if the result is not successful: in particular: 1.a. If the authenticated identity is a non-root attacker, this is ignored, it does not cause the session to be marked as failed. 1.b. If the *_response call is called by an invalid agent or rejected for other reasons, this does not terminate the AuthenticationSession (that happens when the legitimate agent returns from a D-Bus method, which is presumably not possible to spoof), so the legitimate user can still use their own agent to authenticate just fine.

So, AFAICS, there is no DoS possible even with fully public and predictable cookie values. Colin, am I missing anything?

Andy, you seem to be primarily concerned with the UID binding in #90837. If that is what you want to solve, let's discuss that directly; hiding essentially a revert of #90837 inside a patch described as "use /dev/random" is not a great way to do code review.

(Separately, assuming that the serial numbers prevent privilege escalation and no DoS is possible, I am increasingly convinced that #90837 should be just reverted instead of trying to make it work; it is unnecessary, breaks the model of “sessions are externally defined by ConsoleKit/systemd for us”, and making it work would be extra work for pkexec (or more, and much riskier, work for polkit_agent_listener_register*).

vmihalko commented 8 years ago

In GitLab by @bugzilla-migration on Feb 24, 2016, 19:27

:speech_balloon: Colin Walters @walters said:

While GRand isnt a CS PRNG, in practice, it is always going to seed from /dev/urandom and not the current time, because the latter would only basically happen in badly written container environments. (Yes, I know that's a motivation for getrandom()).

I'm not opposed to this patch, it is simpler in many ways. The rationale for having two prefixes is that while obviously the change of collision is basically nonexistent, it helps mitigate it to make use of the fact that we have different "domains" we want to isolate and are in a position to choose a unique prefix. Like how Ethernet MACs work.

The above all said, I would like to try really hard to have the "uid binding" fix work.

Basically I'd say your patch isn't wrong, but neither does it solve anything really other than make the code simpler.

vmihalko commented 8 years ago

In GitLab by @bugzilla-migration on Feb 24, 2016, 21:16

:speech_balloon: Andy Wingo said:

Hi! Yeah sorry, I waxed a bit rhetorical. I don't mean to imply that there is a security hole; I can't convince myself that one exists. Thank you both for both the initial fix for the CVEs and for looking at this patch; cheers. We can keep this bug open for now; if we decide that randomness is unnecessary, we close, otherwise if we need randomness we apply something more like this, just to simplify things and remove potential error cases. See you in bug 90837.