* Re: [PATCH] newrole: preserve environment variable XDG_RUNTIME_DIR
2021-01-21 21:17 ` Nicolas Iooss
@ 2021-01-25 15:43 ` Petr Lautrbach
2021-01-26 7:57 ` Petr Lautrbach
1 sibling, 0 replies; 5+ messages in thread
From: Petr Lautrbach @ 2021-01-25 15:43 UTC (permalink / raw)
To: SElinux list; +Cc: Nicolas Iooss, Christian Göttsche
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.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] newrole: preserve environment variable XDG_RUNTIME_DIR
2021-01-21 21:17 ` Nicolas Iooss
2021-01-25 15:43 ` Petr Lautrbach
@ 2021-01-26 7:57 ` Petr Lautrbach
2021-01-28 14:31 ` Christian Göttsche
1 sibling, 1 reply; 5+ messages in thread
From: Petr Lautrbach @ 2021-01-26 7:57 UTC (permalink / raw)
To: SElinux list; +Cc: Nicolas Iooss, Christian Göttsche
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.
^ permalink raw reply [flat|nested] 5+ messages in thread