[systemd-devel] pam: Don't use loginuid [was: Re: Fix PAM module to not clobber XDG_RUNTIME_DIR with su]
Lennart Poettering
lennart at poettering.net
Wed Nov 20 15:32:39 PST 2013
On Tue, 19.11.13 10:42, Colin Walters (walters at verbum.org) wrote:
> My patch though starts to pave the way for having XDG_RUNTIME_DIR
> consistently point to that of the user's uid
I think this is just bogus. You used "su". So the UID was changed and
little else. Now you start patchign XDG_RUNTIME_DIR too, but what about
access to X11? This kind of "mix and match" is just really really
broken, since in many cases there are connections beteween the
facilities people use. i.e. some apps require that dbus, x11, pa and
XDG_RUNTIME_DIR are always run in the same combination. For example,
dconf requires dbus and XDG_RUNTIME_DIR to stay in sync. PA requires
expects XDG_RUNTIME_DIR and X11 to be ins ync. dbus mantains X11 root
window creds, so binds things together with X11. So yeah, there your mix
and match is broken: if you switch over some things of it things
break... And if we don't get this right for the lower level components,
how on earth do you think that non-trivial user applicatins will ever
get this right?
It's so entirely arbitrary switching some things over and others not. I
am pretty sure the only sensible thing is to say that "su" and "sudo"
are minimal, and doesn't switch anything over but the bare minimum,
which is the UID. It stays inside the same XDG_RUNTIME_DIR, PA, X11,
dbus, logind session id and so on.
And then, make "su -" and "sudo -s" switch over as much as possible,
i.e. XDG_RUNTIME_DIR, PA, X11, dbus, logind session id.
For that, add a new parameter to pam_systemd maybe
"force-new-session=yes/no" or so which is set in "su -"'s PAM config
stack, but not in "su"'s PAM config stack (luckily they are stored in
two separate files). When the boolean is set, the session ID is not
created from the existing session, but a new independent one is made up.
I offered this somewhere in rhbz alread...
Note however, that his will not allow people to su to another user and
then run firefox from there. If they do that with "su -" the display and
audio should have been disconnected. If they do that with "su", then the
app won't be able to access much in XDG_RUNTIME_DIR...
Lennart
>
> >From 7fe51a82a5cc1ad2185b4ee4faa087f17e27d24a Mon Sep 17 00:00:00 2001
> From: Colin Walters <walters at verbum.org>
> Date: Tue, 19 Nov 2013 10:21:16 -0500
> Subject: [PATCH] login: Make XDG_RUNTIME_DIR match target uid with "pkexec/sudo/su"
>
> These get attached to the same session, but we should ensure that the
> XDG_RUNTIME_DIR is matches the new uid. Otherwise bad things can
> happen such as a uid 0 process writing files into uid 1000's runtime
> dir, corrupting it.
>
> This patch currently has the semantics that if you log in via e.g. gdm
> as non-root and then use pkexec, the new shell will have *no*
> XDG_RUNTIME_DIR.
>
> If however root is logged in "for real" elsewhere via getty or ssh,
> then the XDG_RUNTIME_DIR will be set. However, once root logs out,
> that directory will be garbage collected, which is probably
> undesirable. We could either:
>
> 1) Attempt to make "pkexec/su/sudo" have real sessions (invasive)
> 2) Drop this hunk of the patch, and *always* leave XDG_RUNTIME_DIR
> unset (simple)
> 3) Add internal refcounting for XDG_RUNTIME_DIR such that the user
> session holds a ref on the root-owned runtime dir (fairly noninvasive)
> ---
> src/login/logind-dbus.c | 17 ++++++++++++++---
> src/login/pam-module.c | 10 ++++++----
> 2 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c
> index 0f25e23..397750d 100644
> --- a/src/login/logind-dbus.c
> +++ b/src/login/logind-dbus.c
> @@ -553,10 +553,21 @@ static int method_create_session(sd_bus *bus, sd_bus_message *message, void *use
> if (session) {
> _cleanup_free_ char *path = NULL;
> _cleanup_close_ int fifo_fd = -1;
> + User *target_user;
> + const char *runtime_path;
>
> /* Session already exists, client is probably
> - * something like "su" which changes uid but is still
> - * the same session */
> + * something like "su" which changes uid. We want to join
> + * it to the same session. One exception to this is the runtime
> + * path.
> + * See https://bugzilla.redhat.com/show_bug.cgi?id=753882
> + * for some discussion.
> + */
> + target_user = hashmap_get(m->users, ULONG_TO_PTR((unsigned long) uid));
> + if (target_user)
> + runtime_path = target_user->runtime_path;
> + else
> + runtime_path = ""; /* PAM module should treat this as unset */
>
> fifo_fd = session_create_fifo(session);
> if (fifo_fd < 0)
> @@ -570,7 +581,7 @@ static int method_create_session(sd_bus *bus, sd_bus_message *message, void *use
> bus, message, "soshsub",
> session->id,
> path,
> - session->user->runtime_path,
> + runtime_path,
> fifo_fd,
> session->seat ? session->seat->id : "",
> (uint32_t) session->vtnr,
> diff --git a/src/login/pam-module.c b/src/login/pam-module.c
> index 1975d80..f741b27 100644
> --- a/src/login/pam-module.c
> +++ b/src/login/pam-module.c
> @@ -385,10 +385,12 @@ _public_ PAM_EXTERN int pam_sm_open_session(
> return r;
> }
>
> - r = pam_misc_setenv(handle, "XDG_RUNTIME_DIR", runtime_path, 0);
> - if (r != PAM_SUCCESS) {
> - pam_syslog(handle, LOG_ERR, "Failed to set runtime dir.");
> - return r;
> + if (runtime_path && runtime_path[0]) {
> + r = pam_misc_setenv(handle, "XDG_RUNTIME_DIR", runtime_path, 0);
> + if (r != PAM_SUCCESS) {
> + pam_syslog(handle, LOG_ERR, "Failed to set runtime dir.");
> + return r;
> + }
> }
>
> if (!isempty(seat)) {
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Lennart
--
Lennart Poettering, Red Hat
More information about the systemd-devel
mailing list