[libvirt] [PATCH v5 3/3] security_dac: Favour ACLs over chown()
Daniel P. Berrange
berrange at redhat.com
Thu Mar 28 09:44:50 UTC 2013
On Thu, Mar 21, 2013 at 05:50:49PM +0100, Michal Privoznik wrote:
> On filesystems supporting ACLs we don't need to do a chown but we
> can just set ACLs to gain access for qemu. However, since we are
> setting these on too low level, where we don't know if disk is
> just a read only or read write, we set read write access
> unconditionally.
>
> >From implementation POV, a reference counter is introduced, so ACL is
> restored only on the last restore attempt in order to not cut off other
> domains. And since a file may had an ACL for a user already set, we need
> to keep this as well. Both these, the reference counter and original ACL
> are stored as extended attributes named trusted.libvirt.dac.refCount and
> trusted.libvirt.dac.oldACL respectively.
>
> However, some filesystems doesn't support ACLs, XATTRs, or both. So the
> code is made to favour ACLs among with tracking the reference count. If
> this fails, we fall back to chown() with best effort to remember the
> original owner of file.
I'm reminded that we need to do the same thing for the SELinux
driver. ie we should record the original selinux label in an
xattr and apply it on restore, instead of guessing the default
label on restore. No need to block this though - it can be done
as a separate patch.
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 35b90da..989dc50 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,9 @@
>
> #define VIR_FROM_THIS VIR_FROM_SECURITY
> #define SECURITY_DAC_NAME "dac"
> +#define SECURITY_DAC_XATTR_OLD_ACL "trusted.libvirt.dac.oldACL"
> +#define SECURITY_DAC_XATTR_OLD_OWNER "trusted.libvirt.dac.oldOwner"
> +#define SECURITY_DAC_XATTR_REFCOUNT "trusted.libvirt.dac.refCount"
>
> typedef struct _virSecurityDACData virSecurityDACData;
> typedef virSecurityDACData *virSecurityDACDataPtr;
> @@ -234,6 +238,196 @@ int virSecurityDACGetImageIds(virDomainDefPtr def, virSecurityDACDataPtr priv,
> return 0;
> }
>
> +static int
> +virSecurityDACGetXATTRRefcount(const char *path,
> + int *refCount)
> +{
> + int ret = -1;
> + char *refCountStr;
> +
> + if (virFileGetAttr(path, SECURITY_DAC_XATTR_REFCOUNT, &refCountStr) < 0)
> + return ret;
> +
> + VIR_DEBUG("path=%s refCountStr=%s", path, NULLSTR(refCountStr));
> +
> + if (!refCountStr) {
> + *refCount = 0;
> + return 0;
> + }
> +
> + if (virStrToLong_i(refCountStr, NULL, 10, refCount) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Malformed %s attribute: %s"),
> + SECURITY_DAC_XATTR_REFCOUNT,
> + refCountStr);
> + goto cleanup;
> + }
> +
> + ret = 0;
> +cleanup:
> + VIR_FREE(refCountStr);
> + return ret;
> +}
> +
> +static int
> +virSecurityDACSetXATTRRefcount(const char *path,
> + int refCount)
> +{
> + int ret = -1;
> + char *refCountStr;
> +
> + VIR_DEBUG("path=%s refCount=%d", path, refCount);
> +
> + if (refCount == 0) {
> + virFileRemoveAttr(path, SECURITY_DAC_XATTR_REFCOUNT);
> + return 0;
> + }
> +
> + if (virAsprintf(&refCountStr, "%u", refCount) < 0) {
> + virReportOOMError();
> + return ret;
> + }
> +
> + if (virFileSetAttr(path, SECURITY_DAC_XATTR_REFCOUNT, refCountStr) < 0)
> + goto cleanup;
> +
> + ret = 0;
> +cleanup:
> + VIR_FREE(refCountStr);
> + return ret;
> +}
> +
> +static int
> +virSecurityDACSetACL(const char *path,
> + uid_t uid)
> +{
> + int ret = -1;
> + char *oldACL = NULL;
> + mode_t perms;
> +
> + VIR_DEBUG("path=%s uid=%u", path, uid);
> +
> + if (virFileGetACL(path, uid, &perms) < 0) {
> + /* error getting ACL entry for @uid */
> + goto cleanup;
> + }
> +
> + if (virAsprintf(&oldACL, "%u:0%o", uid, perms) < 0) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + if (virFileSetAttr(path, SECURITY_DAC_XATTR_OLD_ACL, oldACL) < 0)
> + goto cleanup;
> +
> + if (virFileSetACL(path, uid, S_IRUSR | S_IWUSR) < 0)
> + goto cleanup;
Hmm, one thing we didn't do in the past was to correctly honour
the 'readonly' attribute. Now we are using ACLs we should do
that. ie not set S_IWUSR if def->readonly is true.
> +
> + ret = 0;
> +cleanup:
> + if (ret < 0)
> + virFileRemoveAttr(path, SECURITY_DAC_XATTR_OLD_ACL);
> + VIR_FREE(oldACL);
> + return ret;
> +}
> +
> +static int
> +virSecurityDACRestoreACL(const char *path)
> +{
> + int ret = -1;
> + char *oldACL = NULL, *c;
> + uid_t uid;
> + mode_t perms;
> +
> + VIR_DEBUG("path=%s", path);
> +
> + if (virFileGetAttr(path, SECURITY_DAC_XATTR_OLD_ACL, &oldACL) < 0)
> + return ret;
> +
> + if (!oldACL) {
> + VIR_WARN("Attribute %s is missing", SECURITY_DAC_XATTR_OLD_ACL);
> + return ret;
> + }
> +
> + if (!(c = strchr(oldACL, ':'))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Malformed %s attribute: %s"),
> + SECURITY_DAC_XATTR_OLD_ACL, oldACL);
> + goto cleanup;
> + }
> +
> + *c = '\0';
> + c++;
> +
> + if (virStrToLong_ui(oldACL, NULL, 10, &uid) < 0 ||
> + virStrToLong_ui(c, NULL, 8, &perms) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Malformed %s attribute: %s"),
> + SECURITY_DAC_XATTR_OLD_ACL, oldACL);
> + goto cleanup;
> + }
> +
> + if ((perms && virFileSetACL(path, uid, perms) < 0) ||
> + (!perms && virFileRemoveACL(path, uid) < 0))
> + goto cleanup;
> +
> + virFileRemoveAttr(path, SECURITY_DAC_XATTR_OLD_ACL);
> + ret = 0;
> +cleanup:
> + VIR_FREE(oldACL);
> + return ret;
> +}
> +
> +static int
> +virSecurityDACRememberLabel(const char *path)
> +{
> + int ret = -1;
> + struct stat sb;
> + char *oldOwner = NULL;
> +
> + VIR_DEBUG("path=%s", path);
> +
> + 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;
> +
> + ret = 0;
> +cleanup:
> + VIR_FREE(oldOwner);
> + return ret;
> +}
> +
> +static int
> +virSecurityDACGetRememberedLabel(const char *path,
> + uid_t *user,
> + uid_t *group)
> +{
> + int ret = -1;
> + char *oldOwner = NULL;
> +
> + 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_OLD_OWNER);
> + ret = 0;
> +cleanup:
> + VIR_FREE(oldOwner);
> + return ret;
> +}
>
> static virSecurityDriverStatus
> virSecurityDACProbe(const char *virtDriver ATTRIBUTE_UNUSED)
> @@ -265,11 +459,8 @@ static const char * virSecurityDACGetDOI(virSecurityManagerPtr mgr ATTRIBUTE_UNU
> }
>
> static int
> -virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
> +virSecurityDACChown(const char *path, uid_t uid, gid_t gid)
> {
> - VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'",
> - path, (long) uid, (long) gid);
> -
> if (chown(path, uid, gid) < 0) {
> struct stat sb;
> int chown_errno = errno;
> @@ -306,29 +497,104 @@ virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
> }
>
> static int
> +virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
> +{
> + int refCount = 0;
> + bool xattrSupported = true;
> + virErrorPtr err;
> +
> + VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'",
> + path, (long) uid, (long) gid);
> +
> + if (virSecurityDACGetXATTRRefcount(path, &refCount) < 0) {
> + err = virGetLastError();
> + if (!err || err->code != VIR_ERR_SYSTEM_ERROR ||
> + (err->int1 != ENOSYS && err->int1 != ENOTSUP))
> + return -1;
> + virResetLastError();
> + xattrSupported = false;
> + }
> +
> + if (refCount || virSecurityDACSetACL(path, uid) == 0) {
Just checking refCount isn't sufficient - we'll need to validate that
the ACL matches what we expect. ie if the existing ACL is allowing
read+write, but this VM is configured read-only we must reject it,
since the ACLs are incompatible.
> + if (xattrSupported &&
> + virSecurityDACSetXATTRRefcount(path, refCount + 1) < 0) {
> + /* Clear out oldACL XATTR */
> + return -1;
> + }
> + return 0;
> + }
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