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 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 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.
Created attachment 116293 [details] [review] 0001-CVE-XXX-XXXX-Use-unpredictable-cookie-values-keep-th.patch
Created attachment 116294 [details] [review] 0001-CVE-XXX-XXXX-Use-unpredictable-cookie-values-keep-th.patch
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.
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)
(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).
Created attachment 116372 [details] [review] 0001-CVE-XXX-XXXX-Use-unpredictable-cookie-values-keep-th.patch Fixed the feof()/errno issue, thanks!
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.
I'm fine with your changes. Does that count as Reviewed-By: you? Anything else to do before we commit this?
Sure, that’s a Reviewed-By.
(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.
We got an assignment, pushed as: http://cgit.freedesktop.org/polkit/commit/
http://cgit.freedesktop.org/polkit/commit/?id=ea544ffc18405237ccd95d28d7f45afef49aca17
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.