Re: [PATCH] newrole: preserve environment variable XDG_RUNTIME_DIR

[Nicolas Iooss <nicolas.iooss@m4x.org>]

On Wed, Jan 6, 2021 at 2:36 PM Christian Göttsche
<cgzones@googlemail.com> 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@googlemail.com>
> ---
>  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