Currently, on domain startup a labeling of domain resources (e.g.
disk images, sockets, ...) is done. However, we don't store the
original owner anywhere. So when domain is shutting down, we
cannot restore the original owner of files we have chown()-ed.
This patch resolves this issue for DAC driver.
---
src/security/security_dac.c | 340 +++++++++++++++++++++++++++++++-------------
1 file changed, 244 insertions(+), 96 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index b115bb0..c0ec707 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -265,50 +265,185 @@ static const char * virSecurityDACGetDOI(virSecurityManagerPtr mgr ATTRIBUTE_UNU
}
static int
-virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
+virSecurityDACAddOldlabel(virSecurityDeviceLabelDefPtr label,
+ uid_t user, gid_t group)
{
- VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'",
- path, (long) uid, (long) gid);
+ int ret = -1;
+ char *userStr = NULL, *groupStr = NULL;
- if (chown(path, uid, gid) < 0) {
- struct stat sb;
- int chown_errno = errno;
-
- if (stat(path, &sb) >= 0) {
- if (sb.st_uid == uid &&
- sb.st_gid == gid) {
- /* It's alright, there's nothing to change anyway. */
- return 0;
+ userStr = virGetUserName(user);
+ groupStr = virGetGroupName(group);
+
+ if ((!userStr && (virAsprintf(&userStr, "+%ld", (long) user) < 0)) ||
+ (!groupStr && (virAsprintf(&groupStr, "+%ld", (long) group) < 0)) ||
+ (virAsprintf(&label->oldlabel, "%s:%s", userStr, groupStr) < 0)) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ ret = 0;
+
+cleanup:
+ VIR_FREE(groupStr);
+ VIR_FREE(userStr);
+ return ret;
+}
+
+static int
+virSecurityDACSaveOwnership(virSecurityDeviceLabelDefPtr **labels,
+ size_t *nseclabels,
+ uid_t user, gid_t group)
+{
+ int ret = -1;
+ size_t i;
+
+ if ((user == (uid_t) -1) &&
+ (group == (gid_t) -1)) {
+ /* nothing to save */
+ return 0;
+ }
+
+ /* find DAC seclabel or create one */
+ for (i = 0; i < *nseclabels; i++) {
+ virSecurityDeviceLabelDefPtr label = (*labels)[i];
+
+ if (STREQ(label->model, SECURITY_DAC_NAME))
+ return virSecurityDACAddOldlabel(label, user, group);
+ }
+
+ if ((VIR_REALLOC_N(*labels, *nseclabels + 1) < 0) ||
+ (VIR_ALLOC((*labels)[*nseclabels]) < 0)) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ if (!((*labels)[*nseclabels]->model = strdup(SECURITY_DAC_NAME))) {
+ VIR_FREE((*labels)[*nseclabels]);
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ if (virSecurityDACAddOldlabel((*labels)[*nseclabels], user, group) < 0) {
+ VIR_FREE((*labels)[*nseclabels]->model);
+ VIR_FREE((*labels)[*nseclabels]);
+ if (VIR_REALLOC_N(*labels, *nseclabels) < 0) {
+ /* ignore harmless */
+ }
+ goto cleanup;
+ }
+
+ (*nseclabels)++;
+
+ ret = 0;
+
+cleanup:
+ return ret;
+}
+
+static int
+virSecurityDACGetOriginalOwnerwhip(virSecurityDeviceLabelDefPtr **labels,
+ size_t *nseclabels,
+ uid_t *user, gid_t *group)
+{
+ int ret;
+ size_t i;
+
+ for (i = 0; i < *nseclabels; i++) {
+ virSecurityDeviceLabelDefPtr label = (*labels)[i];
+
+ if (STREQ(label->model, SECURITY_DAC_NAME) && label->oldlabel) {
+ /* steal oldlabel */
+ char *oldlabel = label->oldlabel;
+ label->oldlabel = NULL;
+
+ /* Moreover, if this is just a dummy seclabel to hold just the
+ * oldlabel, it is the one created by virSecurityDACSaveOwnership
+ * so clear it out */
+ if (!label->label) {
+ VIR_FREE(label);
+ if (*nseclabels > 1) {
+ memmove(*labels + i,
+ *labels + 1,
+ sizeof(*labels) * (*nseclabels) - (i + 1));
+ (*nseclabels)--;
+ if (VIR_REALLOC_N(*labels, *nseclabels) < 0) {
+ /* ignore harmless */
+ }
+ } else {
+ VIR_FREE(*labels);
+ *nseclabels = 0;
+ }
}
+
+ ret = parseIds(oldlabel, user, group);
+ VIR_FREE(oldlabel);
+ return ret;
}
+ }
+
+ return -1;
+}
+
+static int
+virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid,
+ uid_t *olduser, gid_t *oldgroup)
+{
+ struct stat sb;
+ VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'",
+ path, (long) uid, (long) gid);
- if (chown_errno == EOPNOTSUPP || chown_errno == EINVAL) {
+ if (stat(path, &sb) < 0)
+ goto error;
+
+ if (sb.st_uid == uid &&
+ sb.st_gid == gid) {
+ /* It's alright, there's nothing to change anyway. And since we are not
+ * chown()-ing it, don't neither save the original owner */
+ if (olduser)
+ *olduser = (uid_t) -1;
+ if (oldgroup)
+ *oldgroup = (gid_t) -1;
+ return 0;
+ }
+
+ if (chown(path, uid, gid) < 0) {
+ if (errno == EOPNOTSUPP || errno == EINVAL) {
VIR_INFO("Setting user and group to '%ld:%ld' on '%s' not "
"supported by filesystem",
(long) uid, (long) gid, path);
- } else if (chown_errno == EPERM) {
+ } else if (errno == EPERM) {
VIR_INFO("Setting user and group to '%ld:%ld' on '%s' not "
"permitted",
(long) uid, (long) gid, path);
- } else if (chown_errno == EROFS) {
+ } else if (errno == EROFS) {
VIR_INFO("Setting user and group to '%ld:%ld' on '%s' not "
"possible on readonly filesystem",
(long) uid, (long) gid, path);
- } else {
- virReportSystemError(chown_errno,
- _("unable to set user and group to '%ld:%ld' "
- "on '%s'"),
- (long) uid, (long) gid, path);
- return -1;
- }
+ } else
+ goto error;
+ } else {
+ /* save the original owner */
+ if (olduser)
+ *olduser = sb.st_uid;
+ if (oldgroup)
+ *oldgroup = sb.st_gid;
}
+
return 0;
+
+error:
+ virReportSystemError(errno,
+ _("unable to set user and group to '%ld:%ld' "
+ "on '%s'"),
+ (long) uid, (long) gid, path);
+ return -1;
}
static int
-virSecurityDACRestoreSecurityFileLabel(const char *path)
+virSecurityDACRestoreSecurityFileLabel(const char *path,
+ uid_t user,
+ gid_t group)
{
- struct stat buf;
int rc = -1;
char *newpath = NULL;
@@ -317,23 +452,19 @@ virSecurityDACRestoreSecurityFileLabel(const char *path)
if (virFileResolveLink(path, &newpath) < 0) {
virReportSystemError(errno,
_("cannot resolve symlink %s"), path);
- goto err;
+ goto cleanup;
}
- if (stat(newpath, &buf) != 0)
- goto err;
-
- /* XXX record previous ownership */
- rc = virSecurityDACSetOwnership(newpath, 0, 0);
+ rc = virSecurityDACSetOwnership(newpath, user, group, NULL, NULL);
-err:
+cleanup:
VIR_FREE(newpath);
return rc;
}
static int
-virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED,
+virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk,
const char *path,
size_t depth ATTRIBUTE_UNUSED,
void *opaque)
@@ -342,13 +473,18 @@ virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED,
virSecurityManagerPtr mgr = params[0];
virDomainDefPtr def = params[1];
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
- uid_t user;
- gid_t group;
+ uid_t user, olduser;
+ gid_t group, oldgroup;
if (virSecurityDACGetImageIds(def, priv, &user, &group))
return -1;
- return virSecurityDACSetOwnership(path, user, group);
+ if (virSecurityDACSetOwnership(path, user, group, &olduser, &oldgroup) < 0)
+ return -1;
+
+ return virSecurityDACSaveOwnership(&disk->seclabels,
+ &disk->nseclabels,
+ olduser, oldgroup);
}
@@ -383,6 +519,8 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr,
int migrated)
{
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+ uid_t user;
+ gid_t group;
if (!priv->dynamicOwnership)
return 0;
@@ -390,7 +528,7 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr,
if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK)
return 0;
- /* Don't restore labels on readoly/shared disks, because
+ /* Don't restore labels on shared disks, because
* other VMs may still be accessing these
* Alternatively we could iterate over all running
* domains and try to figure out if it is in use, but
@@ -398,7 +536,7 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr,
* we can't see running VMs using the file on other nodes
* Safest bet is thus to skip the restore step.
*/
- if (disk->readonly || disk->shared)
+ if (disk->shared)
return 0;
if (!disk->src)
@@ -420,7 +558,15 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr,
}
}
- return virSecurityDACRestoreSecurityFileLabel(disk->src);
+ if (virSecurityDACGetOriginalOwnerwhip(&disk->seclabels,
+ &disk->nseclabels,
+ &user, &group) < 0) {
+ VIR_INFO("Unable to find the original owner of %s. "
+ "Falling back to root:root", disk->src);
+ user = group = 0;
+ }
+
+ return virSecurityDACRestoreSecurityFileLabel(disk->src, user, group);
}
@@ -448,7 +594,7 @@ virSecurityDACSetSecurityPCILabel(virPCIDevicePtr dev ATTRIBUTE_UNUSED,
if (virSecurityDACGetIds(def, priv, &user, &group))
return -1;
- return virSecurityDACSetOwnership(file, user, group);
+ return virSecurityDACSetOwnership(file, user, group, NULL, NULL);
}
@@ -467,7 +613,7 @@ virSecurityDACSetSecurityUSBLabel(virUSBDevicePtr dev ATTRIBUTE_UNUSED,
if (virSecurityDACGetIds(def, priv, &user, &group))
return -1;
- return virSecurityDACSetOwnership(file, user, group);
+ return virSecurityDACSetOwnership(file, user, group, NULL, NULL);
}
@@ -538,7 +684,8 @@ virSecurityDACRestoreSecurityPCILabel(virPCIDevicePtr dev ATTRIBUTE_UNUSED,
const char *file,
void *opaque ATTRIBUTE_UNUSED)
{
- return virSecurityDACRestoreSecurityFileLabel(file);
+ /* restoring seclabel on PCI devices is not supported yet */
+ return virSecurityDACRestoreSecurityFileLabel(file, 0, 0);
}
@@ -547,7 +694,8 @@ virSecurityDACRestoreSecurityUSBLabel(virUSBDevicePtr dev ATTRIBUTE_UNUSED,
const char *file,
void *opaque ATTRIBUTE_UNUSED)
{
- return virSecurityDACRestoreSecurityFileLabel(file);
+ /* restoring seclabel on USB devices is not supported yet */
+ return virSecurityDACRestoreSecurityFileLabel(file, 0, 0);
}
@@ -613,39 +761,43 @@ done:
static int
-virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
- virDomainDefPtr def,
- virDomainChrSourceDefPtr dev)
-
+virSecurityDACSetChardevCallback(virDomainDefPtr def,
+ virDomainChrDefPtr dev,
+ void *opaque)
{
- virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+ virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(opaque);
+ virDomainChrSourceDefPtr src = &dev->source;
char *in = NULL, *out = NULL;
int ret = -1;
- uid_t user;
- gid_t group;
+ uid_t user, olduser = (uid_t) -1;
+ gid_t group, oldgroup = (gid_t) -1;
if (virSecurityDACGetIds(def, priv, &user, &group))
return -1;
- switch (dev->type) {
+ switch (src->type) {
case VIR_DOMAIN_CHR_TYPE_DEV:
case VIR_DOMAIN_CHR_TYPE_FILE:
- ret = virSecurityDACSetOwnership(dev->data.file.path, user, group);
+ ret = virSecurityDACSetOwnership(src->data.file.path, user, group,
+ &olduser, &oldgroup);
break;
case VIR_DOMAIN_CHR_TYPE_PIPE:
- if ((virAsprintf(&in, "%s.in", dev->data.file.path) < 0) ||
- (virAsprintf(&out, "%s.out", dev->data.file.path) < 0)) {
+ if ((virAsprintf(&in, "%s.in", src->data.file.path) < 0) ||
+ (virAsprintf(&out, "%s.out", src->data.file.path) < 0)) {
virReportOOMError();
goto done;
}
if (virFileExists(in) && virFileExists(out)) {
- if ((virSecurityDACSetOwnership(in, user, group) < 0) ||
- (virSecurityDACSetOwnership(out, user, group) < 0)) {
+ if ((virSecurityDACSetOwnership(in, user, group,
+ &olduser, &oldgroup) < 0) ||
+ (virSecurityDACSetOwnership(out, user, group,
+ &olduser, &oldgroup) < 0)) {
goto done;
}
- } else if (virSecurityDACSetOwnership(dev->data.file.path,
- user, group) < 0) {
+ } else if (virSecurityDACSetOwnership(src->data.file.path,
+ user, group,
+ &olduser, &oldgroup) < 0) {
goto done;
}
ret = 0;
@@ -656,6 +808,10 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
break;
}
+ ret = virSecurityDACSaveOwnership(&dev->seclabels,
+ &dev->nseclabels,
+ olduser, oldgroup);
+
done:
VIR_FREE(in);
VIR_FREE(out);
@@ -663,30 +819,44 @@ done:
}
static int
-virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
- virDomainChrSourceDefPtr dev)
+virSecurityDACRestoreChardevCallback(virDomainDefPtr def ATTRIBUTE_UNUSED,
+ virDomainChrDefPtr dev,
+ void *opaque ATTRIBUTE_UNUSED)
{
+ virDomainChrSourceDefPtr src = &dev->source;
char *in = NULL, *out = NULL;
int ret = -1;
+ uid_t user;
+ gid_t group;
- switch (dev->type) {
+ if (virSecurityDACGetOriginalOwnerwhip(&dev->seclabels,
+ &dev->nseclabels,
+ &user, &group) < 0) {
+ VIR_INFO("Unable to find the original owner of %s. "
+ "Falling back to root:root", src->data.file.path);
+ user = group = 0;
+ }
+
+ switch (src->type) {
case VIR_DOMAIN_CHR_TYPE_DEV:
case VIR_DOMAIN_CHR_TYPE_FILE:
- ret = virSecurityDACRestoreSecurityFileLabel(dev->data.file.path);
+ ret = virSecurityDACRestoreSecurityFileLabel(src->data.file.path,
+ user, group);
break;
case VIR_DOMAIN_CHR_TYPE_PIPE:
- if ((virAsprintf(&out, "%s.out", dev->data.file.path) < 0) ||
- (virAsprintf(&in, "%s.in", dev->data.file.path) < 0)) {
+ if ((virAsprintf(&out, "%s.out", src->data.file.path) < 0) ||
+ (virAsprintf(&in, "%s.in", src->data.file.path) < 0)) {
virReportOOMError();
goto done;
}
if (virFileExists(in) && virFileExists(out)) {
- if ((virSecurityDACRestoreSecurityFileLabel(out) < 0) ||
- (virSecurityDACRestoreSecurityFileLabel(in) < 0)) {
+ if ((virSecurityDACRestoreSecurityFileLabel(out, user, group) < 0) ||
+ (virSecurityDACRestoreSecurityFileLabel(in, user, group) < 0)) {
goto done;
}
- } else if (virSecurityDACRestoreSecurityFileLabel(dev->data.file.path) < 0) {
+ } else if (virSecurityDACRestoreSecurityFileLabel(src->data.file.path,
+ user, group) < 0) {
goto done;
}
ret = 0;
@@ -703,18 +873,6 @@ done:
return ret;
}
-
-static int
-virSecurityDACRestoreChardevCallback(virDomainDefPtr def ATTRIBUTE_UNUSED,
- virDomainChrDefPtr dev,
- void *opaque)
-{
- virSecurityManagerPtr mgr = opaque;
-
- return virSecurityDACRestoreChardevLabel(mgr, &dev->source);
-}
-
-
static int
virSecurityDACRestoreSecurityAllLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
@@ -753,28 +911,16 @@ virSecurityDACRestoreSecurityAllLabel(virSecurityManagerPtr mgr,
rc = -1;
if (def->os.kernel &&
- virSecurityDACRestoreSecurityFileLabel(def->os.kernel) < 0)
+ virSecurityDACRestoreSecurityFileLabel(def->os.kernel, 0, 0) < 0)
rc = -1;
if (def->os.initrd &&
- virSecurityDACRestoreSecurityFileLabel(def->os.initrd) < 0)
+ virSecurityDACRestoreSecurityFileLabel(def->os.initrd, 0, 0) < 0)
rc = -1;
return rc;
}
-
-static int
-virSecurityDACSetChardevCallback(virDomainDefPtr def ATTRIBUTE_UNUSED,
- virDomainChrDefPtr dev,
- void *opaque)
-{
- virSecurityManagerPtr mgr = opaque;
-
- return virSecurityDACSetChardevLabel(mgr, def, &dev->source);
-}
-
-
static int
virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
@@ -815,11 +961,13 @@ virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr mgr,
return -1;
if (def->os.kernel &&
- virSecurityDACSetOwnership(def->os.kernel, user, group) < 0)
+ virSecurityDACSetOwnership(def->os.kernel, user, group,
+ NULL, NULL) < 0)
return -1;
if (def->os.initrd &&
- virSecurityDACSetOwnership(def->os.initrd, user, group) < 0)
+ virSecurityDACSetOwnership(def->os.initrd, user, group,
+ NULL, NULL) < 0)
return -1;
return 0;
@@ -838,7 +986,7 @@ virSecurityDACSetSavedStateLabel(virSecurityManagerPtr mgr,
if (virSecurityDACGetImageIds(def, priv, &user, &group))
return -1;
- return virSecurityDACSetOwnership(savefile, user, group);
+ return virSecurityDACSetOwnership(savefile, user, group, NULL, NULL);
}
@@ -852,7 +1000,7 @@ virSecurityDACRestoreSavedStateLabel(virSecurityManagerPtr mgr,
if (!priv->dynamicOwnership)
return 0;
- return virSecurityDACRestoreSecurityFileLabel(savefile);
+ return virSecurityDACRestoreSecurityFileLabel(savefile, 0, 0);
}
--
1.8.0.2
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list