Libguestfs - Sep 2020 - Re: [common PATCH 3/3] mlcustomize: do not relabel if not enforcing (RHBZ#1828952)

On Wed, Sep 23, 2020 at 05:57:50PM +0200, Pino Toscano
wrote:
> Do not attempt to relabel a guest in case its SELinux enforcing mode is
> not "enforcing", as it is either pointless, or it may fail
because of an
> invalid policy configured.
> ---
>  mlcustomize/SELinux_relabel.ml | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/mlcustomize/SELinux_relabel.ml
b/mlcustomize/SELinux_relabel.ml
> index 647aeda..db00e59 100644
> --- a/mlcustomize/SELinux_relabel.ml
> +++ b/mlcustomize/SELinux_relabel.ml
> @@ -24,6 +24,9 @@ open Printf
>  
>  module G = Guestfs
>  
> +exception SELinux_not_enforcing
> +(* Interal exception to signal a non-enforcing SELinux. *)
> +
>  (* Simple reimplementation of Array.mem, available only with OCaml >=
4.03. *)
>  let array_find a l >    List.mem a (Array.to_list l)
> @@ -35,12 +38,18 @@ let rec relabel (g : G.guestfs) >       
use_setfiles g;
>        (* That worked, so we don't need to autorelabel. *)
>        g#rm_f "/.autorelabel"
> -    with Failure _ ->
> +    with
> +    | Failure _ ->
>        (* This is the fallback in case something in the setfiles
>         * method didn't work.  That includes the case where a
non-SELinux
>         * host is processing an SELinux guest, and other things.
>         *)
>        g#touch "/.autorelabel"
> +    | SELinux_not_enforcing ->
> +      (* This means that SELinux was not configured to be in enforcing
mode,
> +       * so silently accept this.
> +       *)
> +      ()
>    )
>  
>  and is_selinux_guest g > @@ -59,6 +68,21 @@ and use_setfiles g >   
g#aug_load ();
>    debug_augeas_errors g;
>  
> +  (* Get the SELinux enforcing mode, eg "enforcing",
"permissive",
> +   * "disabled".
> +   * Use "disabled" if not specified, just like libselinux seems
to do.
> +   *)
> +  let typ = read_selinux_config_key g "SELINUX"
"disabled" in
> +  (* Do not attempt any relabelling if the SELinux is not
"enforcing":
> +   * - in "permissive" mode SELinux is still running, however
nothing is
> +   *   enforced: this means labels can be wrong, and "it is
fine"
I don't think it's fine.  As I showed here:

https://www.redhat.com/archives/libguestfs/2020-June/msg00115.html

in permissive mode labels are still being updated on disk.

TBH I don't understand what you said here:

https://www.redhat.com/archives/libguestfs/2020-June/msg00117.html

about "both the labels and the policy may be all wrong".  If the
administrator set the policy to permissive then labels ought still to
be updated when the guest is running, and we ought to try to keep them
updated if we can in v2v.  It's also fine for an administrator to
switch a system to permissive and then back to enforcing without
relabelling or rebooting.
> +   * - when "disabled" means SELinux is not running, so any
relabelling
> +   *   is pointless (other than potentially fail due to an invalid
> +   *   SELINUXTYPE configuration)
Here you're correct.  Once the admin disabled SELinux, labels are
going to quickly get out of step with reality, and so attempting to
relabel in virt-v2v is indeed a waste of time.

I'd accept this series if you changed "not enforcing" to
"not enforcing or permissive".

Rich.
> +   *)
> +  if typ <> "enforcing" then
> +    raise SELinux_not_enforcing;
> +
>    (* Get the SELinux policy name, eg. "targeted",
"minimum".
>     * Use "targeted" if not specified, just like libselinux does.
>     *)
> -- 
> 2.26.2
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On Thu, Sep 24, 2020 at 11:15:29AM +0100, Richard W.M. Jones
wrote:
> On Wed, Sep 23, 2020 at 05:57:50PM +0200, Pino Toscano wrote:
> > Do not attempt to relabel a guest in case its SELinux enforcing mode
is
> > not "enforcing", as it is either pointless, or it may fail
because of an
> > invalid policy configured.
If invalid policy is configured, you're doomed whether you do labelling
or not. 

If valid policy is configured, and you don't do labelling you're
creating
a new failure mode.

IOW, always doing labelling is the right thing, because at least one of
the 4 scenarios will be an improvement, and it isn't creating any downsides
other than the time required to do the labelling.
> > ---
> >  mlcustomize/SELinux_relabel.ml | 26 +++++++++++++++++++++++++-
> >  1 file changed, 25 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mlcustomize/SELinux_relabel.ml
b/mlcustomize/SELinux_relabel.ml
> > index 647aeda..db00e59 100644
> > --- a/mlcustomize/SELinux_relabel.ml
> > +++ b/mlcustomize/SELinux_relabel.ml
> > @@ -24,6 +24,9 @@ open Printf
> >  
> >  module G = Guestfs
> >  
> > +exception SELinux_not_enforcing
> > +(* Interal exception to signal a non-enforcing SELinux. *)
> > +
> >  (* Simple reimplementation of Array.mem, available only with OCaml
>= 4.03. *)
> >  let array_find a l > >    List.mem a (Array.to_list l)
> > @@ -35,12 +38,18 @@ let rec relabel (g : G.guestfs) > >       
use_setfiles g;
> >        (* That worked, so we don't need to autorelabel. *)
> >        g#rm_f "/.autorelabel"
> > -    with Failure _ ->
> > +    with
> > +    | Failure _ ->
> >        (* This is the fallback in case something in the setfiles
> >         * method didn't work.  That includes the case where a
non-SELinux
> >         * host is processing an SELinux guest, and other things.
> >         *)
> >        g#touch "/.autorelabel"
> > +    | SELinux_not_enforcing ->
> > +      (* This means that SELinux was not configured to be in
enforcing mode,
> > +       * so silently accept this.
> > +       *)
> > +      ()
> >    )
> >  
> >  and is_selinux_guest g > > @@ -59,6 +68,21 @@ and use_setfiles
g > >    g#aug_load ();
> >    debug_augeas_errors g;
> >  
> > +  (* Get the SELinux enforcing mode, eg "enforcing",
"permissive",
> > +   * "disabled".
> > +   * Use "disabled" if not specified, just like libselinux
seems to do.
> > +   *)
> > +  let typ = read_selinux_config_key g "SELINUX"
"disabled" in
> > +  (* Do not attempt any relabelling if the SELinux is not
"enforcing":
> > +   * - in "permissive" mode SELinux is still running,
however nothing is
> > +   *   enforced: this means labels can be wrong, and "it is
fine"
> 
> I don't think it's fine.  As I showed here:
> 
> https://www.redhat.com/archives/libguestfs/2020-June/msg00115.html
> 
> in permissive mode labels are still being updated on disk.
> 
> TBH I don't understand what you said here:
> 
> https://www.redhat.com/archives/libguestfs/2020-June/msg00117.html
> 
> about "both the labels and the policy may be all wrong".  If the
> administrator set the policy to permissive then labels ought still to
> be updated when the guest is running, and we ought to try to keep them
> updated if we can in v2v.  It's also fine for an administrator to
> switch a system to permissive and then back to enforcing without
> relabelling or rebooting.
Yep, in permissive mode apps / admins should still work on the
assumption that SELinux is operating normally. ie assume you
can be switched to enforcing mode at any time. You certainly
want to continue to apply labelling as normal on disk.

If you don't care about labelling to be updated, it means you
are not likely to ever switch to enforcing, and that's what the
"disabled" state is for.


> 
> > +   * - when "disabled" means SELinux is not running, so any
relabelling
> > +   *   is pointless (other than potentially fail due to an invalid
> > +   *   SELINUXTYPE configuration)
> 
> Here you're correct.  Once the admin disabled SELinux, labels are
> going to quickly get out of step with reality, and so attempting to
> relabel in virt-v2v is indeed a waste of time.
> 
> I'd accept this series if you changed "not enforcing" to
> "not enforcing or permissive".

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Thursday, 24 September 2020 12:15:29 CEST Richard W.M. Jones
wrote:
> On Wed, Sep 23, 2020 at 05:57:50PM +0200, Pino Toscano wrote:
> > Do not attempt to relabel a guest in case its SELinux enforcing mode
is
> > not "enforcing", as it is either pointless, or it may fail
because of an
> > invalid policy configured.
> > ---
> >  mlcustomize/SELinux_relabel.ml | 26 +++++++++++++++++++++++++-
> >  1 file changed, 25 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mlcustomize/SELinux_relabel.ml
b/mlcustomize/SELinux_relabel.ml
> > index 647aeda..db00e59 100644
> > --- a/mlcustomize/SELinux_relabel.ml
> > +++ b/mlcustomize/SELinux_relabel.ml
> > @@ -24,6 +24,9 @@ open Printf
> >  
> >  module G = Guestfs
> >  
> > +exception SELinux_not_enforcing
> > +(* Interal exception to signal a non-enforcing SELinux. *)
> > +
> >  (* Simple reimplementation of Array.mem, available only with OCaml
>= 4.03. *)
> >  let array_find a l > >    List.mem a (Array.to_list l)
> > @@ -35,12 +38,18 @@ let rec relabel (g : G.guestfs) > >       
use_setfiles g;
> >        (* That worked, so we don't need to autorelabel. *)
> >        g#rm_f "/.autorelabel"
> > -    with Failure _ ->
> > +    with
> > +    | Failure _ ->
> >        (* This is the fallback in case something in the setfiles
> >         * method didn't work.  That includes the case where a
non-SELinux
> >         * host is processing an SELinux guest, and other things.
> >         *)
> >        g#touch "/.autorelabel"
> > +    | SELinux_not_enforcing ->
> > +      (* This means that SELinux was not configured to be in
enforcing mode,
> > +       * so silently accept this.
> > +       *)
> > +      ()
> >    )
> >  
> >  and is_selinux_guest g > > @@ -59,6 +68,21 @@ and use_setfiles
g > >    g#aug_load ();
> >    debug_augeas_errors g;
> >  
> > +  (* Get the SELinux enforcing mode, eg "enforcing",
"permissive",
> > +   * "disabled".
> > +   * Use "disabled" if not specified, just like libselinux
seems to do.
> > +   *)
> > +  let typ = read_selinux_config_key g "SELINUX"
"disabled" in
> > +  (* Do not attempt any relabelling if the SELinux is not
"enforcing":
> > +   * - in "permissive" mode SELinux is still running,
however nothing is
> > +   *   enforced: this means labels can be wrong, and "it is
fine"
> 
> I don't think it's fine.  As I showed here:
> 
> https://www.redhat.com/archives/libguestfs/2020-June/msg00115.html
> 
> in permissive mode labels are still being updated on disk.
This is true for default labels, yes.
> TBH I don't understand what you said here:
> 
> https://www.redhat.com/archives/libguestfs/2020-June/msg00117.html
> 
> about "both the labels and the policy may be all wrong".  If the
> administrator set the policy to permissive then labels ought still to
> be updated when the guest is running, and we ought to try to keep them
> updated if we can in v2v.
There are various cases when, even of an enforcing system, labels are
not kept up-to-date:

$ getenforce 
Enforcing
$ touch /tmp/test
$ ls -lZ /tmp/test 
-rw-rw-r--. 1 ptoscano ptoscano unconfined_u:object_r:user_tmp_t:s0 0 Sep 24
12:26 /tmp/test
$ mv /tmp/test ~/var/
$ ls -lZ ~/var/test 
-rw-rw-r--. 1 ptoscano ptoscano unconfined_u:object_r:user_tmp_t:s0 0 Sep 24
12:26 /home/ptoscano/var/test
$ restorecon -v ~/var/test 
Relabeled /home/ptoscano/var/test from unconfined_u:object_r:user_tmp_t:s0 to
unconfined_u:object_r:user_home_t:s0
$ ls -lZ ~/var/test 
-rw-rw-r--. 1 ptoscano ptoscano unconfined_u:object_r:user_home_t:s0 0 Sep 24
12:26 /home/ptoscano/var/test

Considering that /tmp is a general location for temporary files, it's
common that files may end with a tmp_t-alike label when moved back to
the destination place (e.g. after a rename()). That is not the only
situation like this that I saw in the past.

In permissive mode, all these situation are logged in the audit log,
yes, but they cause no blocks nor errors.
> It's also fine for an administrator to
> switch a system to permissive and then back to enforcing without
> relabelling or rebooting.
A mislabelled /etc/passwd is still read and used fine in permissive
mode. Switch back from permissive to enforcing without a relabelling
is generally not a good idea, especially after the system ran for a
lot of time after the switch to permissive.

-- 
Pino Toscano

On Thu, Sep 24, 2020 at 12:39:02PM +0200, Pino Toscano wrote:
...
> There are various cases when, even of an enforcing system, labels are
> not kept up-to-date:
> 
> $ getenforce 
> Enforcing
> $ touch /tmp/test
> $ ls -lZ /tmp/test 
> -rw-rw-r--. 1 ptoscano ptoscano unconfined_u:object_r:user_tmp_t:s0 0 Sep
24 12:26 /tmp/test
> $ mv /tmp/test ~/var/
> $ ls -lZ ~/var/test 
> -rw-rw-r--. 1 ptoscano ptoscano unconfined_u:object_r:user_tmp_t:s0 0 Sep
24 12:26 /home/ptoscano/var/test
> $ restorecon -v ~/var/test 
> Relabeled /home/ptoscano/var/test from unconfined_u:object_r:user_tmp_t:s0
to unconfined_u:object_r:user_home_t:s0
> $ ls -lZ ~/var/test 
> -rw-rw-r--. 1 ptoscano ptoscano unconfined_u:object_r:user_home_t:s0 0 Sep
24 12:26 /home/ptoscano/var/test
That's definitely a weird thing.  Bug maybe?
> Considering that /tmp is a general location for temporary files, it's
> common that files may end with a tmp_t-alike label when moved back to
> the destination place (e.g. after a rename()). That is not the only
> situation like this that I saw in the past.
> 
> In permissive mode, all these situation are logged in the audit log,
> yes, but they cause no blocks nor errors.
> 
> > It's also fine for an administrator to
> > switch a system to permissive and then back to enforcing without
> > relabelling or rebooting.
> 
> A mislabelled /etc/passwd is still read and used fine in permissive
> mode. Switch back from permissive to enforcing without a relabelling
> is generally not a good idea, especially after the system ran for a
> lot of time after the switch to permissive.
It's seems true from what you wrote above that someone could copy
/tmp/passwd -> /etc/passwd and it would have a wrong label.  But
virt-v2v could fix that label, which even in permissive mode sounds
like a win.

My question is what's the down-side to relabelling in permissive mode?

(I can see in *disabled* mode it's just a waste of time because the
work we do for relabelling in virt-v2v is just going to be undone when
the guest boots with SELinux disabled).

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/