On Tue, Nov 26, 2013 at 02:39:49PM +0000, Colin Guthrie wrote: > 'Twas brillig, and Dr. Werner Fink at 26/11/13 14:21 did gyre and gimble: > > On Tue, Nov 26, 2013 at 10:41:36AM +0000, Colin Guthrie wrote: > >> 'Twas brillig, and Martin Pitt at 26/11/13 06:19 did gyre and gimble: > >>> Hey Lennart, > >>> > >>> Lennart Poettering [2013-11-26 5:12 +0100]: > >>>> I implemented this now, using a different approach than Martin's > >>>> original patch (i.e. I don't think it is a good idea to involve stat() > >>>> here, instead let's just let logind pass all information to > >>>> pam_systemd). > >>> > >>> Thanks! > >> > >> Indeed, thanks for this! > >> > >> If anyone backports this fix to v208 (i.e. pre sd-bus) please share it > >> here. I'll likely do it just to have the "upstream-blessed" fix, but > >> doubt I'll get around it it until later in the week. > > > > I've backported it. > > Can you link to it or attach it please?
Yep ...
> > But during tests I've found that it does not help
> > if the environment variable XDG_RUNTIME_DIR already exists before doing
> > su. It will not unset but exported.
>
> That's expected.
>
> su does not do any env cleaning, su - does, sudo does, pkexec does.
I'm aware ;) neverthelesss ...
>
> su's behaviour is to always not touch stuff and thus this is known and
> expected and has always been a problem.
>
> Longer term we need to solve this more holistically (hence why I've
> tried to get information about "how things should be done" in the future
> to start helping lay the ground work for thise bits), but this is still
> the best fix we have just now.
>
> For the specific case of su'ing to root, (which is the most common and
> potentially problematic one), I will probably use Colin W's most recent
> patch to have a static root runtime dir and for logind to set it. This
> should fix XDG_RUNTIME_DIR when su'ing to root. I'm not so worried about
> su'ing to other users (the damage that can be done is much more
> limited), but longer term we do need to address that nicely too IMO
> (which will likely need changes in su itself and a number of other places)
... I've a bug report here which indeed shows exactly the ``other'' problem.
If you do not like the enhancement in my backport you may replace
+ } else {
+ (void) unsetenv("XDG_RUNTIME_DIR");
+ r = pam_putenv(handle, "XDG_RUNTIME_DIR");
+ if (r != PAM_SUCCESS && r != PAM_BAD_ITEM) {
+ pam_syslog(handle, LOG_ERR, "Failed to unset runtime
dir.");
+ }
with `}' ... otherwise both the pam_putenv() and unsetenv() are required
to make sure that XDG_RUNTIME_DIR is removed.
Werner
--
"Having a smoking section in a restaurant is like having
a peeing section in a swimming pool." -- Edward Burr
Based on upstream baae0358f349870544884e405e82e4be7d8add9f | From: Lennart Poettering <[email protected]> | Date: Tue, 26 Nov 2013 04:05:00 +0000 | Subject: pam_systemd: do not set XDG_RUNTIME_DIR if the session's original user is not the same as the newly logged in one | It's better not to set any XDG_RUNTIME_DIR at all rather than one of a | different user. So let's do this. --- systemd-208/src/login/logind-dbus.c +++ systemd-208/src/login/logind-dbus.c 2013-11-26 13:37:05.730735774 +0000 @@ -523,6 +523,7 @@ static int bus_manager_create_session(Ma DBUS_TYPE_OBJECT_PATH, &path, DBUS_TYPE_STRING, &session->user->runtime_path, DBUS_TYPE_UNIX_FD, &fifo_fd, + DBUS_TYPE_UINT32, &session->user->uid, DBUS_TYPE_STRING, &cseat, DBUS_TYPE_UINT32, &vtnr, DBUS_TYPE_BOOLEAN, &exists, --- systemd-208/src/login/logind-session-dbus.c +++ systemd-208/src/login/logind-session-dbus.c 2013-11-26 13:36:07.478236401 +0000 @@ -755,6 +755,7 @@ int session_send_create_reply(Session *s DBUS_TYPE_OBJECT_PATH, &path, DBUS_TYPE_STRING, &s->user->runtime_path, DBUS_TYPE_UNIX_FD, &fifo_fd, + DBUS_TYPE_UINT32, &s->user->uid, DBUS_TYPE_STRING, &cseat, DBUS_TYPE_UINT32, &vtnr, DBUS_TYPE_BOOLEAN, &exists, --- systemd-208/src/login/pam-module.c +++ systemd-208/src/login/pam-module.c 2013-11-26 14:32:20.194235777 +0000 @@ -93,24 +93,18 @@ static int get_user_data( assert(ret_username); assert(ret_pw); - r = audit_loginuid_from_pid(0, &uid); - if (r >= 0) - pw = pam_modutil_getpwuid(handle, uid); - else { - r = pam_get_user(handle, &username, NULL); - if (r != PAM_SUCCESS) { - pam_syslog(handle, LOG_ERR, "Failed to get user name."); - return r; - } - - if (isempty(username)) { - pam_syslog(handle, LOG_ERR, "User name not valid."); - return PAM_AUTH_ERR; - } + r = pam_get_user(handle, &username, NULL); + if (r != PAM_SUCCESS) { + pam_syslog(handle, LOG_ERR, "Failed to get user name."); + return r; + } - pw = pam_modutil_getpwnam(handle, username); + if (isempty(username)) { + pam_syslog(handle, LOG_ERR, "User name not valid."); + return PAM_AUTH_ERR; } + pw = pam_modutil_getpwnam(handle, username); if (!pw) { pam_syslog(handle, LOG_ERR, "Failed to get user data."); return PAM_USER_UNKNOWN; @@ -123,16 +117,14 @@ static int get_user_data( } static int get_seat_from_display(const char *display, const char **seat, uint32_t *vtnr) { - _cleanup_free_ char *p = NULL; - int r; - _cleanup_close_ int fd = -1; union sockaddr_union sa = { .un.sun_family = AF_UNIX, }; + _cleanup_free_ char *p = NULL, *tty = NULL; + _cleanup_close_ int fd = -1; struct ucred ucred; socklen_t l; - _cleanup_free_ char *tty = NULL; - int v; + int v, r; assert(display); assert(vtnr); @@ -194,13 +186,12 @@ _public_ PAM_EXTERN int pam_sm_open_sess dbus_bool_t remote, existing; int r; uint32_t vtnr = 0; + uid_t original_uid; assert(handle); dbus_error_init(&error); - /* pam_syslog(handle, LOG_INFO, "pam-systemd initializing"); */ - /* Make this a NOP on non-logind systems */ if (!logind_running()) return PAM_SUCCESS; @@ -213,6 +204,9 @@ _public_ PAM_EXTERN int pam_sm_open_sess goto finish; } + if (debug) + pam_syslog(handle, LOG_INFO, "pam-systemd initializing"); + r = get_user_data(handle, &username, &pw); if (r != PAM_SUCCESS) goto finish; @@ -374,7 +368,11 @@ _public_ PAM_EXTERN int pam_sm_open_sess if (debug) pam_syslog(handle, LOG_DEBUG, "Asking logind to create session: " "uid=%u pid=%u service=%s type=%s class=%s seat=%s vtnr=%u tty=%s display=%s remote=%s remote_user=%s remote_host=%s", - uid, pid, service, type, class, seat, vtnr, tty, display, yes_no(remote), remote_user, remote_host); + pw->pw_uid, pid, + strempty(service), + type, class, + seat, vtnr, tty, display, + yes_no(remote), remote_user, remote_host); reply = dbus_connection_send_with_reply_and_block(bus, m, -1, &error); if (!reply) { @@ -388,6 +386,7 @@ _public_ PAM_EXTERN int pam_sm_open_sess DBUS_TYPE_OBJECT_PATH, &object_path, DBUS_TYPE_STRING, &runtime_path, DBUS_TYPE_UNIX_FD, &session_fd, + DBUS_TYPE_UINT32, &original_uid, DBUS_TYPE_STRING, &seat, DBUS_TYPE_UINT32, &vtnr, DBUS_TYPE_BOOLEAN, &existing, @@ -399,8 +398,8 @@ _public_ PAM_EXTERN int pam_sm_open_sess if (debug) pam_syslog(handle, LOG_DEBUG, "Reply from logind: " - "id=%s object_path=%s runtime_path=%s session_fd=%d seat=%s vtnr=%u", - id, object_path, runtime_path, session_fd, seat, vtnr); + "id=%s object_path=%s runtime_path=%s session_fd=%d seat=%s vtnr=%u original_uid=%u", + id, object_path, runtime_path, session_fd, seat, vtnr, original_uid); r = pam_misc_setenv(handle, "XDG_SESSION_ID", id, 0); if (r != PAM_SUCCESS) { @@ -408,10 +407,24 @@ _public_ PAM_EXTERN int pam_sm_open_sess 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."); - goto finish; + if (original_uid == pw->pw_uid) { + /* Don't set $XDG_RUNTIME_DIR if the user we now + * authenticated for does not match the original user + * of the session. We do this in order not to result + * in privileged apps clobbering the runtime directory + * unnecessarily. */ + + 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 { + (void) unsetenv("XDG_RUNTIME_DIR"); + r = pam_putenv(handle, "XDG_RUNTIME_DIR"); + if (r != PAM_SUCCESS && r != PAM_BAD_ITEM) { + pam_syslog(handle, LOG_ERR, "Failed to unset runtime dir."); + } } if (!isempty(seat)) {
pgpyoKYFwGvSv.pgp
Description: PGP signature
_______________________________________________ systemd-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/systemd-devel
