Nicolas Iooss <nicolas.iooss@xxxxxxx> writes: > On Wed, Jan 6, 2021 at 2:36 PM Christian Göttsche > <cgzones@xxxxxxxxxxxxxx> wrote: >> >> XDG_RUNTIME_DIR is required for systemctl --user to work. >> See https://github.com/systemd/systemd/issues/15231 >> >> Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx> >> --- >> policycoreutils/newrole/newrole.c | 20 +++++++++++++++----- >> 1 file changed, 15 insertions(+), 5 deletions(-) >> >> diff --git a/policycoreutils/newrole/newrole.c b/policycoreutils/newrole/newrole.c >> index 36e2ba9c..500969e0 100644 >> --- a/policycoreutils/newrole/newrole.c >> +++ b/policycoreutils/newrole/newrole.c >> @@ -466,7 +466,7 @@ static int extract_pw_data(struct passwd *pw_copy) >> * Either restore the original environment, or set up a minimal one. >> * >> * The minimal environment contains: >> - * TERM, DISPLAY and XAUTHORITY - if they are set, preserve values >> + * TERM, DISPLAY, XAUTHORITY and XDG_RUNTIME_DIR - if they are set, preserve values >> * HOME, SHELL, USER and LOGNAME - set to contents of /etc/passwd >> * PATH - set to default value DEFAULT_PATH >> * >> @@ -478,9 +478,11 @@ static int restore_environment(int preserve_environment, >> char const *term_env; >> char const *display_env; >> char const *xauthority_env; >> - char *term = NULL; /* temporary container */ >> - char *display = NULL; /* temporary container */ >> + char const *xdg_runtime_dir_env; >> + char *term = NULL; /* temporary container */ >> + char *display = NULL; /* temporary container */ >> char *xauthority = NULL; /* temporary container */ >> + char *xdg_runtime_dir = NULL; /* temporary container */ >> int rc; >> >> environ = old_environ; >> @@ -491,6 +493,7 @@ static int restore_environment(int preserve_environment, >> term_env = getenv("TERM"); >> display_env = getenv("DISPLAY"); >> xauthority_env = getenv("XAUTHORITY"); >> + xdg_runtime_dir_env = getenv("XDG_RUNTIME_DIR"); /* needed for `systemd --user` operations */ >> >> /* Save the variable values we want */ >> if (term_env) >> @@ -499,8 +502,12 @@ static int restore_environment(int preserve_environment, >> display = strdup(display_env); >> if (xauthority_env) >> xauthority = strdup(xauthority_env); >> - if ((term_env && !term) || (display_env && !display) || >> - (xauthority_env && !xauthority)) { >> + if (xdg_runtime_dir_env) >> + xdg_runtime_dir = strdup(xdg_runtime_dir_env); >> + if ((term_env && !term) || >> + (display_env && !display) || >> + (xauthority_env && !xauthority) || >> + (xdg_runtime_dir_env && !xdg_runtime_dir)) { >> rc = -1; >> goto out; >> } >> @@ -518,6 +525,8 @@ static int restore_environment(int preserve_environment, >> rc |= setenv("DISPLAY", display, 1); >> if (xauthority) >> rc |= setenv("XAUTHORITY", xauthority, 1); >> + if (xdg_runtime_dir) >> + rc |= setenv("XDG_RUNTIME_DIR", xdg_runtime_dir, 1); >> rc |= setenv("HOME", pw->pw_dir, 1); >> rc |= setenv("SHELL", pw->pw_shell, 1); >> rc |= setenv("USER", pw->pw_name, 1); >> @@ -527,6 +536,7 @@ static int restore_environment(int preserve_environment, >> free(term); >> free(display); >> free(xauthority); >> + free(xdg_runtime_dir); >> return rc; >> } > > Hello, > I am quite uncomfortable with this approach of keeping only one more > variable: why is only XDG_RUNTIME_DIR added, and not also > XDG_DATA_DIRS, XDG_SESSION_ID, XDG_SESSION_PATH, etc.? For example > someone pointed out in > https://github.com/systemd/systemd/issues/18301#issuecomment-763933678 > that DBUS_SESSION_BUS_ADDRESS could also need to be preserved, so > there seem to be a long list of items. > > Moreover I am wondering whether this would be fine to keep such > environment variables while newrole uses the information from another > user (i.e. when newrole is built with USE_AUDIT and > audit_getloginuid() != getuid() because the user changed their UID ; > in such a situation newrole resets $HOME and $SHELL to the HOME of > audit_getloginuid()). > > In my humble opinion, I also do not understand why TERM, DISPLAY and > XAUTHORITY are kept but not LANG, LC_ALL, and all other LC_... > variables. I understand that there exist dangerous environment > variables (LD_LIBRARY_PATH, LD_PRELOAD, ...), that resetting the > environment to a minimal one is nice, and that using "newrole > --preserve-environment" could seem dangerous. For information, sudo > has been maintaining a list of "bad" variables, of variables with > potential dangerous values and of variables preserved by default, in > https://github.com/sudo-project/sudo/blob/SUDO_1_9_5p1/plugins/sudoers/env.c#L134-L228. > > This being said, I have never really used newrole but to expose a bug > in "sudo -r" a few years ago > (https://bugzilla.sudo.ws/show_bug.cgi?id=611 "root user can change > its SELinux context without password"). Since then I have always used > sudo to change role, with the advantage that it can be configured to > keep some environment variables, so I am not really the best reviewer > for such a patch (and also I am a little bit confused about the > "isolation guarantees" that newrole implements, and I am not sure > whether keeping XDG_RUNTIME_DIR would not break such guarantees). > > TL;DR: can another maintainer more familiar with newrole review this > patch, please? > > Thanks, > Nicolas I think it does not make much sense to keep XDG_RUNTIME_DIR When you change a role, type or level, it's like changing a linux user and it should be completely new session. In Fedora, sysadm_t is not even allow to get status of staff_t units: [staff@rawhide ~]$ echo $XDG_RUNTIME_DIR /run/user/1001 [staff@rawhide ~]$ newrole -r sysadm_r Password: [staff@rawhide ~]$ export XDG_RUNTIME_DIR=/run/user/1001 [staff@rawhide ~]$ systemctl --user list-units Failed to list units: Access denied systemd[33326]: selinux: avc: denied { status } for auid=n/a uid=1001 gid=1001 cmdline="" scontext=staff_u:sysadm_r:sysadm_t:s0-s0:c0.c1023 tcontext=staff_u:staff_r:staff_t:s0-s0:c0.c1023 tclass=system permissive=0 systemd[33326]: selinux: avc: denied { status } for auid=n/a uid=1001 gid=1001 cmdline="" scontext=staff_u:sysadm_r:sysadm_t:s0-s0:c0.c1023 tcontext=staff_u:staff_r:staff_t:s0-s0:c0.c1023 tclass=system permissive=0 There's also question why one would use newrole to control their a systemd user session when it's possible to control it directly.
