On Mon, 2013-11-18 at 19:35 -0500, Colin Walters wrote:
> And that's what I'm testing - with Martin's patch in the loop I was
> still getting XDG_DATA_DIR for uid 1000, I'll try to debug soon.
Ok, some discussion on IRC revealed that I was only using the second
patch to s/loginuid/uid/, but we need the first patch too.
Before I continue, a quick TL;DR: For downstreams that want to carry a
noninvasive fix for this problem, Martin's 2 patches are OK.
But I wanted to do a bit more "due diligence" on this and attempt to
understand where Lennart is coming from, and why the code is how it is
today.
Some archeology with git points to this commit:
http://cgit.freedesktop.org/systemd/systemd/commit/?id=21c390ccd1b4f7bc962c16549df929ad518a1d37
Quite old now...and there's not a lot of explanation as to the
real-world scenario that caused it to be introduced.
There's a followup commit here:
http://cgit.freedesktop.org/systemd/systemd/commit/?id=954449b82df7fc2d37a8d854977a1a73a65edfbd
with some more information. Now one thing to note is: we're using the
uid of the original session as the source for all information, including
the runtime dir.
Martin's first patch is creating a situation where logind says one
thing, pam module overrides it. But both come from the same git module,
we can fix logind.
I'm attaching a patch (for discussion only) that needs to go on top of
Martin's "pam: Don't use loginuid" patch.
Both of our patch series currently are basically going to have the
effect that with pkexec, XDG_RUNTIME_DIR is unset. But this is
undesirable because it forces the rest of userspace to go back to the
old dark ages when XDG_RUNTIME_DIR didn't exist and there was no
reliable mechanism for two processes of equal uid but different sessions
find each other and communicate. (Think ssh login + gdm login).
My patch though starts to pave the way for having XDG_RUNTIME_DIR
consistently point to that of the user's uid - I am increasingly
thinking the option #3 I mention in the patch where we refcount the
runtime dir in addition to users and sessions would work.
>From 7fe51a82a5cc1ad2185b4ee4faa087f17e27d24a 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: 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)) {
--
1.7.1
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
