Bug 94269

Summary:	Random cookie generation not cryptographically secure
Product:	PolicyKit	Reporter:	Andy Wingo <wingo>
Component:	daemon	Assignee:	David Zeuthen (not reading bugmail) <zeuthen>
Status:	RESOLVED MOVED	QA Contact:	David Zeuthen (not reading bugmail) <zeuthen>
Severity:	normal
Priority:	medium	CC:	mitr, walters, wingo
Version:	unspecified
Hardware:	Other
OS:	All
Whiteboard:
i915 platform:		i915 features:
Attachments:	Patch from https://github.com/wingo/polkit against master

Description Andy Wingo 2016-02-23 21:23:08 UTC

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.

Comment 1 Andy Wingo 2016-02-24 15:28:17 UTC

Created attachment 121949 [details] [review]
Patch from https://github.com/wingo/polkit against master

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

Comment 2 Miloslav Trmac 2016-02-24 17:38:08 UTC

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*).

Comment 3 Colin Walters 2016-02-24 18:27:19 UTC

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.

Comment 4 Andy Wingo 2016-02-24 20:16:10 UTC

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.

Comment 5 GitLab Migration User 2018-08-20 21:33:55 UTC

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/polkit/polkit/issues/3.

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.