[libvirt] [PATCH 0/2] security: Save labels of resources before libvirt changing them for restoring.
Hi ALL: There is a confusing issue in svirt. If sec_type is dynamic or relabel is yes in VM, when VM stopped, the label of image will be restored to a default label on the path, but not my expected label what it was before VM is started. Example: #virsh dumpxml virt-tests-vm1 ... <disk type='file' device='disk'> <driver name='qemu' type='raw' cache='none'/> <source file='/libvirt_autotest_root/images/fedora17.img'/> <target dev='hda' bus='ide'/> <address type='drive' controller='0' bus='0' target='0' unit='0'/> </disk> ... <seclabel type='dynamic' model='selinux' relabel='yes'/> ... # ll /libvirt_autotest_root/images/fedora17.img -Z -rwxr-xr-x. root root *system_u:object_r:svirt_image_t:s0* /libvirt_autotest_root/images/fedora17.img # virsh start virt-tests-vm1 Domain virt-tests-vm1 started # virsh destroy virt-tests-vm1 Domain virt-tests-vm1 destroyed # ll /libvirt_autotest_root/images/fedora17.img -Z -rwxr-xr-x. root root *system_u:object_r:default_t:s0* /libvirt_autotest_root/images/fedora17.img Label is changed from svirt_image_t to default_t. And the svirt_image_t is accessable for svirt_t process but default_t is not. This patch instroduce a struct named _virSecuritySELinuxBackupContext to save the path and the label before libvirt changing them. And labels will be restored to path in VM being stopped. yangdongsheng (2): util: Introduce virStrcmp into virstring. security: Save contexts of resources for restoring it. src/security/security_selinux.c | 229 +++++++++++++++++++++++++++++++++++++-- src/util/virstring.c | 14 +++ src/util/virstring.h | 2 + 3 files changed, 238 insertions(+), 7 deletions(-) -- 1.7.10.1
Signed-off-by: yangdongsheng <yangds.fnst@cn.fujitsu.com> --- src/util/virstring.c | 14 ++++++++++++++ src/util/virstring.h | 2 ++ 2 files changed, 16 insertions(+) diff --git a/src/util/virstring.c b/src/util/virstring.c index 1937f82..9dbc1b0 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -382,6 +382,20 @@ virStrncpy(char *dest, const char *src, size_t n, size_t destbytes) } /** + * virStrcmp + * + * return 0 if what a point is equal to what b point. + * else return -1. + */ +int +virStrcmp(const char *a, const char *b) +{ + if (strcmp(a, b) != 0) + return -1; + return 0; +} + +/** * virStrcpy * * A safe version of strcpy. The last parameter is the number of bytes diff --git a/src/util/virstring.h b/src/util/virstring.h index 34ffae1..5448665 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -88,6 +88,8 @@ char *virStrcpy(char *dest, const char *src, size_t destbytes) ATTRIBUTE_RETURN_CHECK; # define virStrcpyStatic(dest, src) virStrcpy((dest), (src), sizeof(dest)) +int virStrcmp(const char *a, const char *b); + /* Don't call these directly - use the macros below */ int virStrdup(char **dest, const char *src, bool report, int domcode, const char *filename, const char *funcname, size_t linenr) -- 1.7.10.1
On 06/24/2013 08:42 AM, yangdongsheng wrote:
Signed-off-by: yangdongsheng <yangds.fnst@cn.fujitsu.com> --- src/util/virstring.c | 14 ++++++++++++++ src/util/virstring.h | 2 ++ 2 files changed, 16 insertions(+)
diff --git a/src/util/virstring.c b/src/util/virstring.c index 1937f82..9dbc1b0 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -382,6 +382,20 @@ virStrncpy(char *dest, const char *src, size_t n, size_t destbytes) }
/** + * virStrcmp + * + * return 0 if what a point is equal to what b point. + * else return -1. + */ +int +virStrcmp(const char *a, const char *b)
We already have a 'STREQ' macro defined for that in internal.h: http://libvirt.org/hacking.html#string_comparision Jan
On 06/24/2013 03:07 PM, Ján Tomko wrote:
On 06/24/2013 08:42 AM, yangdongsheng wrote:
Signed-off-by: yangdongsheng<yangds.fnst@cn.fujitsu.com> --- src/util/virstring.c | 14 ++++++++++++++ src/util/virstring.h | 2 ++ 2 files changed, 16 insertions(+)
diff --git a/src/util/virstring.c b/src/util/virstring.c index 1937f82..9dbc1b0 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -382,6 +382,20 @@ virStrncpy(char *dest, const char *src, size_t n, size_t destbytes) }
/** + * virStrcmp + * + * return 0 if what a point is equal to what b point. + * else return -1. + */ +int +virStrcmp(const char *a, const char *b) We already have a 'STREQ' macro defined for that in internal.h:
http://libvirt.org/hacking.html#string_comparision
Jan
Oh, yes, my fault. I will use STREQ and remove this patch in V2. Thanx.
Before this patch, if relabel is yes or sec_type is dynamic in VM, after VM stopped, resources this VM accessed will be restored to default label on their path, but not the label before VM started. This patch instroduce a struct named _virSecuritySELinuxBackupContext to save the path and the label before libvirt changing them. And labels will be restored to path in VM being stopped. TODO: There is only restoring for imageFile in this patch, files of other types will be implemented later. Signed-off-by: yangdongsheng <yangds.fnst@cn.fujitsu.com> --- src/security/security_selinux.c | 229 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 222 insertions(+), 7 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 6fe063e..a780569 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -57,12 +57,16 @@ typedef virSecuritySELinuxData *virSecuritySELinuxDataPtr; typedef struct _virSecuritySELinuxCallbackData virSecuritySELinuxCallbackData; typedef virSecuritySELinuxCallbackData *virSecuritySELinuxCallbackDataPtr; +typedef struct _virSecuritySELinuxBackupContext virSecuritySELinuxBackupContext; +typedef virSecuritySELinuxBackupContext *virSecuritySELinuxBackupContextPtr; + struct _virSecuritySELinuxData { char *domain_context; char *alt_domain_context; char *file_context; char *content_context; virHashTablePtr mcs; + virSecuritySELinuxBackupContextPtr backup_header; bool skipAllLabel; #if HAVE_SELINUX_LABEL_H struct selabel_handle *label_handle; @@ -74,6 +78,21 @@ struct _virSecuritySELinuxCallbackData { virSecurityLabelDefPtr secdef; }; +struct _virSecuritySELinuxBackupContext { + const char *path; + security_context_t *context; + virSecuritySELinuxBackupContextPtr next; +}; + +/* + *Operators on backup context for function virSecuritySELinuxBackupContextHelper. + */ +enum BCONOPERATOR { + ADD = 0, + GET = 1, + DEL = 2 +}; + #define SECURITY_SELINUX_VOID_DOI "0" #define SECURITY_SELINUX_NAME "selinux" @@ -992,8 +1011,8 @@ virSecuritySELinuxFSetFilecon(int fd, char *tcon) /* Set fcon to the appropriate label for path and mode, or return -1. */ static int -getContext(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - const char *newpath, mode_t mode, security_context_t *fcon) +getDefaultContext(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + const char *newpath, mode_t mode, security_context_t *fcon) { #if HAVE_SELINUX_LABEL_H virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); @@ -1004,6 +1023,146 @@ getContext(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, #endif } +/*Get the current label on path.*/ +static int +getContext(const char *path, security_context_t *fcon) +{ + return getfilecon(path, fcon); +} + +/* + *Helper for backup context. + *If operator is ADD, helper will get the current label on path + *to build a new _virSecuritySELinuxBackupContext and add it into mgr. + *If operator is GET, helper will search the all backups in mgr + *for the path, if path matches, set fcon_ptr to the related context + *and return 0, if no path matches, set fcon_ptr to NULL and return -1. + *If operator is DEL, helper will delete the backup in mgr whose + *path is equal to path. If no path matches, return 0. + */ +static int +virSecuritySELinuxBackupContextHelper(virSecurityManagerPtr mgr, + const char *path, + security_context_t **fcon_ptr ATTRIBUTE_UNUSED, + enum BCONOPERATOR operator) +{ + virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); + virSecuritySELinuxBackupContextPtr cur_backup = data->backup_header; + virSecuritySELinuxBackupContextPtr pre_backup = cur_backup; + virSecuritySELinuxBackupContextPtr new_backup_ptr = NULL; + security_context_t *new_backup_fcon_ptr = NULL; + int rc = -1; + + switch (operator) { + case ADD: + //Init a new virSecuritySELinuxBackupContext. + if ((VIR_ALLOC(new_backup_fcon_ptr) < 0)||(VIR_ALLOC(new_backup_ptr)<0)) { + virReportOOMError(); + goto cleanup; + } + new_backup_ptr->next = NULL; + new_backup_ptr->path = path; + + //Set the context of new backup to current label on path. + if (getContext(path, new_backup_fcon_ptr) < 0){ + goto cleanup; + } + else { + new_backup_ptr->context = new_backup_fcon_ptr; + } + //Append the new backup into mgr. + if (cur_backup) { + while(cur_backup->next) { + cur_backup = cur_backup->next; + } + cur_backup->next = new_backup_ptr; + } + else { + //Backup header does not exists. + data->backup_header = new_backup_ptr; + } + rc = 0; + break; + case GET: + case DEL: + //Search the backup whose path is equal to path. + if (cur_backup) { + do + { + if (virStrcmp(cur_backup->path, path) == 0) { + if (operator == GET) { + (*fcon_ptr) = cur_backup->context; + rc = 0; + goto cleanup; + }else { + //operator is DEL. + if (pre_backup == cur_backup) { + //There is only one backup in mgr. + data->backup_header = NULL; + } + else { + //Remove cur_backup from backup list. + pre_backup->next = cur_backup->next; + } + rc = 0; + goto cleanup; + } + } + //Step to next. + pre_backup = cur_backup; + cur_backup = cur_backup->next; + } + while(cur_backup); + } + VIR_INFO("No backup found for %s", path); + rc = -1; + break; + default: + VIR_DEBUG("Operator %d is not supported.", operator); + break; + } +cleanup: + if (rc == 0) { + if (operator == DEL) { + //Free the backup removed from list. + freecon(*cur_backup->context); + VIR_FREE(cur_backup); + } + } + else { + //Free the pointers if function failed when operator is ADD. + if (new_backup_ptr) + VIR_FREE(new_backup_ptr); + if (new_backup_fcon_ptr) + freecon(*new_backup_fcon_ptr); + } + return rc; +} + +static int +virSecuritySELinuxAddBackupContext(virSecurityManagerPtr mgr, + const char *path) +{ + enum BCONOPERATOR operator = ADD; + return virSecuritySELinuxBackupContextHelper(mgr, path, NULL, operator); +} + +static int +virSecuritySELinuxDelBackupContext(virSecurityManagerPtr mgr, + const char *path) +{ + enum BCONOPERATOR operator = DEL; + return virSecuritySELinuxBackupContextHelper(mgr, path, NULL, operator); +} + +static int +virSecuritySELinuxGetBackupContext(virSecurityManagerPtr mgr, + const char *path, + security_context_t **fcon) +{ + enum BCONOPERATOR operator = GET; + return virSecuritySELinuxBackupContextHelper(mgr, path, fcon, operator); +} /* This method shouldn't raise errors, since they'll overwrite * errors that the caller(s) are already dealing with */ @@ -1013,25 +1172,37 @@ virSecuritySELinuxRestoreSecurityFileLabel(virSecurityManagerPtr mgr, { struct stat buf; security_context_t fcon = NULL; + security_context_t *backup_con_ptr = NULL; int rc = -1; char *newpath = NULL; char ebuf[1024]; VIR_INFO("Restoring SELinux context on '%s'", path); + if ((virSecuritySELinuxGetBackupContext(mgr, path, &backup_con_ptr)) == 0) + { + VIR_INFO("Get a backup context. path: %s, context: %s.", + path, (char*)(*backup_con_ptr)); + rc = virSecuritySELinuxSetFilecon(path, (*backup_con_ptr)); + if (virSecuritySELinuxDelBackupContext(mgr, path) < 0) { + VIR_DEBUG("DEL backup of %s failed.", path); + } + goto cleanup; + } + if (virFileResolveLink(path, &newpath) < 0) { VIR_WARN("cannot resolve symlink %s: %s", path, virStrerror(errno, ebuf, sizeof(ebuf))); - goto err; + goto cleanup; } if (stat(newpath, &buf) != 0) { VIR_WARN("cannot stat %s: %s", newpath, virStrerror(errno, ebuf, sizeof(ebuf))); - goto err; + goto cleanup; } - if (getContext(mgr, newpath, buf.st_mode, &fcon) < 0) { + if (getDefaultContext(mgr, newpath, buf.st_mode, &fcon) < 0) { /* Any user created path likely does not have a default label, * which makes this an expected non error */ @@ -1041,7 +1212,7 @@ virSecuritySELinuxRestoreSecurityFileLabel(virSecurityManagerPtr mgr, rc = virSecuritySELinuxSetFilecon(newpath, fcon); } -err: +cleanup: freecon(fcon); VIR_FREE(newpath); return rc; @@ -1186,6 +1357,23 @@ virSecuritySELinuxRestoreSecurityImageLabel(virSecurityManagerPtr mgr, static int +virSecuritySELinuxSaveSecurityFileLabel(virDomainDiskDefPtr disk, + const char *path, + size_t depth ATTRIBUTE_UNUSED, + void *opaque) +{ + virSecurityDeviceLabelDefPtr disk_seclabel; + virSecuritySELinuxCallbackDataPtr cbdata = opaque; + + disk_seclabel = virDomainDiskDefGetSecurityLabelDef(disk, + SECURITY_SELINUX_NAME); + if (disk_seclabel && disk_seclabel->norelabel) + return 0; + + return virSecuritySELinuxAddBackupContext(cbdata->manager, path); +} + +static int virSecuritySELinuxSetSecurityFileLabel(virDomainDiskDefPtr disk, const char *path, size_t depth, @@ -1238,6 +1426,30 @@ virSecuritySELinuxSetSecurityFileLabel(virDomainDiskDefPtr disk, } static int +virSecuritySELinuxSaveSecurityImageLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainDiskDefPtr disk) +{ + virSecuritySELinuxCallbackData cbdata; + cbdata.manager = mgr; + cbdata.secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); + + if (cbdata.secdef == NULL) + return -1; + + if (cbdata.secdef->norelabel) + return 0; + + if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) + return 0; + + return virDomainDiskDefForeachPath(disk, + true, + virSecuritySELinuxSaveSecurityFileLabel, + &cbdata); +} + +static int virSecuritySELinuxSetSecurityImageLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainDiskDefPtr disk) @@ -2264,6 +2476,9 @@ virSecuritySELinuxSetSecurityAllLabel(virSecurityManagerPtr mgr, def->disks[i]->src, def->disks[i]->dst); continue; } + if (virSecuritySELinuxSaveSecurityImageLabel(mgr, + def, def->disks[i]) < 0) + return -1; if (virSecuritySELinuxSetSecurityImageLabel(mgr, def, def->disks[i]) < 0) return -1; @@ -2363,7 +2578,7 @@ virSecuritySELinuxSetTapFDLabel(virSecurityManagerPtr mgr, goto cleanup; } - if (getContext(mgr, "/dev/tap.*", buf.st_mode, &fcon) < 0) { + if (getDefaultContext(mgr, "/dev/tap.*", buf.st_mode, &fcon) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot lookup default selinux label for tap fd %d"), fd); goto cleanup; -- 1.7.10.1
On Mon, Jun 24, 2013 at 02:42:16PM +0800, yangdongsheng wrote:
Before this patch, if relabel is yes or sec_type is dynamic in VM, after VM stopped, resources this VM accessed will be restored to default label on their path, but not the label before VM started.
This patch instroduce a struct named _virSecuritySELinuxBackupContext to save the path and the label before libvirt changing them. And labels will be restored to path in VM being stopped.
TODO: There is only restoring for imageFile in this patch, files of other types will be implemented later.
Signed-off-by: yangdongsheng <yangds.fnst@cn.fujitsu.com> --- src/security/security_selinux.c | 229 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 222 insertions(+), 7 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 6fe063e..a780569 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -57,12 +57,16 @@ typedef virSecuritySELinuxData *virSecuritySELinuxDataPtr; typedef struct _virSecuritySELinuxCallbackData virSecuritySELinuxCallbackData; typedef virSecuritySELinuxCallbackData *virSecuritySELinuxCallbackDataPtr;
+typedef struct _virSecuritySELinuxBackupContext virSecuritySELinuxBackupContext; +typedef virSecuritySELinuxBackupContext *virSecuritySELinuxBackupContextPtr; + struct _virSecuritySELinuxData { char *domain_context; char *alt_domain_context; char *file_context; char *content_context; virHashTablePtr mcs; + virSecuritySELinuxBackupContextPtr backup_header;
NACK You cannot store this data inside libvirtd. We need to cope with libvirtd being restarted at any time for software upgrades. We also need to cope with migration where the libvirtd restoring labels is not the same as the libvirtd setting the original labels. We also need to cope with shared disks where multiple VMs use a label & have ref counting. There was an attempt to fix this problem for the DAC labels, which stalled. Whatever approach is used for the DAC labels, should also be used for the SELinux labels http://www.redhat.com/archives/libvir-list/2013-March/msg01289.html Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 06/24/2013 05:50 PM, Daniel P. Berrange wrote:
On Mon, Jun 24, 2013 at 02:42:16PM +0800, yangdongsheng wrote:
Before this patch, if relabel is yes or sec_type is dynamic in VM, after VM stopped, resources this VM accessed will be restored to default label on their path, but not the label before VM started.
This patch instroduce a struct named _virSecuritySELinuxBackupContext to save the path and the label before libvirt changing them. And labels will be restored to path in VM being stopped.
TODO: There is only restoring for imageFile in this patch, files of other types will be implemented later.
Signed-off-by: yangdongsheng<yangds.fnst@cn.fujitsu.com> --- src/security/security_selinux.c | 229 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 222 insertions(+), 7 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 6fe063e..a780569 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -57,12 +57,16 @@ typedef virSecuritySELinuxData *virSecuritySELinuxDataPtr; typedef struct _virSecuritySELinuxCallbackData virSecuritySELinuxCallbackData; typedef virSecuritySELinuxCallbackData *virSecuritySELinuxCallbackDataPtr;
+typedef struct _virSecuritySELinuxBackupContext virSecuritySELinuxBackupContext; +typedef virSecuritySELinuxBackupContext *virSecuritySELinuxBackupContextPtr; + struct _virSecuritySELinuxData { char *domain_context; char *alt_domain_context; char *file_context; char *content_context; virHashTablePtr mcs; + virSecuritySELinuxBackupContextPtr backup_header; NACK
You cannot store this data inside libvirtd. We need to cope with libvirtd being restarted at any time for software upgrades. We also need to cope with migration where the libvirtd restoring labels is not the same as the libvirtd setting the original labels. We also need to cope with shared disks where multiple VMs use a label& have ref counting.
There was an attempt to fix this problem for the DAC labels, which stalled. Whatever approach is used for the DAC labels, should also be used for the SELinux labels
http://www.redhat.com/archives/libvir-list/2013-March/msg01289.html
Daniel Got it. Thanx :)
participants (4)
-
Daniel P. Berrange -
Ján Tomko -
Yang Dongsheng -
yangdongsheng