0001-CVE-XXX-XXXX-Bind-use-of-cookies-to-specific-uids.patch

polkit-github-migration-bot commented 9 years ago

In gitlab.freedesktop.org by bugzilla-migration on Jun 4, 2015, 03:44

Link to the original issue: https://gitlab.freedesktop.org/polkit/polkit/-/issues/39

Submitted by Colin Walters `@walters`

Assigned to David Zeuthen @david

Link to original bug (#90837)

Description

Created attachment 116273 0001-CVE-XXX-XXXX-Bind-use-of-cookies-to-specific-uids.patch

Note: CVE not assigned yet

http://lists.freedesktop.org/archives/polkit-devel/2015-June/000425.html

The "cookie" value that Polkit hands out is global to all polkit users. And when AuthenticationAgentResponse is invoked, we previously only received the cookie and target identity, and attempted to find an agent from that.

The problem is that the current cookie is just an integer counter, and if it overflowed, it would be possible for an successful authorization in one session to trigger a response in another session.

One way to fix this would be to make the cookie unforgeable, but this approach passes through the uid of the caller from the setuid binary, ensuring that we only look up AuthenticationAgents that were created by a matching uid.

Signed-off-by: Colin Walters walters@verbum.org

~~Attachment 116273~~, "0001-CVE-XXX-XXXX-Bind-use-of-cookies-to-specific-uids.patch":
0001-CVE-XXX-XXXX-Bind-use-of-cookies-to-specific-uids.patch

polkit-github-migration-bot commented 9 years ago

In gitlab.freedesktop.org by bugzilla-migration on Jun 4, 2015, 04:07

:speech_balloon: Colin Walters @walters said:

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

polkit-github-migration-bot commented 9 years ago

In gitlab.freedesktop.org by bugzilla-migration on Jun 4, 2015, 15:53

:hammer_and_wrench: Colin Walters @walters submitted a patch:

~~Patch 116289~~, "0001-authority-Explicitly-operate-on-UnixUser-uids.patch":
0001-authority-Explicitly-operate-on-UnixUser-uids.patch

polkit-github-migration-bot commented 9 years ago

In gitlab.freedesktop.org by bugzilla-migration on Jun 4, 2015, 15:53

:hammer_and_wrench: Colin Walters @walters submitted a patch:

~~Patch 116290~~, "0001-CVE-XXX-XXXX-Bind-use-of-cookies-to-specific-uids.patch":
0001-CVE-XXX-XXXX-Bind-use-of-cookies-to-specific-uids.patch

polkit-github-migration-bot commented 9 years ago

In gitlab.freedesktop.org by bugzilla-migration on Jun 4, 2015, 23:39

:speech_balloon: Colin Walters @walters said:

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

polkit-github-migration-bot commented 9 years ago

In gitlab.freedesktop.org by bugzilla-migration on Jun 5, 2015, 23:40

:speech_balloon: Miloslav Trmac commented on patch 116289:

Review of attachment 116289:

::: 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.

polkit-github-migration-bot commented 9 years ago

In gitlab.freedesktop.org by bugzilla-migration on Jun 6, 2015, 24:05

:speech_balloon: Miloslav Trmac commented on patch 116290:

Review of attachment 116290:

::: 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">`

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">

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.

polkit-github-migration-bot commented 9 years ago

In gitlab.freedesktop.org by bugzilla-migration on Jun 6, 2015, 24:43

:speech_balloon: Colin Walters @walters said:

(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.

polkit-github-migration-bot commented 9 years ago

In gitlab.freedesktop.org by bugzilla-migration on Jun 6, 2015, 01:01

:speech_balloon: Miloslav Trmac said:

(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.

polkit-github-migration-bot commented 9 years ago

In gitlab.freedesktop.org by bugzilla-migration on Jun 6, 2015, 16:42

:speech_balloon: Colin Walters @walters said:

(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.

polkit-github-migration-bot commented 9 years ago

In gitlab.freedesktop.org by bugzilla-migration on Jun 7, 2015, 16:09

:hammer_and_wrench: Colin Walters @walters submitted a patch:

~~Patch 116337~~, "0001-authority-Add-a-helper-method-for-checking-whether-a.patch":
0001-authority-Add-a-helper-method-for-checking-whether-a.patch

polkit-github-migration-bot commented 9 years ago

In gitlab.freedesktop.org by bugzilla-migration on Jun 7, 2015, 16:09

:hammer_and_wrench: Colin Walters @walters submitted a patch:

Patch 116338, "0002-CVE-XXX-XXXX-Bind-use-of-cookies-to-specific-uids.patch":
0002-CVE-XXX-XXXX-Bind-use-of-cookies-to-specific-uids.patch

polkit-github-migration-bot commented 9 years ago

In gitlab.freedesktop.org by bugzilla-migration on Jun 7, 2015, 16:14

:speech_balloon: Colin Walters @walters said:

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.

polkit-github-migration-bot commented 9 years ago

In gitlab.freedesktop.org by bugzilla-migration on Jun 10, 2015, 18:38

:speech_balloon: Miloslav Trmac commented on patch 116337:

Review of attachment 116337:

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.

polkit-github-migration-bot commented 9 years ago

In gitlab.freedesktop.org by bugzilla-migration on Jun 10, 2015, 20:18

:speech_balloon: Colin Walters @walters commented on patch 116337:

Committed 1/2.

polkit-github-migration-bot commented 9 years ago

In gitlab.freedesktop.org by bugzilla-migration on Jun 17, 2015, 01:04

:hammer_and_wrench: Miloslav Trmac submitted a 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.

Patch 116542, "0001-Additional-code-fixes.patch":
0001-Additional-code-fixes.patch

polkit-github-migration-bot commented 9 years ago

In gitlab.freedesktop.org by bugzilla-migration on Jun 17, 2015, 01:15

:hammer_and_wrench: Miloslav Trmac submitted a 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.

Patch 116543, "0002-Documentation-fixes-and-additions.patch":
0002-Documentation-fixes-and-additions.patch

polkit-github-migration-bot commented 9 years ago

In gitlab.freedesktop.org by bugzilla-migration on Jun 17, 2015, 01:17

:speech_balloon: Miloslav Trmac said:

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

polkit-github-migration-bot commented 9 years ago

In gitlab.freedesktop.org by bugzilla-migration on Jun 17, 2015, 20:03

:speech_balloon: Colin Walters @walters said:

(In reply to Miloslav Trmac from comment 16) Created attachment 116543 [details] [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.

polkit-github-migration-bot commented 9 years ago

In gitlab.freedesktop.org by bugzilla-migration on Jun 17, 2015, 20:05

:speech_balloon: Colin Walters @walters said:

(In reply to Miloslav Trmac from comment 15) 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.

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

polkit-github-migration-bot commented 9 years ago

In gitlab.freedesktop.org by bugzilla-migration on Sep 1, 2015, 03:25

:speech_balloon: Miloslav Trmac said:

Revisiting this, because it breaks TTY sessions:

(Create an unprivileged account.)
Log in using the unprivileged account ON A CONSOLE (not using (su -) in a terminal window, that will inherit the GUI session)
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:

pkexec is started, with ruid=$non-zero euid=0
pkexec registers a local authentication agent.
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.
pkexec calls CheckAuthorization, which results in a callback to its agent.
The agent runs polkit-agent-helper-1, getting euid=0 and inheriting ruid=$non-zero
The agent calls AuthenticationAgentResponse2 with uid=$ruid=$non-zero
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?

polkit-github-migration-bot commented 9 years ago

In gitlab.freedesktop.org by bugzilla-migration on Sep 1, 2015, 04:06

:speech_balloon: Colin Walters @walters said:

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.

polkit-github-migration-bot commented 8 years ago

In gitlab.freedesktop.org by bugzilla-migration on Oct 15, 2015, 22:09

:speech_balloon: Florian Hubold said:

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

(Create an unprivileged account.)

Log in using the unprivileged account ON A CONSOLE (not using (su -) in a terminal window, that will inherit the GUI session)

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:

pkexec is started, with ruid=$non-zero euid=0

pkexec registers a local authentication agent.

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.

pkexec calls CheckAuthorization, which results in a callback to its agent.

The agent runs polkit-agent-helper-1, getting euid=0 and inheriting ruid=$non-zero

The agent calls AuthenticationAgentResponse2 with uid=$ruid=$non-zero

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?

polkit-github-migration-bot commented 8 years ago

In gitlab.freedesktop.org by bugzilla-migration on Oct 15, 2015, 22:11

:speech_balloon: Florian Hubold said:

(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"

polkit-github-migration-bot commented 8 years ago

In gitlab.freedesktop.org by bugzilla-migration on Oct 15, 2015, 22:39

:speech_balloon: Miloslav Trmac said:

(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.

polkit-github-migration-bot commented 8 years ago

In gitlab.freedesktop.org by bugzilla-migration on Oct 16, 2015, 24:11

:speech_balloon: Colin Walters @walters said:

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.

polkit-github-migration-bot commented 8 years ago

In gitlab.freedesktop.org by bugzilla-migration on Feb 23, 2016, 20:26

:speech_balloon: Andy Wingo said:

(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.

polkit-github-migration-bot commented 8 years ago

In gitlab.freedesktop.org by bugzilla-migration on Feb 24, 2016, 18:40

:speech_balloon: Miloslav Trmac said:

(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?

polkit-github-migration-bot commented 8 years ago

In gitlab.freedesktop.org by bugzilla-migration on Feb 24, 2016, 19:28

:speech_balloon: Colin Walters @walters said:

(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

polkit-github-migration-bot commented 8 years ago

In gitlab.freedesktop.org by bugzilla-migration on Feb 24, 2016, 21:34

:speech_balloon: Andy Wingo said:

(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.

polkit-github-migration-bot commented 8 years ago

In gitlab.freedesktop.org by bugzilla-migration on Feb 25, 2016, 16:46

:speech_balloon: Andy Wingo said:

(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?

polkit-github-migration-bot commented 8 years ago

In gitlab.freedesktop.org by bugzilla-migration on Feb 25, 2016, 17:44

:speech_balloon: Miloslav Trmac said:

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

polkit-github-migration-bot commented 8 years ago

In gitlab.freedesktop.org by bugzilla-migration on Feb 25, 2016, 18:30

:speech_balloon: Miloslav Trmac said:

(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.

polkit-github-migration-bot commented 8 years ago

In gitlab.freedesktop.org by bugzilla-migration on Feb 26, 2016, 15:44

:speech_balloon: Andy Wingo said:

(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.

polkit-github-migration-bot commented 8 years ago

In gitlab.freedesktop.org by bugzilla-migration on Mar 3, 2016, 21:41

:speech_balloon: Andy Wingo said:

(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 :)

polkit-github-migration-bot commented 8 years ago

In gitlab.freedesktop.org by bugzilla-migration on Mar 5, 2016, 24:02

:speech_balloon: Miloslav Trmac said:

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

polkit-github-migration-bot commented 8 years ago

In gitlab.freedesktop.org by bugzilla-migration on Mar 8, 2016, 02:49

:speech_balloon: Colin Walters @walters said:

(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 ?

polkit-github-migration-bot commented 8 years ago

In gitlab.freedesktop.org by bugzilla-migration on Mar 8, 2016, 15:49

:speech_balloon: Miloslav Trmac said:

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

polkit-github-migration-bot commented 8 years ago

In gitlab.freedesktop.org by bugzilla-migration on Mar 8, 2016, 19:44

:speech_balloon: Colin Walters @walters said:

(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.

polkit-github-migration-bot commented 5 years ago

In gitlab.freedesktop.org by azzaronea on Mar 27, 2019, 16:25

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.

What about binding the cookie to the UID of the subject instead of the caller?

polkit-github-migration-bot / t4_polkit