7:47 a.m.
This is a reworked version of my previous patch:
https://www.redhat.com/archives/libvir-list/2015-July/msg00576.html
Michal Privoznik (4):
virSecuritySELinuxSetSecurityAllLabel: drop useless virFileIsSharedFSType
security_selinux: Replace SELinuxSCSICallbackData with proper struct
virSecurityManager: Track if running as privileged
security_selinux: Take @privileged into account
src/lxc/lxc_controller.c | 2 +-
src/lxc/lxc_driver.c | 3 +-
src/qemu/qemu_driver.c | 7 +-
src/security/security_manager.c | 29 ++++++--
src/security/security_manager.h | 5 +-
src/security/security_selinux.c | 147 ++++++++++++++++++++++++---------------
tests/qemuhotplugtest.c | 2 +-
tests/seclabeltest.c | 2 +-
tests/securityselinuxlabeltest.c | 2 +-
tests/securityselinuxtest.c | 2 +-
10 files changed, 127 insertions(+), 74 deletions(-)
--
2.4.6
7:47 a.m.
New subject: [libvirt] [PATCH v2 1/4] virSecuritySELinuxSetSecurityAllLabel: drop useless virFileIsSharedFSType
The check is done in virSecuritySELinuxSetFilecon itself. There's
no need to check it again.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_selinux.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index d2e5aa2..46080bf 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -2306,7 +2306,7 @@ virSecuritySELinuxSetSecurityAllLabel(virSecurityManagerPtr mgr,
continue;
}
if (virSecuritySELinuxSetSecurityDiskLabel(mgr,
- def, def->disks[i]) < 0)
+ def, def->disks[i]) < 0)
return -1;
}
/* XXX fixme process def->fss if relabel == true */
@@ -2355,11 +2355,9 @@ virSecuritySELinuxSetSecurityAllLabel(virSecurityManagerPtr mgr,
virSecuritySELinuxSetFilecon(def->os.dtb, data->content_context) < 0)
return -1;
- if (stdin_path) {
- if (virSecuritySELinuxSetFilecon(stdin_path, data->content_context) < 0
&&
- virFileIsSharedFSType(stdin_path, VIR_FILE_SHFS_NFS) != 1)
- return -1;
- }
+ if (stdin_path &&
+ virSecuritySELinuxSetFilecon(stdin_path, data->content_context) < 0)
+ return -1;
return 0;
}
--
2.4.6
7:47 a.m.
New subject: [libvirt] [PATCH v2 2/4] security_selinux: Replace SELinuxSCSICallbackData with proper struct
We have plenty of callbacks in the driver. Some of these
callbacks require more than one argument to be passed. For that
we currently have a data type (struct) per each callback. Well,
so far for only one - SELinuxSCSICallbackData. But lets turn it
into more general name so it can be reused in other callbacks too
instead of each one introducing a new, duplicate data type.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_selinux.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 46080bf..c6da6b0 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -68,15 +68,18 @@ struct _virSecuritySELinuxData {
#endif
};
-#define SECURITY_SELINUX_VOID_DOI "0"
-#define SECURITY_SELINUX_NAME "selinux"
+/* Data structure to pass to various callbacks so we have everything we need */
+typedef struct _virSecuritySELinuxCallbackData virSecuritySELinuxCallbackData;
+typedef virSecuritySELinuxCallbackData *virSecuritySELinuxCallbackDataPtr;
-/* Data structure to pass to *FileIterate so we have everything we need */
-struct SELinuxSCSICallbackData {
+struct _virSecuritySELinuxCallbackData {
virSecurityManagerPtr mgr;
virDomainDefPtr def;
};
+#define SECURITY_SELINUX_VOID_DOI "0"
+#define SECURITY_SELINUX_NAME "selinux"
+
static int
virSecuritySELinuxRestoreSecurityTPMFileLabelInt(virSecurityManagerPtr mgr,
virDomainDefPtr def,
@@ -1319,7 +1322,7 @@ virSecuritySELinuxSetSecuritySCSILabel(virSCSIDevicePtr dev,
const char *file, void *opaque)
{
virSecurityLabelDefPtr secdef;
- struct SELinuxSCSICallbackData *ptr = opaque;
+ virSecuritySELinuxCallbackDataPtr ptr = opaque;
virSecurityManagerPtr mgr = ptr->mgr;
virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr);
@@ -1400,7 +1403,7 @@
virSecuritySELinuxSetSecurityHostdevSubsysLabel(virSecurityManagerPtr mgr,
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: {
virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host;
- struct SELinuxSCSICallbackData data = {.mgr = mgr, .def = def};
+ virSecuritySELinuxCallbackData data = {.mgr = mgr, .def = def};
virSCSIDevicePtr scsi =
virSCSIDeviceNew(NULL,
--
2.4.6
7:47 a.m.
New subject: [libvirt] [PATCH v2 3/4] virSecurityManager: Track if running as privileged
We may want to do some decisions in drivers based on fact if we
are running as privileged user or not. Propagate this info there.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/lxc/lxc_controller.c | 2 +-
src/lxc/lxc_driver.c | 3 ++-
src/qemu/qemu_driver.c | 7 +++++--
src/security/security_manager.c | 29 ++++++++++++++++++++++-------
src/security/security_manager.h | 5 ++++-
tests/qemuhotplugtest.c | 2 +-
tests/seclabeltest.c | 2 +-
tests/securityselinuxlabeltest.c | 2 +-
tests/securityselinuxtest.c | 2 +-
9 files changed, 38 insertions(+), 16 deletions(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 0e6a3e1..b064919 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -2646,7 +2646,7 @@ int main(int argc, char *argv[])
if (!(ctrl->securityManager = virSecurityManagerNew(securityDriver,
LXC_DRIVER_NAME,
- false, false, false)))
+ false, false, false, false)))
goto cleanup;
if (ctrl->def->seclabels) {
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 35df18f..71be9c7 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -1558,7 +1558,8 @@ lxcSecurityInit(virLXCDriverConfigPtr cfg)
LXC_DRIVER_NAME,
false,
cfg->securityDefaultConfined,
- cfg->securityRequireConfined);
+ cfg->securityRequireConfined,
+ true);
if (!mgr)
goto error;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 91eb661..fcf86b6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -398,7 +398,8 @@ qemuSecurityInit(virQEMUDriverPtr driver)
QEMU_DRIVER_NAME,
cfg->allowDiskFormatProbing,
cfg->securityDefaultConfined,
- cfg->securityRequireConfined)))
+ cfg->securityRequireConfined,
+ virQEMUDriverIsPrivileged(driver))))
goto error;
if (!stack) {
if (!(stack = virSecurityManagerNewStack(mgr)))
@@ -415,7 +416,8 @@ qemuSecurityInit(virQEMUDriverPtr driver)
QEMU_DRIVER_NAME,
cfg->allowDiskFormatProbing,
cfg->securityDefaultConfined,
- cfg->securityRequireConfined)))
+ cfg->securityRequireConfined,
+ virQEMUDriverIsPrivileged(driver))))
goto error;
if (!(stack = virSecurityManagerNewStack(mgr)))
goto error;
@@ -429,6 +431,7 @@ qemuSecurityInit(virQEMUDriverPtr driver)
cfg->allowDiskFormatProbing,
cfg->securityDefaultConfined,
cfg->securityRequireConfined,
+ virQEMUDriverIsPrivileged(driver),
cfg->dynamicOwnership,
qemuSecurityChownCallback)))
goto error;
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 1098558..28d7dfd 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -40,6 +40,7 @@ struct _virSecurityManager {
bool allowDiskFormatProbing;
bool defaultConfined;
bool requireConfined;
+ bool privileged;
const char *virtDriver;
void *privateData;
};
@@ -78,7 +79,8 @@ virSecurityManagerNewDriver(virSecurityDriverPtr drv,
const char *virtDriver,
bool allowDiskFormatProbing,
bool defaultConfined,
- bool requireConfined)
+ bool requireConfined,
+ bool privileged)
{
virSecurityManagerPtr mgr;
char *privateData;
@@ -87,10 +89,10 @@ virSecurityManagerNewDriver(virSecurityDriverPtr drv,
return NULL;
VIR_DEBUG("drv=%p (%s) virtDriver=%s allowDiskFormatProbing=%d "
- "defaultConfined=%d requireConfined=%d",
+ "defaultConfined=%d requireConfined=%d privileged=%d",
drv, drv->name, virtDriver,
allowDiskFormatProbing, defaultConfined,
- requireConfined);
+ requireConfined, privileged);
if (VIR_ALLOC_N(privateData, drv->privateDataLen) < 0)
return NULL;
@@ -104,6 +106,7 @@ virSecurityManagerNewDriver(virSecurityDriverPtr drv,
mgr->allowDiskFormatProbing = allowDiskFormatProbing;
mgr->defaultConfined = defaultConfined;
mgr->requireConfined = requireConfined;
+ mgr->privileged = privileged;
mgr->virtDriver = virtDriver;
mgr->privateData = privateData;
@@ -124,7 +127,8 @@ virSecurityManagerNewStack(virSecurityManagerPtr primary)
virSecurityManagerGetDriver(primary),
virSecurityManagerGetAllowDiskFormatProbing(primary),
virSecurityManagerGetDefaultConfined(primary),
- virSecurityManagerGetRequireConfined(primary));
+ virSecurityManagerGetRequireConfined(primary),
+ virSecurityManagerGetPrivileged(primary));
if (!mgr)
return NULL;
@@ -153,6 +157,7 @@ virSecurityManagerNewDAC(const char *virtDriver,
bool defaultConfined,
bool requireConfined,
bool dynamicOwnership,
+ bool privileged,
virSecurityManagerDACChownCallback chownCallback)
{
virSecurityManagerPtr mgr =
@@ -160,7 +165,8 @@ virSecurityManagerNewDAC(const char *virtDriver,
virtDriver,
allowDiskFormatProbing,
defaultConfined,
- requireConfined);
+ requireConfined,
+ privileged);
if (!mgr)
return NULL;
@@ -182,7 +188,8 @@ virSecurityManagerNew(const char *name,
const char *virtDriver,
bool allowDiskFormatProbing,
bool defaultConfined,
- bool requireConfined)
+ bool requireConfined,
+ bool privileged)
{
virSecurityDriverPtr drv = virSecurityDriverLookup(name, virtDriver);
if (!drv)
@@ -212,7 +219,8 @@ virSecurityManagerNew(const char *name,
virtDriver,
allowDiskFormatProbing,
defaultConfined,
- requireConfined);
+ requireConfined,
+ privileged);
}
@@ -333,6 +341,13 @@ virSecurityManagerGetRequireConfined(virSecurityManagerPtr mgr)
}
+bool
+virSecurityManagerGetPrivileged(virSecurityManagerPtr mgr)
+{
+ return mgr->privileged;
+}
+
+
/**
* virSecurityManagerRestoreDiskLabel:
* @mgr: security manager object
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index 78f34a0..53e56f6 100644
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -34,7 +34,8 @@ virSecurityManagerPtr virSecurityManagerNew(const char *name,
const char *virtDriver,
bool allowDiskFormatProbing,
bool defaultConfined,
- bool requireConfined);
+ bool requireConfined,
+ bool privileged);
virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary);
int virSecurityManagerStackAddNested(virSecurityManagerPtr stack,
@@ -62,6 +63,7 @@ virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver,
bool defaultConfined,
bool requireConfined,
bool dynamicOwnership,
+ bool privileged,
virSecurityManagerDACChownCallback
chownCallback);
int virSecurityManagerPreFork(virSecurityManagerPtr mgr);
@@ -77,6 +79,7 @@ const char *virSecurityManagerGetBaseLabel(virSecurityManagerPtr mgr,
int virtTy
bool virSecurityManagerGetAllowDiskFormatProbing(virSecurityManagerPtr mgr);
bool virSecurityManagerGetDefaultConfined(virSecurityManagerPtr mgr);
bool virSecurityManagerGetRequireConfined(virSecurityManagerPtr mgr);
+bool virSecurityManagerGetPrivileged(virSecurityManagerPtr mgr);
int virSecurityManagerRestoreDiskLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index 368a5e7..48318bd 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -361,7 +361,7 @@ mymain(void)
if (!driver.lockManager)
return EXIT_FAILURE;
- if (!(mgr = virSecurityManagerNew("none", "qemu", false, false,
false)))
+ if (!(mgr = virSecurityManagerNew("none", "qemu", false, false,
false, true)))
return EXIT_FAILURE;
if (!(driver.securityManager = virSecurityManagerNewStack(mgr)))
return EXIT_FAILURE;
diff --git a/tests/seclabeltest.c b/tests/seclabeltest.c
index 51765c9..93ddcbb 100644
--- a/tests/seclabeltest.c
+++ b/tests/seclabeltest.c
@@ -17,7 +17,7 @@ main(int argc ATTRIBUTE_UNUSED, char **argv ATTRIBUTE_UNUSED)
if (virThreadInitialize() < 0)
return EXIT_FAILURE;
- mgr = virSecurityManagerNew(NULL, "QEMU", false, true, false);
+ mgr = virSecurityManagerNew(NULL, "QEMU", false, true, false, false);
if (mgr == NULL) {
fprintf(stderr, "Failed to start security driver");
return EXIT_FAILURE;
diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c
index 85fad37..4808eea 100644
--- a/tests/securityselinuxlabeltest.c
+++ b/tests/securityselinuxlabeltest.c
@@ -351,7 +351,7 @@ mymain(void)
if (!rc)
return EXIT_AM_SKIP;
- if (!(mgr = virSecurityManagerNew("selinux", "QEMU", false, true,
false))) {
+ if (!(mgr = virSecurityManagerNew("selinux", "QEMU", false, true,
false, true))) {
virErrorPtr err = virGetLastError();
VIR_TEST_VERBOSE("Unable to initialize security driver: %s\n",
err->message);
diff --git a/tests/securityselinuxtest.c b/tests/securityselinuxtest.c
index 38ab70e..3a7862f 100644
--- a/tests/securityselinuxtest.c
+++ b/tests/securityselinuxtest.c
@@ -272,7 +272,7 @@ mymain(void)
int ret = 0;
virSecurityManagerPtr mgr;
- if (!(mgr = virSecurityManagerNew("selinux", "QEMU", false, true,
false))) {
+ if (!(mgr = virSecurityManagerNew("selinux", "QEMU", false, true,
false, true))) {
virErrorPtr err = virGetLastError();
fprintf(stderr, "Unable to initialize security driver: %s\n",
err->message);
--
2.4.6
7:47 a.m.
New subject: [libvirt] [PATCH v2 4/4] security_selinux: Take @privileged into account
https://bugzilla.redhat.com/show_bug.cgi?id=1124841
If running in session mode it may happen that we fail to set
correct SELinux label, but the image may still be readable to
the qemu process. Take this into account.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_selinux.c | 126 +++++++++++++++++++++++++---------------
1 file changed, 78 insertions(+), 48 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index c6da6b0..c2464c2 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -886,7 +886,8 @@ virSecuritySELinuxGetSecurityProcessLabel(virSecurityManagerPtr mgr
ATTRIBUTE_UN
* return 1 if labelling was not possible. Otherwise, require a label
* change, and return 0 for success, -1 for failure. */
static int
-virSecuritySELinuxSetFileconHelper(const char *path, char *tcon, bool optional)
+virSecuritySELinuxSetFileconHelper(const char *path, char *tcon,
+ bool optional, bool privileged)
{
security_context_t econ;
@@ -915,7 +916,10 @@ virSecuritySELinuxSetFileconHelper(const char *path, char *tcon, bool
optional)
virReportSystemError(setfilecon_errno,
_("unable to set security context '%s' on
'%s'"),
tcon, path);
- if (security_getenforce() == 1)
+ /* However, don't claim error if SELinux is in Enforcing mode and
+ * we are running as unprivileged user and we really did see EPERM.
+ * Otherwise we want to return error if SELinux is Enforcing. */
+ if (security_getenforce() == 1 && (setfilecon_errno != EPERM ||
privileged))
return -1;
} else {
const char *msg;
@@ -939,15 +943,19 @@ virSecuritySELinuxSetFileconHelper(const char *path, char *tcon,
bool optional)
}
static int
-virSecuritySELinuxSetFileconOptional(const char *path, char *tcon)
+virSecuritySELinuxSetFileconOptional(virSecurityManagerPtr mgr,
+ const char *path, char *tcon)
{
- return virSecuritySELinuxSetFileconHelper(path, tcon, true);
+ bool privileged = virSecurityManagerGetPrivileged(mgr);
+ return virSecuritySELinuxSetFileconHelper(path, tcon, true, privileged);
}
static int
-virSecuritySELinuxSetFilecon(const char *path, char *tcon)
+virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr,
+ const char *path, char *tcon)
{
- return virSecuritySELinuxSetFileconHelper(path, tcon, false);
+ bool privileged = virSecurityManagerGetPrivileged(mgr);
+ return virSecuritySELinuxSetFileconHelper(path, tcon, false, privileged);
}
static int
@@ -1037,7 +1045,7 @@ virSecuritySELinuxRestoreSecurityFileLabel(virSecurityManagerPtr
mgr,
VIR_WARN("cannot lookup default selinux label for %s", newpath);
rc = 0;
} else {
- rc = virSecuritySELinuxSetFilecon(newpath, fcon);
+ rc = virSecuritySELinuxSetFilecon(mgr, newpath, fcon);
}
err:
@@ -1064,12 +1072,13 @@ virSecuritySELinuxSetSecurityTPMFileLabel(virSecurityManagerPtr
mgr,
switch (tpm->type) {
case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
tpmdev = tpm->data.passthrough.source.data.file.path;
- rc = virSecuritySELinuxSetFilecon(tpmdev, seclabel->imagelabel);
+ rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel);
if (rc < 0)
return -1;
if ((cancel_path = virTPMCreateCancelPath(tpmdev)) != NULL) {
- rc = virSecuritySELinuxSetFilecon(cancel_path,
+ rc = virSecuritySELinuxSetFilecon(mgr,
+ cancel_path,
seclabel->imagelabel);
VIR_FREE(cancel_path);
if (rc < 0) {
@@ -1223,22 +1232,26 @@
virSecuritySELinuxSetSecurityImageLabelInternal(virSecurityManagerPtr mgr,
return 0;
if (disk_seclabel && disk_seclabel->relabel &&
disk_seclabel->label) {
- ret = virSecuritySELinuxSetFilecon(src->path, disk_seclabel->label);
+ ret = virSecuritySELinuxSetFilecon(mgr, src->path, disk_seclabel->label);
} else if (first) {
if (src->shared) {
- ret = virSecuritySELinuxSetFileconOptional(src->path,
+ ret = virSecuritySELinuxSetFileconOptional(mgr,
+ src->path,
data->file_context);
} else if (src->readonly) {
- ret = virSecuritySELinuxSetFileconOptional(src->path,
+ ret = virSecuritySELinuxSetFileconOptional(mgr,
+ src->path,
data->content_context);
} else if (secdef->imagelabel) {
- ret = virSecuritySELinuxSetFileconOptional(src->path,
+ ret = virSecuritySELinuxSetFileconOptional(mgr,
+ src->path,
secdef->imagelabel);
} else {
ret = 0;
}
} else {
- ret = virSecuritySELinuxSetFileconOptional(src->path,
+ ret = virSecuritySELinuxSetFileconOptional(mgr,
+ src->path,
data->content_context);
}
@@ -1290,17 +1303,18 @@ virSecuritySELinuxSetSecurityDiskLabel(virSecurityManagerPtr mgr,
return 0;
}
-
static int
virSecuritySELinuxSetSecurityHostdevLabelHelper(const char *file, void *opaque)
{
virSecurityLabelDefPtr secdef;
- virDomainDefPtr def = opaque;
+ virSecuritySELinuxCallbackDataPtr data = opaque;
+ virSecurityManagerPtr mgr = data->mgr;
+ virDomainDefPtr def = data->def;
secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME);
if (secdef == NULL)
return 0;
- return virSecuritySELinuxSetFilecon(file, secdef->imagelabel);
+ return virSecuritySELinuxSetFilecon(mgr, file, secdef->imagelabel);
}
static int
@@ -1331,13 +1345,14 @@ virSecuritySELinuxSetSecuritySCSILabel(virSCSIDevicePtr dev,
return 0;
if (virSCSIDeviceGetShareable(dev))
- return virSecuritySELinuxSetFileconOptional(file,
+ return virSecuritySELinuxSetFileconOptional(mgr, file,
data->file_context);
else if (virSCSIDeviceGetReadonly(dev))
- return virSecuritySELinuxSetFileconOptional(file,
+ return virSecuritySELinuxSetFileconOptional(mgr, file,
data->content_context);
else
- return virSecuritySELinuxSetFileconOptional(file, secdef->imagelabel);
+ return virSecuritySELinuxSetFileconOptional(mgr, file,
+ secdef->imagelabel);
}
static int
@@ -1350,6 +1365,8 @@
virSecuritySELinuxSetSecurityHostdevSubsysLabel(virSecurityManagerPtr mgr,
virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb;
virDomainHostdevSubsysPCIPtr pcisrc = &dev->source.subsys.u.pci;
virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi;
+ virSecuritySELinuxCallbackData data = {.mgr = mgr, .def = def};
+
int ret = -1;
/* Like virSecuritySELinuxSetSecurityImageLabelInternal() for a networked
@@ -1372,7 +1389,7 @@
virSecuritySELinuxSetSecurityHostdevSubsysLabel(virSecurityManagerPtr mgr,
if (!usb)
goto done;
- ret = virUSBDeviceFileIterate(usb, virSecuritySELinuxSetSecurityUSBLabel, def);
+ ret = virUSBDeviceFileIterate(usb, virSecuritySELinuxSetSecurityUSBLabel,
&data);
virUSBDeviceFree(usb);
break;
}
@@ -1392,10 +1409,10 @@
virSecuritySELinuxSetSecurityHostdevSubsysLabel(virSecurityManagerPtr mgr,
virPCIDeviceFree(pci);
goto done;
}
- ret = virSecuritySELinuxSetSecurityPCILabel(pci, vfioGroupDev, def);
+ ret = virSecuritySELinuxSetSecurityPCILabel(pci, vfioGroupDev, &data);
VIR_FREE(vfioGroupDev);
} else {
- ret = virPCIDeviceFileIterate(pci, virSecuritySELinuxSetSecurityPCILabel,
def);
+ ret = virPCIDeviceFileIterate(pci, virSecuritySELinuxSetSecurityPCILabel,
&data);
}
virPCIDeviceFree(pci);
break;
@@ -1403,7 +1420,6 @@
virSecuritySELinuxSetSecurityHostdevSubsysLabel(virSecurityManagerPtr mgr,
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: {
virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host;
- virSecuritySELinuxCallbackData data = {.mgr = mgr, .def = def};
virSCSIDevicePtr scsi =
virSCSIDeviceNew(NULL,
@@ -1433,7 +1449,8 @@
virSecuritySELinuxSetSecurityHostdevSubsysLabel(virSecurityManagerPtr mgr,
static int
-virSecuritySELinuxSetSecurityHostdevCapsLabel(virDomainDefPtr def,
+virSecuritySELinuxSetSecurityHostdevCapsLabel(virSecurityManagerPtr mgr,
+ virDomainDefPtr def,
virDomainHostdevDefPtr dev,
const char *vroot)
{
@@ -1455,7 +1472,7 @@ virSecuritySELinuxSetSecurityHostdevCapsLabel(virDomainDefPtr def,
if (VIR_STRDUP(path, dev->source.caps.u.storage.block) < 0)
return -1;
}
- ret = virSecuritySELinuxSetFilecon(path, secdef->imagelabel);
+ ret = virSecuritySELinuxSetFilecon(mgr, path, secdef->imagelabel);
VIR_FREE(path);
break;
}
@@ -1469,7 +1486,7 @@ virSecuritySELinuxSetSecurityHostdevCapsLabel(virDomainDefPtr def,
if (VIR_STRDUP(path, dev->source.caps.u.misc.chardev) < 0)
return -1;
}
- ret = virSecuritySELinuxSetFilecon(path, secdef->imagelabel);
+ ret = virSecuritySELinuxSetFilecon(mgr, path, secdef->imagelabel);
VIR_FREE(path);
break;
}
@@ -1502,7 +1519,8 @@ virSecuritySELinuxSetSecurityHostdevLabel(virSecurityManagerPtr
mgr,
dev, vroot);
case VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES:
- return virSecuritySELinuxSetSecurityHostdevCapsLabel(def, dev, vroot);
+ return virSecuritySELinuxSetSecurityHostdevCapsLabel(mgr, def,
+ dev, vroot);
default:
return 0;
@@ -1707,7 +1725,8 @@ virSecuritySELinuxRestoreSecurityHostdevLabel(virSecurityManagerPtr
mgr,
static int
-virSecuritySELinuxSetSecurityChardevLabel(virDomainDefPtr def,
+virSecuritySELinuxSetSecurityChardevLabel(virSecurityManagerPtr mgr,
+ virDomainDefPtr def,
virDomainChrDefPtr dev,
virDomainChrSourceDefPtr dev_source)
@@ -1737,13 +1756,15 @@ virSecuritySELinuxSetSecurityChardevLabel(virDomainDefPtr def,
switch (dev_source->type) {
case VIR_DOMAIN_CHR_TYPE_DEV:
case VIR_DOMAIN_CHR_TYPE_FILE:
- ret = virSecuritySELinuxSetFilecon(dev_source->data.file.path,
+ ret = virSecuritySELinuxSetFilecon(mgr,
+ dev_source->data.file.path,
imagelabel);
break;
case VIR_DOMAIN_CHR_TYPE_UNIX:
if (!dev_source->data.nix.listen) {
- if (virSecuritySELinuxSetFilecon(dev_source->data.nix.path,
+ if (virSecuritySELinuxSetFilecon(mgr,
+ dev_source->data.nix.path,
imagelabel) < 0)
goto done;
}
@@ -1755,11 +1776,12 @@ virSecuritySELinuxSetSecurityChardevLabel(virDomainDefPtr def,
(virAsprintf(&out, "%s.out", dev_source->data.file.path)
< 0))
goto done;
if (virFileExists(in) && virFileExists(out)) {
- if ((virSecuritySELinuxSetFilecon(in, imagelabel) < 0) ||
- (virSecuritySELinuxSetFilecon(out, imagelabel) < 0)) {
+ if ((virSecuritySELinuxSetFilecon(mgr, in, imagelabel) < 0) ||
+ (virSecuritySELinuxSetFilecon(mgr, out, imagelabel) < 0)) {
goto done;
}
- } else if (virSecuritySELinuxSetFilecon(dev_source->data.file.path,
+ } else if (virSecuritySELinuxSetFilecon(mgr,
+ dev_source->data.file.path,
imagelabel) < 0) {
goto done;
}
@@ -2000,7 +2022,7 @@ virSecuritySELinuxReleaseSecurityLabel(virSecurityManagerPtr mgr,
static int
-virSecuritySELinuxSetSavedStateLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
+virSecuritySELinuxSetSavedStateLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
const char *savefile)
{
@@ -2010,7 +2032,7 @@ virSecuritySELinuxSetSavedStateLabel(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
if (!secdef || !secdef->relabel)
return 0;
- return virSecuritySELinuxSetFilecon(savefile, secdef->imagelabel);
+ return virSecuritySELinuxSetFilecon(mgr, savefile, secdef->imagelabel);
}
@@ -2242,14 +2264,16 @@ virSecuritySELinuxClearSecuritySocketLabel(virSecurityManagerPtr
mgr ATTRIBUTE_U
static int
virSecuritySELinuxSetSecurityChardevCallback(virDomainDefPtr def,
virDomainChrDefPtr dev,
- void *opaque ATTRIBUTE_UNUSED)
+ void *opaque)
{
+ virSecurityManagerPtr mgr = opaque;
+
/* This is taken care of by processing of def->serials */
if (dev->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE &&
dev->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL)
return 0;
- return virSecuritySELinuxSetSecurityChardevLabel(def, dev, &dev->source);
+ return virSecuritySELinuxSetSecurityChardevLabel(mgr, def, dev,
&dev->source);
}
@@ -2270,10 +2294,11 @@ virSecuritySELinuxSetSecuritySmartcardCallback(virDomainDefPtr
def,
database = dev->data.cert.database;
if (!database)
database = VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE;
- return virSecuritySELinuxSetFilecon(database, data->content_context);
+ return virSecuritySELinuxSetFilecon(mgr, database, data->content_context);
case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
- return virSecuritySELinuxSetSecurityChardevLabel(def, NULL,
&dev->data.passthru);
+ return virSecuritySELinuxSetSecurityChardevLabel(mgr, def, NULL,
+ &dev->data.passthru);
default:
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -2330,7 +2355,7 @@ virSecuritySELinuxSetSecurityAllLabel(virSecurityManagerPtr mgr,
if (virDomainChrDefForeach(def,
true,
virSecuritySELinuxSetSecurityChardevCallback,
- NULL) < 0)
+ mgr) < 0)
return -1;
if (virDomainSmartcardDefForeach(def,
@@ -2343,23 +2368,28 @@ virSecuritySELinuxSetSecurityAllLabel(virSecurityManagerPtr mgr,
* is really a disk, qemu can read and write to it. */
if (def->os.loader && def->os.loader->nvram &&
secdef && secdef->imagelabel &&
- virSecuritySELinuxSetFilecon(def->os.loader->nvram, secdef->imagelabel)
< 0)
+ virSecuritySELinuxSetFilecon(mgr, def->os.loader->nvram,
+ secdef->imagelabel) < 0)
return -1;
if (def->os.kernel &&
- virSecuritySELinuxSetFilecon(def->os.kernel, data->content_context) <
0)
+ virSecuritySELinuxSetFilecon(mgr, def->os.kernel,
+ data->content_context) < 0)
return -1;
if (def->os.initrd &&
- virSecuritySELinuxSetFilecon(def->os.initrd, data->content_context) <
0)
+ virSecuritySELinuxSetFilecon(mgr, def->os.initrd,
+ data->content_context) < 0)
return -1;
if (def->os.dtb &&
- virSecuritySELinuxSetFilecon(def->os.dtb, data->content_context) < 0)
+ virSecuritySELinuxSetFilecon(mgr, def->os.dtb,
+ data->content_context) < 0)
return -1;
if (stdin_path &&
- virSecuritySELinuxSetFilecon(stdin_path, data->content_context) < 0)
+ virSecuritySELinuxSetFilecon(mgr, stdin_path,
+ data->content_context) < 0)
return -1;
return 0;
@@ -2507,7 +2537,7 @@ virSecuritySELinuxGetSecurityMountOptions(virSecurityManagerPtr
mgr,
}
static int
-virSecuritySELinuxDomainSetDirLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
+virSecuritySELinuxDomainSetDirLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
const char *path)
{
@@ -2517,7 +2547,7 @@ virSecuritySELinuxDomainSetDirLabel(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
if (!seclabel || !seclabel->relabel)
return 0;
- return virSecuritySELinuxSetFilecon(path, seclabel->imagelabel);
+ return virSecuritySELinuxSetFilecon(mgr, path, seclabel->imagelabel);
}
virSecurityDriver virSecurityDriverSELinux = {
--
2.4.6
10:23 a.m.
New subject: [libvirt] [PATCH v2 4/4] security_selinux: Take @privileged into account
On 09/10/2015 08:47 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1124841
If running in session mode it may happen that we fail to set
correct SELinux label, but the image may still be readable to
the qemu process. Take this into account.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_selinux.c | 126 +++++++++++++++++++++++++---------------
1 file changed, 78 insertions(+), 48 deletions(-)
I was just poking around in this code trying to get back up to speed so I can
post patches for the ideas I proposed here:
https://www.redhat.com/archives/libvir-list/2015-April/msg01400.html
diff --git a/src/security/security_selinux.c
b/src/security/security_selinux.c
index c6da6b0..c2464c2 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -886,7 +886,8 @@ virSecuritySELinuxGetSecurityProcessLabel(virSecurityManagerPtr mgr
ATTRIBUTE_UN
* return 1 if labelling was not possible. Otherwise, require a label
* change, and return 0 for success, -1 for failure. */
static int
-virSecuritySELinuxSetFileconHelper(const char *path, char *tcon, bool optional)
+virSecuritySELinuxSetFileconHelper(const char *path, char *tcon,
+ bool optional, bool privileged)
{
security_context_t econ;
@@ -915,7 +916,10 @@ virSecuritySELinuxSetFileconHelper(const char *path, char *tcon,
bool optional)
virReportSystemError(setfilecon_errno,
_("unable to set security context '%s' on
'%s'"),
tcon, path);
- if (security_getenforce() == 1)
+ /* However, don't claim error if SELinux is in Enforcing mode and
+ * we are running as unprivileged user and we really did see EPERM.
+ * Otherwise we want to return error if SELinux is Enforcing. */
+ if (security_getenforce() == 1 && (setfilecon_errno != EPERM ||
privileged))
return -1;
Ignoring a labelling EPERM failure for readonly media for qemu:///session
seems fine to me (like the initial bug report), but this also ignores failure
for RW disk images, so it throws out main security benefit of svirt if the
source permissions are in the wrong state, which is worrying. I suggest
amending this to only skip the !privileged error if disk->readonly. Thoughts?
- Cole
8:18 a.m.
On Thu, Sep 10, 2015 at 02:47:14PM +0200, Michal Privoznik wrote:
This is a reworked version of my previous patch:
https://www.redhat.com/archives/libvir-list/2015-July/msg00576.html
Michal Privoznik (4):
virSecuritySELinuxSetSecurityAllLabel: drop useless virFileIsSharedFSType
security_selinux: Replace SELinuxSCSICallbackData with proper struct
virSecurityManager: Track if running as privileged
security_selinux: Take @privileged into account
ACK series, although next time I'd prefer the movement/whitespaces in
[12]/4 to be split into different patches, it's a bit confusing
sometimes.
src/lxc/lxc_controller.c | 2 +-
src/lxc/lxc_driver.c | 3 +-
src/qemu/qemu_driver.c | 7 +-
src/security/security_manager.c | 29 ++++++--
src/security/security_manager.h | 5 +-
src/security/security_selinux.c | 147 ++++++++++++++++++++++++---------------
tests/qemuhotplugtest.c | 2 +-
tests/seclabeltest.c | 2 +-
tests/securityselinuxlabeltest.c | 2 +-
tests/securityselinuxtest.c | 2 +-
10 files changed, 127 insertions(+), 74 deletions(-)
--
2.4.6
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
3315
days inactive
3343
days old
6 comments
3 participants
participants (3)
-
Cole Robinson -
Martin Kletzander -
Michal Privoznik