[libvirt] [PATCH v2 2/4] security_dac: Remember owner prior chown() and restore on relabel
Daniel P. Berrange
berrange at redhat.com
Thu Mar 7 15:52:45 UTC 2013
On Wed, Mar 06, 2013 at 03:05:54PM +0100, Michal Privoznik wrote:
> 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 | 187 ++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 153 insertions(+), 34 deletions(-)
>
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 0b274b7..76a1dc6 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -25,6 +25,7 @@
>
> #include "security_dac.h"
> #include "virerror.h"
> +#include "virfile.h"
> #include "virutil.h"
> #include "viralloc.h"
> #include "virlog.h"
> @@ -34,6 +35,8 @@
>
> #define VIR_FROM_THIS VIR_FROM_SECURITY
> #define SECURITY_DAC_NAME "dac"
> +#define SECURITY_DAC_XATTR_OLD_OWNER "trusted.oldOwner"
> +#define SECURITY_DAC_XATTR_REFCOUNT "trusted.refCount"
>
> typedef struct _virSecurityDACData virSecurityDACData;
> typedef virSecurityDACData *virSecurityDACDataPtr;
> @@ -234,6 +237,117 @@ int virSecurityDACGetImageIds(virDomainDefPtr def, virSecurityDACDataPtr priv,
> return 0;
> }
>
> +static int
> +virSecurityDACRememberLabel(const char *path)
> +{
> + int ret = -1;
> + char *refCountStr = NULL;
> + char *oldOwner = NULL;
> + unsigned int refCount = 0;
> +
> + if (virFileGetAttr(path, SECURITY_DAC_XATTR_REFCOUNT, &refCountStr) < 0) {
> + /* Not all filesystems support XATTR */
> + if (virStorageFileIsSharedFS(path))
> + return 0;
> + else
> + return ret;
> + }
> +
> + if (!refCountStr) {
> + struct stat sb;
> +
> + if (stat(path, &sb) < 0) {
> + virReportSystemError(errno, _("Unable to stat '%s'"), path);
> + return ret;
> + }
> +
> + if (virAsprintf(&oldOwner, "+%u:+%u",
> + (unsigned) sb.st_uid, (unsigned) sb.st_gid) < 0) {
> + virReportOOMError();
> + return ret;
> + }
> +
> + if (virFileSetAttr(path, SECURITY_DAC_XATTR_OLD_OWNER, oldOwner) < 0)
> + goto cleanup;
> + } else if (virStrToLong_ui(refCountStr, NULL, 10, &refCount) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Malformed %s attribute: %s"),
> + SECURITY_DAC_XATTR_REFCOUNT,
> + refCountStr);
> + goto cleanup;
> + }
> +
> + if (virAsprintf(&refCountStr, "%u", refCount+1) < 0) {
> + virReportOOMError();
> + return ret;
> + }
> +
> + if (virFileSetAttr(path, SECURITY_DAC_XATTR_REFCOUNT, refCountStr) < 0)
> + goto cleanup;
> +
> + ret = 0;
> +cleanup:
> + VIR_FREE(oldOwner);
> + VIR_FREE(refCountStr);
> + return 0;
> +}
> +
> +static int
> +virSecurityDACGetRememberedLabel(const char *path,
> + uid_t *user,
> + uid_t *group)
> +{
> + int ret = -1;
> + char *refCountStr = NULL;
> + char *oldOwner = NULL;
> + unsigned int refCount;
> +
> + if (virFileGetAttr(path, SECURITY_DAC_XATTR_REFCOUNT, &refCountStr) < 0)
> + return ret;
> +
> + if (!refCountStr) {
> + /* Backward compatibility. If domain was started with older libvirt,
> + * it doesn't have any XATTR set so we should not fail here */
> + return 0;
> + }
> +
> + if (virStrToLong_ui(refCountStr, NULL, 10, &refCount) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Malformed %s attribute: %s"),
> + SECURITY_DAC_XATTR_REFCOUNT,
> + refCountStr);
> + goto cleanup;
> + }
> + VIR_FREE(refCountStr);
> +
> + if (--refCount) {
> + if (virAsprintf(&refCountStr, "%u", refCount) < 0) {
> + virReportOOMError();
> + return ret;
> + }
> + if (virFileSetAttr(path, SECURITY_DAC_XATTR_REFCOUNT, refCountStr) < 0)
> + goto cleanup;
> + ret = refCount;
> + } else {
> + if (virFileGetAttr(path, SECURITY_DAC_XATTR_OLD_OWNER, &oldOwner) < 0 ||
> + !oldOwner)
> + return ret;
> +
> + if (parseIds(oldOwner, user, group) < 0)
> + goto cleanup;
> +
> + virFileRemoveAttr(path, SECURITY_DAC_XATTR_REFCOUNT);
> + virFileRemoveAttr(path, SECURITY_DAC_XATTR_OLD_OWNER);
> + }
> +
> + ret = refCount;
> +
> +cleanup:
> + VIR_FREE(oldOwner);
> + VIR_FREE(refCountStr);
> + return ret;
> +}
> +
>
> static virSecurityDriverStatus
> virSecurityDACProbe(const char *virtDriver ATTRIBUTE_UNUSED)
> @@ -265,11 +379,18 @@ static const char * virSecurityDACGetDOI(virSecurityManagerPtr mgr ATTRIBUTE_UNU
> }
>
> static int
> -virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
> +virSecurityDACSetOwnership(const char *path,
> + uid_t uid,
> + gid_t gid,
> + bool remember)
I don't think you should be adding this 'remember' flag here, because while
it seems fine in this patch, when you add ACL support this gets really
dubious semantics. Just stop virSecurityDACRestoreSecurityFileLabel from
calling this method entirely - trying to share doesn't make sense when
you introduce ACLs to the equation since you need fundamentally differnt
APIs in the set vs restore case the.
> {
> VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'",
> path, (long) uid, (long) gid);
>
> + if (remember &&
> + virSecurityDACRememberLabel(path) < 0)
> + return -1;
> +
> if (chown(path, uid, gid) < 0) {
> struct stat sb;
> int chown_errno = errno;
> @@ -310,23 +431,35 @@ virSecurityDACRestoreSecurityFileLabel(const char *path)
> {
> struct stat buf;
> int rc = -1;
> + int ret;
> char *newpath = NULL;
> + uid_t user = (uid_t) -1;
> + gid_t group = (gid_t) -1;
>
> VIR_INFO("Restoring DAC user and group on '%s'", path);
>
> if (virFileResolveLink(path, &newpath) < 0) {
> virReportSystemError(errno,
> _("cannot resolve symlink %s"), path);
> - goto err;
> + goto cleanup;
> }
>
> if (stat(newpath, &buf) != 0)
> - goto err;
> + goto cleanup;
>
> - /* XXX record previous ownership */
> - rc = virSecurityDACSetOwnership(newpath, 0, 0);
> + ret = virSecurityDACGetRememberedLabel(newpath, &user, &group);
> + if (ret < 0)
> + goto cleanup;
> + else if (ret > 0) {
> + VIR_DEBUG("Skipping label restore on '%s' as it is still "
> + "in use (refCount=%d)", newpath, ret);
> + rc = 0;
> + goto cleanup;
> + }
> +
> + rc = virSecurityDACSetOwnership(newpath, user, group, false);
>
> -err:
> +cleanup:
> VIR_FREE(newpath);
> return rc;
> }
> @@ -348,7 +481,7 @@ virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED,
> if (virSecurityDACGetImageIds(def, priv, &user, &group))
> return -1;
>
> - return virSecurityDACSetOwnership(path, user, group);
> + return virSecurityDACSetOwnership(path, user, group, true);
...and this change & all that follow will go away if you don't add the
extra 'remember' parameter.
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 :|
More information about the libvir-list
mailing list