[v2] mcstrans: start early and stop late
diff mbox series

Message ID 20191211134423.2357254-1-dac.override@gmail.com
State Accepted
Headers show
Series
  • [v2] mcstrans: start early and stop late
Related show

Commit Message

Dac Override Dec. 11, 2019, 1:44 p.m. UTC
It stopped too early, exposing a bug in sudo selinux_restore_tty():

SELINUX_ERR op=setxattr invalid_context="wheel.id:wheel.role:users.terminals.pty.pty_file:SystemLow"
avc:  denied  { mac_admin } for  pid=859 comm="sudo" capability=33 scontext=wheel.id:wheel.role:sudo.wheel.subj:s0 tcontext=wheel.id:wheel.role:sudo.wheel.subj:s0 tclass=capability2 permissive=0

If we want to be able to reference human readable contexts in SELinuxContext= and nspawn -Z and -L then we need mcstrans ASAP

v2: stop late, but do stop
Signed-off-by: Dominick Grift <dac.override@gmail.com>
---
 mcstrans/src/mcstrans.service | 3 +++
 1 file changed, 3 insertions(+)

Comments

Dac Override Dec. 11, 2019, 2:21 p.m. UTC | #1
On Wed, Dec 11, 2019 at 02:44:23PM +0100, Dominick Grift wrote:
> It stopped too early, exposing a bug in sudo selinux_restore_tty():
> 
> SELINUX_ERR op=setxattr invalid_context="wheel.id:wheel.role:users.terminals.pty.pty_file:SystemLow"
> avc:  denied  { mac_admin } for  pid=859 comm="sudo" capability=33 scontext=wheel.id:wheel.role:sudo.wheel.subj:s0 tcontext=wheel.id:wheel.role:sudo.wheel.subj:s0 tclass=capability2 permissive=0
> 
> If we want to be able to reference human readable contexts in SELinuxContext= and nspawn -Z and -L then we need mcstrans ASAP

Unfortunately it does not quite seem to address this challenge, at least currently, but still
I think systemd would need to refresh its label cache when mcstrans is started, as per systemd v245 that should be a little less painful than it is today
Something like a: ExecStartPost=/bin/systemctl daemon-reload would do that then

> 
> v2: stop late, but do stop
> Signed-off-by: Dominick Grift <dac.override@gmail.com>
> ---
>  mcstrans/src/mcstrans.service | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mcstrans/src/mcstrans.service b/mcstrans/src/mcstrans.service
> index 09529432..c13cd09a 100644
> --- a/mcstrans/src/mcstrans.service
> +++ b/mcstrans/src/mcstrans.service
> @@ -2,6 +2,9 @@
>  Description=Translates SELinux MCS/MLS labels to human readable form
>  Documentation=man:mcstransd(8)
>  ConditionSecurity=selinux
> +DefaultDependencies=no
> +Before=shutdown.target sysinit.target
> +Conflicts=shutdown.target
>  
>  [Service]
>  ExecStart=/sbin/mcstransd -f
> -- 
> 2.24.0
>
Stephen Smalley Dec. 12, 2019, 1:45 p.m. UTC | #2
On 12/11/19 9:21 AM, Dominick Grift wrote:
> On Wed, Dec 11, 2019 at 02:44:23PM +0100, Dominick Grift wrote:
>> It stopped too early, exposing a bug in sudo selinux_restore_tty():
>>
>> SELINUX_ERR op=setxattr invalid_context="wheel.id:wheel.role:users.terminals.pty.pty_file:SystemLow"
>> avc:  denied  { mac_admin } for  pid=859 comm="sudo" capability=33 scontext=wheel.id:wheel.role:sudo.wheel.subj:s0 tcontext=wheel.id:wheel.role:sudo.wheel.subj:s0 tclass=capability2 permissive=0
>>
>> If we want to be able to reference human readable contexts in SELinuxContext= and nspawn -Z and -L then we need mcstrans ASAP
> 
> Unfortunately it does not quite seem to address this challenge, at least currently, but still
> I think systemd would need to refresh its label cache when mcstrans is started, as per systemd v245 that should be a little less painful than it is today
> Something like a: ExecStartPost=/bin/systemctl daemon-reload would do that then

