Bug 90837

So this patch:
 - Doesn't appear to regress polkit (tested by patching the current RHEL7 RPM and doing some time zone changes with gnome-control-center)
 - From my reading of the code, is likely to close this security hole

But, I'm not aware of anyone having written an actual reproducer for this bug yet.  I'm playing now with this patch:

[PATCH] DO NOT SHIP: Always use the same cookie for testing

This way it's easy to reproduce an overflow.
---
 src/polkitbackend/polkitbackendinteractiveauthority.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/src/polkitbackend/polkitbackendinteractiveauthority.c b/src/polkitbackend/polkitbackendinteractiveauthority.c
index 2e29520..97ab6c2 100644
--- a/src/polkitbackend/polkitbackendinteractiveauthority.c
+++ b/src/polkitbackend/polkitbackendinteractiveauthority.c
@@ -1490,11 +1490,8 @@ authentication_session_free (AuthenticationSession *session)
 static gchar *
 authentication_agent_new_cookie (AuthenticationAgent *agent)
 {
-  static gint counter = 0;
-
-  /* TODO: use a more random-looking cookie */
-
-  return g_strdup_printf ("cookie%d", counter++);
+  /* Always make the same cookie for testing */
+  return g_strdup_printf ("cookie0");
 }
 
 static PolkitSubject *
-- 
1.8.3.1

Created attachment 116289 [details] [review]
0001-authority-Explicitly-operate-on-UnixUser-uids.patch

Created attachment 116290 [details] [review]
0001-CVE-XXX-XXXX-Bind-use-of-cookies-to-specific-uids.patch

I have verified with GDB that this patch firmly closes the hole; sessions with different uids are skipped, even if the cookie matches.

Comment on attachment 116289 [details] [review]
0001-authority-Explicitly-operate-on-UnixUser-uids.patch

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

::: src/polkitbackend/polkitbackendinteractiveauthority.c
@@ +2429,4 @@
>        goto out;
>      }
>  
> +  user_of_caller = (PolkitUnixUser*)polkit_backend_session_monitor_get_user_for_subject (priv->session_monitor, caller, NULL);

Isn‘t the GObject way of doing this the typecheck macro POLKIT_UNIX_USER()?

Or, I suppose, we could just have polkit_backend_session_monitor_get_uid_for_subject, we start with the UID anyway.

But then… see the other patch.

Comment on attachment 116290 [details] [review]
0001-CVE-XXX-XXXX-Bind-use-of-cookies-to-specific-uids.patch

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

::: data/org.freedesktop.PolicyKit1.AuthenticationAgent.xml
@@ +8,4 @@
>      <annotation name="org.gtk.EggDBus.DocString" value="<para>This D-Bus interface is used for communication between the system-wide PolicyKit daemon and one or more authentication agents each running in a user session.</para><para>An authentication agent must implement this interface and register (passing the object path of the object implementing the interface) using the org.freedesktop.PolicyKit1.Authority.RegisterAuthenticationAgent() and org.freedesktop.PolicyKit1.Authority.UnregisterAuthenticationAgent() methods on the #org.freedesktop.PolicyKit1.Authority interface of the PolicyKit daemon.</para>"/>
>  
>      <method name="BeginAuthentication">
> +      <annotation name="org.gtk.EggDBus.DocString" value="<para>Called by the PolicyKit daemon when the authentication agent needs the user to authenticate as one of the identities in @identities for the action with the identifier @action_id.</para><para>Upon succesful authentication, the authentication agent must invoke the org.freedesktop.PolicyKit1.Authority.AuthenticationAgentResponse() method on the #org.freedesktop.PolicyKit1.Authority interface of the PolicyKit daemon before returning.  Note this method is normally only accessible to root, and called via a setuid helper program.</para><para>If the user dismisses the authentication dialog, the authentication agent should return an error.</para>"/>

I was at first confused by “this method is normally only accessible to root”; it’s not obvious that it refers to AuthenticationAgentResponse rather than BeginAuthentication.

::: data/org.freedesktop.PolicyKit1.Authority.xml
@@ +314,5 @@
>  
>      <method name="AuthenticationAgentResponse">
> +      <annotation name="org.gtk.EggDBus.DocString" value="Method for authentication agents to invoke on successful
> +authentication, intended only for use by a privileged helper process
> +internal to polkit.  Note the signature for this method changed in

