Bug 76358

Summary: Please use sd_uid_get_state() to make security decisions, not sd_pid_get_session()
Product: PolicyKit Reporter: Lennart Poettering <lennart>
Component: daemonAssignee: David Zeuthen (not reading bugmail) <zeuthen>
Status: RESOLVED FIXED QA Contact: David Zeuthen (not reading bugmail) <zeuthen>
Severity: normal    
Priority: medium CC: bugzilla, walters
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: sessionmonitor-systemd: Use sd_uid_get_state() to check session activity
sessionmonitor-systemd: Use sd_uid_get_state() to check session activity

Description Lennart Poettering 2014-03-19 13:50:14 UTC 
We are working on making the desktop session work correctly with "systemd --user" as user service manager. This makes one major change: most user code will not run inside the logind session anymore, but will be forked off the per-user singleton service user@.service. This currently confuses Polkit quite a bit, since it wants to make security decisions based on whether processes are in foreground sessions or not, but this way most user processes won't be in any session at all, neither foreground nor background.

I'd like to see polkit changed to no longer check whether a process is in an fg session or not, but instead whether a process is owned by a user that has at least one fg session. This is more in-line with traditional UNIX access checks anyway, since on UNIX access is granted to users, never to specific sessions. Moreover, trying to lock down specific sessions is easily circumventable anyway...

So, long story short: currently polkit uses sd_pid_get_session() and then sd_session_is_active(), to make its decisions. Instead, please use sd_uid_get_state() and check whether the user is "active" or just "online". See sd_uid_get_state(3) for details. This should actually make the code of polkit quite a bit simpler, too.
Comment 1 Colin Walters 2014-06-03 18:06:14 UTC 
This is still a nice-to-have after https://bugs.freedesktop.org/show_bug.cgi?id=78905 right?

But not strictly necessary since we're falling back to looking at the primary session?
Comment 2 Simon McVittie 2015-03-31 07:57:29 UTC 
See also Bug #67728.
Comment 3 Philip Withnall 2015-06-02 15:27:07 UTC 
Created attachment 116248 [details] [review]
sessionmonitor-systemd: Use sd_uid_get_state() to check session activity

Instead of using sd_pid_get_session() then sd_session_is_active() to
determine whether the user is active, use sd_uid_get_state() directly.
This gets the maximum of the states of all the user’s sessions, rather
than the state of the session containing the subject process. Since the
user is the security boundary, this is fine.

This change is necessary for `systemd --user` sessions, where most user
code will be forked off user@.service, rather than running inside the
logind session (whether that be a foreground/active or background/online
session).

Policy-wise, the change is from checking whether the subject process is
in an active session; to checking whether the subject process is owned
by a user with at least one active session.
Comment 4 Colin Walters 2015-06-02 16:13:49 UTC 
Comment on attachment 116248 [details] [review]
sessionmonitor-systemd: Use sd_uid_get_state() to check session activity

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

Two minor things.  Want to trade for review of http://lists.freedesktop.org/archives/polkit-devel/2015-May/000421.html ?  I can file as a bug if it's easier.

::: src/polkitbackend/polkitbackendsessionmonitor-systemd.c
@@ +399,5 @@
> +  g_debug ("Checking whether session %s is active.", session_id);
> +
> +  /* Check whether *any* of the user's current sessions are active. */
> +  if (sd_session_get_uid (session_id, &uid) < 0)
> +      goto fallback;

polkit is GNU, 2 space indents please.

@@ +404,5 @@
> +
> +  g_debug ("Session %s has UID %u.", session_id, uid);
> +
> +  if (sd_uid_get_state (uid, &state) < 0)
> +      goto fallback;

In this case we leak `state`
Comment 5 Philip Withnall 2015-06-03 13:18:43 UTC 
Created attachment 116266 [details] [review]
sessionmonitor-systemd: Use sd_uid_get_state() to check session activity

Instead of using sd_pid_get_session() then sd_session_is_active() to
determine whether the user is active, use sd_uid_get_state() directly.
This gets the maximum of the states of all the user’s sessions, rather
than the state of the session containing the subject process. Since the
user is the security boundary, this is fine.

This change is necessary for `systemd --user` sessions, where most user
code will be forked off user@.service, rather than running inside the
logind session (whether that be a foreground/active or background/online
session).

Policy-wise, the change is from checking whether the subject process is
in an active session; to checking whether the subject process is owned
by a user with at least one active session.

---

Updated patch to fix the style issue.

> @@ +404,5 @@
> > +
> > +  g_debug ("Session %s has UID %u.", session_id, uid);
> > +
> > +  if (sd_uid_get_state (uid, &state) < 0)
> > +      goto fallback;
> 
> In this case we leak `state`

Nope, sd_uid_get_state() does not return a state on failure (returning < 0).

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.