I'm a little unclear on where the bug lies - you show a sudo denial, but 
refer to systemd as the culprit?

If we don't care about being able to use translated contexts in systemd 
unit files or options, it could always use the _raw interfaces to ensure 
that it is always dealing with the raw kernel contexts.  The translated 
contexts are mostly for display purposes for MLS labels/policies.

> 
>>
>> v2: stop late, but do stop
>> Signed-off-by: Dominick Grift <dac.override@gmail.com>
>> ---
>>   mcstrans/src/mcstrans.service | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/mcstrans/src/mcstrans.service b/mcstrans/src/mcstrans.service
>> index 09529432..c13cd09a 100644
>> --- a/mcstrans/src/mcstrans.service
>> +++ b/mcstrans/src/mcstrans.service
>> @@ -2,6 +2,9 @@
>>   Description=Translates SELinux MCS/MLS labels to human readable form
>>   Documentation=man:mcstransd(8)
>>   ConditionSecurity=selinux
>> +DefaultDependencies=no
>> +Before=shutdown.target sysinit.target
>> +Conflicts=shutdown.target
>>   
>>   [Service]
>>   ExecStart=/sbin/mcstransd -f
>> -- 
>> 2.24.0
>>
>
Dac Override Dec. 12, 2019, 2:24 p.m. UTC | #3
On Thu, Dec 12, 2019 at 08:45:29AM -0500, Stephen Smalley wrote:
> On 12/11/19 9:21 AM, Dominick Grift wrote:
> > On Wed, Dec 11, 2019 at 02:44:23PM +0100, Dominick Grift wrote:
> > > It stopped too early, exposing a bug in sudo selinux_restore_tty():
> > > 
> > > SELINUX_ERR op=setxattr invalid_context="wheel.id:wheel.role:users.terminals.pty.pty_file:SystemLow"
> > > avc:  denied  { mac_admin } for  pid=859 comm="sudo" capability=33 scontext=wheel.id:wheel.role:sudo.wheel.subj:s0 tcontext=wheel.id:wheel.role:sudo.wheel.subj:s0 tclass=capability2 permissive=0
> > > 
> > > If we want to be able to reference human readable contexts in SELinuxContext= and nspawn -Z and -L then we need mcstrans ASAP
> > 
> > Unfortunately it does not quite seem to address this challenge, at least currently, but still
> > I think systemd would need to refresh its label cache when mcstrans is started, as per systemd v245 that should be a little less painful than it is today
> > Something like a: ExecStartPost=/bin/systemctl daemon-reload would do that then
> 
> I'm a little unclear on where the bug lies - you show a sudo denial, but
> refer to systemd as the culprit?

The sudo bug is fixed here: https://github.com/sudo-project/sudo/commit/718e6997fcaae6ea065ce74d08dd4aae5917df5e

> 
> If we don't care about being able to use translated contexts in systemd unit
> files or options, it could always use the _raw interfaces to ensure that it
> is always dealing with the raw kernel contexts.  The translated contexts are
> mostly for display purposes for MLS labels/policies.

The thing with systemd is that since systemd runs before mcstrans is started it doesnt use mcstrans.
So if you try to reference translated contexts using systemd then it will refuse.
Running a systemctl daemon-reexec after mcstrans is started fixes that issue but that is not really an option.

I am wondering what causes this behavior, i suppose this is some libselinux thing.

Regardless of all the above though, i think it makes sense for mcstrans to start early and stop late.