Hum; I am rather uncomfortable with making a method private in a security patch.  Third-party agents are not entirely unheard of (see e.g. http://lists.freedesktop.org/archives/polkit-devel/2013-September/000387.html ) and I don’t know that they have ever been discouraged.

A possible alternative would be to add a RegisterAuthenticationAgent2, which would opt-in into only using AuthenticationAgentResponse2 with the new UID parameter; but that’s more work and more long-term mainteinance.

Let me mull this over during the week-end…

@@ +318,5 @@
> +internal to polkit.  Note the signature for this method changed in
> +0.114."/>
> +
> +      <arg name="uid" direction="in" type="i">
> +        <annotation name="org.gtk.EggDBus.DocString" value="The real uid of the agent.  Normally set by the setuid helper program."/>

At least this should be type="u", if not something wider; uid_t is uint32_t on Linux. (Sadly, PolkitUnixUser uses gint32 in its C interfaces, but its D-Bus representation does use G_VARIANT_TYPE_UINT32.)

More generally, this would probably be an appropriate place to actually bother with the PolkitIdentity abstraction so that we don’t have to break the declared interfaces when/if app containers come. (And that would make the benefit of the other patch, to deal with uids instead of PolkitIdentities directly, smaller.)

::: src/polkitbackend/polkitbackendinteractiveauthority.c
@@ +1685,5 @@
>      {
>        GList *l;
>  
> +      /* We need to ensure that if somehow we have duplicate cookies
> +       * due to wrapping, that the cookie used is matched to the user

After bug #90832, the “due to wrapping” part might be confusing.

(In reply to Miloslav Trmac from comment #6)
> 
> Hum; I am rather uncomfortable with making a method private in a security
> patch.  Third-party agents are not entirely unheard of (see e.g.
> http://lists.freedesktop.org/archives/polkit-devel/2013-September/000387.
> html ) and I don’t know that they have ever been discouraged.

This is third party agents not using libpolkit-agent-1?  I wonder if they're really using it now.  I believe all of the main desktop agents are using libpolkit, although that's just an offhand feeling, I didn't look.

The real issue is, [next para]

> A possible alternative would be to add a RegisterAuthenticationAgent2, which
> would opt-in into only using AuthenticationAgentResponse2 with the new UID
> parameter; but that’s more work and more long-term mainteinance.

Remember, this API can only be invoked by root.  So effectively, I claim it's always been a private implmentation detail of the setuid helper.

Now...the question becomes, are there agents not using libpolkit-agent-1 that are directly invoking the setuid binary?

If so, they'll leak the cookie value...which is a discussion for the other patch, not this one.

> At least this should be type="u", if not something wider; uid_t is uint32_t
> on Linux. (Sadly, PolkitUnixUser uses gint32 in its C interfaces, but its
> D-Bus representation does use G_VARIANT_TYPE_UINT32.)

Yes...except polkit_unix_user_get_uid() returns a signed int?  I don't know why.  It looks like you're right though, the variants are unsigned.

I'll adjust the code to match what polkit is doing now, though maybe we should teach PolkitUnixUser new guint32 APIs.
 
> More generally, this would probably be an appropriate place to actually
> bother with the PolkitIdentity abstraction so that we don’t have to break
> the declared interfaces when/if app containers come. 

Hmm, you're thinking of something like keeping track of the user namespace?  I'll look at that.

(In reply to Colin Walters from comment #7)
> (In reply to Miloslav Trmac from comment #6)
> > 
> > Hum; I am rather uncomfortable with making a method private in a security
> > patch.  Third-party agents are not entirely unheard of (see e.g.
> > http://lists.freedesktop.org/archives/polkit-devel/2013-September/000387.
> > html ) and I don’t know that they have ever been discouraged.
> 
> This is third party agents not using libpolkit-agent-1?  I wonder if they're
> really using it now.

I have only found a .pkla file in kodi.tv, but I have spent about 1 minute on it.

> > A possible alternative would be to add a RegisterAuthenticationAgent2, which
> > would opt-in into only using AuthenticationAgentResponse2 with the new UID
> > parameter; but that’s more work and more long-term mainteinance.
> 
> Remember, this API can only be invoked by root.  So effectively, I claim
> it's always been a private implmentation detail of the setuid helper.

I don't see how “only invoked by root” implies “only invoked by _our_ setuid helper”. (It might well be true in practice.)

> Now...the question becomes, are there agents not using libpolkit-agent-1
> that are directly invoking the setuid binary?

Either that, or invoking a different setuid binary with a different policy; or just running a session agent as root and ignoring the setuid mechanism completely.

Any of these are possible in principle, I haven’t found much to suggest that they actually exist. I will research this a little more.




> If so, they'll leak the cookie value...which is a discussion for the other
> patch, not this one.

Good point. I was leaning towards relying only on the other patch making cookies both unpredictable and secret; but if the third-party agent/helper case is realistic at all, it is also an argument for having something like this patch.

> > At least this should be type="u", if not something wider; uid_t is uint32_t
> > on Linux. (Sadly, PolkitUnixUser uses gint32 in its C interfaces, but its
> > D-Bus representation does use G_VARIANT_TYPE_UINT32.)
> 
> Yes...except polkit_unix_user_get_uid() returns a signed int?

Yes, that’s what I mean by the C interfaces above.

> I'll adjust the code to match what polkit is doing now, though maybe we
> should teach PolkitUnixUser new guint32 APIs.

If so, think we should just use uid_t; if glibc ever changes the uid_t ABI to widen the type, polkit’s effective ABI will break in both cases. Though printing uid_t portably is awkward (the best way I know is casting to uintmax_t) and having polkit “promise” that %d is enough is convenient.

> > More generally, this would probably be an appropriate place to actually
> > bother with the PolkitIdentity abstraction so that we don’t have to break
> > the declared interfaces when/if app containers come. 
> 
> Hmm, you're thinking of something like keeping track of the user namespace? 

Yes; not right now but eventually (see also https://bugzilla.redhat.com/show_bug.cgi?id=1206069 ).

We would still have to create new PolkitIdentity subclasses, and still add handling for those new subclasses all over the place (and _maybe_ start refusing PolkitUnixUser, effectively breaking ABI anyway), but the D-Bus interface would not need to be broken/replaced/augmented obsoleting the explicit-UID interface, we would merely start shipping a different PolkitIdentity subclass through the existing interface.

(In reply to Miloslav Trmac from comment #8)

> I don't see how “only invoked by root” implies “only invoked by _our_ setuid
> helper”. (It might well be true in practice.)

You'd have to really go out of your way to implement a new setuid helper.  I guess if you're not using polkit-agent-1.so, it's the next step to not depend on polkit, but...

But ok, thinking about this - if we add AuthenticationAgentResponse2, then *our* setuid helper will only invoke it, and hence be secure.  But any custom setuid helpers would need to adjust their code.  That's OK though.

I'll take a look at adding a new DBus API.  (I'd originally started adding a new internal vfunc, but that's just a lot of pain for no reason, because polkitbackend isn't public API, so no external app can actually make their own).

> Yes; not right now but eventually (see also
> https://bugzilla.redhat.com/show_bug.cgi?id=1206069 ).

I don't understand how that bug is related to user namespaces?

Looking at this quickly, the kernel does what I'd expect here, when you use `SO_PEERCRED`, it maps the uid into the current process' namespace.  So for example if uid 1000 creates a process with a user namespace, and is uid 0 inside the namespace, we'll still see them as 1000 outside.

Which is correct, right?

I still think this boils down to polkit not being designed for a uid-per-app world, and it'd likely involve a lot of fundamental design issues.  I don't see the PolkitUnixUser abstraction buying us something here.

Created attachment 116337 [details] [review]
0001-authority-Add-a-helper-method-for-checking-whether-a.patch

Created attachment 116338 [details] [review]
0002-CVE-XXX-XXXX-Bind-use-of-cookies-to-specific-uids.patch

Changes:

 - Dropped the first patch for extracting uids from PolkitIdentity in favor of a different cleanup
 - Kept the existing AuthenticationAgentResponse, added AuthenticationAgentResponse2
 - Use uid_t internally as much as possible
   BTW, I think I remembered why David chose to use gint in public API - it's because at the time gobject-introspection barfed on some POSIX types.  See https://git.gnome.org/browse/gobject-introspection/commit/?id=26ac70c37e4ea2f7e1caf8cebed3577695ce20e3
   So we could start using it in public API now...but not sure it's worth it
 - Support the case where AuthenticationAgentResponse is used - we use the special value (uid_t)-1 to denote "unknown".  This means any third party setuid helpers will continue to be vulnerable.  But I doubt any exist.  And if they do, they have to pay the price of reimplementing things.

Comment on attachment 116337 [details] [review]
0001-authority-Add-a-helper-method-for-checking-whether-a.patch

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

Great cleanup, ACK.

The function can also be used in polkit_backend_interactive_authority_authentication_agent_response, feel free to commit without an extra review roundtrip.

Comment on attachment 116337 [details] [review]
0001-authority-Add-a-helper-method-for-checking-whether-a.patch

Committed 1/2.

Created attachment 116542 [details] [review]
0001-Additional-code-fixes.patch

AFAICS the patch doesn’t currently work; it adds the UID parameter but does not change the method name when calling AuthenticationAgentResponse.

This should fix it, and also fixes a warning.

Created attachment 116543 [details] [review]
0002-Documentation-fixes-and-additions.patch

Here are a few proposed improvements to the documentation:
* Refer to PolkitAgentSession in general instead of to _response only
* Revert to the original description of authentication cancellation, the agent really needs to return an error to the caller (in addition to dealing with the session if any).
* Explicitly document the UID assumption; in the process fixing bug #69980.
* Keep documenting that we need a sufficiently privileged caller.
* Refer to the ...Response2 API in more places.
* Also update docbook documentation.
* Drop a paragraph suggesting non-PolkitAgentSession implementations are expected and commonplace.

(What is the deal with the introspection XML in data/*? AFAICS it is not used for anything, neither to generate code, nor to send to callers who ask for introspection, nor to generate documentation. Should we just delete it, or perhaps more tightly integrate with gdbus-codegen? If either, I will open a separate bug to deal with this later; right now I just want to know whether I am missing anything.

(Splitting these out only for ease of review; if you agree, please commit them or at least with 0001-Addition-code-fixes.patch, squashed, so that we don’t have a broken commit in the tree.)

(In reply to Miloslav Trmac from comment #16)
> Created attachment 116543 [details] [review] [review]
> 0002-Documentation-fixes-and-additions.patch
> 
> Here are a few proposed improvements to the documentation:

These look fine to me; I added this to the commit message.
 
> (What is the deal with the introspection XML in data/*?

I believe the intent was to install to /usr/share/dbus-1/interfaces.  Which is not a standard...but several projects install things there.  I'm not sure if anything today actively uses it.

(In reply to Miloslav Trmac from comment #15)
> Created attachment 116542 [details] [review] [review]
> 0001-Additional-code-fixes.patch
> 
> AFAICS the patch doesn’t currently work; it adds the UID parameter but does
> not change the method name when calling AuthenticationAgentResponse.

Argh, thanks for catching that!  I obviously tested earlier versions but only verified that the new version that added AuthenticationAgentResponse2 still worked as far as authenticating.

> This should fix it, and also fixes a warning.

Merged that into the patch and pushed, thanks!

http://cgit.freedesktop.org/polkit/commit/?id=493aa5dc1d278ab9097110c1262f5229bbaf1766
http://cgit.freedesktop.org/polkit/commit/?id=fb5076b7c05d01a532d593a4079a29cf2d63a228

Revisiting this, because it breaks TTY sessions:

0. (Create an unprivileged account.)
1. Log in using the unprivileged account ON A CONSOLE (not using (su -) in a terminal window, that will inherit the GUI session)
2. Try (pkexec bash)

Result:
> ==== AUTHENTICATING FOR org.freedesktop.policykit.exec ===
> Authentication is needed to run `/usr/bin/bash' as the super user
> Authenticating as: root
> Password: 
> polkit-agent-helper-1: error response to PolicyKit daemon: GDBus.Error:org.freedesktop.PolicyKit1.Error.Failed: No session for cookie
> ==== AUTHENTICATION FAILED ===
> Error executing command as another user: Not authorized
> 
> This incident has been reported.

The cause of this is:
1. pkexec is started, with ruid=$non-zero euid=0
2. pkexec registers a local authentication agent.
3. polkitd looks up the agent’s uid through polkit_system_bus_name_new (g_dbus_method_invocation_get_sender (invocation)) etc., ultimately calling DBus’ GetConnectionUnixUser. This value is the EUID (0), necessarily, because AF_UNIX sockets only provide the EUID.
4. pkexec calls CheckAuthorization, which results in a callback to its agent.
5. The agent runs polkit-agent-helper-1, getting euid=0 and inheriting ruid=$non-zero
6. The agent calls AuthenticationAgentResponse2 with uid=$ruid=$non-zero
7. polkitd finds the cookie, but the response’s uid=$non-zero doesn’t match the agent’s recorded uid=0.

(I do wonder why nobody has reported this. I have seen various reports of breakage with 0.113, but all which specified it were in a GUI, and traced back either to missing daemon restart, necessary for this patch to recognize …Response2, or to systemd broken session tracking.  But no reports of in-console breakage AFAIK.)


This is not trivially fixable; in step 3 we have to use the EUID because it is the only one we can get in a race-free way, and in step 7 we have to use the RUID because EUID is always 0.

Also, the public PolkitAgentListener API promises that applications can run agents with implied in-process semantics; we can’t just change the EUID of the whole process, even for a moment to pass the check. I guess we could fork of a subprocess which would set its euid to ruid (i.e. drop privileges), register itself as the agent, and delegate the actual processing of BeginAuthentication/CancelAuthentication calls and responses to the parent through a pipe or something like that, which is pretty awkward.

But considering that #90832 is the real fix, I am sorely tempted to basically revert this patch (keep a …Response2 implementation for backward compatibility which ignores the UID).

Colin, what do you think? Is there perhaps an easy fix I can’t see now?

Would it work to have pkexec spawn the agent as the calling user?

I like the uid binding as it's never vulnerable to information leaks (think seeing an ABRT crash dump uploaded) or brute force.

(In reply to Miloslav Trmac from comment #20)
> Revisiting this, because it breaks TTY sessions:
> 
> 0. (Create an unprivileged account.)
> 1. Log in using the unprivileged account ON A CONSOLE (not using (su -) in a
> terminal window, that will inherit the GUI session)
> 2. Try (pkexec bash)
> 
> Result:
> > ==== AUTHENTICATING FOR org.freedesktop.policykit.exec ===
> > Authentication is needed to run `/usr/bin/bash' as the super user
> > Authenticating as: root
> > Password: 
> > polkit-agent-helper-1: error response to PolicyKit daemon: GDBus.Error:org.freedesktop.PolicyKit1.Error.Failed: No session for cookie
> > ==== AUTHENTICATION FAILED ===
> > Error executing command as another user: Not authorized
> > 
> > This incident has been reported.
> 
> The cause of this is:
> 1. pkexec is started, with ruid=$non-zero euid=0
> 2. pkexec registers a local authentication agent.
> 3. polkitd looks up the agent’s uid through polkit_system_bus_name_new
> (g_dbus_method_invocation_get_sender (invocation)) etc., ultimately calling
> DBus’ GetConnectionUnixUser. This value is the EUID (0), necessarily,
> because AF_UNIX sockets only provide the EUID.
> 4. pkexec calls CheckAuthorization, which results in a callback to its agent.
> 5. The agent runs polkit-agent-helper-1, getting euid=0 and inheriting
> ruid=$non-zero
> 6. The agent calls AuthenticationAgentResponse2 with uid=$ruid=$non-zero
> 7. polkitd finds the cookie, but the response’s uid=$non-zero doesn’t match
> the agent’s recorded uid=0.
> 
> (I do wonder why nobody has reported this. I have seen various reports of
> breakage with 0.113, but all which specified it were in a GUI, and traced
> back either to missing daemon restart, necessary for this patch to recognize
> …Response2, or to systemd broken session tracking.  But no reports of
> in-console breakage AFAIK.)
> 

FWIW, just recently saw a few similar responses "GDBus.Error:org.freedesktop.PolicyKit1.Error.Failed: No session for cookie" but have no real explanation for that.

Is there some way to find out the actual cause of the session/cookie like you did, and what would be appropriate tool for that? strace, or maybe dbus-monitor?

(In reply to Florian Hubold from comment #22)
> to find out the actual cause of the session/cookie like you did

Make that "to find out the actual cause of the session/cookie mismatch like you did"

(In reply to Florian Hubold from comment #22)
> Is there some way to find out the actual cause of the session/cookie like
> you did, and what would be appropriate tool for that? strace, or maybe
> dbus-monitor?

You can see the UID used in AuthenticatonAgentResponse2 through either of those (if you can read DBus in strace, or if you can get dbus-monitor to work; not sure which is easier), and compare it with the EUID of the agent.

If you want to be 100% sure that this is an UID mismatch and not some other reason for the session not to exist, the only way I can think of is a breakpoint to the function iterating through the session list.

There's currently a separate `pkttyagent` program even, I believe we just need to spawn it as $ruid, and get some details right like tying its lifetime to pkexec via e.g. prctl(PR_SET_PDEATHSIG) or the more portable EOF-on-PIPE.

(In reply to Miloslav Trmac from comment #20)
> Revisiting this, because it breaks TTY sessions:

I see this in GuixSD, where we haven't packaged system polkit authentication services yet.  It is a new bug in 0.113 relative to 0.112.  I confirm it's the UID mismatch; first of all if you disable the creator_uid check, the search does find a session for the cookie:

      if (uid != (uid_t)-1 && 0)
        {
          if (agent->creator_uid != uid)
            continue;
        }

Secondly if you add a print before that line:

      printf ("uid %d creator %d\n", uid, agent->creator_uid);

then the daemon prints on the console:

  uid 1000 creator 0

And indeed this follows Miloslav's analysis.

I too think that reverting this part of the patch could be the right thing to do.  If the cookie is indeed cryptographically unique, then there is no need to do the per-user dance.  If we also switch to doing a constant-time compare, an attacker can detect that there are other cookies that polkit knows about, but not the contents of the cookie.

(In reply to Colin Walters from comment #21)
> Would it work to have pkexec spawn the agent as the calling user?

Yes, for pkexec; doing this for polkit_agent_register_listener et.al. would be more awkward.

> I like the uid binding as it's never vulnerable to information leaks (think
> seeing an ABRT crash dump uploaded) or brute force.

Actually, AFAICS we don’t need to care about information leaks at all, see bug  94269 comment 2.  Am I missing anything?

(In reply to Andy Wingo from comment #26)

> I too think that reverting this part of the patch could be the right thing
> to do.  If the cookie is indeed cryptographically unique, then there is no
> need to do the per-user dance.  If we also switch to doing a constant-time
> compare, an attacker can detect that there are other cookies that polkit
> knows about, but not the contents of the cookie.

I just think it's hacky to rely on secret data when we're on the *same OS instance* and the software involved can fully control the end-to-end.

I can't promise I'll do it myself in the near future, but I'd be grumpy if we didn't at least try to do https://bugs.freedesktop.org/show_bug.cgi?id=90837#c25

(In reply to Miloslav Trmac from comment #27)
> Actually, AFAICS we don’t need to care about information leaks at all, see
> bug  94269 comment 2.  Am I missing anything?

You mean, if the caller UID checks are working unlike with pkexec at a tty?  In that case AFAIU no.  My only idea would be if a user can cause a series of requests to polkit using an ambient agent like the one GNOME installs, which will create a new cookie but succeed silently without requiring a password, then they could predict the value of the next cookie and then wait for some other action to cause the user to identify themselves and somehow hijack that authentication.  It's a bit far-fetched but I don't understand things well enough to know if it's impossible or not.

(In reply to Colin Walters from comment #28)
> I can't promise I'll do it myself in the near future, but I'd be grumpy if
> we didn't at least try to do
> https://bugs.freedesktop.org/show_bug.cgi?id=90837#c25

Yeah I know what you mean.  In this case there were two setuid processes involved (pkexec and polkit-agent-helper-1) and I wasn't able to understand how they worked together or what it would mean to replace polkit-agent-helper-1 with non-suid pkttyagent.

(In reply to Colin Walters from comment #28)
> (In reply to Andy Wingo from comment #26)
> 
> > I too think that reverting this part of the patch could be the right thing
> > to do.  If the cookie is indeed cryptographically unique, then there is no
> > need to do the per-user dance.  If we also switch to doing a constant-time
> > compare, an attacker can detect that there are other cookies that polkit
> > knows about, but not the contents of the cookie.
> 
> I just think it's hacky to rely on secret data when we're on the *same OS
> instance* and the software involved can fully control the end-to-end.
> 
> I can't promise I'll do it myself in the near future, but I'd be grumpy if
> we didn't at least try to do
> https://bugs.freedesktop.org/show_bug.cgi?id=90837#c25

What is the next step here, Colin?  The current situation where TTY authentication is broken is not great.  I do not feel comfortable changing the behavior of the setuid helpers.  If Miloslav is right that secrets are not needed then we could simplify code and prevent collision by using counters, but that doesn't solve this problem; or we could simplify code and prevent collision by using truly random cookies.  If you don't have time to try the pkttyagent thing, I don't know that this bug is going anywhere; thoughts?

(In reply to Andy Wingo from comment #29)
> (In reply to Miloslav Trmac from comment #27)
> > Actually, AFAICS we don’t need to care about information leaks at all, see
> > bug  94269 comment 2.  Am I missing anything?
> 
> You mean, if the caller UID checks are working unlike with pkexec at a tty? 

No, _at all_. Neither keeping the information secret nor binding to UIDs is necessary AFAICS, as long as cookies never collide. (But documenting this behavior and the assumptions in *AgentResponse* to make sure they will be maintained into the future would be desirable.)

> In that case AFAIU no.  My only idea would be if a user can cause a series
> of requests to polkit using an ambient agent like the one GNOME installs,
> which will create a new cookie but succeed silently without requiring a
> password,

I have no idea what you mean here. If an action succeeds silently through setting allow_$something to "yes", there is no session and no cookie. GNOME cannot install an agent succeeding silently because it needs cooperation of the setuid polkit-agent-helper-*, which asks for a password.

> then they could predict the value of the next cookie and then wait
> for some other action to cause the user to identify themselves and somehow
> hijack that authentication.

Each agent gets a separate cookie sequence; so each user running an agent can only attack themselves (for some value of “attack”).

> (In reply to Colin Walters from comment #28)
> > I can't promise I'll do it myself in the near future, but I'd be grumpy if
> > we didn't at least try to do
> > https://bugs.freedesktop.org/show_bug.cgi?id=90837#c25

(I’m not a huge fan of fixing pkexec and keeping the listener_register etc. API broken and unfixable, OTOH there really aren’t many users of it I can find.)

(In reply to Andy Wingo from comment #29)
> In this case there were two setuid processes
> involved (pkexec and polkit-agent-helper-1) and I wasn't able to understand
> how they worked together
polkit(8) is a reasonable starting point; pkexec is an ordinary D-Bus client in this respect, and polkit-agent-helper-1 a component of the agent.

> or what it would mean to replace
> polkit-agent-helper-1 with non-suid pkttyagent.

The two are not replacements in any way. pkttyagent would be an out-of-process replacment for an in-process PolkitAgentListener agent; both eventually call polkit-agent-helper-1.

(In reply to Miloslav Trmac from comment #31)
> (In reply to Andy Wingo from comment #29)
> > (In reply to Miloslav Trmac from comment #27)
> > > Actually, AFAICS we don’t need to care about information leaks at all, see
> > > bug  94269 comment 2.  Am I missing anything?
> > 
> > You mean, if the caller UID checks are working unlike with pkexec at a tty? 
> 
> No, _at all_. Neither keeping the information secret nor binding to UIDs is
> necessary AFAICS, as long as cookies never collide. (But documenting this
> behavior and the assumptions in *AgentResponse* to make sure they will be
> maintained into the future would be desirable.)

I see, thank you.

> > In that case AFAIU no.  My only idea would be if a user can cause a series
> > of requests to polkit using an ambient agent like the one GNOME installs,
> > which will create a new cookie but succeed silently without requiring a
> > password,
> 
> I have no idea what you mean here. If an action succeeds silently through
> setting allow_$something to "yes", there is no session and no cookie. GNOME
> cannot install an agent succeeding silently because it needs cooperation of
> the setuid polkit-agent-helper-*, which asks for a password.

Yes, you managed to get my half-formed thought; thanks for walking me through it.

Finally, just to verify: because the _response() call must come from root (possibly via the setuid helper), your argument is that we are effectively trusting it not to forge a cookie, and so using predictable cookie values would be OK.  Is this this is the case though?  polkit-agent-helper-1 certainly gets the cookie from its caller, so I dunno.

(In reply to Andy Wingo from comment #33)
> Finally, just to verify: because the _response() call must come from root
> (possibly via the setuid helper), your argument is that we are effectively
> trusting it not to forge a cookie, and so using predictable cookie values
> would be OK.  Is this this is the case though?  polkit-agent-helper-1
> certainly gets the cookie from its caller, so I dunno.

Ping :)

I understand that neither of you have a lot of time for this, but this bug exists and I think the two of you have the necessary off-list info to understand the original motivating bug.  I have a patch which fixes this bug but Miloslav thinks randomness is completely unnecessary.  I can rework the patch to use counters but I can't convince myself that cookies should not be secret and so I would appreciate an argument as to why they are not secret.  Cheers :)

> I understand that neither of you have a lot of time for this, but this bug
> exists and I think the two of you have the necessary off-list info to
> understand the original motivating bug.

I think Colin and me disagree on whether UID binding is necessary, and I don’t want to commit a change to the policy he would object to. So here’s hoping that writing out the reasoning in more detail will allow us eventually to agree on the underlying assumptions.

(Sorry, this is probably too long and I don’t have time to shorten it. Colin, please check at least the last section.)

(In reply to Andy Wingo from comment #34)
> (In reply to Andy Wingo from comment #33)
> > Finally, just to verify: because the _response() call must come from root
> > (possibly via the setuid helper), your argument is that we are effectively
> > trusting it not to forge a cookie, and so using predictable cookie values
> > would be OK.  Is this this is the case though?  polkit-agent-helper-1
> > certainly gets the cookie from its caller, so I dunno.
> 
> Ping :)

(Sorry, no idea why I can‘t find a mail with this comment.)

(I will be separating “human” as a human being in front of a screen+keyboard, and “UID” as an OS concept.)

No; as you say, that helper gets a cookie from an unprivileged caller, with no way to verify the cookie’s origin.

But cookies cannot be “forged”; a cookie value alone is useless without polkitd’s internal state recognizing the cookie.

The cookies (or the auth sessions they represent) can only be not approved, or approved (by going through the setuid helper). (“Approved” is not a word used in the code; the code talks about UIDs authenticating, but I think it is an useful way to think about a _cookie_ as opposed to UIDs and agent scopes, which is what is the intent of the code.)

A legitimate UID’s setuid helper (as long as the attacker is not already in control of the legitimate human’s UI, in which case all bets are off) will regularly only be invoked by the humans’s session’s agent, which only does something when polkitd prompts for cookie approval _for that session_. Assuming D-Bus does not misroute messages or break its forwarding policy (again, all bets off in that case), this is fine.


CVE-2015-4625 is that an attacker could cause a cookie collision to happen; in that case a human could be (intentionally) approving their own operation, but due to the cookie collision attacker’s cookie would be approved.

But the cookies are chosen by polkitd, not by the unprivileged UID; so as long as polkitd ensures that attackers’ and legitimate UID’s cookies never collide, the attacker can never get their cookie approved by the human.

---

All of this only works if the two UID’s UIs are sufficiently separate; if the two UID’s processes had both access to the same display, and there weren’t an effective way for the human to tell apart windows drawn by the legitimate UID’s processes and the attacker’s processes, the attacker could, indeed, launch an auth dialog saying something plausible about the legitimate UID doing something needing approval (“Enter your root password to install a kernel update”), getting the human to authenticate as an admin UID, and thereby getting the attacker’s cookie approved.

The two UID’s UIs are “sufficiently separate” by assumption with X11, because X11 gives no inter-process/inter-UID security and there are various other ways for an attacker's UID to take control of victim’s UID if they share a X11 display. This is one of the reasons why polkit generally thinks in terms of “sessions”, i.e. humans, rather than UIDs; with X11, all UIDs within a session are essentially interchangeable.

Still, this is an interesting and important to think about single-screen-multiple-UID situations, because Wayland will hopefully make that concept meaningful, and because this will need to work for effective “app-store”-like sandboxing with untrusted apps running.

---

Note that UID binding, while it does prevent CVE-2015-4625, does not prevent the UI sharing attacks: if an attacker’s UID can show a dialog prompting for a passphrase, the attacker can get attacker’s cookie approved by running the helper from the attacker's UID (i.e. satisfying the binding): because “UID binding” binds the setuid helper to the cookie, but not the human’s understanding of the UID/operation to the setuid helper. The whole chain is secure only when we have a good UI (of course, out of scope) for the user to tell apart the two UID’s password prompts (or, as a special case, to prohibit other UIDs from displaying anything). We really want “binding to an UI scope” (and assuming that each sandboxed app will get its own “UI scope” with some kind of UI which will allow the human to express unambiguous approval for a specific “UI scope”).

UID binding is too strong, even; apart from our current pkexec woes, with the auth_admin_keep semantics, if the human has authenticated within a “UI scope”, sharing that authentication and not prompting again for any processes and any UIDs within that “UI scope” is the right thing to do. So while I maintain that with non-colliding cookies any further _response* restrictions should be unnecessary, binding a cookie to a specific “UI scope” would be semantically precise and correct.

It turns out we already have the “UI scope” modeled within Polkit: PolkitUnixSession is precisely this; although originally modeling the “inclusive” semantics of X11 and many UIDs on a screen sharing security properties, it can equally model the semantics of compartmentalized UI sandboxes.

So, how about PolkitUnixSession binding instead of UID binding? That would involve storing the PolkitUnixSession, which we already look up, in AuthenticationSession, and verifying it in _response*.  There is already a precedent for this policy, actually: RegisterAuthenticationAgent requires the caller and the sesssion for which the agent is registered to be equal.

(Note: I didn’t write the code to test whether it would actually work. Also, the PID→session lookup is racy against PID reuse, but this is only hardening.)

(In reply to Miloslav Trmac from comment #35)

> So, how about PolkitUnixSession binding instead of UID binding? That would
> involve storing the PolkitUnixSession, which we already look up, in
> AuthenticationSession, and verifying it in _response*.  There is already a
> precedent for this policy, actually: RegisterAuthenticationAgent requires
> the caller and the sesssion for which the agent is registered to be equal.

Sounds reasonable to me, yes.

(In reply to Miloslav Trmac from comment #20)
>
> But considering that #90832 is the real fix, I am sorely tempted to
> basically revert this patch (keep a …Response2 implementation for backward
> compatibility which ignores the UID).

Which is it though?  Are we going to scope the cookies or not?

(In reply to Miloslav Trmac from comment #35)

> But the cookies are chosen by polkitd, not by the unprivileged UID; so as long > as polkitd ensures that attackers’ and legitimate UID’s cookies never collide, > the attacker can never get their cookie approved by the human.

I read your whole comment but I'm still feeling uncertain as to your suggestion.  

To make things concrete:

 - Change the setuid helper call sd_pid_get_session (getpid (), &s)
 - Introduce AuthenticationResponse3 which takes the session instead of uid
 - Change polkitd to look up cookies scoped to that rather than the uid
 - Back out the "random cookie" changes and just go with a guint64 counter incremented per session-authsession pair ?

(In reply to Colin Walters from comment #36)
> (In reply to Miloslav Trmac from comment #20)
> >
> > But considering that #90832 is the real fix, I am sorely tempted to
> > basically revert this patch (keep a …Response2 implementation for backward
> > compatibility which ignores the UID).
> 
> Which is it though?  Are we going to scope the cookies or not?

That comment is 6 months old. I would be fine with reverting this patch but I am looking for something we can agree on, and it seems session binding can be it.

> I read your whole comment but I'm still feeling uncertain as to your
> suggestion.  
> 
> To make things concrete:
> 
>  - Change the setuid helper call sd_pid_get_session (getpid (), &s)
Not necessary (see below). Probably change it to call the non-UID AuthenticationResponse again, just to simplify.
>  - Introduce AuthenticationResponse3 which takes the session instead of uid
Not necessary either. Treat AuthenticationResponse and AuthenticationResponse2 the same, ignoring the UID.
>  - Change polkitd to look up cookies scoped to that rather than the uid
Change polkitd to:
- After looking up the subject’s PolkitUnixSession (to check the active/local/any state), keep a reference to it and store it in AuthenticationSession.
- In AuthenticationResponse*, (ignore the UID if any), look up the PolkitUnixSession of the caller (= of the agent helper, presumably = of the agent), and ensure that it matches the session stored within the AuthenticationSession referenced by the cookie (= that the helper and the subject live in the same PolkitUnixSession).

>  - Back out the "random cookie" changes and just go with a guint64 counter
> incremented per session-authsession pair ?
AFAICT the (agent counter, auth session counter) pair is sufficient but I don’t think the extra randomness hurts anything. Probably document that the lack of collisions is the absolutely necessary property, and randomness is just an extra precaution.


Having the ~ultimately sd_pid_get_session() lookup in polkitd instead of the agent helper is simpler because we don’t need to change the D-Bus API again, and it should be equally secure (the helper is running as euid 0, i.e. not an attacker ~by definition, and we know that it is blocking for a response to the method call, so the PID→session lookup is not racy).

(In reply to Miloslav Trmac from comment #37)
> 
> - In AuthenticationResponse*, (ignore the UID if any), look up the
> PolkitUnixSession of the caller (= of the agent helper, presumably = of the
> agent), and ensure that it matches the session stored within the
> AuthenticationSession referenced by the cookie (= that the helper and the
> subject live in the same PolkitUnixSession).

Ah yes, of course, which we have via dbus.  OK.  

> >  - Back out the "random cookie" changes and just go with a guint64 counter
> > incremented per session-authsession pair ?
> AFAICT the (agent counter, auth session counter) pair is sufficient but I
> don’t think the extra randomness hurts anything. Probably document that the
> lack of collisions is the absolutely necessary property, and randomness is
> just an extra precaution.

Right, agreed.

-- 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/39.

Glad to find an informative article that not only share details but easily explain how to bind use of cookies for specific-uids patch. Thank you so much for providing attachment of it. 

Caroline,
Personal statement tutor, who design attractive nursing personal statement - http://www.personalstatementfolks.co.uk/nursing-personal-statement/ using UCAS guidelines at Personal Statement Folks in UK.