Libguestfs - May 2020 - [PATCH libguestfs-common 1/2] mlcustomize: Refactor SELinux_relabel code.

This shouldn't change the effect of this code.
---
 mlcustomize/SELinux_relabel.ml | 121 ++++++++++++++++++---------------
 1 file changed, 65 insertions(+), 56 deletions(-)

diff --git a/mlcustomize/SELinux_relabel.ml b/mlcustomize/SELinux_relabel.ml
index 44995df..5df1f08 100644
--- a/mlcustomize/SELinux_relabel.ml
+++ b/mlcustomize/SELinux_relabel.ml
@@ -28,65 +28,74 @@ module G = Guestfs
 let array_find a l    List.mem a (Array.to_list l)
 
-let relabel (g : G.guestfs) -  (* Is the guest using SELinux? *)
-  if g#is_file ~followsymlinks:true "/usr/sbin/load_policy"
&&
-     g#is_file ~followsymlinks:true "/etc/selinux/config" then (
-    (* Is setfiles / SELinux relabelling functionality available? *)
-    if g#feature_available [| "selinuxrelabel" |] then (
-      (* Use Augeas to parse /etc/selinux/config. *)
-      g#aug_init "/" (16+32) (* AUG_SAVE_NOOP | AUG_NO_LOAD *);
-      (* See: https://bugzilla.redhat.com/show_bug.cgi?id=975412#c0 *)
-      ignore (g#aug_rm "/augeas/load/*[\"/etc/selinux/config/\"
!~ regexp('^') + glob(incl) + regexp('/.*')]");
-      g#aug_load ();
-      debug_augeas_errors g;
-
-      (* Get the SELinux policy name, eg. "targeted",
"minimum".
-       * Use "targeted" if not specified, just like libselinux does.
+let rec relabel (g : G.guestfs) +  (* Is the guest using SELinux?  (Otherwise
this is a no-op). *)
+  if is_selinux_guest g then (
+    try
+      use_setfiles g;
+      (* That worked, so we don't need to autorelabel. *)
+      g#rm_f "/.autorelabel"
+    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.
        *)
-      let policy -        let config_path =
"/files/etc/selinux/config" in
-        let selinuxtype_path = config_path ^ "/SELINUXTYPE" in
-        let keys = g#aug_ls config_path in
-        if array_find selinuxtype_path keys then
-          g#aug_get selinuxtype_path
-        else
-          "targeted" in
+      g#touch "/.autorelabel"
+  )
 
-      g#aug_close ();
+and is_selinux_guest g +  g#is_file ~followsymlinks:true
"/usr/sbin/load_policy" &&
+  g#is_file ~followsymlinks:true "/etc/selinux/config"
 
-      (* Get the spec file name. *)
-      let specfile -        sprintf
"/etc/selinux/%s/contexts/files/file_contexts" policy in
+and use_setfiles g +  (* Is setfiles / SELinux relabelling functionality
available? *)
+  if not (g#feature_available [| "selinuxrelabel" |]) then
+    failwith "no selinux relabel feature";
 
-      (* RHEL 6.2 - 6.5 had a malformed specfile that contained the
-       * invalid regular expression "/var/run/spice-vdagentd.\pid"
-       * (instead of "\.p").  This stops setfiles from working on
-       * the guest.
-       *
-       * Because an SELinux relabel writes all over the filesystem,
-       * it seems reasonable to fix this problem in the specfile
-       * at the same time.  (RHBZ#1374232)
-       *)
-      if g#grep ~fixed:true "vdagentd.\\pid" specfile <> [||]
then (
-        debug "fixing invalid regular expression in %s" specfile;
-        let old_specfile = specfile ^ "~" in
-        g#mv specfile old_specfile;
-        let content = g#read_file old_specfile in
-        let content -          String.replace content
"vdagentd.\\pid" "vdagentd\\.pid" in
-        g#write specfile content;
-        g#copy_attributes ~all:true old_specfile specfile
-      );
+  (* Use Augeas to parse /etc/selinux/config. *)
+  g#aug_init "/" (16+32) (* AUG_SAVE_NOOP | AUG_NO_LOAD *);
+  (* See: https://bugzilla.redhat.com/show_bug.cgi?id=975412#c0 *)
+  ignore (g#aug_rm "/augeas/load/*[\"/etc/selinux/config/\" !~
regexp('^') + glob(incl) + regexp('/.*')]");
+  g#aug_load ();
+  debug_augeas_errors g;
 
-      (* Relabel everything. *)
-      g#selinux_relabel ~force:true specfile "/";
+  (* Get the SELinux policy name, eg. "targeted",
"minimum".
+   * Use "targeted" if not specified, just like libselinux does.
+   *)
+  let policy +    let config_path = "/files/etc/selinux/config" in
+    let selinuxtype_path = config_path ^ "/SELINUXTYPE" in
+    let keys = g#aug_ls config_path in
+    if array_find selinuxtype_path keys then
+      g#aug_get selinuxtype_path
+    else
+      "targeted" in
 
-      (* If that worked, we don't need to autorelabel. *)
-      g#rm_f "/.autorelabel"
-    )
-    else (
-      (* SELinux guest, but not SELinux host.  Fallback to this. *)
-      g#touch "/.autorelabel"
-    )
-  )
+  g#aug_close ();
+
+  (* Get the spec file name. *)
+  let specfile +    sprintf
"/etc/selinux/%s/contexts/files/file_contexts" policy in
+
+  (* RHEL 6.2 - 6.5 had a malformed specfile that contained the
+   * invalid regular expression "/var/run/spice-vdagentd.\pid"
+   * (instead of "\.p").  This stops setfiles from working on
+   * the guest.
+   *
+   * Because an SELinux relabel writes all over the filesystem,
+   * it seems reasonable to fix this problem in the specfile
+   * at the same time.  (RHBZ#1374232)
+   *)
+  if g#grep ~fixed:true "vdagentd.\\pid" specfile <> [||] then
(
+    debug "fixing invalid regular expression in %s" specfile;
+    let old_specfile = specfile ^ "~" in
+    g#mv specfile old_specfile;
+    let content = g#read_file old_specfile in
+    let content +      String.replace content "vdagentd.\\pid"
"vdagentd\\.pid" in
+    g#write specfile content;
+    g#copy_attributes ~all:true old_specfile specfile
+  );
+
+  (* Relabel everything. *)
+  g#selinux_relabel ~force:true specfile "/"
-- 
2.24.1

https://bugzilla.redhat.com/show_bug.cgi?id=1828952#c2

If SELINUXTYPE is set to some value other than targeted then we look
for a directory /etc/selinux/<SELINUXTYPE> which does not exist.
However this should not cause a fatal error.  Using setfiles to do the
relabelling immediately is a nice-to-have, but we can fallback to
using autorelabel if we're unable to achieve it.
---
 mlcustomize/SELinux_relabel.ml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/mlcustomize/SELinux_relabel.ml b/mlcustomize/SELinux_relabel.ml
index 5df1f08..5ecf7bd 100644
--- a/mlcustomize/SELinux_relabel.ml
+++ b/mlcustomize/SELinux_relabel.ml
@@ -77,6 +77,12 @@ and use_setfiles g    let specfile      sprintf
"/etc/selinux/%s/contexts/files/file_contexts" policy in
 
+  (* If the spec file doesn't exist then fall back to using
+   * autorelabel (RHBZ#1828952).
+   *)
+  if not (g#is_file ~followsymlinks:true specfile) then
+    failwith "no spec file";
+
   (* RHEL 6.2 - 6.5 had a malformed specfile that contained the
    * invalid regular expression "/var/run/spice-vdagentd.\pid"
    * (instead of "\.p").  This stops setfiles from working on
-- 
2.24.1

On Tuesday, 5 May 2020 17:44:14 CEST Richard W.M. Jones
wrote:
> This shouldn't change the effect of this code.
> ---
This is generally OK for me, however I'd create a custom exception to
signal the "cannot relabel using selinux_relabel" instead of reusing
Failure. This way we can catch this specific exception, and avoid
catching potential Failure issues in the future.

-- 
Pino Toscano

On Tuesday, 5 May 2020 17:44:15 CEST Richard W.M. Jones
wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1828952#c2
I think we need to do a different approach than this patch.

The biggest thing is that currently we check only SELINUXTYPE for the
actual policy, however we do not check SELINUX in case SELinux is in
enforcing mode at all.

IMHO we rather need to read /etc/selinux/<SELINUX> first:
- if enforcing, go ahead with the current relabeling: check SELINUXTYPE,
  get the policy path, etc; if set like this, then most probably the
  SELINUXTYPE points to a valid policy, otherwise the guest would not
  even boot
- if permissive or disabled, do not perform any relabeling, including
  touching /.autorelabel; this is because SELinux was disabled, so
  attempting any relabeling might result in failures

-- 
Pino Toscano