Pino Toscano
2014-May-26 09:21 UTC
[Libguestfs] [PATCH 2/2] Use setfiles from the appliance for the SELinux relabel (RHBZ#1089100).
Rewrite the relabel API to read the policy configured in the guest,
invoking setfiles (added as part of the appliance, as part of
policycoreutils) to relabel the specified root. In case of failure at
any point of the process, a touch of .autorelabel in the root is tried
as last-attempt measure to do the relabel.
Considering that running SELinux tools in the appliance might be
affected by the SELinux state (leading to wrong results),
selinux_relabel now bails out if SELinux is enabled in the appliance.
As a result of this, virt-builder and virt-customize explicitly disable
it if the relabel is enabled.
---
appliance/packagelist.in | 1 +
builder/builder.ml | 5 +-
customize/customize_main.ml | 5 +-
daemon/selinux.c | 198 ++++++++++++++++++++++++++++++++++++++++----
generator/actions.ml | 7 ++
5 files changed, 197 insertions(+), 19 deletions(-)
diff --git a/appliance/packagelist.in b/appliance/packagelist.in
index e4a9c8b..6336048 100644
--- a/appliance/packagelist.in
+++ b/appliance/packagelist.in
@@ -217,6 +217,7 @@ dnl Enabling this pulls out 140 extra packages
dnl into the appliance:
dnl ocfs2-tools
parted
+policycoreutils
procps
procps-ng
psmisc
diff --git a/builder/builder.ml b/builder/builder.ml
index e7be901..a0ef6d7 100644
--- a/builder/builder.ml
+++ b/builder/builder.ml
@@ -585,7 +585,10 @@ let main () (match smp with None -> () | Some smp
-> g#set_smp smp);
g#set_network network;
- g#set_selinux ops.flags.selinux_relabel;
+ (* If a relabel is needed, make sure to turn SELinux off to avoid
+ * awkward interactions with the relabel process.
+ *)
+ if ops.flags.selinux_relabel then g#set_selinux false;
(* The output disk is being created, so use cache=unsafe here. *)
g#add_drive_opts ~format:output_format ~cachemode:"unsafe"
output_filename;
diff --git a/customize/customize_main.ml b/customize/customize_main.ml
index 00d5bae..a6c6643 100644
--- a/customize/customize_main.ml
+++ b/customize/customize_main.ml
@@ -193,7 +193,10 @@ read the man page virt-customize(1).
(match memsize with None -> () | Some memsize -> g#set_memsize
memsize);
(match smp with None -> () | Some smp -> g#set_smp smp);
g#set_network network;
- g#set_selinux ops.flags.selinux_relabel;
+ (* If a relabel is needed, make sure to turn SELinux off to avoid
+ * awkward interactions with the relabel process.
+ *)
+ if ops.flags.selinux_relabel then g#set_selinux false;
(* Add disks. *)
add g dryrun;
diff --git a/daemon/selinux.c b/daemon/selinux.c
index d0a3832..879bc61 100644
--- a/daemon/selinux.c
+++ b/daemon/selinux.c
@@ -21,6 +21,8 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
+#include <sys/types.h>
+#include <sys/stat.h>
#ifdef HAVE_SELINUX_SELINUX_H
#include <selinux/selinux.h>
@@ -31,6 +33,8 @@
#include "actions.h"
#include "optgroups.h"
+GUESTFSD_EXT_CMD(str_setfiles, setfiles);
+
#if defined(HAVE_LIBSELINUX)
int
@@ -92,28 +96,188 @@ OPTGROUP_SELINUX_NOT_AVAILABLE
#endif /* !HAVE_LIBSELINUX */
int
-do_selinux_relabel (const char *root)
+optgroup_selinuxtools_available (void)
+{
+ return prog_exists (str_setfiles);
+}
+
+#define SELINUXTYPE "SELINUXTYPE"
+
+static int
+has_selinux_mountpoint (void)
+{
+ static int has_mp = -1;
+ if (has_mp == -1) {
+ struct stat sb;
+ dev_t root_dev;
+
+ if (stat ("/", &sb) == -1)
+ return -1;
+ root_dev = sb.st_dev;
+
+ if (stat ("/sys/fs/selinux", &sb) == 0 && sb.st_dev
!= root_dev) {
+ has_mp = 1;
+ return has_mp;
+ }
+
+ if (stat ("/selinux", &sb) == 0 && sb.st_dev !=
root_dev) {
+ has_mp = 1;
+ return has_mp;
+ }
+ }
+ return has_mp;
+}
+
+static int
+read_selinux_policy (const char *file, char **policy)
+{
+ FILE *f;
+ ssize_t r;
+ CLEANUP_FREE char *line = NULL;
+ size_t len;
+
+ f = fopen (file, "r");
+ if (f == NULL)
+ return -1;
+
+ while ((r = getline (&line, &len, f)) != -1) {
+ if ((size_t) r >= sizeof (SELINUXTYPE) + 1
+ && STRPREFIX (line, SELINUXTYPE "=")) {
+ *policy = strndup (line + sizeof (SELINUXTYPE), r - sizeof (SELINUXTYPE)
- 1);
+ if (*policy == NULL) {
+ int errno_save = errno;
+ fclose (f);
+ errno = errno_save;
+ return -1;
+ }
+ trim (*policy);
+ break;
+ }
+ }
+
+ fclose (f);
+
+ return 0;
+}
+
+static int
+length_without_training_slash (const char *path)
{
- CLEANUP_FREE char *cmd = NULL, *out = NULL;
- const char cmd_fmt[] - "if load_policy && fixfiles restore;
then\n"
- " rm -f %.*s/.autorelabel\n"
- "else\n"
- " touch %.*s/.autorelabel\n"
- " echo 'SELinux relabelling failed, will relabel at boot
instead.'\n"
- "fi\n";
- int len = strlen (root);
-
- if (root[len - 1] == '/')
+ int len = strlen (path);
+ if (path[len - 1] == '/')
--len;
+ return len;
+}
+
+int
+do_selinux_relabel (const char *root)
+{
+ CLEANUP_FREE char *selinux_config = NULL;
+ CLEANUP_FREE char *policy = NULL;
+ CLEANUP_FREE char *sys_rootpath = NULL;
+ CLEANUP_FREE char *sys_filecontextpath = NULL;
+ CLEANUP_FREE char *sys_autorelabelpath = NULL;
+ CLEANUP_FREE char *err = NULL;
+ CLEANUP_FREE char *out = NULL;
+ int r;
+ int len;
+ const size_t MAX_ARGS = 16;
+ const char *argv[MAX_ARGS];
+ size_t i = 0;
- if (asprintf (&cmd, cmd_fmt, len, root, len, root) == -1) {
- reply_with_perror ("asprintf");
+ if (has_selinux_mountpoint () > 0) {
+ reply_with_error ("cannot work when SELinux is enabled in the "
+ "appliance\n");
return -1;
}
- out = do_sh (cmd);
+ len = length_without_training_slash (root);
+
+ if (asprintf (&selinux_config, "%s%.*s/etc/selinux/config",
+ sysroot, len, root) == -1) {
+ if (verbose)
+ fprintf (stderr, "asprintf/selinux_config failed\n");
+ goto do_autorelabel;
+ }
+
+ r = read_selinux_policy (selinux_config, &policy);
+ if (r == -1) {
+ if (verbose)
+ fprintf (stderr, "cannot read policy from %s\n",
selinux_config);
+ goto do_autorelabel;
+ }
+ if (verbose)
+ fprintf (stderr, "policy in %s: %s\n", root, policy);
+
+ if (policy[0] == '\0')
+ goto do_autorelabel;
+
+ /* Make the root path relative to /sysroot. */
+ sys_rootpath = sysroot_path (root);
+ if (sys_rootpath == NULL) {
+ if (verbose)
+ fprintf (stderr, "sysroot_path failed\n");
+ goto do_autorelabel;
+ }
+
+ len = length_without_training_slash (sys_rootpath);
+
+ if (asprintf (&sys_filecontextpath,
+ "%.*s/etc/selinux/%s/contexts/files/file_contexts",
+ len, sys_rootpath, policy) == -1) {
+ if (verbose)
+ fprintf (stderr, "asprintf/file_contexts failed\n");
+ goto do_autorelabel;
+ }
+
+ ADD_ARG (argv, i, str_setfiles);
+ ADD_ARG (argv, i, "-W");
if (verbose)
- fprintf (stderr, "%s\n", out);
- return out == NULL ? -1 : 0;
+ ADD_ARG (argv, i, "-v");
+ ADD_ARG (argv, i, "-r");
+ ADD_ARG (argv, i, sys_rootpath);
+ ADD_ARG (argv, i, sys_filecontextpath);
+ ADD_ARG (argv, i, sys_rootpath);
+ ADD_ARG (argv, i, NULL);
+
+ r = commandv (verbose ? &out : NULL, &err, argv);
+ if (r == -1) {
+ if (verbose)
+ fprintf (stderr, "failed setfiles %s for %s: %s\n", policy,
root, err);
+ goto do_autorelabel;
+ }
+ if (verbose)
+ fprintf (stderr, "output from setfiles:\n%s\n", out);
+
+ if (asprintf (&sys_autorelabelpath, "%.*s/.autorelabel",
+ len, sys_rootpath) == -1) {
+ if (verbose)
+ fprintf (stderr, "asprintf/autorelabelpath failed\n");
+ /* We can safely exit in this case as well, since at most there is a
+ * ~/.autorelabel left in the guest.
+ */
+ return 0;
+ }
+
+ unlink (sys_autorelabelpath);
+
+ return 0;
+
+ do_autorelabel:
+ /* All of the above failed -- try to touch .autorelabel in the root
+ * and hope for the best.
+ */
+ if (verbose)
+ fprintf (stderr, "trying /.autorelabel\n");
+
+ len = length_without_training_slash (root);
+
+ if (asprintf (&sys_autorelabelpath, "%.*s/.autorelabel",
+ len, root) == -1) {
+ if (verbose)
+ fprintf (stderr, "asprintf/autorelabelpath failed\n");
+ return -1;
+ }
+
+ return do_touch (sys_autorelabelpath);
}
diff --git a/generator/actions.ml b/generator/actions.ml
index a59fe31..3bb91c8 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -11928,12 +11928,19 @@ New (SVR4) portable format with a checksum.
name = "selinux_relabel";
style = RErr, [Pathname "root"], [];
proc_nr = Some 420;
+ optional = Some "selinuxtools";
shortdesc = "do the SELinux relabel of the files";
longdesc = "\
This does a relabel of the files of the system under the specified
C<root> according to the SELinux policy in the system mounted in that
C<root>.
+I<Note:> the relabel does not work when SELinux is enabled (see
+C<guestfs_set_selinux>), as the kernel and the policy in the
+appliance might not be compatible with the policy found inside the
+guest mounted at C<root>; hence, this function will error out in this
+case.
+
See the documentation about SELINUX in L<guestfs(3)>." };
]
--
1.9.3
Richard W.M. Jones
2014-May-27 08:08 UTC
Re: [Libguestfs] [PATCH 2/2] Use setfiles from the appliance for the SELinux relabel (RHBZ#1089100).
On Mon, May 26, 2014 at 11:21:59AM +0200, Pino Toscano wrote:> Rewrite the relabel API to read the policy configured in the guest, > invoking setfiles (added as part of the appliance, as part of > policycoreutils) to relabel the specified root. In case of failure at > any point of the process, a touch of .autorelabel in the root is tried > as last-attempt measure to do the relabel. > > Considering that running SELinux tools in the appliance might be > affected by the SELinux state (leading to wrong results), > selinux_relabel now bails out if SELinux is enabled in the appliance. > As a result of this, virt-builder and virt-customize explicitly disable > it if the relabel is enabled.> - g#set_selinux ops.flags.selinux_relabel; > + (* If a relabel is needed, make sure to turn SELinux off to avoid > + * awkward interactions with the relabel process. > + *) > + if ops.flags.selinux_relabel then g#set_selinux false;This defaults to false, so AFAICT you could just remove this hunk. Or call g#set_selinux false unconditionally to make your intention explicit? (Same for the customize_main.ml hunk)> + len = length_without_training_slash (root); > + > + if (asprintf (&selinux_config, "%s%.*s/etc/selinux/config", > + sysroot, len, root) == -1) { > + if (verbose) > + fprintf (stderr, "asprintf/selinux_config failed\n"); > + goto do_autorelabel; > + } > + > + r = read_selinux_policy (selinux_config, &policy); > + if (r == -1) { > + if (verbose) > + fprintf (stderr, "cannot read policy from %s\n", selinux_config); > + goto do_autorelabel; > + } > + if (verbose) > + fprintf (stderr, "policy in %s: %s\n", root, policy); > + > + if (policy[0] == '\0') > + goto do_autorelabel;You'll probably find this is much easier to write and a lot more robust using augeas calls. But yes, generally looks good. 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/
Pino Toscano
2014-May-27 08:52 UTC
Re: [Libguestfs] [PATCH 2/2] Use setfiles from the appliance for the SELinux relabel (RHBZ#1089100).
On Tuesday 27 May 2014 09:08:27 Richard W.M. Jones wrote:> On Mon, May 26, 2014 at 11:21:59AM +0200, Pino Toscano wrote: > > Rewrite the relabel API to read the policy configured in the guest, > > invoking setfiles (added as part of the appliance, as part of > > policycoreutils) to relabel the specified root. In case of failure > > at > > any point of the process, a touch of .autorelabel in the root is > > tried as last-attempt measure to do the relabel. > > > > Considering that running SELinux tools in the appliance might be > > affected by the SELinux state (leading to wrong results), > > selinux_relabel now bails out if SELinux is enabled in the > > appliance. > > As a result of this, virt-builder and virt-customize explicitly > > disable it if the relabel is enabled. > > > > - g#set_selinux ops.flags.selinux_relabel; > > + (* If a relabel is needed, make sure to turn SELinux off to > > avoid + * awkward interactions with the relabel process. > > + *) > > + if ops.flags.selinux_relabel then g#set_selinux false; > > This defaults to false, so AFAICT you could just remove this hunk. Or > call g#set_selinux false unconditionally to make your intention > explicit? > > (Same for the customize_main.ml hunk)Yes, that together its comment above is done to make that setting explicit, so it is not changed in the future creating issues.> > + len = length_without_training_slash (root); > > + > > + if (asprintf (&selinux_config, "%s%.*s/etc/selinux/config", > > + sysroot, len, root) == -1) { > > + if (verbose) > > + fprintf (stderr, "asprintf/selinux_config failed\n"); > > + goto do_autorelabel; > > + } > > + > > + r = read_selinux_policy (selinux_config, &policy); > > + if (r == -1) { > > + if (verbose) > > + fprintf (stderr, "cannot read policy from %s\n", > > selinux_config); + goto do_autorelabel; > > + } > > + if (verbose) > > + fprintf (stderr, "policy in %s: %s\n", root, policy); > > + > > + if (policy[0] == '\0') > > + goto do_autorelabel; > > You'll probably find this is much easier to write and a lot more > robust using augeas calls.Good idea, I will use it. Thanks, -- Pino Toscano