> 
> > 
> > > 
> > > v2: stop late, but do stop
> > > Signed-off-by: Dominick Grift <dac.override@gmail.com>
> > > ---
> > >   mcstrans/src/mcstrans.service | 3 +++
> > >   1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/mcstrans/src/mcstrans.service b/mcstrans/src/mcstrans.service
> > > index 09529432..c13cd09a 100644
> > > --- a/mcstrans/src/mcstrans.service
> > > +++ b/mcstrans/src/mcstrans.service
> > > @@ -2,6 +2,9 @@
> > >   Description=Translates SELinux MCS/MLS labels to human readable form
> > >   Documentation=man:mcstransd(8)
> > >   ConditionSecurity=selinux
> > > +DefaultDependencies=no
> > > +Before=shutdown.target sysinit.target
> > > +Conflicts=shutdown.target
> > >   [Service]
> > >   ExecStart=/sbin/mcstransd -f
> > > -- 
> > > 2.24.0
> > > 
> > 
>
Dac Override Dec. 12, 2019, 2:31 p.m. UTC | #4
On Thu, Dec 12, 2019 at 03:24:36PM +0100, Dominick Grift wrote:
> On Thu, Dec 12, 2019 at 08:45:29AM -0500, Stephen Smalley wrote:
> > On 12/11/19 9:21 AM, Dominick Grift wrote:
> > > On Wed, Dec 11, 2019 at 02:44:23PM +0100, Dominick Grift wrote:
> > > > It stopped too early, exposing a bug in sudo selinux_restore_tty():
> > > > 
> > > > SELINUX_ERR op=setxattr invalid_context="wheel.id:wheel.role:users.terminals.pty.pty_file:SystemLow"
> > > > avc:  denied  { mac_admin } for  pid=859 comm="sudo" capability=33 scontext=wheel.id:wheel.role:sudo.wheel.subj:s0 tcontext=wheel.id:wheel.role:sudo.wheel.subj:s0 tclass=capability2 permissive=0
> > > > 
> > > > If we want to be able to reference human readable contexts in SELinuxContext= and nspawn -Z and -L then we need mcstrans ASAP
> > > 
> > > Unfortunately it does not quite seem to address this challenge, at least currently, but still
> > > I think systemd would need to refresh its label cache when mcstrans is started, as per systemd v245 that should be a little less painful than it is today
> > > Something like a: ExecStartPost=/bin/systemctl daemon-reload would do that then
> > 
> > I'm a little unclear on where the bug lies - you show a sudo denial, but
> > refer to systemd as the culprit?
> 
> The sudo bug is fixed here: https://github.com/sudo-project/sudo/commit/718e6997fcaae6ea065ce74d08dd4aae5917df5e
> 
> > 
> > If we don't care about being able to use translated contexts in systemd unit
> > files or options, it could always use the _raw interfaces to ensure that it
> > is always dealing with the raw kernel contexts.  The translated contexts are
> > mostly for display purposes for MLS labels/policies.

The bevavior of mcstrans seems to  contradict your suggestion that its mostly for display purposes.

If you reference a set of categories in the raw manner then mcstrans seems to not resolve that until that same set of categories is referenced in human readable form.
It think it actually makes sense that people reference catagories in human readable form rather than in a raw manner when they opt in for mcstrans.

