Bug 90832 - [PATCH] authority: Avoid cookie wrapping by using u64 counter
Summary: [PATCH] authority: Avoid cookie wrapping by using u64 counter
Status: RESOLVED FIXED
Alias: None
Product: PolicyKit
Classification: Unclassified
Component: daemon (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: David Zeuthen (not reading bugmail)
QA Contact: David Zeuthen (not reading bugmail)
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-06-03 19:06 UTC by Colin Walters
Modified: 2015-06-17 17:02 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
0001-authority-Avoid-cookie-wrapping-by-using-u64-counter.patch (3.63 KB, patch)
2015-06-03 19:06 UTC, Colin Walters
Details | Splinter Review
0001-CVE-XXX-XXXX-Use-unpredictable-cookie-values-keep-th.patch (17.67 KB, patch)
2015-06-04 17:47 UTC, Colin Walters
Details | Splinter Review
0001-CVE-XXX-XXXX-Use-unpredictable-cookie-values-keep-th.patch (17.77 KB, patch)
2015-06-04 17:55 UTC, Colin Walters
Details | Splinter Review
0001-CVE-XXX-XXXX-Use-unpredictable-cookie-values-keep-th.patch (17.97 KB, patch)
2015-06-08 20:09 UTC, Colin Walters
Details | Splinter Review
0001-CVE-XXX-XXXX-Use-unpredictable-cookie-values-keep-th.patch (18.00 KB, patch)
2015-06-09 18:02 UTC, Miloslav Trmac
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Walters 2015-06-03 19:06:17 UTC
Created attachment 116268 [details] [review]
0001-authority-Avoid-cookie-wrapping-by-using-u64-counter.patch

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@google.com>
Signed-off-by: Colin Walters <walters@redhat.com>
Comment 1 Colin Walters 2015-06-03 19:06:32 UTC
Comment on attachment 116268 [details] [review]
0001-authority-Avoid-cookie-wrapping-by-using-u64-counter.patch

>From 87b2290c03f28841594451c7276e0ca44970c1fe Mon Sep 17 00:00:00 2001
>From: Colin Walters <walters@redhat.com>
>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 <taviso@google.com>
>Signed-off-by: Colin Walters <walters@redhat.com>
>---
> .../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
>
Comment 2 Miloslav Trmac 2015-06-04 11:28:35 UTC
Comment on attachment 116268 [details] [review]
0001-authority-Avoid-cookie-wrapping-by-using-u64-counter.patch

Review of attachment 116268 [details] [review]:
-----------------------------------------------------------------

::: src/polkitbackend/polkitbackendinteractiveauthority.c
@@ +1504,5 @@
>  {
> +  PolkitBackendInteractiveAuthorityPrivate *priv =
> +    POLKIT_BACKEND_INTERACTIVE_AUTHORITY_GET_PRIVATE (authority);
> +  guint32 rv = g_random_int ();
> +  priv->cookie++;

This technically introduces yet another way for cookie values to alias; g_get_monotonic_time only has a microsecond resolution, so if someone (but then, who? polkitd doesn’t do this) instantiated two authorities in quick succession, they could end up using the same values.

This is AFAICS perfectly safe because the two different authority instances would have to use different D-Bus object paths, so there is no possibility of confusion between the two, but perhaps it would be worth a comment.
Comment 3 Colin Walters 2015-06-04 17:47:40 UTC
Created attachment 116293 [details] [review]
0001-CVE-XXX-XXXX-Use-unpredictable-cookie-values-keep-th.patch
Comment 4 Colin Walters 2015-06-04 17:55:28 UTC
Created attachment 116294 [details] [review]
0001-CVE-XXX-XXXX-Use-unpredictable-cookie-values-keep-th.patch
Comment 5 Miloslav Trmac 2015-06-05 20:27:58 UTC
Comment on attachment 116294 [details] [review]
0001-CVE-XXX-XXXX-Use-unpredictable-cookie-values-keep-th.patch

Review of attachment 116294 [details] [review]:
-----------------------------------------------------------------

::: src/polkitagent/polkitagenthelper-pam.c
@@ +97,4 @@
>    openlog ("polkit-agent-helper-1", LOG_CONS | LOG_PID, LOG_AUTHPRIV);
>  
>    /* check for correct invocation */
> +  if (!(argc == 2 || argc == 3))

Wouldn’t
 if (argc !=2 && argc != 3)
be simpler? This is probably a way too individual style issue, feel free to leave it as it is. (Would also affect polkitagenthelper-shadow.c.)

::: src/polkitagent/polkitagenthelperprivate.c
@@ +60,5 @@
> +    return strdup (argv[2]);
> +  else
> +    {
> +      char *ret = NULL;
> +      ssize_t r = getline (&ret, NULL, stdin);

How does this work? The string returned by getline includes the terminating '\n' which is not a part of the cookie.  Is there some other piece of code which strips it again?

@@ +63,5 @@
> +      char *ret = NULL;
> +      ssize_t r = getline (&ret, NULL, stdin);
> +      if (r == -1)
> +        {
> +          perror ("getline");

if (feof(stdin))
    fprintf (stderr, "Unexpected EOF\n");
  else
    perror ("getline");
to avoid spurious error codes depending on value of errno, “getline: Success” in the best case.  Or just not bother and fprintf (stderr, "Error reading cookie from stdin\n") without referring to errno at all.

Also, the error path should have
  free (ret);

::: src/polkitagent/polkitagenthelperprivate.h
@@ +38,4 @@
>  
>  int _polkit_clearenv (void);
>  
> +char * read_cookie (int argc, char **argv);

(Pointless pedant) Please drop the space after * to be consistent with the rest of the code.

::: src/polkitagent/polkitagentsession.c
@@ +623,4 @@
>    if (G_UNLIKELY (_show_debug ()))
>      g_print ("PolkitAgentSession: spawned helper with pid %d\n", (gint) session->child_pid);
>  
> +  session->child_stdin = (GOutputStream*)g_unix_input_stream_new (stdin_fd, TRUE);

Shouldn’t this be g_unix_output_stream_new?

::: src/polkitbackend/polkitbackendinteractiveauthority.c
@@ +2531,4 @@
>        goto out;
>      }
>  
> +  agent_serial = priv->agent_serial++;

The single-read single-write variable is not worth it IMHO; the function already has more than enough local variables to keep track of.
Comment 6 Colin Walters 2015-06-08 15:01:45 UTC
I'd apparently pushed an older version of this patch.  I'm actually maintaining the canonical copies in github because it's easier, here: https://github.com/cgwalters/polkit/tree/private-cookies

It fixes some of the obvious things like the input/output confusion.   But I'll still post an updated version here.

(In reply to Miloslav Trmac from comment #5)

> Wouldn’t
>  if (argc !=2 && argc != 3)
> be simpler? This is probably a way too individual style issue, feel free to
> leave it as it is. (Would also affect polkitagenthelper-shadow.c.)

I guess which way is simpler depends on your mental model.  For me, I see "if (!(precondition))" as a very very common thing in C - the leading ! tells me that.   Whereas with the above I have to parse each term.  

> How does this work? The string returned by getline includes the terminating
> '\n' which is not a part of the cookie.  Is there some other piece of code
> which strips it again?

Fixed.  I'm not sure how it worked, or I possibly didn't actually test the patch I thought I did.

> to avoid spurious error codes depending on value of errno, “getline:
> Success” in the best case.  Or just not bother and fprintf (stderr, "Error
> reading cookie from stdin\n") without referring to errno at all.

I don't understand this - if r == -1, errno will be set.  How can it be spurious?  And I would prefer to log errno.


(Fixed everything else you pointed out)
Comment 7 Miloslav Trmac 2015-06-08 16:20:18 UTC
(In reply to Colin Walters from comment #6)
> I'd apparently pushed an older version of this patch.  I'm actually
> maintaining the canonical copies in github because it's easier, here:
> https://github.com/cgwalters/polkit/tree/private-cookies

I’ll take a look there, then.

> > Wouldn’t
> >  if (argc !=2 && argc != 3)
> > be simpler? This is probably a way too individual style issue, feel free to
> > leave it as it is. (Would also affect polkitagenthelper-shadow.c.)
> 
> I guess which way is simpler depends on your mental model.  For me, I see
> "if (!(precondition))" as a very very common thing in C - the leading !
> tells me that.   Whereas with the above I have to parse each term.  
Sure, let’s leave it as it is.

> > to avoid spurious error codes depending on value of errno, “getline:
> > Success” in the best case.  Or just not bother and fprintf (stderr, "Error
> > reading cookie from stdin\n") without referring to errno at all.
> 
> I don't understand this - if r == -1, errno will be set.

No, errno is not set on EOF. See getdelim(3p).
Comment 8 Colin Walters 2015-06-08 20:09:48 UTC
Created attachment 116372 [details] [review]
0001-CVE-XXX-XXXX-Use-unpredictable-cookie-values-keep-th.patch

Fixed the feof()/errno issue, thanks!
Comment 9 Miloslav Trmac 2015-06-09 18:02:10 UTC
Created attachment 116398 [details] [review]
0001-CVE-XXX-XXXX-Use-unpredictable-cookie-values-keep-th.patch

Not sure how the github workflow should work when polkit is not on github…

Anyway, here is a version based on https://github.com/cgwalters/polkit/commit/a32c28721a8ce635823b26294881b64a0a0948f8 , which fixes a memory leak in the getline() error case, and initializes n as Application Usage recommends.

Tested:
* New helper with old polkitd and agents
* New agent+helper with old polkitd (new agent+old helper is not possible, which is OK).
* New polkitd with old agent+helper

and everything seems to work fine.
Comment 10 Colin Walters 2015-06-09 18:45:23 UTC
I'm fine with your changes.  Does that count as Reviewed-By: you?  Anything else to do before we commit this?
Comment 11 Miloslav Trmac 2015-06-10 16:02:17 UTC
Sure, that’s a Reviewed-By.
Comment 12 Colin Walters 2015-06-10 16:16:24 UTC
(In reply to Miloslav Trmac from comment #11)
> Sure, that’s a Reviewed-By.

Ok.  I'm still waiting on a reply from MITRE for a CVE allocation.  If they don't get back to me by today, I'll just drop it from the commit message.
Comment 13 Colin Walters 2015-06-17 17:02:10 UTC
We got an assignment, pushed as:

http://cgit.freedesktop.org/polkit/commit/


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.