'Twas brillig, and Colin Guthrie at 14/11/13 09:48 did gyre and gimble: > Hi Martin, > > Thanks for looking at this. > > 'Twas brillig, and Martin Pitt at 14/11/13 07:45 did gyre and gimble: >> pam_systemd currently causes some havoc when you run programs or >> shells with su: it passes on the $XDG_RUNTIME_DIR from the original >> user session, so that programs like pulseaudio or dconf end up >> scribbling into the original user's runtime dir. This has been >> discussed at length at [1][2] and is leading people to consider >> workarounds like [3]. >> >> It seems Lennart is against giving the new user a new logind session >> and runtime dir; I think it would be right to give it a fresh (or an >> already existing one for the target user) runtime dir, but in either >> case passing it the original user's runtime dir is actively wrong and >> harmful. >> >> Until then I recommend applying this patch (or something equivalent) >> which at least stops destroying existing runtime dirs and makes it >> compliant to the spec [4]. With that, things like pulse, dconf, or >> dbus will still need to keep their internal fallback if there is no >> runtime dir, but that's a less pressing matter. >> >> Thanks for considering, >> >> Martin >> >> [1] https://bugzilla.redhat.com/show_bug.cgi?id=753882 >> [2] https://launchpad.net/bugs/1197395 >> [3] >> http://lists.freedesktop.org/archives/pulseaudio-discuss/2013-November/019121.html >> [4] http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html > > I'm somewhat on the fence, but I think this patch is sensible in the > short term at least. > > I do still think we need some kind of new su which is actually able to > properly proxy graphics and sound (like SSH kinda does - at least for > graphics), but this should prevent the nasty side effects in the short term. > > I've not considered any unwanted side effects this may cause so > hopefully someone else can chime in accordingly. > > Your argument about it making it spec compliant seems rather compelling > tho'.
OK, I just tried this but I can't seem to make it work and prevent the XDG_* vars being set. I applied the attached variation to my 208 build and then ran "pkexec /bin/bash" which also suffers from the same problems. pkexec cleans out the environment quite well, but then pam_systemd re-injects these variables (I discussed this on IRC the other day with Colin Walters). I would have thought that this should have fixed things. I didn't get any joy from su or su - either (although I'd expect su to still have it set due to not cleaning the environment - is this a correct assumption?). The problem is that the pw_uid in this case is creating the session for my user, not the destination user (note the UID 603): Nov 14 10:25:45 jimmy pkexec[14287]: pam_systemd(polkit-1:session): Asking logind to create session: uid=603 pid=14287 service=polkit-1 type=unspecified class=background seat= vtnr=0 tty= display= remote=no remote_user= remote_host= Nov 14 10:25:45 jimmy pkexec[14287]: pam_systemd(polkit-1:session): Reply from logind: id=2 object_path=/org/freedesktop/login1/session/_32 runtime_path=/run/user/603 session_fd=10 seat=seat0 vtnr=1 Ditto for su: Nov 14 10:27:00 jimmy su[14355]: pam_systemd(su:session): Asking logind to create session: uid=603 pid=14355 service=su type=tty class=user seat=seat0 vtnr=1 tty=pts/2 display= remote=no remote_user=colin remote_host= Nov 14 10:27:00 jimmy su[14355]: pam_systemd(su:session): Reply from logind: id=2 object_path=/org/freedesktop/login1/session/_32 runtime_path=/run/user/603 session_fd=6 seat=seat0 vtnr=1 And su -: Nov 14 10:27:22 jimmy su[14419]: pam_systemd(su-l:session): Asking logind to create session: uid=603 pid=14419 service=su-l type=tty class=user seat=seat0 vtnr=1 tty=pts/2 display= remote=no remote_user=colin remote_host= Nov 14 10:27:22 jimmy su[14419]: pam_systemd(su-l:session): Reply from logind: id=2 object_path=/org/freedesktop/login1/session/_32 runtime_path=/run/user/603 session_fd=6 seat=seat0 vtnr=1 Thus the comparison of the dir owner and the uid succeeds. Am I missing something? Col -- Colin Guthrie gmane(at)colin.guthr.ie http://colin.guthr.ie/ Day Job: Tribalogic Limited http://www.tribalogic.net/ Open Source: Mageia Contributor http://www.mageia.org/ PulseAudio Hacker http://www.pulseaudio.org/ Trac Hacker http://trac.edgewall.org/
>From b516dd4ea5187bfa9b5a237a0f50edc53f401468 Mon Sep 17 00:00:00 2001 From: Martin Pitt <[email protected]> Date: Wed, 13 Nov 2013 13:02:28 +0100 Subject: [PATCH] pam: Check $XDG_RUNTIME_DIR owner http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html requires that $XDG_RUNTIME_DIR "MUST be owned by the user, and he MUST be the only one having read and write access to it.". Don't set an existing $XDG_RUNTIME_DIR in the PAM module if it isn't owned by the session user. Otherwise su sessions get a runtime dir from a different user which leads to either permission errors or scribbling over the other user's files. https://bugzilla.redhat.com/show_bug.cgi?id=753882 https://launchpad.net/bugs/1197395 Conflicts: src/login/pam-module.c --- src/login/pam-module.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/login/pam-module.c b/src/login/pam-module.c index 49296b5..d5872dd 100644 --- a/src/login/pam-module.c +++ b/src/login/pam-module.c @@ -194,6 +194,7 @@ _public_ PAM_EXTERN int pam_sm_open_session( dbus_bool_t remote, existing; int r; uint32_t vtnr = 0; + struct stat st; assert(handle); @@ -408,12 +409,27 @@ _public_ PAM_EXTERN int pam_sm_open_session( goto finish; } - 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."); + /* only set $XDG_RUNTIME_DIR if it is owned by the target user, as per + * XDG basedir-spec; this avoids su sessions to scribble over a runtime + * dir of a different user */ + r = lstat(runtime_path, &st); + if (r != 0) { + pam_syslog(handle, LOG_ERR, "Failed to stat runtime dir: %s", strerror(errno)); + r = PAM_SYSTEM_ERR; goto finish; } + if (st.st_uid == uid) { + 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."); + goto finish; + } + } else { + pam_syslog(handle, LOG_DEBUG, "Runtime dir %s is not owned by the target uid %u, ignoring.", + runtime_path, uid); + } + if (!isempty(seat)) { r = pam_misc_setenv(handle, "XDG_SEAT", seat, 0); if (r != PAM_SUCCESS) { -- 1.8.4.3
_______________________________________________ systemd-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/systemd-devel
