From 04b7241a14015f67b68d9779be71f9e8f91791c3 Mon Sep 17 00:00:00 2001

Message-Id: <04b7241a14015f67b68d9779be71f9e8f91791c3@dist-git>

From: Michal Privoznik <mprivozn@redhat.com>

Date: Mon, 9 Mar 2020 14:58:56 +0100

Subject: [PATCH] security: Introduce

 VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP flag

Our decision whether to remember seclabel for a disk image

depends on a few factors. If the image is readonly or shared or

not the chain top the remembering is suppressed for the image.

However, the virSecurityManagerSetImageLabel() is too low level

to determine whether passed @src is chain top or not. Even though

the function has the @parent argument it does not necessarily

reflect the chain top - it only points to the top level image in

the chain we want to relabel and not to the topmost image of the

whole chain. And this can't be derived from the passed domain

definition reliably neither - in some cases (like snapshots or

block copy) the @src is added to the definition only after the

operation succeeded. Therefore, introduce a flag which callers

can use to help us with the decision.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

(cherry picked from commit 62f3d8adbc0381223499ff2bef45b23e7dca401d)

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1803551

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Message-Id: <bd4b1cca6feabd0c6d421603abe38397090c5394.1583760062.git.mprivozn@redhat.com>

Reviewed-by: Jiri Denemark <jdenemar@redhat.com>

---

 src/security/security_dac.c     | 15 ++++++++++-----

 src/security/security_manager.h |  4 ++++

 src/security/security_selinux.c | 17 +++++++++++------

 3 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c

index 0cfe3626d4..66906245eb 100644

--- a/src/security/security_dac.c

+++ b/src/security/security_dac.c

@@ -885,14 +885,14 @@ static int

 virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr,

                                     virDomainDefPtr def,

                                     virStorageSourcePtr src,

-                                    virStorageSourcePtr parent)

+                                    virStorageSourcePtr parent,

+                                    bool isChainTop)

 {

     virSecurityLabelDefPtr secdef;

     virSecurityDeviceLabelDefPtr disk_seclabel;

     virSecurityDeviceLabelDefPtr parent_seclabel = NULL;

     virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);

     bool remember;

-    bool is_toplevel = parent == src || parent->externalDataStore == src;

     uid_t user;

     gid_t group;

@@ -950,7 +950,7 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr,

      * but the top layer, or read only image, or disk explicitly

      * marked as shared.

      */

-    remember = is_toplevel && !src->readonly && !src->shared;

+    remember = isChainTop && !src->readonly && !src->shared;

     return virSecurityDACSetOwnership(mgr, src, NULL, user, group, remember);

 }

@@ -966,7 +966,9 @@ virSecurityDACSetImageLabelRelative(virSecurityManagerPtr mgr,

     virStorageSourcePtr n;

     for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) {

-        if (virSecurityDACSetImageLabelInternal(mgr, def, n, parent) < 0)

+        const bool isChainTop = flags & VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP;

+

+        if (virSecurityDACSetImageLabelInternal(mgr, def, n, parent, isChainTop) < 0)

             return -1;

         if (n->externalDataStore &&

@@ -979,6 +981,8 @@ virSecurityDACSetImageLabelRelative(virSecurityManagerPtr mgr,

         if (!(flags & VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN))

             break;

+

+        flags &= ~VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP;

     }

     return 0;

@@ -2106,7 +2110,8 @@ virSecurityDACSetAllLabel(virSecurityManagerPtr mgr,

         if (virDomainDiskGetType(def->disks[i]) == VIR_STORAGE_TYPE_DIR)

             continue;

         if (virSecurityDACSetImageLabel(mgr, def, def->disks[i]->src,

-                                        VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN) < 0)

+                                        VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN |

+                                        VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP) < 0)

             return -1;

     }

diff --git a/src/security/security_manager.h b/src/security/security_manager.h

index b92ea5dc87..7699bcbc6f 100644

--- a/src/security/security_manager.h

+++ b/src/security/security_manager.h

@@ -151,6 +151,10 @@ virSecurityManagerPtr* virSecurityManagerGetNested(virSecurityManagerPtr mgr);

 typedef enum {

     VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN = 1 << 0,

+    /* The VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP should be set if the

+     * image passed to virSecurityManagerSetImageLabel() is the top parent of

+     * the whole backing chain. */

+    VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP = 1 << 1,

 } virSecurityDomainImageLabelFlags;

 int virSecurityManagerSetImageLabel(virSecurityManagerPtr mgr,

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c

index d7362327e6..985c7eda1a 100644

--- a/src/security/security_selinux.c

+++ b/src/security/security_selinux.c

@@ -1824,7 +1824,8 @@ static int

 virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr,

                                         virDomainDefPtr def,

                                         virStorageSourcePtr src,

-                                        virStorageSourcePtr parent)

+                                        virStorageSourcePtr parent,

+                                        bool isChainTop)

 {

     virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr);

     virSecurityLabelDefPtr secdef;

@@ -1832,7 +1833,6 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr,

     virSecurityDeviceLabelDefPtr parent_seclabel = NULL;

     char *use_label = NULL;

     bool remember;

-    bool is_toplevel = parent == src || parent->externalDataStore == src;

     g_autofree char *vfioGroupDev = NULL;

     const char *path = src->path;

     int ret;

@@ -1856,7 +1856,7 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr,

      * but the top layer, or read only image, or disk explicitly

      * marked as shared.

      */

-    remember = is_toplevel && !src->readonly && !src->shared;

+    remember = isChainTop && !src->readonly && !src->shared;

     disk_seclabel = virStorageSourceGetSecurityLabelDef(src,

                                                         SECURITY_SELINUX_NAME);

@@ -1873,7 +1873,7 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr,

             return 0;

         use_label = parent_seclabel->label;

-    } else if (is_toplevel) {

+    } else if (parent == src || parent->externalDataStore == src) {

         if (src->shared) {

             use_label = data->file_context;

         } else if (src->readonly) {

@@ -1930,7 +1930,9 @@ virSecuritySELinuxSetImageLabelRelative(virSecurityManagerPtr mgr,

     virStorageSourcePtr n;

     for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) {

-        if (virSecuritySELinuxSetImageLabelInternal(mgr, def, n, parent) < 0)

+        const bool isChainTop = flags & VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP;

+

+        if (virSecuritySELinuxSetImageLabelInternal(mgr, def, n, parent, isChainTop) < 0)

             return -1;

         if (n->externalDataStore &&

@@ -1943,6 +1945,8 @@ virSecuritySELinuxSetImageLabelRelative(virSecurityManagerPtr mgr,

         if (!(flags & VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN))

             break;

+

+        flags &= ~VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP;

     }

     return 0;

@@ -3142,7 +3146,8 @@ virSecuritySELinuxSetAllLabel(virSecurityManagerPtr mgr,

             continue;

         }

         if (virSecuritySELinuxSetImageLabel(mgr, def, def->disks[i]->src,

-                                            VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN) < 0)

+                                            VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN |

+                                            VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP) < 0)

             return -1;

     }

     /* XXX fixme process  def->fss if relabel == true */

-- 

2.25.1