> 
> The thing with systemd is that since systemd runs before mcstrans is started it doesnt use mcstrans.
> So if you try to reference translated contexts using systemd then it will refuse.
> Running a systemctl daemon-reexec after mcstrans is started fixes that issue but that is not really an option.
> 
> I am wondering what causes this behavior, i suppose this is some libselinux thing.
> 
> Regardless of all the above though, i think it makes sense for mcstrans to start early and stop late.
> 
> > 
> > > 
> > > > 
> > > > v2: stop late, but do stop
> > > > Signed-off-by: Dominick Grift <dac.override@gmail.com>
> > > > ---
> > > >   mcstrans/src/mcstrans.service | 3 +++
> > > >   1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/mcstrans/src/mcstrans.service b/mcstrans/src/mcstrans.service
> > > > index 09529432..c13cd09a 100644
> > > > --- a/mcstrans/src/mcstrans.service
> > > > +++ b/mcstrans/src/mcstrans.service
> > > > @@ -2,6 +2,9 @@
> > > >   Description=Translates SELinux MCS/MLS labels to human readable form
> > > >   Documentation=man:mcstransd(8)
> > > >   ConditionSecurity=selinux
> > > > +DefaultDependencies=no
> > > > +Before=shutdown.target sysinit.target
> > > > +Conflicts=shutdown.target
> > > >   [Service]
> > > >   ExecStart=/sbin/mcstransd -f
> > > > -- 
> > > > 2.24.0
> > > > 
> > > 
> > 
> 
> -- 
> Key fingerprint = 5F4D 3CDB D3F8 3652 FBD8 02D5 3B6C 5F1D 2C7B 6B02
> https://sks-keyservers.net/pks/lookup?op=get&search=0x3B6C5F1D2C7B6B02
> Dominick Grift
Stephen Smalley Dec. 12, 2019, 2:36 p.m. UTC | #5
On 12/12/19 9:24 AM, Dominick Grift wrote:
> On Thu, Dec 12, 2019 at 08:45:29AM -0500, Stephen Smalley wrote:
>> On 12/11/19 9:21 AM, Dominick Grift wrote:
>>> On Wed, Dec 11, 2019 at 02:44:23PM +0100, Dominick Grift wrote:
>>>> It stopped too early, exposing a bug in sudo selinux_restore_tty():
>>>>
>>>> SELINUX_ERR op=setxattr invalid_context="wheel.id:wheel.role:users.terminals.pty.pty_file:SystemLow"
>>>> avc:  denied  { mac_admin } for  pid=859 comm="sudo" capability=33 scontext=wheel.id:wheel.role:sudo.wheel.subj:s0 tcontext=wheel.id:wheel.role:sudo.wheel.subj:s0 tclass=capability2 permissive=0
>>>>
>>>> If we want to be able to reference human readable contexts in SELinuxContext= and nspawn -Z and -L then we need mcstrans ASAP
>>>
>>> Unfortunately it does not quite seem to address this challenge, at least currently, but still
>>> I think systemd would need to refresh its label cache when mcstrans is started, as per systemd v245 that should be a little less painful than it is today
>>> Something like a: ExecStartPost=/bin/systemctl daemon-reload would do that then
>>
>> I'm a little unclear on where the bug lies - you show a sudo denial, but
>> refer to systemd as the culprit?
> 
> The sudo bug is fixed here: https://github.com/sudo-project/sudo/commit/718e6997fcaae6ea065ce74d08dd4aae5917df5e
> 
>>
>> If we don't care about being able to use translated contexts in systemd unit
>> files or options, it could always use the _raw interfaces to ensure that it
>> is always dealing with the raw kernel contexts.  The translated contexts are
>> mostly for display purposes for MLS labels/policies.
> 
> The thing with systemd is that since systemd runs before mcstrans is started it doesnt use mcstrans.
> So if you try to reference translated contexts using systemd then it will refuse.
> Running a systemctl daemon-reexec after mcstrans is started fixes that issue but that is not really an option.
> 
> I am wondering what causes this behavior, i suppose this is some libselinux thing.

Yes, libselinux checks for the existence of the mcstrans socket exactly 
once on the first attempted translation, and if it does not exist at 
that time, it never tries again.  That is to avoid overhead on systems 
that are not running mcstrans, which is the majority of systems. 
mcstrans is only really needed for MLS policies and those are a minority 
of SELinux systems.

> Regardless of all the above though, i think it makes sense for mcstrans to start early and stop late.

Perhaps; I'll defer to the distro maintainers on that.  I don't really 
recommend using mcstrans unless you truly need it.

