[PATCH] newrole: preserve environment variable XDG_RUNTIME_DIR

[PATCH] newrole: preserve environment variable XDG_RUNTIME_DIR

[Christian Göttsche]

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;
 }
 
-- 
2.30.0

Re: [PATCH] newrole: preserve environment variable XDG_RUNTIME_DIR

[Nicolas Iooss]

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

Re: [PATCH] newrole: preserve environment variable XDG_RUNTIME_DIR

[Petr Lautrbach]

Nicolas Iooss <nicolas.iooss@m4x.org> writes:

> 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?
>

From my POV, it does not make much sense to keep XDG_RUNTIME_DIR

As I see it when you change a role, type or level, it's like changing a
linux user and it should be completely new session.

Also using Fedora policy, 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

newrole could change level as well and it would be simply inappropriate
to control systemd user session using different levels.

And you don't need to run newrole to control your user systemd session.

Re: [PATCH] newrole: preserve environment variable XDG_RUNTIME_DIR

[Petr Lautrbach]

Nicolas Iooss <nicolas.iooss@m4x.org> writes:

> 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

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. 

Re: [PATCH] newrole: preserve environment variable XDG_RUNTIME_DIR

[Christian Göttsche]

Am Di., 26. Jan. 2021 um 08:57 Uhr schrieb Petr Lautrbach <plautrba@redhat.com>:
>
> Nicolas Iooss <nicolas.iooss@m4x.org> writes:
>
> >
> > 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.
>

Thanks for your comments.
Seems this uncommon use case is not worth the trouble at all, and one
can as fallback set environment variables via the user's shell
settings (~/.bashrc).
Please disregard this patch.