On Tue, 2013-11-19 at 18:15 +0100, Martin Pitt wrote: > For the record, I much prefer something like this to my original patch > which simply unsets it. I just shied away from that as Lennart > repeatedly said on the RHBZ bug that he doesn't want su behave that > way.
This is a complex discussion because there are many different cases. As I mentioned before, I care primarily about the login as non-root, run pkexec to get a root shell case. And only a shell - no $DISPLAY, no pulseaudio. What Lennart was more referring to in the RHBZ was the people asking for more than that out of "su", like $DISPLAY proxying. But from the PolicyKit side, we're trying hard to move people away from that. Then there's the case of going root -> non-root. The below patch doesn't affect that. And these can all be combined, e.g. you can go non-root -> root -> different non-root. That's actually not that crazy; login as your user, become root, then "sudo -u mysql" to admin the database. > I disagree, but his word counts more than mine in this situation, > so I at least want to stop sessions using the wrong runtime dir. Right. > If logind would actually give you the session data for the uid you > call it for, instead of only looking at the seat/logind session data, > that would indeed be more useful/correct in my opinion. Doing > "~user$ su - otheruser" or "ssh otheruser@localhost" should effectively behave > the same, but right now logind gives you the session info for ~user in > the first, and for ~otheruser in the second case. The tricky thing is it's not just about the data - it's the *lifecycle* of that data. If we hand out an XDG_RUNTIME_DIR, we need to ensure it isn't garbage collected by logind. This new patch is a laser-targeted fix for what I consider the #1 most important case of non-root gdm -> pkexec/sudo. We solve the lifecycle issue for /run/user/0 in a simple way - it just always exists now via tmpfiles.d. Something else in systemd is creating that currently anyways. Solving some of the other cases would be tricky; we'd have to make them be at least partial sessions to ensure that the XDG_RUNTIME_DIR for their uid stays around. We could do this on the systemd side with explicit "child" sessions or so, but that's a high degree of complexity that I'm not sure is warranted right now. Perhaps the real fix would just be for a special kernel automount type setup that where a process of uid N reads /run/userdir it is a symlink to /run/user/N which is automatically mounted as a tmpfs. Anyways, new tested patch attached. Lennart, any objections?
>From 98da613a2dfcf4bb6bee709f29aba142cd34f118 Mon Sep 17 00:00:00 2001 From: Colin Walters <walt...@verbum.org> Date: Tue, 19 Nov 2013 10:21:16 -0500 Subject: [PATCH] login: Use correct XDG_RUNTIME_DIR for uid 0 with "pkexec" and similar This fixes a special case of the more general issue of uid-changing programs such as "pkexec/su/sudo/runuser". For the case where a user logs into a desktop and then runs "pkexec bash" or "sudo su -" or equivalent to become root, we should ensure that XDG_RUNTIME_DIR contains the correct information. Previous to this patch, we'd retain the incorrect XDG_RUNTIME_DIR from the current session. There bad things can happen such as a uid 0 process writing files into uid 1000's runtime dir, corrupting it. However, for changing to a *non-root* uid, we now leave XDG_RUNTIME_DIR unset, because we cannot guarantee it exists for the new uid. This occurs with "sudo mysql" or variants as recommended by some MySQL documentation. But those tools long predate the runtime dir, and they likely don't even use it. If they do, the'll now just fall back to the pre-runtime dir code. See https://bugzilla.redhat.com/show_bug.cgi?id=753882 See http://lists.freedesktop.org/archives/systemd-devel/2013-November/014370.html --- src/login/logind-dbus.c | 26 +++++++++++++++++++++++--- src/login/pam-module.c | 10 ++++++---- tmpfiles.d/systemd.conf | 1 + 3 files changed, 30 insertions(+), 7 deletions(-) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index 0f25e23..7cb6d36 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -553,10 +553,30 @@ 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; + 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" or "pkexec" which changes uid. + * Here we avoid creating a full new session (since we + * don't have "child" sessions), so we're just reusing + * the existing session data almost unchanged. + * + * One exception to this is the runtime path. See + * https://bugzilla.redhat.com/show_bug.cgi?id=753882 + * for some discussion. + */ + if (uid == 0) { + /* For uid 0, we always have a runtime dir that's created + * via tmpfiles.d. + */ + runtime_path = "/run/user/0"; + } else { + /* PAM module should treat this as unset; this means that + * in the case of e.g. "su - mysql", there will be no XDG_RUNTIME_DIR. + * Which is likely fine for them. + */ + runtime_path = ""; + } fifo_fd = session_create_fifo(session); if (fifo_fd < 0) @@ -570,7 +590,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 68cba2c..419f582 100644 --- a/src/login/pam-module.c +++ b/src/login/pam-module.c @@ -379,10 +379,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 (!isempty(runtime_path)) { + 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)) { diff --git a/tmpfiles.d/systemd.conf b/tmpfiles.d/systemd.conf index a05c657..18d4688 100644 --- a/tmpfiles.d/systemd.conf +++ b/tmpfiles.d/systemd.conf @@ -8,6 +8,7 @@ # See tmpfiles.d(5) for details d /run/user 0755 root root ~10d +d /run/user/0 0755 root root 10d F /run/utmp 0664 root utmp - f /var/log/wtmp 0664 root utmp - -- 1.7.1
_______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