> 
>>
>>>
>>>>
>>>> v2: stop late, but do stop
>>>> Signed-off-by: Dominick Grift <dac.override@gmail.com>
>>>> ---
>>>>    mcstrans/src/mcstrans.service | 3 +++
>>>>    1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/mcstrans/src/mcstrans.service b/mcstrans/src/mcstrans.service
>>>> index 09529432..c13cd09a 100644
>>>> --- a/mcstrans/src/mcstrans.service
>>>> +++ b/mcstrans/src/mcstrans.service
>>>> @@ -2,6 +2,9 @@
>>>>    Description=Translates SELinux MCS/MLS labels to human readable form
>>>>    Documentation=man:mcstransd(8)
>>>>    ConditionSecurity=selinux
>>>> +DefaultDependencies=no
>>>> +Before=shutdown.target sysinit.target
>>>> +Conflicts=shutdown.target
>>>>    [Service]
>>>>    ExecStart=/sbin/mcstransd -f
>>>> -- 
>>>> 2.24.0
>>>>
>>>
>>
>
Dac Override Dec. 12, 2019, 2:59 p.m. UTC | #6
On Thu, Dec 12, 2019 at 09:36:19AM -0500, Stephen Smalley wrote:
> On 12/12/19 9:24 AM, Dominick Grift wrote:
> > On Thu, Dec 12, 2019 at 08:45:29AM -0500, Stephen Smalley wrote:
> > > On 12/11/19 9:21 AM, Dominick Grift wrote:
> > > > On Wed, Dec 11, 2019 at 02:44:23PM +0100, Dominick Grift wrote:
> > > > > It stopped too early, exposing a bug in sudo selinux_restore_tty():
> > > > > 
> > > > > SELINUX_ERR op=setxattr invalid_context="wheel.id:wheel.role:users.terminals.pty.pty_file:SystemLow"
> > > > > avc:  denied  { mac_admin } for  pid=859 comm="sudo" capability=33 scontext=wheel.id:wheel.role:sudo.wheel.subj:s0 tcontext=wheel.id:wheel.role:sudo.wheel.subj:s0 tclass=capability2 permissive=0
> > > > > 
> > > > > If we want to be able to reference human readable contexts in SELinuxContext= and nspawn -Z and -L then we need mcstrans ASAP
> > > > 
> > > > Unfortunately it does not quite seem to address this challenge, at least currently, but still
> > > > I think systemd would need to refresh its label cache when mcstrans is started, as per systemd v245 that should be a little less painful than it is today
> > > > Something like a: ExecStartPost=/bin/systemctl daemon-reload would do that then
> > > 
> > > I'm a little unclear on where the bug lies - you show a sudo denial, but
> > > refer to systemd as the culprit?
> > 
> > The sudo bug is fixed here: https://github.com/sudo-project/sudo/commit/718e6997fcaae6ea065ce74d08dd4aae5917df5e
> > 
> > > 
> > > If we don't care about being able to use translated contexts in systemd unit
> > > files or options, it could always use the _raw interfaces to ensure that it
> > > is always dealing with the raw kernel contexts.  The translated contexts are
> > > mostly for display purposes for MLS labels/policies.
> > 
> > The thing with systemd is that since systemd runs before mcstrans is started it doesnt use mcstrans.
> > So if you try to reference translated contexts using systemd then it will refuse.
> > Running a systemctl daemon-reexec after mcstrans is started fixes that issue but that is not really an option.
> > 
> > I am wondering what causes this behavior, i suppose this is some libselinux thing.
> 
> Yes, libselinux checks for the existence of the mcstrans socket exactly once
> on the first attempted translation, and if it does not exist at that time,
> it never tries again.  That is to avoid overhead on systems that are not
> running mcstrans, which is the majority of systems. mcstrans is only really
> needed for MLS policies and those are a minority of SELinux systems.
> 
> > Regardless of all the above though, i think it makes sense for mcstrans to start early and stop late.
> 
> Perhaps; I'll defer to the distro maintainers on that.  I don't really
> recommend using mcstrans unless you truly need it.

Me neither. There was a request for support on IRC. So i looked into it.
I certainly think I made it clear to stay away from it if possible.

> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > v2: stop late, but do stop
> > > > > Signed-off-by: Dominick Grift <dac.override@gmail.com>
> > > > > ---
> > > > >    mcstrans/src/mcstrans.service | 3 +++
> > > > >    1 file changed, 3 insertions(+)
> > > > > 
> > > > > diff --git a/mcstrans/src/mcstrans.service b/mcstrans/src/mcstrans.service
> > > > > index 09529432..c13cd09a 100644
> > > > > --- a/mcstrans/src/mcstrans.service
> > > > > +++ b/mcstrans/src/mcstrans.service
> > > > > @@ -2,6 +2,9 @@
> > > > >    Description=Translates SELinux MCS/MLS labels to human readable form
> > > > >    Documentation=man:mcstransd(8)
> > > > >    ConditionSecurity=selinux
> > > > > +DefaultDependencies=no
> > > > > +Before=shutdown.target sysinit.target
> > > > > +Conflicts=shutdown.target
> > > > >    [Service]
> > > > >    ExecStart=/sbin/mcstransd -f
> > > > > -- 
> > > > > 2.24.0
> > > > > 
> > > > 
> > > 
> > 
>
Petr Lautrbach May 11, 2020, 7:19 a.m. UTC | #7
On Wed, Dec 11, 2019 at 02:44:23PM +0100, Dominick Grift wrote:
> It stopped too early, exposing a bug in sudo selinux_restore_tty():
> 
> SELINUX_ERR op=setxattr invalid_context="wheel.id:wheel.role:users.terminals.pty.pty_file:SystemLow"
> avc:  denied  { mac_admin } for  pid=859 comm="sudo" capability=33 scontext=wheel.id:wheel.role:sudo.wheel.subj:s0 tcontext=wheel.id:wheel.role:sudo.wheel.subj:s0 tclass=capability2 permissive=0
> 
> If we want to be able to reference human readable contexts in SELinuxContext= and nspawn -Z and -L then we need mcstrans ASAP
> 
> v2: stop late, but do stop
> Signed-off-by: Dominick Grift <dac.override@gmail.com>

