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
