[PATCH v2 0/5] Fix SELinux security contexts so we can access shared disks (RHBZ#912499).
Link to version 1: https://www.redhat.com/archives/libguestfs/2013-February/thread.html#00122 Changes since version 1: - I've pushed two (of the three) code refactoring patches. The third one proved rather hard to move. - selinuxnorelabel option is no more. Instead there is a second internal API (internal_set_libvirt_selinux_norelabel_disks). - fixed bogus commit message - various other very minor fixes Rich.
From: "Richard W.M. Jones" <rjones@redhat.com> This internal API sets two SELinux labels in the handle (the process label and the image label -- they are closely related). If using the libvirt attach-method with SELinux and sVirt, then this will cause the following XML to be added to the appliance definition: <seclabel type=static model=selinux relabel=yes> <label>[LABEL HERE]</label> <imagelabel>[IMAGELABEL HERE]</imagelabel> </seclabel> It is ignored by other attach-methods. --- generator/actions.ml | 12 ++++++++++++ src/guestfs-internal.h | 2 ++ src/handle.c | 2 ++ src/launch-libvirt.c | 36 ++++++++++++++++++++++++++++++++++++ 4 files changed, 52 insertions(+) diff --git a/generator/actions.ml b/generator/actions.ml index 8a8e3ff..59e667d 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -2694,6 +2694,18 @@ the default. Else C</var/tmp> is the default." }; longdesc = "\ Get the directory used by the handle to store the appliance cache." }; + { defaults with + name = "internal_set_libvirt_selinux_label"; + style = RErr, [String "label"; String "imagelabel"], []; + blocking = false; + visibility = VInternal; + shortdesc = "set SELinux label used by the libvirt attach method"; + longdesc = "\ +This internal function sets the SELinux security label (in +reality, two labels: the process label and the image label) +used by the appliance when the libvirt attach method is selected +(it is ignored by other attach methods)." }; + ] (* daemon_functions are any functions which cause some action diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h index e1a7d31..78e2bf5 100644 --- a/src/guestfs-internal.h +++ b/src/guestfs-internal.h @@ -322,6 +322,8 @@ struct guestfs_h virDomainPtr dom; /* libvirt domain */ } virt; #endif + char *virt_selinux_label; + char *virt_selinux_imagelabel; }; /* Per-filesystem data stored for inspect_os. */ diff --git a/src/handle.c b/src/handle.c index c630daf..2f44632 100644 --- a/src/handle.c +++ b/src/handle.c @@ -326,6 +326,8 @@ guestfs_close (guestfs_h *g) if (g->pda) hash_free (g->pda); + free (g->virt_selinux_label); + free (g->virt_selinux_imagelabel); free (g->tmpdir); free (g->env_tmpdir); free (g->int_tmpdir); diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c index 7db2ce5..68d875e 100644 --- a/src/launch-libvirt.c +++ b/src/launch-libvirt.c @@ -855,6 +855,31 @@ construct_libvirt_xml_seclabel (guestfs_h *g, BAD_CAST "none")); XMLERROR (-1, xmlTextWriterEndElement (xo)); } + else if (g->virt_selinux_label && g->virt_selinux_imagelabel) { + /* Enable sVirt and pass a custom <seclabel/> inherited from the + * original libvirt domain (when guestfs_add_domain was called). + * https://bugzilla.redhat.com/show_bug.cgi?id=912499#c7 + */ + XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "seclabel")); + XMLERROR (-1, + xmlTextWriterWriteAttribute (xo, BAD_CAST "type", + BAD_CAST "static")); + XMLERROR (-1, + xmlTextWriterWriteAttribute (xo, BAD_CAST "model", + BAD_CAST "selinux")); + XMLERROR (-1, + xmlTextWriterWriteAttribute (xo, BAD_CAST "relabel", + BAD_CAST "yes")); + XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "label")); + XMLERROR (-1, xmlTextWriterWriteString (xo, + BAD_CAST g->virt_selinux_label)); + XMLERROR (-1, xmlTextWriterEndElement (xo)); + XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "imagelabel")); + XMLERROR (-1, xmlTextWriterWriteString (xo, + BAD_CAST g->virt_selinux_imagelabel)); + XMLERROR (-1, xmlTextWriterEndElement (xo)); + XMLERROR (-1, xmlTextWriterEndElement (xo)); + } return 0; } @@ -1603,3 +1628,14 @@ struct attach_ops attach_ops_libvirt = { }; #endif /* no libvirt or libxml2 at compile time */ + +int +guestfs__internal_set_libvirt_selinux_label (guestfs_h *g, const char *label, + const char *imagelabel) +{ + free (g->virt_selinux_label); + g->virt_selinux_label = safe_strdup (g, label); + free (g->virt_selinux_imagelabel); + g->virt_selinux_imagelabel = safe_strdup (g, imagelabel); + return 0; +} -- 1.8.1.2
From: "Richard W.M. Jones" <rjones@redhat.com> If set, this causes <seclabel model=selinux relabel=no> to be added to the disk element in the libvirt XML. It has no effect *except* on the libvirt attach method when SELinux and sVirt is being used. --- generator/actions.ml | 11 +++++++++++ src/guestfs-internal.h | 1 + src/launch-libvirt.c | 30 ++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+) diff --git a/generator/actions.ml b/generator/actions.ml index 59e667d..f685e92 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -2706,6 +2706,17 @@ reality, two labels: the process label and the image label) used by the appliance when the libvirt attach method is selected (it is ignored by other attach methods)." }; + { defaults with + name = "internal_set_libvirt_selinux_norelabel_disks"; + style = RErr, [Bool "norelabeldisks"], []; + blocking = false; + visibility = VInternal; + shortdesc = "tell libvirt attach method not to relabel disks"; + longdesc = "\ +This internal function adds E<lt>seclabel model=selinux relabel=noE<gt> +to all application disks. It is only used by the libvirt attach method +and is ignored by other attach methods." }; + ] (* daemon_functions are any functions which cause some action diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h index 78e2bf5..c25b893 100644 --- a/src/guestfs-internal.h +++ b/src/guestfs-internal.h @@ -324,6 +324,7 @@ struct guestfs_h #endif char *virt_selinux_label; char *virt_selinux_imagelabel; + bool virt_selinux_norelabel_disks; }; /* Per-filesystem data stored for inspect_os. */ diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c index 68d875e..318847a 100644 --- a/src/launch-libvirt.c +++ b/src/launch-libvirt.c @@ -651,6 +651,7 @@ static int construct_libvirt_xml_lifecycle (guestfs_h *g, const struct libvirt_x static int construct_libvirt_xml_devices (guestfs_h *g, const struct libvirt_xml_params *params, xmlTextWriterPtr xo); static int construct_libvirt_xml_qemu_cmdline (guestfs_h *g, const struct libvirt_xml_params *params, xmlTextWriterPtr xo); static int construct_libvirt_xml_disk (guestfs_h *g, xmlTextWriterPtr xo, struct drive *drv, size_t drv_index); +static int construct_libvirt_xml_disk_source_seclabel (guestfs_h *g, xmlTextWriterPtr xo); static int construct_libvirt_xml_appliance (guestfs_h *g, const struct libvirt_xml_params *params, xmlTextWriterPtr xo); /* Note this macro is rather specialized: It assumes that any local @@ -1035,6 +1036,8 @@ construct_libvirt_xml_disk (guestfs_h *g, XMLERROR (-1, xmlTextWriterWriteAttribute (xo, BAD_CAST "file", BAD_CAST drv_priv->path)); + if (construct_libvirt_xml_disk_source_seclabel (g, xo) == -1) + return -1; XMLERROR (-1, xmlTextWriterEndElement (xo)); } else { @@ -1046,6 +1049,8 @@ construct_libvirt_xml_disk (guestfs_h *g, XMLERROR (-1, xmlTextWriterWriteAttribute (xo, BAD_CAST "dev", BAD_CAST drv_priv->path)); + if (construct_libvirt_xml_disk_source_seclabel (g, xo) == -1) + return -1; XMLERROR (-1, xmlTextWriterEndElement (xo)); } @@ -1131,6 +1136,24 @@ construct_libvirt_xml_disk (guestfs_h *g, } static int +construct_libvirt_xml_disk_source_seclabel (guestfs_h *g, + xmlTextWriterPtr xo) +{ + if (g->virt_selinux_norelabel_disks) { + XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "seclabel")); + XMLERROR (-1, + xmlTextWriterWriteAttribute (xo, BAD_CAST "model", + BAD_CAST "selinux")); + XMLERROR (-1, + xmlTextWriterWriteAttribute (xo, BAD_CAST "relabel", + BAD_CAST "no")); + XMLERROR (-1, xmlTextWriterEndElement (xo)); + } + + return 0; +} + +static int construct_libvirt_xml_appliance (guestfs_h *g, const struct libvirt_xml_params *params, xmlTextWriterPtr xo) @@ -1639,3 +1662,10 @@ guestfs__internal_set_libvirt_selinux_label (guestfs_h *g, const char *label, g->virt_selinux_imagelabel = safe_strdup (g, imagelabel); return 0; } + +int +guestfs__internal_set_libvirt_selinux_norelabel_disks (guestfs_h *g, int flag) +{ + g->virt_selinux_norelabel_disks = flag; + return 0; +} -- 1.8.1.2
On Thu, 2013-02-28 at 16:02 +0000, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones@redhat.com>
If set, this causes <seclabel model=selinux relabel=no> to be added to the disk element in the libvirt XML.
It has no effect *except* on the libvirt attach method when SELinux and sVirt is being used. --- generator/actions.ml | 11 +++++++++++ src/guestfs-internal.h | 1 + src/launch-libvirt.c | 30 ++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+)
ACK. Matt
From: "Richard W.M. Jones" <rjones@redhat.com> When a disk is opened readonly, the libvirt attach-method privately creates a qcow2 overlay on top. This commit lets that overlay get an SELinux label, and sets it to the imagelabel specified by guestfs_internal_set_libvirt_selinux_label. The above only applies to the libvirt attach-method. --- src/launch-libvirt.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c index 318847a..b692fd6 100644 --- a/src/launch-libvirt.c +++ b/src/launch-libvirt.c @@ -133,8 +133,8 @@ static int is_custom_qemu (guestfs_h *g); static int is_blk (const char *path); static int random_chars (char *ret, size_t len); static void ignore_errors (void *ignore, virErrorPtr ignore2); -static char *make_qcow2_overlay (guestfs_h *g, const char *path, const char *format); -static int make_qcow2_overlay_for_drive (guestfs_h *g, struct drive *drv); +static char *make_qcow2_overlay (guestfs_h *g, const char *path, const char *format, const char *selinux_imagelabel); +static int make_qcow2_overlay_for_drive (guestfs_h *g, struct drive *drv, const char *selinux_imagelabel); static void drive_free_priv (void *); static void set_socket_create_context (guestfs_h *g); static void clear_socket_create_context (guestfs_h *g); @@ -235,13 +235,13 @@ launch_libvirt (guestfs_h *g, const char *libvirt_uri) * Note that appliance can be NULL if using the old-style appliance. */ if (appliance) { - params.appliance_overlay = make_qcow2_overlay (g, appliance, "raw"); + params.appliance_overlay = make_qcow2_overlay (g, appliance, "raw", NULL); if (!params.appliance_overlay) goto cleanup; } ITER_DRIVES (g, i, drv) { - if (make_qcow2_overlay_for_drive (g, drv) == -1) + if (make_qcow2_overlay_for_drive (g, drv, g->virt_selinux_imagelabel) == -1) goto cleanup; } @@ -1353,7 +1353,8 @@ ignore_errors (void *ignore, virErrorPtr ignore2) /* Create a temporary qcow2 overlay on top of 'path'. */ static char * -make_qcow2_overlay (guestfs_h *g, const char *path, const char *format) +make_qcow2_overlay (guestfs_h *g, const char *path, const char *format, + const char *selinux_imagelabel) { char *tmpfile = NULL; CLEANUP_CMD_CLOSE struct command *cmd = guestfs___new_command (g); @@ -1384,6 +1385,15 @@ make_qcow2_overlay (guestfs_h *g, const char *path, const char *format) goto error; } +#if HAVE_LIBSELINUX + if (selinux_imagelabel) { + debug (g, "setting SELinux label on %s to %s", + tmpfile, selinux_imagelabel); + if (setfilecon (tmpfile, (security_context_t) selinux_imagelabel) == -1) + selinux_warning (g, __func__, "setfilecon", tmpfile); + } +#endif + return tmpfile; /* caller frees */ error: @@ -1393,7 +1403,8 @@ make_qcow2_overlay (guestfs_h *g, const char *path, const char *format) } static int -make_qcow2_overlay_for_drive (guestfs_h *g, struct drive *drv) +make_qcow2_overlay_for_drive (guestfs_h *g, struct drive *drv, + const char *selinux_imagelabel) { char *path; struct drive_libvirt *drv_priv; @@ -1417,7 +1428,8 @@ make_qcow2_overlay_for_drive (guestfs_h *g, struct drive *drv) drv_priv->format = drv->format ? safe_strdup (g, drv->format) : NULL; } else { - drv_priv->path = make_qcow2_overlay (g, path, drv->format); + drv_priv->path = make_qcow2_overlay (g, path, drv->format, + selinux_imagelabel); free (path); if (!drv_priv->path) return -1; @@ -1534,7 +1546,7 @@ hot_add_drive_libvirt (guestfs_h *g, struct drive *drv, size_t drv_index) /* Create overlay for read-only drive. This works around lack of * support for <transient/> disks in libvirt. */ - if (make_qcow2_overlay_for_drive (g, drv) == -1) + if (make_qcow2_overlay_for_drive (g, drv, g->virt_selinux_imagelabel) == -1) return -1; /* Create the XML for the new disk. */ -- 1.8.1.2
On Thu, 2013-02-28 at 16:02 +0000, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones@redhat.com>
When a disk is opened readonly, the libvirt attach-method privately creates a qcow2 overlay on top.
This commit lets that overlay get an SELinux label, and sets it to the imagelabel specified by guestfs_internal_set_libvirt_selinux_label.
The above only applies to the libvirt attach-method. --- src/launch-libvirt.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c index 318847a..b692fd6 100644 --- a/src/launch-libvirt.c +++ b/src/launch-libvirt.c @@ -133,8 +133,8 @@ static int is_custom_qemu (guestfs_h *g); static int is_blk (const char *path); static int random_chars (char *ret, size_t len); static void ignore_errors (void *ignore, virErrorPtr ignore2); -static char *make_qcow2_overlay (guestfs_h *g, const char *path, const char *format); -static int make_qcow2_overlay_for_drive (guestfs_h *g, struct drive *drv); +static char *make_qcow2_overlay (guestfs_h *g, const char *path, const char *format, const char *selinux_imagelabel); +static int make_qcow2_overlay_for_drive (guestfs_h *g, struct drive *drv, const char *selinux_imagelabel); static void drive_free_priv (void *); static void set_socket_create_context (guestfs_h *g); static void clear_socket_create_context (guestfs_h *g); @@ -235,13 +235,13 @@ launch_libvirt (guestfs_h *g, const char *libvirt_uri) * Note that appliance can be NULL if using the old-style appliance. */ if (appliance) { - params.appliance_overlay = make_qcow2_overlay (g, appliance, "raw"); + params.appliance_overlay = make_qcow2_overlay (g, appliance, "raw", NULL); if (!params.appliance_overlay) goto cleanup; }
I remain convinced that this is going to bite us at some point in the future. The fact that it works now is essentially a quirk of the default SELinux policy. I still don't understand at all how the confined guest can access the underlying appliance image, which libvirt presumably doesn't relabel. Given that it does currently work I don't think we should hold up these patches for a solution. However, I think we should document the problem as we currently understand it somewhere in the source tree, perhaps the TODO file.
ITER_DRIVES (g, i, drv) { - if (make_qcow2_overlay_for_drive (g, drv) == -1) + if (make_qcow2_overlay_for_drive (g, drv, g->virt_selinux_imagelabel) == -1) goto cleanup; }
@@ -1353,7 +1353,8 @@ ignore_errors (void *ignore, virErrorPtr ignore2)
/* Create a temporary qcow2 overlay on top of 'path'. */ static char * -make_qcow2_overlay (guestfs_h *g, const char *path, const char *format) +make_qcow2_overlay (guestfs_h *g, const char *path, const char *format, + const char *selinux_imagelabel) { char *tmpfile = NULL; CLEANUP_CMD_CLOSE struct command *cmd = guestfs___new_command (g); @@ -1384,6 +1385,15 @@ make_qcow2_overlay (guestfs_h *g, const char *path, const char *format) goto error; }
+#if HAVE_LIBSELINUX + if (selinux_imagelabel) { + debug (g, "setting SELinux label on %s to %s", + tmpfile, selinux_imagelabel); + if (setfilecon (tmpfile, (security_context_t) selinux_imagelabel) == -1) + selinux_warning (g, __func__, "setfilecon", tmpfile); + } +#endif + return tmpfile; /* caller frees */
error: @@ -1393,7 +1403,8 @@ make_qcow2_overlay (guestfs_h *g, const char *path, const char *format) }
static int -make_qcow2_overlay_for_drive (guestfs_h *g, struct drive *drv) +make_qcow2_overlay_for_drive (guestfs_h *g, struct drive *drv, + const char *selinux_imagelabel) { char *path; struct drive_libvirt *drv_priv; @@ -1417,7 +1428,8 @@ make_qcow2_overlay_for_drive (guestfs_h *g, struct drive *drv) drv_priv->format = drv->format ? safe_strdup (g, drv->format) : NULL; } else { - drv_priv->path = make_qcow2_overlay (g, path, drv->format); + drv_priv->path = make_qcow2_overlay (g, path, drv->format, + selinux_imagelabel); free (path); if (!drv_priv->path) return -1; @@ -1534,7 +1546,7 @@ hot_add_drive_libvirt (guestfs_h *g, struct drive *drv, size_t drv_index) /* Create overlay for read-only drive. This works around lack of * support for <transient/> disks in libvirt. */ - if (make_qcow2_overlay_for_drive (g, drv) == -1) + if (make_qcow2_overlay_for_drive (g, drv, g->virt_selinux_imagelabel) == -1) return -1;
/* Create the XML for the new disk. */
I'd prefer documentation of the problem of SELinux and the appliance image, but otherwise ACK. Matt
On Fri, Mar 01, 2013 at 10:22:14AM +0000, Matthew Booth wrote:
On Thu, 2013-02-28 at 16:02 +0000, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones@redhat.com>
When a disk is opened readonly, the libvirt attach-method privately creates a qcow2 overlay on top.
This commit lets that overlay get an SELinux label, and sets it to the imagelabel specified by guestfs_internal_set_libvirt_selinux_label.
The above only applies to the libvirt attach-method. --- src/launch-libvirt.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c index 318847a..b692fd6 100644 --- a/src/launch-libvirt.c +++ b/src/launch-libvirt.c @@ -133,8 +133,8 @@ static int is_custom_qemu (guestfs_h *g); static int is_blk (const char *path); static int random_chars (char *ret, size_t len); static void ignore_errors (void *ignore, virErrorPtr ignore2); -static char *make_qcow2_overlay (guestfs_h *g, const char *path, const char *format); -static int make_qcow2_overlay_for_drive (guestfs_h *g, struct drive *drv); +static char *make_qcow2_overlay (guestfs_h *g, const char *path, const char *format, const char *selinux_imagelabel); +static int make_qcow2_overlay_for_drive (guestfs_h *g, struct drive *drv, const char *selinux_imagelabel); static void drive_free_priv (void *); static void set_socket_create_context (guestfs_h *g); static void clear_socket_create_context (guestfs_h *g); @@ -235,13 +235,13 @@ launch_libvirt (guestfs_h *g, const char *libvirt_uri) * Note that appliance can be NULL if using the old-style appliance. */ if (appliance) { - params.appliance_overlay = make_qcow2_overlay (g, appliance, "raw"); + params.appliance_overlay = make_qcow2_overlay (g, appliance, "raw", NULL); if (!params.appliance_overlay) goto cleanup; }
I remain convinced that this is going to bite us at some point in the future. The fact that it works now is essentially a quirk of the default SELinux policy. I still don't understand at all how the confined guest can access the underlying appliance image, which libvirt presumably doesn't relabel.
There's no mystery. Because the appliance disk has a <shareable/> tag, libvirt labels it as Dan explains here: https://www.redhat.com/archives/libguestfs/2013-February/msg00139.html Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones 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/
On Fri, 2013-03-01 at 10:49 +0000, Richard W.M. Jones wrote:
On Fri, Mar 01, 2013 at 10:22:14AM +0000, Matthew Booth wrote:
On Thu, 2013-02-28 at 16:02 +0000, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones@redhat.com>
When a disk is opened readonly, the libvirt attach-method privately creates a qcow2 overlay on top.
This commit lets that overlay get an SELinux label, and sets it to the imagelabel specified by guestfs_internal_set_libvirt_selinux_label.
The above only applies to the libvirt attach-method. --- src/launch-libvirt.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c index 318847a..b692fd6 100644 --- a/src/launch-libvirt.c +++ b/src/launch-libvirt.c @@ -133,8 +133,8 @@ static int is_custom_qemu (guestfs_h *g); static int is_blk (const char *path); static int random_chars (char *ret, size_t len); static void ignore_errors (void *ignore, virErrorPtr ignore2); -static char *make_qcow2_overlay (guestfs_h *g, const char *path, const char *format); -static int make_qcow2_overlay_for_drive (guestfs_h *g, struct drive *drv); +static char *make_qcow2_overlay (guestfs_h *g, const char *path, const char *format, const char *selinux_imagelabel); +static int make_qcow2_overlay_for_drive (guestfs_h *g, struct drive *drv, const char *selinux_imagelabel); static void drive_free_priv (void *); static void set_socket_create_context (guestfs_h *g); static void clear_socket_create_context (guestfs_h *g); @@ -235,13 +235,13 @@ launch_libvirt (guestfs_h *g, const char *libvirt_uri) * Note that appliance can be NULL if using the old-style appliance. */ if (appliance) { - params.appliance_overlay = make_qcow2_overlay (g, appliance, "raw"); + params.appliance_overlay = make_qcow2_overlay (g, appliance, "raw", NULL); if (!params.appliance_overlay) goto cleanup; }
I remain convinced that this is going to bite us at some point in the future. The fact that it works now is essentially a quirk of the default SELinux policy. I still don't understand at all how the confined guest can access the underlying appliance image, which libvirt presumably doesn't relabel.
There's no mystery. Because the appliance disk has a <shareable/> tag, libvirt labels it as Dan explains here:
https://www.redhat.com/archives/libguestfs/2013-February/msg00139.html
Actually, that doesn't clear it up. Firstly, the <shareable/> attribute uses an alternative labelling. This alternative labelling still requires support from the policy, so it remains policy-specific. Secondly, it doesn't explain if/how the backing file is relabelled. If it isn't, why is it accessible? Again, this would be a quirk of the default policy. Dan? Matt
From: "Richard W.M. Jones" <rjones@redhat.com> When adding a domain (ie. guestfs_add_domain), read the SELinux <label/> and <imagelabel/> from the guest and use them for the appliance. The appliance is statically labelled the same as the guest, so it is able to read its disks. However tell libvirt not to try relabelling the disks, to prevent libvirt from disturbing the existing labels on the disks (in particular when the libvirt connection is closed, we don't want libvirt to try to restore some other label on the disks). --- src/libvirt-domain.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 3f5b1ed..768ed78 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -20,6 +20,7 @@ #include <stdio.h> #include <stdlib.h> +#include <stdbool.h> #include <assert.h> #ifdef HAVE_LIBVIRT @@ -42,6 +43,7 @@ static xmlDocPtr get_domain_xml (guestfs_h *g, virDomainPtr dom); static ssize_t for_each_disk (guestfs_h *g, xmlDocPtr doc, int (*f) (guestfs_h *g, const char *filename, const char *format, int readonly, void *data), void *data); +static int libvirt_selinux_label (guestfs_h *g, xmlDocPtr doc, char **label_rtn, char **imagelabel_rtn); static void ignore_errors (void *ignore, virErrorPtr ignore2) @@ -169,6 +171,7 @@ guestfs___add_libvirt_dom (guestfs_h *g, virDomainPtr dom, size_t ckp; struct add_disk_data data; CLEANUP_XMLFREEDOC xmlDocPtr doc = NULL; + CLEANUP_FREE char *label = NULL, *imagelabel = NULL; readonly = optargs->bitmask & GUESTFS___ADD_LIBVIRT_DOM_READONLY_BITMASK @@ -234,6 +237,16 @@ guestfs___add_libvirt_dom (guestfs_h *g, virDomainPtr dom, if ((doc = get_domain_xml (g, dom)) == NULL) return -1; + /* Find and pass the SELinux security label to the libvirt back end. */ + if (libvirt_selinux_label (g, doc, &label, &imagelabel) == -1) + return -1; + if (label && imagelabel) { + guestfs_internal_set_libvirt_selinux_label (g, label, imagelabel); + guestfs_internal_set_libvirt_selinux_norelabel_disks (g, 1); + } + else + guestfs_internal_set_libvirt_selinux_norelabel_disks (g, 0); + /* Add the disks. */ data.optargs.bitmask = 0; data.readonly = readonly; @@ -374,6 +387,66 @@ connect_live (guestfs_h *g, virDomainPtr dom) return guestfs_set_attach_method (g, attach_method); } +/* Find the <seclabel/> element in the libvirt XML, and if it exists + * get the SELinux process label and image label from it. + * + * The reason for all this is because of sVirt: + * https://bugzilla.redhat.com/show_bug.cgi?id=912499#c7 + */ +static int +libvirt_selinux_label (guestfs_h *g, xmlDocPtr doc, + char **label_rtn, char **imagelabel_rtn) +{ + CLEANUP_XMLXPATHFREECONTEXT xmlXPathContextPtr xpathCtx = NULL; + CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xpathObj = NULL; + xmlNodeSetPtr nodes; + size_t nr_nodes; + xmlNodePtr node, child; + bool gotlabel = 0, gotimagelabel = 0; + + xpathCtx = xmlXPathNewContext (doc); + if (xpathCtx == NULL) { + error (g, _("unable to create new XPath context")); + return -1; + } + + /* This gives us a set of all the <seclabel/> nodes, hopefully only one. */ + xpathObj = xmlXPathEvalExpression (BAD_CAST "/domain/seclabel", + xpathCtx); + if (xpathObj == NULL) { + error (g, _("unable to evaluate XPath expression")); + return -1; + } + + nodes = xpathObj->nodesetval; + nr_nodes = nodes->nodeNr; + + if (nr_nodes == 0 || nr_nodes > 1) + return 0; + + node = nodes->nodeTab[0]; + if (node->type != XML_ELEMENT_NODE) { + error (g, _("expected <seclabel/> to be an XML element")); + return -1; + } + + /* Find the <label/> and <imagelabel/> child nodes. */ + for (child = node->children; child != NULL; child = child->next) { + if (!gotlabel && STREQ ((char *) child->name, "label")) { + /* Get the label content. */ + *label_rtn = (char *) xmlNodeGetContent (child); + gotlabel = 1; + } + if (!gotimagelabel && STREQ ((char *) child->name, "imagelabel")) { + /* Get the imagelabel content. */ + *imagelabel_rtn = (char *) xmlNodeGetContent (child); + gotimagelabel = 1; + } + } + + return 0; +} + /* The callback function 'f' is called once for each disk. * * Returns number of disks, or -1 if there was an error. -- 1.8.1.2
On Thu, 2013-02-28 at 16:02 +0000, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones@redhat.com>
When adding a domain (ie. guestfs_add_domain), read the SELinux <label/> and <imagelabel/> from the guest and use them for the appliance. The appliance is statically labelled the same as the guest, so it is able to read its disks.
However tell libvirt not to try relabelling the disks, to prevent libvirt from disturbing the existing labels on the disks (in particular when the libvirt connection is closed, we don't want libvirt to try to restore some other label on the disks). --- src/libvirt-domain.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 3f5b1ed..768ed78 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -20,6 +20,7 @@
#include <stdio.h> #include <stdlib.h> +#include <stdbool.h> #include <assert.h>
#ifdef HAVE_LIBVIRT @@ -42,6 +43,7 @@
static xmlDocPtr get_domain_xml (guestfs_h *g, virDomainPtr dom); static ssize_t for_each_disk (guestfs_h *g, xmlDocPtr doc, int (*f) (guestfs_h *g, const char *filename, const char *format, int readonly, void *data), void *data); +static int libvirt_selinux_label (guestfs_h *g, xmlDocPtr doc, char **label_rtn, char **imagelabel_rtn);
static void ignore_errors (void *ignore, virErrorPtr ignore2) @@ -169,6 +171,7 @@ guestfs___add_libvirt_dom (guestfs_h *g, virDomainPtr dom, size_t ckp; struct add_disk_data data; CLEANUP_XMLFREEDOC xmlDocPtr doc = NULL; + CLEANUP_FREE char *label = NULL, *imagelabel = NULL;
readonly = optargs->bitmask & GUESTFS___ADD_LIBVIRT_DOM_READONLY_BITMASK @@ -234,6 +237,16 @@ guestfs___add_libvirt_dom (guestfs_h *g, virDomainPtr dom, if ((doc = get_domain_xml (g, dom)) == NULL) return -1;
+ /* Find and pass the SELinux security label to the libvirt back end. */ + if (libvirt_selinux_label (g, doc, &label, &imagelabel) == -1) + return -1; + if (label && imagelabel) { + guestfs_internal_set_libvirt_selinux_label (g, label, imagelabel); + guestfs_internal_set_libvirt_selinux_norelabel_disks (g, 1);
Do we need both internal apis? 1 seems to imply the other.
+ } + else + guestfs_internal_set_libvirt_selinux_norelabel_disks (g, 0); + /* Add the disks. */ data.optargs.bitmask = 0; data.readonly = readonly; @@ -374,6 +387,66 @@ connect_live (guestfs_h *g, virDomainPtr dom) return guestfs_set_attach_method (g, attach_method); }
+/* Find the <seclabel/> element in the libvirt XML, and if it exists + * get the SELinux process label and image label from it. + * + * The reason for all this is because of sVirt: + * https://bugzilla.redhat.com/show_bug.cgi?id=912499#c7 + */ +static int +libvirt_selinux_label (guestfs_h *g, xmlDocPtr doc, + char **label_rtn, char **imagelabel_rtn) +{ + CLEANUP_XMLXPATHFREECONTEXT xmlXPathContextPtr xpathCtx = NULL; + CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xpathObj = NULL; + xmlNodeSetPtr nodes; + size_t nr_nodes; + xmlNodePtr node, child; + bool gotlabel = 0, gotimagelabel = 0;
gotlabel and gotimagelabel appear to be unused apart from assignment.
+ xpathCtx = xmlXPathNewContext (doc); + if (xpathCtx == NULL) { + error (g, _("unable to create new XPath context")); + return -1; + } + + /* This gives us a set of all the <seclabel/> nodes, hopefully only one. */ + xpathObj = xmlXPathEvalExpression (BAD_CAST "/domain/seclabel", + xpathCtx); + if (xpathObj == NULL) { + error (g, _("unable to evaluate XPath expression")); + return -1; + } + + nodes = xpathObj->nodesetval; + nr_nodes = nodes->nodeNr; + + if (nr_nodes == 0 || nr_nodes > 1) + return 0;
Not terribly important, but I wouldn't silently bomb out here if nr_nodes > 1. If you're going to catch it at all I'd add a warning and continue.
+ node = nodes->nodeTab[0]; + if (node->type != XML_ELEMENT_NODE) { + error (g, _("expected <seclabel/> to be an XML element")); + return -1; + }
For this to happen would require bugs in both libvirt and xmlXPathEvalExpression(). For clarity and brevity I wouldn't bother error checking this.
+ + /* Find the <label/> and <imagelabel/> child nodes. */ + for (child = node->children; child != NULL; child = child->next) { + if (!gotlabel && STREQ ((char *) child->name, "label")) { + /* Get the label content. */ + *label_rtn = (char *) xmlNodeGetContent (child); + gotlabel = 1; + } + if (!gotimagelabel && STREQ ((char *) child->name, "imagelabel")) { + /* Get the imagelabel content. */ + *imagelabel_rtn = (char *) xmlNodeGetContent (child); + gotimagelabel = 1; + } + }
Alternatively: labelObj = xmlXPathEvalExpression (BAD_CAST "/domain/seclabel[1]/label", xpathCtx); imageLabelObj = xmlXPathEvalExpression (BAD_CAST "/domain/seclabel[1]/imagelabel", xpathCtx); if (labelObj == NULL || imageLabelObj == NULL) { error (g, _("unable to evaluate XPath expression")); return -1; } labelNodes = labelObj->nodesetval; imageLabelNodes = imageLabelObj->nodesetval; if (labelNodes->nodeNr == 0 || imageLabelNodes->nodeNr == 0) return 0; *label_rtn = (char *) xmlNodeGetContent (labelNodes->nodeTab[0]); *imagelabel_rtn = (char *) xmlNodeGetContent (imageLabelNodes->nodeTab[0]);
+ + return 0; +} + /* The callback function 'f' is called once for each disk. * * Returns number of disks, or -1 if there was an error.
Matt
On Fri, Mar 01, 2013 at 12:33:49PM +0000, Matthew Booth wrote:
On Thu, 2013-02-28 at 16:02 +0000, Richard W.M. Jones wrote:
+ /* Find and pass the SELinux security label to the libvirt back end. */ + if (libvirt_selinux_label (g, doc, &label, &imagelabel) == -1) + return -1; + if (label && imagelabel) { + guestfs_internal_set_libvirt_selinux_label (g, label, imagelabel); + guestfs_internal_set_libvirt_selinux_norelabel_disks (g, 1);
Do we need both internal apis? 1 seems to imply the other.
They're congruent, but I think it makes the code clearer not to overload one API.
+ bool gotlabel = 0, gotimagelabel = 0;
gotlabel and gotimagelabel appear to be unused apart from assignment.
They are used later to ensure that we don't assign to the label_rtn / imagelabel_rtn parameters twice (which would be a memory leak). eg. If the XML had two <image/> elements.
+ if (nr_nodes == 0 || nr_nodes > 1) + return 0;
Not terribly important, but I wouldn't silently bomb out here if nr_nodes > 1. If you're going to catch it at all I'd add a warning and continue. [...]
It's possible to supply multiple <seclabel/> elements, ie. libvirt will parse that, but at the moment it wouldn't generate it. We should also probably check that any seclabel element we do use has the model=selinux property. I'll have a look at whether it's possible to use xpath exprs to do all of the above. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones 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/
From: "Richard W.M. Jones" <rjones@redhat.com> This is just code motion. --- src/libvirt-domain.c | 138 +++++++++++++++++++++++++-------------------------- 1 file changed, 69 insertions(+), 69 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 768ed78..e2068dd 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -318,75 +318,6 @@ add_disk (guestfs_h *g, return guestfs__add_drive_opts (g, filename, &optargs); } -static int -connect_live (guestfs_h *g, virDomainPtr dom) -{ - int i; - CLEANUP_XMLFREEDOC xmlDocPtr doc = NULL; - CLEANUP_XMLXPATHFREECONTEXT xmlXPathContextPtr xpathCtx = NULL; - CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xpathObj = NULL; - CLEANUP_FREE char *path = NULL, *attach_method = NULL; - xmlNodeSetPtr nodes; - - /* Domain XML. */ - if ((doc = get_domain_xml (g, dom)) == NULL) - return -1; - - xpathCtx = xmlXPathNewContext (doc); - if (xpathCtx == NULL) { - error (g, _("unable to create new XPath context")); - return -1; - } - - /* This gives us a set of all the <channel> nodes related to the - * guestfsd virtio-serial channel. - */ - xpathObj = xmlXPathEvalExpression (BAD_CAST - "//devices/channel[@type=\"unix\" and " - "./source/@mode=\"bind\" and " - "./source/@path and " - "./target/@type=\"virtio\" and " - "./target/@name=\"org.libguestfs.channel.0\"]", - xpathCtx); - if (xpathObj == NULL) { - error (g, _("unable to evaluate XPath expression")); - return -1; - } - - nodes = xpathObj->nodesetval; - for (i = 0; i < nodes->nodeNr; ++i) { - CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xppath = NULL; - xmlAttrPtr attr; - - /* See note in function above. */ - xpathCtx->node = nodes->nodeTab[i]; - - /* The path is in <source path=..> attribute. */ - xppath = xmlXPathEvalExpression (BAD_CAST "./source/@path", xpathCtx); - if (xppath == NULL || - xppath->nodesetval == NULL || - xppath->nodesetval->nodeNr == 0) { - xmlXPathFreeObject (xppath); - continue; /* no type attribute, skip it */ - } - assert (xppath->nodesetval->nodeTab[0]); - assert (xppath->nodesetval->nodeTab[0]->type == XML_ATTRIBUTE_NODE); - attr = (xmlAttrPtr) xppath->nodesetval->nodeTab[0]; - path = (char *) xmlNodeListGetString (doc, attr->children, 1); - break; - } - - if (path == NULL) { - error (g, _("this guest has no libvirt <channel> definition for guestfsd\n" - "See ATTACHING TO RUNNING DAEMONS in guestfs(3) for further information.")); - return -1; - } - - /* Got a path. */ - attach_method = safe_asprintf (g, "unix:%s", path); - return guestfs_set_attach_method (g, attach_method); -} - /* Find the <seclabel/> element in the libvirt XML, and if it exists * get the SELinux process label and image label from it. * @@ -580,6 +511,75 @@ for_each_disk (guestfs_h *g, return nr_added; } +static int +connect_live (guestfs_h *g, virDomainPtr dom) +{ + int i; + CLEANUP_XMLFREEDOC xmlDocPtr doc = NULL; + CLEANUP_XMLXPATHFREECONTEXT xmlXPathContextPtr xpathCtx = NULL; + CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xpathObj = NULL; + CLEANUP_FREE char *path = NULL, *attach_method = NULL; + xmlNodeSetPtr nodes; + + /* Domain XML. */ + if ((doc = get_domain_xml (g, dom)) == NULL) + return -1; + + xpathCtx = xmlXPathNewContext (doc); + if (xpathCtx == NULL) { + error (g, _("unable to create new XPath context")); + return -1; + } + + /* This gives us a set of all the <channel> nodes related to the + * guestfsd virtio-serial channel. + */ + xpathObj = xmlXPathEvalExpression (BAD_CAST + "//devices/channel[@type=\"unix\" and " + "./source/@mode=\"bind\" and " + "./source/@path and " + "./target/@type=\"virtio\" and " + "./target/@name=\"org.libguestfs.channel.0\"]", + xpathCtx); + if (xpathObj == NULL) { + error (g, _("unable to evaluate XPath expression")); + return -1; + } + + nodes = xpathObj->nodesetval; + for (i = 0; i < nodes->nodeNr; ++i) { + CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xppath = NULL; + xmlAttrPtr attr; + + /* See note in function above. */ + xpathCtx->node = nodes->nodeTab[i]; + + /* The path is in <source path=..> attribute. */ + xppath = xmlXPathEvalExpression (BAD_CAST "./source/@path", xpathCtx); + if (xppath == NULL || + xppath->nodesetval == NULL || + xppath->nodesetval->nodeNr == 0) { + xmlXPathFreeObject (xppath); + continue; /* no type attribute, skip it */ + } + assert (xppath->nodesetval->nodeTab[0]); + assert (xppath->nodesetval->nodeTab[0]->type == XML_ATTRIBUTE_NODE); + attr = (xmlAttrPtr) xppath->nodesetval->nodeTab[0]; + path = (char *) xmlNodeListGetString (doc, attr->children, 1); + break; + } + + if (path == NULL) { + error (g, _("this guest has no libvirt <channel> definition for guestfsd\n" + "See ATTACHING TO RUNNING DAEMONS in guestfs(3) for further information.")); + return -1; + } + + /* Got a path. */ + attach_method = safe_asprintf (g, "unix:%s", path); + return guestfs_set_attach_method (g, attach_method); +} + static xmlDocPtr get_domain_xml (guestfs_h *g, virDomainPtr dom) { -- 1.8.1.2
On Thu, 2013-02-28 at 16:02 +0000, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones@redhat.com>
This is just code motion. --- src/libvirt-domain.c | 138 +++++++++++++++++++++++++-------------------------- 1 file changed, 69 insertions(+), 69 deletions(-)
ACK. Matt
participants (2)
-
Matthew Booth -
Richard W.M. Jones