I've asked for review one of Fedora systemd maintainers and he's fine with this
change. He even suggested to follow udev and to drop 'Conflicts=shutdown.target'
and let mstrans to be stopped by a signal when everything else is stopped as it
was in your first version.

I'm fine with both versions.

Acked-by: Petr Lautrbach <plautrba@redhat.com>



> ---
>  mcstrans/src/mcstrans.service | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mcstrans/src/mcstrans.service b/mcstrans/src/mcstrans.service
> index 09529432..c13cd09a 100644
> --- a/mcstrans/src/mcstrans.service
> +++ b/mcstrans/src/mcstrans.service
> @@ -2,6 +2,9 @@
>  Description=Translates SELinux MCS/MLS labels to human readable form
>  Documentation=man:mcstransd(8)
>  ConditionSecurity=selinux
> +DefaultDependencies=no
> +Before=shutdown.target sysinit.target
> +Conflicts=shutdown.target
>  
>  [Service]
>  ExecStart=/sbin/mcstransd -f
Petr Lautrbach May 13, 2020, 3:09 p.m. UTC | #8
On Wed, Dec 11, 2019 at 02:44:23PM +0100, Dominick Grift wrote:
> It stopped too early, exposing a bug in sudo selinux_restore_tty():
> 
> SELINUX_ERR op=setxattr invalid_context="wheel.id:wheel.role:users.terminals.pty.pty_file:SystemLow"
> avc:  denied  { mac_admin } for  pid=859 comm="sudo" capability=33 scontext=wheel.id:wheel.role:sudo.wheel.subj:s0 tcontext=wheel.id:wheel.role:sudo.wheel.subj:s0 tclass=capability2 permissive=0
> 
> If we want to be able to reference human readable contexts in SELinuxContext= and nspawn -Z and -L then we need mcstrans ASAP
> 
> v2: stop late, but do stop
> Signed-off-by: Dominick Grift <dac.override@gmail.com>
> Acked-by: Petr Lautrbach <plautrba@redhat.com>

Applied, thanks!

> ---
>  mcstrans/src/mcstrans.service | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mcstrans/src/mcstrans.service b/mcstrans/src/mcstrans.service
> index 09529432..c13cd09a 100644
> --- a/mcstrans/src/mcstrans.service
> +++ b/mcstrans/src/mcstrans.service
> @@ -2,6 +2,9 @@
>  Description=Translates SELinux MCS/MLS labels to human readable form
>  Documentation=man:mcstransd(8)
>  ConditionSecurity=selinux
> +DefaultDependencies=no
> +Before=shutdown.target sysinit.target
> +Conflicts=shutdown.target
>  
>  [Service]
>  ExecStart=/sbin/mcstransd -f

Patch
diff mbox series

diff --git a/mcstrans/src/mcstrans.service b/mcstrans/src/mcstrans.service
index 09529432..c13cd09a 100644
--- a/mcstrans/src/mcstrans.service
+++ b/mcstrans/src/mcstrans.service
@@ -2,6 +2,9 @@ 
 Description=Translates SELinux MCS/MLS labels to human readable form
 Documentation=man:mcstransd(8)
 ConditionSecurity=selinux
+DefaultDependencies=no
+Before=shutdown.target sysinit.target
+Conflicts=shutdown.target
 
 [Service]
 ExecStart=/sbin/mcstransd -f