polkit-org / polkit

polkit (formerly PolicyKit) is a toolkit for defining and handling authorizations. It is used for allowing unprivileged processes to speak to privileged processes.
Other
75 stars 35 forks source link

polkit-agent-helper-1 is setuid root and runnable by ordinary users, does it need to be? #169

Open polkit-github-migration-bot opened 2 years ago

polkit-github-migration-bot commented 2 years ago

In gitlab.freedesktop.org by smcv on Feb 2, 2022, 23:29

Link to the original issue: https://gitlab.freedesktop.org/polkit/polkit/-/issues/168 I've been thinking about ways to reduce setuid attack surface to avoid things like CVE-2021-4034 happening in the future.

polkit currently installs polkit-agent-helper-1, which is used by polkit agents to re-authenticate a user. This is typically done by running a PAM stack, which is required to be done as root and with privileges. The -shadow implementation bypasses PAM and reads /etc/shadow directly, which similarly requires privileges.

setuid executables run in a hostile environment: a potential attacker controls every aspect of their execution environment (environment variables, argv, open fds, controlling terminal, signal dispositions...), and if setuid executable makes a security-sensitive assumption about any of those process parameters, an attacker might be able to exploit that.

One possibility to harden this would be to use systemd socket-activation to launch one instance of something functionally equivalent to the current polkit-agent-helper-1 (except not setuid, and with the user to authenticate written to its stdin instead of being passed in argv) per connection to a socket. libpolkit-agent-1 would run a separate non-setuid program, reusing the current polkit-agent-helper-1 filename for backwards compatibility, which would just connect to the socket and proxy the PAM conversation. Non-systemd systems would have to either continue to build and install a setuid polkit-agent-helper-1 (no improvement, but no regression either), or implement socket-activation with inetd or something.

Another possible route would be for the program that is run by libpolkit-agent-1 to be unprivileged, and just make a D-Bus call to polkitd, fd-passing a pair of pipes (or maybe a single AF_UNIX socket). polkitd could execute a setuid program (directly equivalent to the current helper) installed with rwsr-x--- permissions and owned by root:polkitd (this would need a new polkitd group, obviously), so that users other than polkitd cannot possibly exploit it via creative choices of execution environment. This is similar to the way dbus-daemon-launch-helper is setuid root, but only the system bus uid (typically dbus or messagebus) can run it.

(In old versions of polkit with less privilege-separation, this would ironically have been easier, because polkitd ran as root and so no setuid would have been required.)

This would also allow rate-limiting authentication attempts, if that's desirable: if using systemd socket activation, this would be systemd configuration, or if using D-Bus calls to polkitd, it would be up to polkitd.

polkit-github-migration-bot commented 2 years ago

In gitlab.freedesktop.org by smcv on Feb 3, 2022, 01:45

Limitations of both the routes I suggested:

polkit-github-migration-bot commented 2 years ago

In gitlab.freedesktop.org by clx814727823 on Jul 19, 2022, 08:39

The PAM (or equivalent) code obviously still needs to run with privileges

Why? I mean that, in fact, pam_unix.so will fork and exec unix_chkpwd, which have suid to check /etc/shadow.

check https://github.com/linux-pam/linux-pam/blob/b872b6e68a60ae351ca4c7eea6dfe95cd8f8d130/modules/pam_unix/support.c#L469

So why we need a suid helper at all?

bluca commented 2 months ago

I've opted for a slight variation: the agent will talk directory to the socket-activated privileged helper via AF_UNIX, instead of going through a third intermediate binary. The privileged socket-activated helper will identify the caller via PID FD so that it is safe against reuse attacks.

soloturn commented 1 month ago

@poettering you said once run0 does not depend on a setuid binary. here is one left.