[libvirt] [PATCH 0/7] Improve performance of polkit checks
This series improves the performance of the polkit driver by switching from use of the pk-check command, to the DBus APIs. As a convenient side effect, this means we are no longer vulnerable to CVE-2013-4311, on any polkit version, since we no longer need pk-check (which is what had the flaw). In terms of performance, with access control checking turned on for all APIs, the time to list 100 VMs dropps from 2.7 secs to 1 sec on my machine. To improve on this further, we would need to find a way to parallelize the issuing of DBus calls for each VM, instead of serialize the access checks. Daniel P. Berrange (7): Add common API for doing polkit authentication Add typesafe APIs for virIdentity attributes Convert callers to use typesafe APIs for setting identity attrs Convert callers to use typesafe APIs for getting identity attrs Convert remote daemon & acl code to use polkit API Support passing dict by reference for dbus messages Convert polkit code to use DBus API instead of CLI helper cfg.mk | 3 + daemon/remote.c | 235 ++---------------------- include/libvirt/virterror.h | 2 + po/POTFILES.in | 2 + src/Makefile.am | 1 + src/access/viraccessdriverpolkit.c | 97 ++++------ src/libvirt_private.syms | 22 +++ src/rpc/virnetserverclient.c | 115 +++--------- src/util/virdbus.c | 274 +++++++++++++++++++--------- src/util/virerror.c | 2 + src/util/viridentity.c | 320 +++++++++++++++++++++++++++------ src/util/viridentity.h | 40 +++++ src/util/virpolkit.c | 255 ++++++++++++++++++++++++++ src/util/virpolkit.h | 34 ++++ src/util/virstring.c | 14 ++ src/util/virstring.h | 2 + tests/Makefile.am | 9 +- tests/virdbustest.c | 218 +++++++++++++++++++++- tests/virpolkittest.c | 360 +++++++++++++++++++++++++++++++++++++ 19 files changed, 1485 insertions(+), 520 deletions(-) create mode 100644 src/util/virpolkit.c create mode 100644 src/util/virpolkit.h create mode 100644 tests/virpolkittest.c -- 1.9.3
There are now two places in libvirt which use polkit. Currently they use pkexec, which is set to be replaced by direct DBus API calls. Add a common API which they will both be able to use for this purpose. No tests are added at this time, since the impl will be gutted in favour of a DBus API call shortly. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- include/libvirt/virterror.h | 2 + po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 4 + src/util/virerror.c | 2 + src/util/virpolkit.c | 263 ++++++++++++++++++++++++++++++++++++++++++++ src/util/virpolkit.h | 34 ++++++ 7 files changed, 307 insertions(+) create mode 100644 src/util/virpolkit.c create mode 100644 src/util/virpolkit.h diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index 15ba4f1..85dd74c 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -124,6 +124,8 @@ typedef enum { VIR_FROM_CRYPTO = 58, /* Error from crypto code */ VIR_FROM_FIREWALL = 59, /* Error from firewall */ + VIR_FROM_POLKIT = 60, /* Error from polkit code */ + # ifdef VIR_ENUM_SENTINELS VIR_ERR_DOMAIN_LAST # endif diff --git a/po/POTFILES.in b/po/POTFILES.in index f17b35f..1a0b75e 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -194,6 +194,7 @@ src/util/virnuma.c src/util/virobject.c src/util/virpci.c src/util/virpidfile.c +src/util/virpolkit.c src/util/virportallocator.c src/util/virprocess.c src/util/virrandom.c diff --git a/src/Makefile.am b/src/Makefile.am index 46e411e..a05b2da 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -141,6 +141,7 @@ UTIL_SOURCES = \ util/virobject.c util/virobject.h \ util/virpci.c util/virpci.h \ util/virpidfile.c util/virpidfile.h \ + util/virpolkit.c util/virpolkit.h \ util/virportallocator.c util/virportallocator.h \ util/virprobe.h \ util/virprocess.c util/virprocess.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 10ebd12..de334bb 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1787,6 +1787,10 @@ virPidFileWrite; virPidFileWritePath; +# util/virpolkit.h +virPolkitCheckAuth; + + # util/virportallocator.h virPortAllocatorAcquire; virPortAllocatorNew; diff --git a/src/util/virerror.c b/src/util/virerror.c index 6bd3d09..4aa6d04 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -130,6 +130,8 @@ VIR_ENUM_IMPL(virErrorDomain, VIR_ERR_DOMAIN_LAST, "Bhyve", "Crypto", "Firewall", + + "Polkit", /* 60 */ ) diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c new file mode 100644 index 0000000..620bfda --- /dev/null +++ b/src/util/virpolkit.c @@ -0,0 +1,263 @@ +/* + * virpolkit.c: helpers for using polkit APIs + * + * Copyright (C) 2013 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#include <config.h> + +#if WITH_POLKIT0 +# include <polkit/polkit.h> +# include <polkit-dbus/polkit-dbus.h> +#endif + +#include "virpolkit.h" +#include "vircommand.h" +#include "virerror.h" +#include "virlog.h" +#include "virstring.h" +#include "virprocess.h" +#include "viralloc.h" +#include "virdbus.h" + +#define VIR_FROM_THIS VIR_FROM_POLKIT + +VIR_LOG_INIT("util.polkit"); + +#if WITH_POLKIT1 +/* + * virPolkitCheckAuth: + * @actionid: permission to check + * @pid: client process ID + * @startTime: process start time, or 0 + * @uid: client process user ID + * @details: NULL terminated (key, value) pair list + * @allowInteraction: true if auth prompts are allowed + * + * Check if a client is authenticated with polkit + * + * Returns 0 on success, -1 on failure, -2 on auth denied + */ +int virPolkitCheckAuth(const char *actionid, + pid_t pid, + unsigned long long startTime, + uid_t uid, + const char **details, + bool allowInteraction) +{ + int status = -1; + bool authdismissed = 0; + bool supportsuid = 0; + char *pkout = NULL; + virCommandPtr cmd = NULL; + int ret = -1; + static bool polkitInsecureWarned = false; + + VIR_DEBUG("Checking PID %lld UID %d startTime %llu", + (long long)pid, (int)uid, startTime); + + if (startTime == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Start time is required for polkit auth")); + return -1; + } + + cmd = virCommandNewArgList(PKCHECK_PATH, "--action-id", actionid, NULL); + virCommandSetOutputBuffer(cmd, &pkout); + virCommandSetErrorBuffer(cmd, &pkout); + + virCommandAddArg(cmd, "--process"); +# ifdef PKCHECK_SUPPORTS_UID + supportsuid = 1; +# endif + if (supportsuid) { + virCommandAddArgFormat(cmd, "%lld,%llu,%lu", + (long long)pid, startTime, (unsigned long)uid); + } else { + if (!polkitInsecureWarned) { + VIR_WARN("No support for caller UID with pkcheck. This deployment is known to be insecure."); + polkitInsecureWarned = true; + } + virCommandAddArgFormat(cmd, "%lld,%llu", + (long long)pid, startTime); + } + if (allowInteraction) + virCommandAddArg(cmd, "--allow-user-interaction"); + + while (details && details[0] && details[1]) { + virCommandAddArgList(cmd, "--detail", details[0], details[1], NULL); + details += 2; + } + + if (virCommandRun(cmd, &status) < 0) + goto cleanup; + + authdismissed = (pkout && strstr(pkout, "dismissed=true")); + if (status != 0) { + char *tmp = virProcessTranslateStatus(status); + VIR_DEBUG("Policy kit denied action %s from pid %lld, uid %d: %s", + actionid, (long long)pid, (int)uid, NULLSTR(tmp)); + VIR_FREE(tmp); + ret = -2; + goto cleanup; + } + + VIR_DEBUG("Policy allowed action %s from pid %lld, uid %d", + actionid, (long long)pid, (int)uid); + + ret = 0; + + cleanup: + if (ret < 0) { + virResetLastError(); + + if (authdismissed) { + virReportError(VIR_ERR_AUTH_CANCELLED, "%s", + _("authentication cancelled by user")); + } else if (pkout && *pkout) { + virReportError(VIR_ERR_AUTH_FAILED, _("polkit: %s"), pkout); + } else { + virReportError(VIR_ERR_AUTH_FAILED, "%s", _("authentication failed")); + } + } + + virCommandFree(cmd); + VIR_FREE(pkout); + return ret; +} + + +#elif WITH_POLKIT0 +int virPolkitCheckAuth(const char *actionid, + pid_t pid, + unsigned long long startTime ATTRIBUTE_UNUSED, + uid_t uid, + const char **details, + bool allowInteraction ATTRIBUTE_UNUSED) +{ + PolKitCaller *pkcaller = NULL; + PolKitAction *pkaction = NULL; + PolKitContext *pkcontext = NULL; + PolKitError *pkerr = NULL; + PolKitResult pkresult; + DBusError err; + DBusConnection *sysbus; + int ret = -1; + + if (details) { + virReportError(VIR_ERR_AUTH_FAILED, "%s", + _("Details not supported with polkit v0")); + return -1; + } + + if (!(sysbus = virDBusGetSystemBus())) + goto cleanup; + + VIR_INFO("Checking PID %lld running as %d", + (long long) pid, uid); + dbus_error_init(&err); + if (!(pkcaller = polkit_caller_new_from_pid(sysbus, + pid, &err))) { + VIR_DEBUG("Failed to lookup policy kit caller: %s", err.message); + dbus_error_free(&err); + goto cleanup; + } + + if (!(pkaction = polkit_action_new())) { + char ebuf[1024]; + VIR_DEBUG("Failed to create polkit action %s", + virStrerror(errno, ebuf, sizeof(ebuf))); + goto cleanup; + } + polkit_action_set_action_id(pkaction, actionid); + + if (!(pkcontext = polkit_context_new()) || + !polkit_context_init(pkcontext, &pkerr)) { + char ebuf[1024]; + VIR_DEBUG("Failed to create polkit context %s", + (pkerr ? polkit_error_get_error_message(pkerr) + : virStrerror(errno, ebuf, sizeof(ebuf)))); + if (pkerr) + polkit_error_free(pkerr); + dbus_error_free(&err); + goto cleanup; + } + +# if HAVE_POLKIT_CONTEXT_IS_CALLER_AUTHORIZED + pkresult = polkit_context_is_caller_authorized(pkcontext, + pkaction, + pkcaller, + 0, + &pkerr); + if (pkerr && polkit_error_is_set(pkerr)) { + VIR_DEBUG("Policy kit failed to check authorization %d %s", + polkit_error_get_error_code(pkerr), + polkit_error_get_error_message(pkerr)); + goto cleanup; + } +# else + pkresult = polkit_context_can_caller_do_action(pkcontext, + pkaction, + pkcaller); +# endif + if (pkresult != POLKIT_RESULT_YES) { + VIR_DEBUG("Policy kit denied action %s from pid %lld, uid %d, result: %s", + actionid, (long long) pid, uid, + polkit_result_to_string_representation(pkresult)); + ret = -2; + goto cleanup; + } + + VIR_DEBUG("Policy allowed action %s from pid %lld, uid %d", + actionid, (long long)pid, (int)uid); + + ret = 0; + + cleanup: + if (ret < 0) { + virResetLastError(); + virReportError(VIR_ERR_AUTH_FAILED, "%s", + _("authentication failed")); + } + if (pkcontext) + polkit_context_unref(pkcontext); + if (pkcaller) + polkit_caller_unref(pkcaller); + if (pkaction) + polkit_action_unref(pkaction); + return ret; +} + + +#else /* ! WITH_POLKIT1 && ! WITH_POLKIT0 */ + +int virPolkitCheckAuth(const char *actionid, + pid_t pid, + unsigned long long startTime, + uid_t uid, + const char **details, + bool allowInteraction) +{ + VIR_ERROR(_("Polkit auth attempted, even though polkit is not available")); + virReportError(VIR_ERR_AUTH_FAILED, "%s", + _("authentication failed")); + return -1; +} + + +#endif /* WITH_POLKIT1 */ diff --git a/src/util/virpolkit.h b/src/util/virpolkit.h new file mode 100644 index 0000000..36122d0 --- /dev/null +++ b/src/util/virpolkit.h @@ -0,0 +1,34 @@ +/* + * virpolkit.h: helpers for using polkit APIs + * + * Copyright (C) 2013 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#ifndef __VIR_POLKIT_H__ +# define __VIR_POLKIT_H__ + +# include "internal.h" + +int virPolkitCheckAuth(const char *actionid, + pid_t pid, + unsigned long long startTime, + uid_t uid, + const char **details, + bool allowInteraction); + +#endif /* __VIR_POLKIT_H__ */ -- 1.9.3
Instead of requiring the caller to format to/from strings, add typesafe APIs todo this work. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 18 ++++ src/util/viridentity.c | 241 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/viridentity.h | 40 ++++++++ 3 files changed, 299 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index de334bb..7f0bb63 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1395,11 +1395,29 @@ virHostdevUpdateDomainActiveDevices; # util/viridentity.h virIdentityGetAttr; virIdentityGetCurrent; +virIdentityGetSASLUserName; +virIdentityGetSELinuxContext; virIdentityGetSystem; +virIdentityGetUNIXGroupID; +virIdentityGetUNIXGroupName; +virIdentityGetUNIXProcessID; +virIdentityGetUNIXProcessTime; +virIdentityGetUNIXUserID; +virIdentityGetUNIXUserName; +virIdentityGetX509DName; virIdentityIsEqual; virIdentityNew; virIdentitySetAttr; virIdentitySetCurrent; +virIdentitySetSASLUserName; +virIdentitySetSELinuxContext; +virIdentitySetUNIXGroupID; +virIdentitySetUNIXGroupName; +virIdentitySetUNIXProcessID; +virIdentitySetUNIXProcessTime; +virIdentitySetUNIXUserID; +virIdentitySetUNIXUserName; +virIdentitySetX509DName; # util/virinitctl.h diff --git a/src/util/viridentity.c b/src/util/viridentity.c index a997385..68ccab9 100644 --- a/src/util/viridentity.c +++ b/src/util/viridentity.c @@ -352,3 +352,244 @@ bool virIdentityIsEqual(virIdentityPtr identA, cleanup: return ret; } + + +int virIdentityGetUNIXUserName(virIdentityPtr ident, + const char **username) +{ + return virIdentityGetAttr(ident, + VIR_IDENTITY_ATTR_UNIX_USER_NAME, + username); +} + + +int virIdentityGetUNIXUserID(virIdentityPtr ident, + uid_t *uid) +{ + int val; + const char *userid; + + *uid = -1; + if (virIdentityGetAttr(ident, + VIR_IDENTITY_ATTR_UNIX_USER_ID, + &userid) < 0) + return -1; + + if (!userid) + return -1; + + if (virStrToLong_i(userid, NULL, 10, &val) < 0) + return -1; + + *uid = (uid_t)val; + + return 0; +} + +int virIdentityGetUNIXGroupName(virIdentityPtr ident, + const char **groupname) +{ + return virIdentityGetAttr(ident, + VIR_IDENTITY_ATTR_UNIX_GROUP_NAME, + groupname); +} + + +int virIdentityGetUNIXGroupID(virIdentityPtr ident, + gid_t *gid) +{ + int val; + const char *groupid; + + *gid = -1; + if (virIdentityGetAttr(ident, + VIR_IDENTITY_ATTR_UNIX_GROUP_ID, + &groupid) < 0) + return -1; + + if (!groupid) + return -1; + + if (virStrToLong_i(groupid, NULL, 10, &val) < 0) + return -1; + + *gid = (gid_t)val; + + return 0; +} + + +int virIdentityGetUNIXProcessID(virIdentityPtr ident, + pid_t *pid) +{ + unsigned long long val; + const char *processid; + + *pid = 0; + if (virIdentityGetAttr(ident, + VIR_IDENTITY_ATTR_UNIX_PROCESS_ID, + &processid) < 0) + return -1; + + if (!processid) + return -1; + + if (virStrToLong_ull(processid, NULL, 10, &val) < 0) + return -1; + + *pid = (pid_t)val; + + return 0; +} + + +int virIdentityGetUNIXProcessTime(virIdentityPtr ident, + unsigned long long *timestamp) +{ + const char *processtime; + if (virIdentityGetAttr(ident, + VIR_IDENTITY_ATTR_UNIX_PROCESS_TIME, + &processtime) < 0) + return -1; + + if (!processtime) + return -1; + + if (virStrToLong_ull(processtime, NULL, 10, timestamp) < 0) + return -1; + + return 0; +} + + +int virIdentityGetSASLUserName(virIdentityPtr ident, + const char **username) +{ + return virIdentityGetAttr(ident, + VIR_IDENTITY_ATTR_SASL_USER_NAME, + username); +} + + +int virIdentityGetX509DName(virIdentityPtr ident, + const char **dname) +{ + return virIdentityGetAttr(ident, + VIR_IDENTITY_ATTR_X509_DISTINGUISHED_NAME, + dname); +} + + +int virIdentityGetSELinuxContext(virIdentityPtr ident, + const char **context) +{ + return virIdentityGetAttr(ident, + VIR_IDENTITY_ATTR_SELINUX_CONTEXT, + context); +} + + +int virIdentitySetUNIXUserName(virIdentityPtr ident, + const char *username) +{ + return virIdentitySetAttr(ident, + VIR_IDENTITY_ATTR_UNIX_USER_NAME, + username); +} + + +int virIdentitySetUNIXUserID(virIdentityPtr ident, + uid_t uid) +{ + char *val; + int ret; + if (virAsprintf(&val, "%d", (int)uid) < 0) + return -1; + ret = virIdentitySetAttr(ident, + VIR_IDENTITY_ATTR_UNIX_USER_ID, + val); + VIR_FREE(val); + return ret; +} + + +int virIdentitySetUNIXGroupName(virIdentityPtr ident, + const char *groupname) +{ + return virIdentitySetAttr(ident, + VIR_IDENTITY_ATTR_UNIX_GROUP_NAME, + groupname); +} + + +int virIdentitySetUNIXGroupID(virIdentityPtr ident, + gid_t gid) +{ + char *val; + int ret; + if (virAsprintf(&val, "%d", (int)gid) < 0) + return -1; + ret = virIdentitySetAttr(ident, + VIR_IDENTITY_ATTR_UNIX_GROUP_ID, + val); + VIR_FREE(val); + return ret; +} + + +int virIdentitySetUNIXProcessID(virIdentityPtr ident, + pid_t pid) +{ + char *val; + int ret; + if (virAsprintf(&val, "%llu", (unsigned long long)pid) < 0) + return -1; + ret = virIdentitySetAttr(ident, + VIR_IDENTITY_ATTR_UNIX_PROCESS_ID, + val); + VIR_FREE(val); + return ret; +} + + +int virIdentitySetUNIXProcessTime(virIdentityPtr ident, + unsigned long long timestamp) +{ + char *val; + int ret; + if (virAsprintf(&val, "%llu", timestamp) < 0) + return -1; + ret = virIdentitySetAttr(ident, + VIR_IDENTITY_ATTR_UNIX_PROCESS_TIME, + val); + VIR_FREE(val); + return ret; +} + + + +int virIdentitySetSASLUserName(virIdentityPtr ident, + const char *username) +{ + return virIdentitySetAttr(ident, + VIR_IDENTITY_ATTR_SASL_USER_NAME, + username); +} + + +int virIdentitySetX509DName(virIdentityPtr ident, + const char *dname) +{ + return virIdentitySetAttr(ident, + VIR_IDENTITY_ATTR_X509_DISTINGUISHED_NAME, + dname); +} + + +int virIdentitySetSELinuxContext(virIdentityPtr ident, + const char *context) +{ + return virIdentitySetAttr(ident, + VIR_IDENTITY_ATTR_SELINUX_CONTEXT, + context); +} diff --git a/src/util/viridentity.h b/src/util/viridentity.h index a240c2d..63aa63d 100644 --- a/src/util/viridentity.h +++ b/src/util/viridentity.h @@ -65,4 +65,44 @@ bool virIdentityIsEqual(virIdentityPtr identA, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int virIdentityGetUNIXUserName(virIdentityPtr ident, + const char **username); +int virIdentityGetUNIXUserID(virIdentityPtr ident, + uid_t *uid); +int virIdentityGetUNIXGroupName(virIdentityPtr ident, + const char **groupname); +int virIdentityGetUNIXGroupID(virIdentityPtr ident, + gid_t *gid); +int virIdentityGetUNIXProcessID(virIdentityPtr ident, + pid_t *pid); +int virIdentityGetUNIXProcessTime(virIdentityPtr ident, + unsigned long long *timestamp); +int virIdentityGetSASLUserName(virIdentityPtr ident, + const char **username); +int virIdentityGetX509DName(virIdentityPtr ident, + const char **dname); +int virIdentityGetSELinuxContext(virIdentityPtr ident, + const char **context); + + +int virIdentitySetUNIXUserName(virIdentityPtr ident, + const char *username); +int virIdentitySetUNIXUserID(virIdentityPtr ident, + uid_t uid); +int virIdentitySetUNIXGroupName(virIdentityPtr ident, + const char *groupname); +int virIdentitySetUNIXGroupID(virIdentityPtr ident, + gid_t gid); +int virIdentitySetUNIXProcessID(virIdentityPtr ident, + pid_t pid); +int virIdentitySetUNIXProcessTime(virIdentityPtr ident, + unsigned long long timestamp); +int virIdentitySetSASLUserName(virIdentityPtr ident, + const char *username); +int virIdentitySetX509DName(virIdentityPtr ident, + const char *dname); +int virIdentitySetSELinuxContext(virIdentityPtr ident, + const char *context); + + #endif /* __VIR_IDENTITY_H__ */ -- 1.9.3
Update virNetServerClientCreateIdentity and virIdentityGetSystem to use the new typesafe APIs for setting identity attributes Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/rpc/virnetserverclient.c | 115 ++++++++++--------------------------------- src/util/viridentity.c | 79 ++++++++--------------------- 2 files changed, 46 insertions(+), 148 deletions(-) diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 3493ef5..c6ef84c 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -653,21 +653,14 @@ int virNetServerClientGetUNIXIdentity(virNetServerClientPtr client, static virIdentityPtr virNetServerClientCreateIdentity(virNetServerClientPtr client) { - char *processid = NULL; - char *processtime = NULL; char *username = NULL; - char *userid = NULL; char *groupname = NULL; - char *groupid = NULL; -#if WITH_SASL - char *saslname = NULL; -#endif -#if WITH_GNUTLS - char *x509dname = NULL; -#endif char *seccontext = NULL; virIdentityPtr ret = NULL; + if (!(ret = virIdentityNew())) + goto error; + if (client->sock && virNetSocketIsLocal(client->sock)) { gid_t gid; uid_t uid; @@ -676,116 +669,60 @@ virNetServerClientCreateIdentity(virNetServerClientPtr client) if (virNetSocketGetUNIXIdentity(client->sock, &uid, &gid, &pid, ×tamp) < 0) - goto cleanup; + goto error; if (!(username = virGetUserName(uid))) - goto cleanup; - if (virAsprintf(&userid, "%d", (int)uid) < 0) - goto cleanup; + goto error; + if (virIdentitySetUNIXUserName(ret, username) < 0) + goto error; + if (virIdentitySetUNIXUserID(ret, uid) < 0) + goto error; + if (!(groupname = virGetGroupName(gid))) - goto cleanup; - if (virAsprintf(&groupid, "%d", (int)gid) < 0) - goto cleanup; - if (virAsprintf(&processid, "%llu", - (unsigned long long)pid) < 0) - goto cleanup; - if (virAsprintf(&processtime, "%llu", - timestamp) < 0) - goto cleanup; + goto error; + if (virIdentitySetUNIXGroupName(ret, groupname) < 0) + goto error; + if (virIdentitySetUNIXGroupID(ret, gid) < 0) + goto error; + + if (virIdentitySetUNIXProcessID(ret, pid) < 0) + goto error; + if (virIdentitySetUNIXProcessTime(ret, timestamp) < 0) + goto error; } #if WITH_SASL if (client->sasl) { const char *identity = virNetSASLSessionGetIdentity(client->sasl); - if (VIR_STRDUP(saslname, identity) < 0) - goto cleanup; + if (virIdentitySetSASLUserName(ret, identity) < 0) + goto error; } #endif #if WITH_GNUTLS if (client->tls) { const char *identity = virNetTLSSessionGetX509DName(client->tls); - if (VIR_STRDUP(x509dname, identity) < 0) - goto cleanup; + if (virIdentitySetX509DName(ret, identity) < 0) + goto error; } #endif if (client->sock && virNetSocketGetSELinuxContext(client->sock, &seccontext) < 0) - goto cleanup; - - if (!(ret = virIdentityNew())) - goto cleanup; - - if (username && - virIdentitySetAttr(ret, - VIR_IDENTITY_ATTR_UNIX_USER_NAME, - username) < 0) - goto error; - if (userid && - virIdentitySetAttr(ret, - VIR_IDENTITY_ATTR_UNIX_USER_ID, - userid) < 0) - goto error; - if (groupname && - virIdentitySetAttr(ret, - VIR_IDENTITY_ATTR_UNIX_GROUP_NAME, - groupname) < 0) - goto error; - if (groupid && - virIdentitySetAttr(ret, - VIR_IDENTITY_ATTR_UNIX_GROUP_ID, - groupid) < 0) - goto error; - if (processid && - virIdentitySetAttr(ret, - VIR_IDENTITY_ATTR_UNIX_PROCESS_ID, - processid) < 0) - goto error; - if (processtime && - virIdentitySetAttr(ret, - VIR_IDENTITY_ATTR_UNIX_PROCESS_TIME, - processtime) < 0) goto error; -#if WITH_SASL - if (saslname && - virIdentitySetAttr(ret, - VIR_IDENTITY_ATTR_SASL_USER_NAME, - saslname) < 0) - goto error; -#endif -#if WITH_GNUTLS - if (x509dname && - virIdentitySetAttr(ret, - VIR_IDENTITY_ATTR_X509_DISTINGUISHED_NAME, - x509dname) < 0) - goto error; -#endif if (seccontext && - virIdentitySetAttr(ret, - VIR_IDENTITY_ATTR_SELINUX_CONTEXT, - seccontext) < 0) + virIdentitySetSELinuxContext(ret, seccontext) < 0) goto error; cleanup: VIR_FREE(username); - VIR_FREE(userid); VIR_FREE(groupname); - VIR_FREE(groupid); - VIR_FREE(processid); - VIR_FREE(processtime); VIR_FREE(seccontext); -#if WITH_SASL - VIR_FREE(saslname); -#endif -#if WITH_GNUTLS - VIR_FREE(x509dname); -#endif return ret; error: virObjectUnref(ret); - ret = NULL; + ret = 0; goto cleanup; } diff --git a/src/util/viridentity.c b/src/util/viridentity.c index 68ccab9..6f3baee 100644 --- a/src/util/viridentity.c +++ b/src/util/viridentity.c @@ -135,38 +135,38 @@ int virIdentitySetCurrent(virIdentityPtr ident) virIdentityPtr virIdentityGetSystem(void) { char *username = NULL; - char *userid = NULL; char *groupname = NULL; - char *groupid = NULL; - char *seccontext = NULL; + unsigned long long startTime; virIdentityPtr ret = NULL; #if WITH_SELINUX security_context_t con; #endif - char *processid = NULL; - unsigned long long timestamp; - char *processtime = NULL; - if (virAsprintf(&processid, "%llu", - (unsigned long long)getpid()) < 0) - goto cleanup; + if (!(ret = virIdentityNew())) + goto error; - if (virProcessGetStartTime(getpid(), ×tamp) < 0) - goto cleanup; + if (virIdentitySetUNIXProcessID(ret, getpid()) < 0) + goto error; - if (timestamp != 0 && - virAsprintf(&processtime, "%llu", timestamp) < 0) - goto cleanup; + if (virProcessGetStartTime(getpid(), &startTime) < 0) + goto error; + if (startTime != 0 && + virIdentitySetUNIXProcessTime(ret, startTime) < 0) + goto error; if (!(username = virGetUserName(geteuid()))) goto cleanup; - if (virAsprintf(&userid, "%d", (int)geteuid()) < 0) - goto cleanup; + if (virIdentitySetUNIXUserName(ret, username) < 0) + goto error; + if (virIdentitySetUNIXUserID(ret, getuid()) < 0) + goto error; if (!(groupname = virGetGroupName(getegid()))) goto cleanup; - if (virAsprintf(&groupid, "%d", (int)getegid()) < 0) - goto cleanup; + if (virIdentitySetUNIXGroupName(ret, groupname) < 0) + goto error; + if (virIdentitySetUNIXGroupID(ret, getgid()) < 0) + goto error; #if WITH_SELINUX if (is_selinux_enabled() > 0) { @@ -175,56 +175,17 @@ virIdentityPtr virIdentityGetSystem(void) _("Unable to lookup SELinux process context")); goto cleanup; } - if (VIR_STRDUP(seccontext, con) < 0) { + if (virIdentitySetSELinuxContext(ret, con) < 0) { freecon(con); - goto cleanup; + goto error; } freecon(con); } #endif - if (!(ret = virIdentityNew())) - goto cleanup; - - if (virIdentitySetAttr(ret, - VIR_IDENTITY_ATTR_UNIX_USER_NAME, - username) < 0) - goto error; - if (virIdentitySetAttr(ret, - VIR_IDENTITY_ATTR_UNIX_USER_ID, - userid) < 0) - goto error; - if (virIdentitySetAttr(ret, - VIR_IDENTITY_ATTR_UNIX_GROUP_NAME, - groupname) < 0) - goto error; - if (virIdentitySetAttr(ret, - VIR_IDENTITY_ATTR_UNIX_GROUP_ID, - groupid) < 0) - goto error; - if (seccontext && - virIdentitySetAttr(ret, - VIR_IDENTITY_ATTR_SELINUX_CONTEXT, - seccontext) < 0) - goto error; - if (virIdentitySetAttr(ret, - VIR_IDENTITY_ATTR_UNIX_PROCESS_ID, - processid) < 0) - goto error; - if (processtime && - virIdentitySetAttr(ret, - VIR_IDENTITY_ATTR_UNIX_PROCESS_TIME, - processtime) < 0) - goto error; - cleanup: VIR_FREE(username); - VIR_FREE(userid); VIR_FREE(groupname); - VIR_FREE(groupid); - VIR_FREE(seccontext); - VIR_FREE(processid); - VIR_FREE(processtime); return ret; error: -- 1.9.3
Convert virAccessDriverPolkitFormatProcess to use typesafe API for getting process ID attribute. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/access/viraccessdriverpolkit.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/src/access/viraccessdriverpolkit.c b/src/access/viraccessdriverpolkit.c index 8ffb828..bdb35db 100644 --- a/src/access/viraccessdriverpolkit.c +++ b/src/access/viraccessdriverpolkit.c @@ -75,9 +75,9 @@ static char * virAccessDriverPolkitFormatProcess(const char *actionid) { virIdentityPtr identity = virIdentityGetCurrent(); - const char *callerPid = NULL; - const char *callerTime = NULL; - const char *callerUid = NULL; + pid_t pid; + unsigned long long startTime; + uid_t uid; char *ret = NULL; #ifndef PKCHECK_SUPPORTS_UID static bool polkitInsecureWarned; @@ -89,39 +89,35 @@ virAccessDriverPolkitFormatProcess(const char *actionid) actionid); return NULL; } - if (virIdentityGetAttr(identity, VIR_IDENTITY_ATTR_UNIX_PROCESS_ID, &callerPid) < 0) + if (virIdentityGetUNIXProcessID(identity, &pid) < 0) goto cleanup; - if (virIdentityGetAttr(identity, VIR_IDENTITY_ATTR_UNIX_PROCESS_TIME, &callerTime) < 0) + if (virIdentityGetUNIXProcessTime(identity, &startTime) < 0) goto cleanup; - if (virIdentityGetAttr(identity, VIR_IDENTITY_ATTR_UNIX_USER_ID, &callerUid) < 0) + if (virIdentityGetUNIXUserID(identity, &uid) < 0) goto cleanup; - if (!callerPid) { + if (!pid) { virAccessError(VIR_ERR_INTERNAL_ERROR, "%s", _("No UNIX process ID available")); goto cleanup; } - if (!callerTime) { + if (!startTime) { virAccessError(VIR_ERR_INTERNAL_ERROR, "%s", _("No UNIX process start time available")); goto cleanup; } - if (!callerUid) { - virAccessError(VIR_ERR_INTERNAL_ERROR, "%s", - _("No UNIX caller UID available")); - goto cleanup; - } #ifdef PKCHECK_SUPPORTS_UID - if (virAsprintf(&ret, "%s,%s,%s", callerPid, callerTime, callerUid) < 0) + if (virAsprintf(&ret, "%llu,%llu,%llu", + (unsigned long long)pid, startTime, (unsigned long long)uid) < 0) goto cleanup; #else if (!polkitInsecureWarned) { - VIR_WARN("No support for caller UID with pkcheck. " - "This deployment is known to be insecure."); + VIR_WARN("No support for caller UID with pkcheck. This deployment is known to be insecure."); polkitInsecureWarned = true; } - if (virAsprintf(&ret, "%s,%s", callerPid, callerTime) < 0) + if (virAsprintf(&ret, "%llu,%llu", + (unsigned long long)pid, startTime) < 0) goto cleanup; #endif -- 1.9.3
Convert the remote daemon auth check and the access control code to use the common polkit API for checking auth. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- daemon/remote.c | 235 ++----------------------------------- src/access/viraccessdriverpolkit.c | 91 ++++++-------- 2 files changed, 45 insertions(+), 281 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 0ea2815..5548d98 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -24,11 +24,6 @@ #include "virerror.h" -#if WITH_POLKIT0 -# include <polkit/polkit.h> -# include <polkit-dbus/polkit-dbus.h> -#endif - #include "remote.h" #include "libvirtd.h" #include "libvirt_internal.h" @@ -55,6 +50,7 @@ #include "virprobe.h" #include "viraccessapicheck.h" #include "viraccessapicheckqemu.h" +#include "virpolkit.h" #define VIR_FROM_THIS VIR_FROM_RPC @@ -3185,7 +3181,6 @@ remoteDispatchAuthSaslStep(virNetServerPtr server ATTRIBUTE_UNUSED, -#if WITH_POLKIT1 static int remoteDispatchAuthPolkit(virNetServerPtr server, virNetServerClientPtr client, @@ -3198,26 +3193,16 @@ remoteDispatchAuthPolkit(virNetServerPtr server, uid_t callerUid = -1; unsigned long long timestamp; const char *action; - int status = -1; char *ident = NULL; - bool authdismissed = 0; - char *pkout = NULL; struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); - virCommandPtr cmd = NULL; -# ifndef PKCHECK_SUPPORTS_UID - static bool polkitInsecureWarned; -# endif + int rv; virMutexLock(&priv->lock); action = virNetServerClientGetReadonly(client) ? "org.libvirt.unix.monitor" : "org.libvirt.unix.manage"; - cmd = virCommandNewArgList(PKCHECK_PATH, "--action-id", action, NULL); - virCommandSetOutputBuffer(cmd, &pkout); - virCommandSetErrorBuffer(cmd, &pkout); - VIR_DEBUG("Start PolicyKit auth %d", virNetServerClientGetFD(client)); if (virNetServerClientGetAuth(client) != VIR_NET_SERVER_SERVICE_AUTH_POLKIT) { VIR_ERROR(_("client tried invalid PolicyKit init request")); @@ -3238,38 +3223,17 @@ remoteDispatchAuthPolkit(virNetServerPtr server, VIR_INFO("Checking PID %lld running as %d", (long long) callerPid, callerUid); - virCommandAddArg(cmd, "--process"); - -# ifdef PKCHECK_SUPPORTS_UID - virCommandAddArgFormat(cmd, "%lld,%llu,%lu", - (long long) callerPid, - timestamp, - (unsigned long) callerUid); -# else - if (!polkitInsecureWarned) { - VIR_WARN("No support for caller UID with pkcheck. " - "This deployment is known to be insecure."); - polkitInsecureWarned = true; - } - virCommandAddArgFormat(cmd, "%lld,%llu", (long long) callerPid, timestamp); -# endif - - virCommandAddArg(cmd, "--allow-user-interaction"); - - if (virAsprintf(&ident, "pid:%lld,uid:%d", - (long long) callerPid, callerUid) < 0) - goto authfail; - - if (virCommandRun(cmd, &status) < 0) + rv = virPolkitCheckAuth(action, + callerPid, + timestamp, + callerUid, + NULL, + true); + if (rv == -1) goto authfail; - - authdismissed = (pkout && strstr(pkout, "dismissed=true")); - if (status != 0) { - VIR_ERROR(_("Policy kit denied action %s from pid %lld, uid %d " - "with status %d"), - action, (long long) callerPid, callerUid, status); + else if (rv == -2) goto authdeny; - } + PROBE(RPC_SERVER_CLIENT_AUTH_ALLOW, "client=%p auth=%d identity=%s", client, REMOTE_AUTH_POLKIT, ident); @@ -3280,170 +3244,10 @@ remoteDispatchAuthPolkit(virNetServerPtr server, virNetServerClientSetAuth(client, 0); virNetServerTrackCompletedAuth(server); virMutexUnlock(&priv->lock); - virCommandFree(cmd); - VIR_FREE(pkout); - VIR_FREE(ident); - - return 0; - - error: - virCommandFree(cmd); - VIR_FREE(ident); - virResetLastError(); - - if (authdismissed) { - virReportError(VIR_ERR_AUTH_CANCELLED, "%s", - _("authentication cancelled by user")); - } else if (pkout && *pkout) { - virReportError(VIR_ERR_AUTH_FAILED, _("polkit: %s"), pkout); - } else { - virReportError(VIR_ERR_AUTH_FAILED, "%s", _("authentication failed")); - } - - VIR_FREE(pkout); - virNetMessageSaveError(rerr); - virMutexUnlock(&priv->lock); - return -1; - - authfail: - PROBE(RPC_SERVER_CLIENT_AUTH_FAIL, - "client=%p auth=%d", - client, REMOTE_AUTH_POLKIT); - goto error; - - authdeny: - PROBE(RPC_SERVER_CLIENT_AUTH_DENY, - "client=%p auth=%d identity=%s", - client, REMOTE_AUTH_POLKIT, ident); - goto error; -} -#elif WITH_POLKIT0 -static int -remoteDispatchAuthPolkit(virNetServerPtr server, - virNetServerClientPtr client, - virNetMessagePtr msg ATTRIBUTE_UNUSED, - virNetMessageErrorPtr rerr, - remote_auth_polkit_ret *ret) -{ - pid_t callerPid; - gid_t callerGid; - uid_t callerUid; - PolKitCaller *pkcaller = NULL; - PolKitAction *pkaction = NULL; - PolKitContext *pkcontext = NULL; - PolKitError *pkerr = NULL; - PolKitResult pkresult; - DBusError err; - const char *action; - char *ident = NULL; - struct daemonClientPrivate *priv = - virNetServerClientGetPrivateData(client); - DBusConnection *sysbus; - unsigned long long timestamp; - - virMutexLock(&priv->lock); - action = virNetServerClientGetReadonly(client) ? - "org.libvirt.unix.monitor" : - "org.libvirt.unix.manage"; - - VIR_DEBUG("Start PolicyKit auth %d", virNetServerClientGetFD(client)); - if (virNetServerClientGetAuth(client) != VIR_NET_SERVER_SERVICE_AUTH_POLKIT) { - VIR_ERROR(_("client tried invalid PolicyKit init request")); - goto authfail; - } - - if (virNetServerClientGetUNIXIdentity(client, &callerUid, &callerGid, - &callerPid, ×tamp) < 0) { - VIR_ERROR(_("cannot get peer socket identity")); - goto authfail; - } - - if (virAsprintf(&ident, "pid:%lld,uid:%d", - (long long) callerPid, callerUid) < 0) - goto authfail; - - if (!(sysbus = virDBusGetSystemBus())) - goto authfail; - - VIR_INFO("Checking PID %lld running as %d", - (long long) callerPid, callerUid); - dbus_error_init(&err); - if (!(pkcaller = polkit_caller_new_from_pid(sysbus, - callerPid, &err))) { - VIR_ERROR(_("Failed to lookup policy kit caller: %s"), err.message); - dbus_error_free(&err); - goto authfail; - } - - if (!(pkaction = polkit_action_new())) { - char ebuf[1024]; - VIR_ERROR(_("Failed to create polkit action %s"), - virStrerror(errno, ebuf, sizeof(ebuf))); - polkit_caller_unref(pkcaller); - goto authfail; - } - polkit_action_set_action_id(pkaction, action); - - if (!(pkcontext = polkit_context_new()) || - !polkit_context_init(pkcontext, &pkerr)) { - char ebuf[1024]; - VIR_ERROR(_("Failed to create polkit context %s"), - (pkerr ? polkit_error_get_error_message(pkerr) - : virStrerror(errno, ebuf, sizeof(ebuf)))); - if (pkerr) - polkit_error_free(pkerr); - polkit_caller_unref(pkcaller); - polkit_action_unref(pkaction); - dbus_error_free(&err); - goto authfail; - } - -# if HAVE_POLKIT_CONTEXT_IS_CALLER_AUTHORIZED - pkresult = polkit_context_is_caller_authorized(pkcontext, - pkaction, - pkcaller, - 0, - &pkerr); - if (pkerr && polkit_error_is_set(pkerr)) { - VIR_ERROR(_("Policy kit failed to check authorization %d %s"), - polkit_error_get_error_code(pkerr), - polkit_error_get_error_message(pkerr)); - goto authfail; - } -# else - pkresult = polkit_context_can_caller_do_action(pkcontext, - pkaction, - pkcaller); -# endif - polkit_context_unref(pkcontext); - polkit_caller_unref(pkcaller); - polkit_action_unref(pkaction); - if (pkresult != POLKIT_RESULT_YES) { - VIR_ERROR(_("Policy kit denied action %s from pid %lld, uid %d, result: %s"), - action, (long long) callerPid, callerUid, - polkit_result_to_string_representation(pkresult)); - goto authdeny; - } - PROBE(RPC_SERVER_CLIENT_AUTH_ALLOW, - "client=%p auth=%d identity=%s", - client, REMOTE_AUTH_POLKIT, ident); - VIR_INFO("Policy allowed action %s from pid %lld, uid %d, result %s", - action, (long long) callerPid, callerUid, - polkit_result_to_string_representation(pkresult)); - ret->complete = 1; - - virNetServerClientSetAuth(client, 0); - virNetServerTrackCompletedAuth(server); - virMutexUnlock(&priv->lock); - VIR_FREE(ident); return 0; error: - VIR_FREE(ident); - virResetLastError(); - virReportError(VIR_ERR_AUTH_FAILED, "%s", - _("authentication failed")); virNetMessageSaveError(rerr); virMutexUnlock(&priv->lock); return -1; @@ -3461,23 +3265,6 @@ remoteDispatchAuthPolkit(virNetServerPtr server, goto error; } -#else /* !WITH_POLKIT0 & !HAVE_POLKIT1*/ - -static int -remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, - virNetServerClientPtr client ATTRIBUTE_UNUSED, - virNetMessagePtr msg ATTRIBUTE_UNUSED, - virNetMessageErrorPtr rerr, - remote_auth_polkit_ret *ret ATTRIBUTE_UNUSED) -{ - VIR_ERROR(_("client tried unsupported PolicyKit init request")); - virReportError(VIR_ERR_AUTH_FAILED, "%s", - _("authentication failed")); - virNetMessageSaveError(rerr); - return -1; -} -#endif /* WITH_POLKIT1 */ - /*************************************************************** * NODE INFO APIS diff --git a/src/access/viraccessdriverpolkit.c b/src/access/viraccessdriverpolkit.c index bdb35db..2bc1842 100644 --- a/src/access/viraccessdriverpolkit.c +++ b/src/access/viraccessdriverpolkit.c @@ -26,6 +26,7 @@ #include "virlog.h" #include "virprocess.h" #include "virerror.h" +#include "virpolkit.h" #include "virstring.h" #define VIR_FROM_THIS VIR_FROM_ACCESS @@ -71,29 +72,26 @@ virAccessDriverPolkitFormatAction(const char *typename, } -static char * -virAccessDriverPolkitFormatProcess(const char *actionid) +static int +virAccessDriverPolkitGetCaller(const char *actionid, + pid_t *pid, + unsigned long long *startTime, + uid_t *uid) { virIdentityPtr identity = virIdentityGetCurrent(); - pid_t pid; - unsigned long long startTime; - uid_t uid; - char *ret = NULL; -#ifndef PKCHECK_SUPPORTS_UID - static bool polkitInsecureWarned; -#endif + int ret = -1; if (!identity) { virAccessError(VIR_ERR_ACCESS_DENIED, _("Policy kit denied action %s from <anonymous>"), actionid); - return NULL; + return -1; } - if (virIdentityGetUNIXProcessID(identity, &pid) < 0) + if (virIdentityGetUNIXProcessID(identity, pid) < 0) goto cleanup; - if (virIdentityGetUNIXProcessTime(identity, &startTime) < 0) + if (virIdentityGetUNIXProcessTime(identity, startTime) < 0) goto cleanup; - if (virIdentityGetUNIXUserID(identity, &uid) < 0) + if (virIdentityGetUNIXUserID(identity, uid) < 0) goto cleanup; if (!pid) { @@ -101,25 +99,14 @@ virAccessDriverPolkitFormatProcess(const char *actionid) _("No UNIX process ID available")); goto cleanup; } - if (!startTime) { - virAccessError(VIR_ERR_INTERNAL_ERROR, "%s", - _("No UNIX process start time available")); - goto cleanup; - } -#ifdef PKCHECK_SUPPORTS_UID - if (virAsprintf(&ret, "%llu,%llu,%llu", - (unsigned long long)pid, startTime, (unsigned long long)uid) < 0) + if (virIdentityGetUNIXProcessTime(identity, startTime) < 0) goto cleanup; -#else - if (!polkitInsecureWarned) { - VIR_WARN("No support for caller UID with pkcheck. This deployment is known to be insecure."); - polkitInsecureWarned = true; - } - if (virAsprintf(&ret, "%llu,%llu", - (unsigned long long)pid, startTime) < 0) + + if (virIdentityGetUNIXUserID(identity, uid) < 0) goto cleanup; -#endif + + ret = 0; cleanup: virObjectUnref(identity); @@ -134,53 +121,43 @@ virAccessDriverPolkitCheck(virAccessManagerPtr manager ATTRIBUTE_UNUSED, const char **attrs) { char *actionid = NULL; - char *process = NULL; - virCommandPtr cmd = NULL; - int status; int ret = -1; + pid_t pid; + uid_t uid; + unsigned long long startTime; + int rv; if (!(actionid = virAccessDriverPolkitFormatAction(typename, permname))) goto cleanup; - if (!(process = virAccessDriverPolkitFormatProcess(actionid))) + if (virAccessDriverPolkitGetCaller(actionid, + &pid, + &startTime, + &uid) < 0) goto cleanup; - VIR_DEBUG("Check action '%s' for process '%s'", actionid, process); + VIR_DEBUG("Check action '%s' for process '%d' time %lld uid %d", + actionid, pid, startTime, uid); - cmd = virCommandNewArgList(PKCHECK_PATH, - "--action-id", actionid, - "--process", process, - NULL); + rv = virPolkitCheckAuth(actionid, + pid, + startTime, + uid, + attrs, + false); - while (attrs && attrs[0] && attrs[1]) { - virCommandAddArgList(cmd, "--detail", attrs[0], attrs[1], NULL); - attrs += 2; - } - - if (virCommandRun(cmd, &status) < 0) - goto cleanup; - - if (status == 0) { + if (rv == 0) { ret = 1; /* Allowed */ } else { - if (status == 1 || - status == 2 || - status == 3) { + if (rv == -2) { ret = 0; /* Denied */ } else { ret = -1; /* Error */ - virAccessError(VIR_ERR_ACCESS_DENIED, - _("Policy kit denied action %s from %s: " - "exit status %d"), - actionid, process, status); } - goto cleanup; } cleanup: - virCommandFree(cmd); VIR_FREE(actionid); - VIR_FREE(process); return ret; } -- 1.9.3
On 09/10/2014 10:20 AM, Daniel P. Berrange wrote:
Convert the remote daemon auth check and the access control code to use the common polkit API for checking auth.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- daemon/remote.c | 235 ++----------------------------------- src/access/viraccessdriverpolkit.c | 91 ++++++-------- 2 files changed, 45 insertions(+), 281 deletions(-)
<...snip...> Since this was pushed yesterday - my morning Coverity picked up on an issue this morning...
-static char * -virAccessDriverPolkitFormatProcess(const char *actionid) +static int +virAccessDriverPolkitGetCaller(const char *actionid, + pid_t *pid, + unsigned long long *startTime, + uid_t *uid) { virIdentityPtr identity = virIdentityGetCurrent(); - pid_t pid; - unsigned long long startTime; - uid_t uid; - char *ret = NULL; -#ifndef PKCHECK_SUPPORTS_UID - static bool polkitInsecureWarned; -#endif + int ret = -1;
if (!identity) { virAccessError(VIR_ERR_ACCESS_DENIED, _("Policy kit denied action %s from <anonymous>"), actionid); - return NULL; + return -1; } - if (virIdentityGetUNIXProcessID(identity, &pid) < 0) + if (virIdentityGetUNIXProcessID(identity, pid) < 0)
(1) Event deref_ptr_in_call: Dereferencing pointer "pid". [details] Also see events: [check_after_deref]
goto cleanup; - if (virIdentityGetUNIXProcessTime(identity, &startTime) < 0) + if (virIdentityGetUNIXProcessTime(identity, startTime) < 0) goto cleanup; - if (virIdentityGetUNIXUserID(identity, &uid) < 0) + if (virIdentityGetUNIXUserID(identity, uid) < 0) goto cleanup;
if (!pid) {
(2) Event check_after_deref: Null-checking "pid" suggests that it may be null, but it has already been dereferenced on all paths leading to the check. Also see events: [deref_ptr_in_call] Should this reference now be "(!*pid)" ? John
@@ -101,25 +99,14 @@ virAccessDriverPolkitFormatProcess(const char *actionid) _("No UNIX process ID available")); goto cleanup; } - if (!startTime) { - virAccessError(VIR_ERR_INTERNAL_ERROR, "%s", - _("No UNIX process start time available")); - goto cleanup; - }
-#ifdef PKCHECK_SUPPORTS_UID - if (virAsprintf(&ret, "%llu,%llu,%llu", - (unsigned long long)pid, startTime, (unsigned long long)uid) < 0) + if (virIdentityGetUNIXProcessTime(identity, startTime) < 0) goto cleanup; -#else - if (!polkitInsecureWarned) { - VIR_WARN("No support for caller UID with pkcheck. This deployment is known to be insecure."); - polkitInsecureWarned = true; - } - if (virAsprintf(&ret, "%llu,%llu", - (unsigned long long)pid, startTime) < 0) + + if (virIdentityGetUNIXUserID(identity, uid) < 0) goto cleanup; -#endif + + ret = 0;
cleanup: virObjectUnref(identity);
On 09/25/2014 07:30 AM, John Ferlan wrote:
On 09/10/2014 10:20 AM, Daniel P. Berrange wrote:
Convert the remote daemon auth check and the access control code to use the common polkit API for checking auth.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- daemon/remote.c | 235 ++----------------------------------- src/access/viraccessdriverpolkit.c | 91 ++++++-------- 2 files changed, 45 insertions(+), 281 deletions(-)
<...snip...>
Since this was pushed yesterday - my morning Coverity picked up on an issue this morning...
Should have gone through the rest of my libvir-list backlog as I see Pavel already sent patches on this... John
Currently DBus dict values must be passed inline virDBusMessageEncode("a{ss}", 3, "key1", "val1", "key2", "val2", "key3", "val3"); virDBusMessageDecode("a{ss}", 3, &key1, &val1, &key2, &val2, &key3, &val3); This allows them to be passed by reference const char **dictin = { "key1", "val1", "key2", "val2", "key3", "val3" }; char **dictout; size_t ndictout; virDBusMessageEncode("a&{ss}", ARRAY_CARDINALITY(dict) / 2, dictin); virDBusMessageDecode("a&{ss}", &ndictout, &dictout); Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virdbus.c | 274 +++++++++++++++++++++++++++++++++++---------------- src/util/virstring.c | 14 +++ src/util/virstring.h | 2 + tests/virdbustest.c | 218 +++++++++++++++++++++++++++++++++++++++- 4 files changed, 423 insertions(+), 85 deletions(-) diff --git a/src/util/virdbus.c b/src/util/virdbus.c index a63338a..dc3a535 100644 --- a/src/util/virdbus.c +++ b/src/util/virdbus.c @@ -313,15 +313,18 @@ virDBusSignatureLengthInternal(const char *s, bool allowDict, unsigned arrayDepth, unsigned structDepth, - size_t *l) + size_t *skiplen, + size_t *siglen) { if (virDBusIsBasicType(*s) || *s == DBUS_TYPE_VARIANT) { - *l = 1; + *skiplen = *siglen = 1; return 0; } if (*s == DBUS_TYPE_ARRAY) { - size_t t; + size_t skiplencont; + size_t siglencont; + bool arrayref = false; if (arrayDepth >= VIR_DBUS_TYPE_STACK_MAX_DEPTH) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -330,14 +333,23 @@ virDBusSignatureLengthInternal(const char *s, return -1; } + if (*(s + 1) == '&') { + arrayref = true; + s++; + } + if (virDBusSignatureLengthInternal(s + 1, true, arrayDepth + 1, structDepth, - &t) < 0) + &skiplencont, + &siglencont) < 0) return -1; - *l = t + 1; + *skiplen = skiplencont + 1; + *siglen = siglencont + 1; + if (arrayref) + (*skiplen)++; return 0; } @@ -351,20 +363,25 @@ virDBusSignatureLengthInternal(const char *s, return -1; } + *skiplen = *siglen = 2; + while (*p != DBUS_STRUCT_END_CHAR) { - size_t t; + size_t skiplencont; + size_t siglencont; if (virDBusSignatureLengthInternal(p, false, arrayDepth, structDepth + 1, - &t) < 0) + &skiplencont, + &siglencont) < 0) return -1; - p += t; + p += skiplencont; + *skiplen += skiplencont; + *siglen += siglencont; } - *l = p - s + 1; return 0; } @@ -378,8 +395,11 @@ virDBusSignatureLengthInternal(const char *s, return -1; } + *skiplen = *siglen = 2; + while (*p != DBUS_DICT_ENTRY_END_CHAR) { - size_t t; + size_t skiplencont; + size_t siglencont; if (n == 0 && !virDBusIsBasicType(*p)) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -392,10 +412,13 @@ virDBusSignatureLengthInternal(const char *s, false, arrayDepth, structDepth + 1, - &t) < 0) + &skiplencont, + &siglencont) < 0) return -1; - p += t; + p += skiplencont; + *skiplen += skiplencont; + *siglen += siglencont; n++; } @@ -406,7 +429,6 @@ virDBusSignatureLengthInternal(const char *s, return -1; } - *l = p - s + 1; return 0; } @@ -416,12 +438,40 @@ virDBusSignatureLengthInternal(const char *s, } -static int virDBusSignatureLength(const char *s, size_t *l) +static int virDBusSignatureLength(const char *s, size_t *skiplen, size_t *siglen) { - return virDBusSignatureLengthInternal(s, true, 0, 0, l); + return virDBusSignatureLengthInternal(s, true, 0, 0, skiplen, siglen); } +static char *virDBusCopyContainerSignature(const char *sig, + size_t *skiplen, + size_t *siglen) +{ + size_t i, j; + char *contsig; + bool isGroup; + + isGroup = (sig[0] == DBUS_STRUCT_BEGIN_CHAR || + sig[0] == DBUS_DICT_ENTRY_BEGIN_CHAR); + + if (virDBusSignatureLength(isGroup ? sig : sig + 1, skiplen, siglen) < 0) + return NULL; + + if (VIR_ALLOC_N(contsig, *siglen + 1) < 0) + return NULL; + + for (i = 0, j = 0; i < *skiplen && j < *siglen; i++) { + if (sig[i + 1] == '&') + continue; + contsig[j] = sig[i + 1]; + j++; + } + contsig[*siglen] = '\0'; + VIR_DEBUG("Extracted '%s' from '%s'", contsig, sig); + return contsig; +} + /* Ideally, we'd just call ourselves recursively on every * complex type. However, the state of a va_list that is @@ -458,7 +508,8 @@ static int virDBusTypeStackPush(virDBusTypeStack **stack, (*stack)[(*nstack) - 1].types = types; (*stack)[(*nstack) - 1].nstruct = nstruct; (*stack)[(*nstack) - 1].narray = narray; - VIR_DEBUG("Pushed types='%s' nstruct=%zu narray=%zu", types, nstruct, narray); + VIR_DEBUG("Pushed types='%s' nstruct=%zu narray=%zd", + types, nstruct, (ssize_t)narray); return 0; } @@ -480,7 +531,8 @@ static int virDBusTypeStackPop(virDBusTypeStack **stack, *types = (*stack)[(*nstack) - 1].types; *nstruct = (*stack)[(*nstack) - 1].nstruct; *narray = (*stack)[(*nstack) - 1].narray; - VIR_DEBUG("Popped types='%s' nstruct=%zu narray=%zu", *types, *nstruct, *narray); + VIR_DEBUG("Popped types='%s' nstruct=%zu narray=%zd", + *types, *nstruct, (ssize_t)*narray); VIR_SHRINK_N(*stack, *nstack, 1); return 0; @@ -501,6 +553,28 @@ static void virDBusTypeStackFree(virDBusTypeStack **stack, } +static bool +virDBusIsAllowedRefType(const char *sig) +{ + if (*sig == '{') { + if (strlen(sig) != 4) + return false; + if (!virDBusIsBasicType(sig[1]) || + !virDBusIsBasicType(sig[2]) || + sig[1] != sig[2]) + return false; + if (sig[3] != '}') + return false; + } else { + if (strlen(sig) != 1) + return false; + if (!virDBusIsBasicType(sig[0])) + return false; + } + return true; +} + + # define SET_NEXT_VAL(dbustype, vargtype, sigtype, fmt) \ do { \ dbustype x; \ @@ -535,6 +609,7 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter, virDBusTypeStack *stack = NULL; size_t nstack = 0; size_t siglen; + size_t skiplen; char *contsig = NULL; const char *vsig; DBusMessageIter *newiter = NULL; @@ -551,14 +626,17 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter, for (;;) { const char *t; - VIR_DEBUG("Loop stack=%zu array=%zu struct=%zu type='%s'", - nstack, narray, nstruct, types); + VIR_DEBUG("Loop nstack=%zu narray=%zd nstruct=%zu types='%s'", + nstack, (ssize_t)narray, nstruct, types); if (narray == 0 || (narray == (size_t)-1 && nstruct == 0)) { DBusMessageIter *thisiter = iter; - arrayref = false; - arrayptr = NULL; + if (*types != '}') { + VIR_DEBUG("Reset array ref"); + arrayref = false; + arrayptr = NULL; + } VIR_DEBUG("Popping iter=%p", iter); if (nstack == 0) break; @@ -643,28 +721,25 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter, arrayref = false; } - if (virDBusSignatureLength(t + 1, &siglen) < 0) + if (!(contsig = virDBusCopyContainerSignature(t, &skiplen, &siglen))) goto cleanup; - if (VIR_STRNDUP(contsig, t + 1, siglen) < 0) - goto cleanup; - - if (arrayref && (strlen(contsig) > 1 || - !virDBusIsBasicType(*contsig))) { + if (arrayref && !virDBusIsAllowedRefType(contsig)) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Got array ref but '%s' is not a single basic type"), + _("Got array ref but '%s' is not a single basic type " + "or dict with matching key+value type"), contsig); goto cleanup; } if (narray == (size_t)-1) { - types += siglen; - nstruct -= siglen; + types += skiplen; + nstruct -= skiplen; } if (VIR_ALLOC(newiter) < 0) goto cleanup; - VIR_DEBUG("Contsig '%s' '%zu'", contsig, siglen); + VIR_DEBUG("Contsig '%s' skip='%zu' len='%zu'", contsig, skiplen, siglen); if (!dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY, contsig, newiter)) goto cleanup; @@ -678,7 +753,7 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter, iter = newiter; newiter = NULL; types = t + 1; - nstruct = siglen; + nstruct = skiplen; narray = (size_t)va_arg(args, int); if (arrayref) arrayptr = va_arg(args, void *); @@ -711,23 +786,20 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter, case DBUS_STRUCT_BEGIN_CHAR: case DBUS_DICT_ENTRY_BEGIN_CHAR: - if (virDBusSignatureLength(t, &siglen) < 0) - goto cleanup; - - if (VIR_STRNDUP(contsig, t + 1, siglen - 1) < 0) + if (!(contsig = virDBusCopyContainerSignature(t, &skiplen, &siglen))) goto cleanup; if (VIR_ALLOC(newiter) < 0) goto cleanup; - VIR_DEBUG("Contsig '%s' '%zu'", contsig, siglen); + VIR_DEBUG("Contsig '%s' skip='%zu' len='%zu'", contsig, skiplen, siglen); if (!dbus_message_iter_open_container(iter, *t == DBUS_STRUCT_BEGIN_CHAR ? DBUS_TYPE_STRUCT : DBUS_TYPE_DICT_ENTRY, NULL, newiter)) goto cleanup; if (narray == (size_t)-1) { - types += siglen - 1; - nstruct -= siglen - 1; + types += skiplen - 1; + nstruct -= skiplen - 1; } if (virDBusTypeStackPush(&stack, &nstack, @@ -740,15 +812,15 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter, iter = newiter; newiter = NULL; types = t + 1; - nstruct = siglen - 2; + nstruct = skiplen - 2; narray = (size_t)-1; break; default: virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unknown type '%c' in signature '%s'"), - *t, types); + _("Unknown type '%x' in signature '%s'"), + (int)*t, types); goto cleanup; } } @@ -779,6 +851,7 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter, do { \ dbustype *x; \ if (arrayref) { \ + VIR_DEBUG("Use arrayref"); \ vargtype **xptrptr = arrayptr; \ if (VIR_EXPAND_N(*xptrptr, *narrayptr, 1) < 0) \ goto cleanup; \ @@ -806,6 +879,7 @@ virDBusMessageIterDecode(DBusMessageIter *rootiter, size_t *narrayptr = 0; virDBusTypeStack *stack = NULL; size_t nstack = 0; + size_t skiplen; size_t siglen; char *contsig = NULL; const char *vsig; @@ -824,14 +898,12 @@ virDBusMessageIterDecode(DBusMessageIter *rootiter, const char *t; bool advanceiter = true; - VIR_DEBUG("Loop stack=%zu array=%zu struct=%zu type='%s'", - nstack, narray, nstruct, types); + VIR_DEBUG("Loop nstack=%zu narray=%zd nstruct=%zu type='%s'", + nstack, (ssize_t)narray, nstruct, types); if (narray == 0 || (narray == (size_t)-1 && nstruct == 0)) { DBusMessageIter *thisiter = iter; - arrayref = false; - arrayptr = NULL; VIR_DEBUG("Popping iter=%p", iter); if (nstack == 0) break; @@ -839,8 +911,20 @@ virDBusMessageIterDecode(DBusMessageIter *rootiter, &types, &nstruct, &narray) < 0) goto cleanup; VIR_DEBUG("Popped iter=%p types=%s", iter, types); + if (strchr(types, '}') == NULL) { + arrayref = false; + arrayptr = NULL; + VIR_DEBUG("Clear array ref flag"); + } if (thisiter != rootiter) VIR_FREE(thisiter); + if (arrayref) { + if (!dbus_message_iter_has_next(iter)) + narray = 0; + else + narray = 1; + VIR_DEBUG("Pop set narray=%zd", (ssize_t)narray); + } if (!(narray == 0 || (narray == (size_t)-1 && nstruct == 0)) && @@ -854,7 +938,8 @@ virDBusMessageIterDecode(DBusMessageIter *rootiter, t = types; if (narray != (size_t)-1) { - narray--; + if (!arrayref) + narray--; } else { types++; nstruct--; @@ -934,28 +1019,25 @@ virDBusMessageIterDecode(DBusMessageIter *rootiter, } advanceiter = false; - if (virDBusSignatureLength(t + 1, &siglen) < 0) - goto cleanup; - - if (VIR_STRNDUP(contsig, t + 1, siglen) < 0) + if (!(contsig = virDBusCopyContainerSignature(t, &skiplen, &siglen))) goto cleanup; - if (arrayref && (strlen(contsig) > 1 || - !virDBusIsBasicType(*contsig))) { + if (arrayref && !virDBusIsAllowedRefType(contsig)) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Got array ref but '%s' is not a single basic type"), + _("Got array ref but '%s' is not a single basic type / dict"), contsig); goto cleanup; } if (narray == (size_t)-1) { - types += siglen; - nstruct -= siglen; + types += skiplen; + nstruct -= skiplen; } if (VIR_ALLOC(newiter) < 0) goto cleanup; - VIR_DEBUG("Contsig '%s' '%zu' '%s'", contsig, siglen, types); + VIR_DEBUG("Array contsig='%s' skip=%'zu' len='%zu' types='%s'", + contsig, skiplen, siglen, types); dbus_message_iter_recurse(iter, newiter); if (virDBusTypeStackPush(&stack, &nstack, iter, types, @@ -965,7 +1047,7 @@ virDBusMessageIterDecode(DBusMessageIter *rootiter, iter = newiter; newiter = NULL; types = t + 1; - nstruct = siglen; + nstruct = skiplen; if (arrayref) { narrayptr = va_arg(args, size_t *); arrayptr = va_arg(args, void *); @@ -1003,19 +1085,17 @@ virDBusMessageIterDecode(DBusMessageIter *rootiter, case DBUS_STRUCT_BEGIN_CHAR: case DBUS_DICT_ENTRY_BEGIN_CHAR: advanceiter = false; - if (virDBusSignatureLength(t, &siglen) < 0) - goto cleanup; - - if (VIR_STRNDUP(contsig, t + 1, siglen - 1) < 0) + if (!(contsig = virDBusCopyContainerSignature(t, &skiplen, &siglen))) goto cleanup; if (VIR_ALLOC(newiter) < 0) goto cleanup; - VIR_DEBUG("Contsig '%s' '%zu'", contsig, siglen); + VIR_DEBUG("Dict/struct contsig='%s' skip='%zu' len='%zu' types='%s'", + contsig, skiplen, siglen, types); dbus_message_iter_recurse(iter, newiter); if (narray == (size_t)-1) { - types += siglen - 1; - nstruct -= siglen - 1; + types += skiplen - 1; + nstruct -= skiplen - 1; } if (virDBusTypeStackPush(&stack, &nstack, @@ -1026,7 +1106,7 @@ virDBusMessageIterDecode(DBusMessageIter *rootiter, iter = newiter; newiter = NULL; types = t + 1; - nstruct = siglen - 2; + nstruct = skiplen - 2; narray = (size_t)-1; break; @@ -1038,24 +1118,32 @@ virDBusMessageIterDecode(DBusMessageIter *rootiter, goto cleanup; } + VIR_DEBUG("After nstack=%zu narray=%zd nstruct=%zu types='%s'", + nstack, (ssize_t)narray, nstruct, types); + if (arrayref) { - if (*t == '&' || - dbus_message_iter_has_next(iter)) - narray = 1; - else + if (dbus_message_iter_get_arg_type(iter) == DBUS_TYPE_INVALID) { narray = 0; - } - - VIR_DEBUG("After stack=%zu array=%zu struct=%zu type='%s'", - nstack, narray, nstruct, types); - if (advanceiter && - !(narray == 0 || - (narray == (size_t)-1 && - nstruct == 0)) && - !dbus_message_iter_next(iter)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Not enough fields in message for signature")); - goto cleanup; + } else { + if (advanceiter) + dbus_message_iter_next(iter); + if (dbus_message_iter_get_arg_type(iter) == DBUS_TYPE_INVALID) { + narray = 0; + } else { + narray = 1; + } + } + VIR_DEBUG("Set narray=%zd", (ssize_t)narray); + } else { + if (advanceiter && + !(narray == 0 || + (narray == (size_t)-1 && + nstruct == 0)) && + !dbus_message_iter_next(iter)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Not enough fields in message for signature")); + goto cleanup; + } } } @@ -1217,7 +1305,27 @@ int virDBusMessageDecode(DBusMessage* msg, * * The first variadic arg for an array, is an 'int' * specifying the number of elements in the array. - * This is then followed by the values for the array + * This is then followed by additional variadic args, + * one for each element of the array. + * + * - Array reference: when 'a' appears in a type signature, + * followed by '&', this signifies an array passed by + * reference. + * + * Array references may only be used when the + * element values are basic types, or a dict + * entry where both keys and values are using + * the same basic type. + * + * The first variadic arg for an array, is an 'int' + * specifying the number of elements in the array. + * When the element is a basic type, the second + * variadic arg is a pointer to an array containing + * the element values. When the element is a dict + * entry, the second variadic arg is a pointer to + * an array containing the dict keys, and the + * third variadic arg is a pointer to an array + * containing the dict values. * * - Struct: when a '(' appears in a type signature, * it must be followed by one or more types describing diff --git a/src/util/virstring.c b/src/util/virstring.c index b14f785..43eab91 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -205,6 +205,20 @@ virStringFreeListCount(char **strings, } +size_t virStringListLen(const char **strings) +{ + size_t i = 0; + + if (!strings) + return 0; + + while (strings[i] != NULL) + i++; + + return i; +} + + bool virStringArrayHasString(char **strings, const char *needle) { diff --git a/src/util/virstring.h b/src/util/virstring.h index 267fbd0..b82ef2a 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -44,6 +44,8 @@ char *virStringJoin(const char **strings, void virStringFreeList(char **strings); void virStringFreeListCount(char **strings, size_t count); +size_t virStringListLen(const char **strings); + bool virStringArrayHasString(char **strings, const char *needle); char *virArgvToString(const char *const *argv); diff --git a/tests/virdbustest.c b/tests/virdbustest.c index 0079b41..002f904 100644 --- a/tests/virdbustest.c +++ b/tests/virdbustest.c @@ -228,6 +228,99 @@ static int testMessageArray(const void *args ATTRIBUTE_UNUSED) return ret; } +static int testMessageEmptyArrayRef(const void *args ATTRIBUTE_UNUSED) +{ + DBusMessage *msg = NULL; + int ret = -1; + const char *in_strv1[] = {}; + size_t out_nstrv1; + char **out_strv1 = NULL; + + if (!(msg = dbus_message_new_method_call("org.libvirt.test", + "/org/libvirt/test", + "org.libvirt.test.astrochicken", + "cluck"))) { + VIR_DEBUG("Failed to allocate method call"); + goto cleanup; + } + + if (virDBusMessageEncode(msg, + "a&s", + 0, in_strv1) < 0) { + VIR_DEBUG("Failed to encode arguments"); + goto cleanup; + } + + if (virDBusMessageDecode(msg, + "a&s", + &out_nstrv1, &out_strv1) < 0) { + VIR_DEBUG("Failed to decode arguments"); + goto cleanup; + } + + + if (out_nstrv1 != 0) { + fprintf(stderr, "Expected 0 string, but got %zu\n", + out_nstrv1); + goto cleanup; + } + + ret = 0; + + cleanup: + dbus_message_unref(msg); + return ret; +} + +static int testMessageSingleArrayRef(const void *args ATTRIBUTE_UNUSED) +{ + DBusMessage *msg = NULL; + int ret = -1; + const char *in_strv1[] = { + "Fishfood", + }; + char **out_strv1 = NULL; + size_t out_nstrv1 = 0; + + if (!(msg = dbus_message_new_method_call("org.libvirt.test", + "/org/libvirt/test", + "org.libvirt.test.astrochicken", + "cluck"))) { + VIR_DEBUG("Failed to allocate method call"); + goto cleanup; + } + + if (virDBusMessageEncode(msg, + "a&s", + 1, in_strv1) < 0) { + VIR_DEBUG("Failed to encode arguments"); + goto cleanup; + } + + if (virDBusMessageDecode(msg, + "a&s", + &out_nstrv1, &out_strv1) < 0) { + VIR_DEBUG("Failed to decode arguments"); + goto cleanup; + } + + + if (out_nstrv1 != 1) { + fprintf(stderr, "Expected 1 string, but got %zu\n", + out_nstrv1); + goto cleanup; + } + VERIFY_STR("strv1[0]", in_strv1[0], out_strv1[0], "%s"); + + ret = 0; + + cleanup: + if (out_strv1) + VIR_FREE(out_strv1[0]); + dbus_message_unref(msg); + return ret; +} + static int testMessageArrayRef(const void *args ATTRIBUTE_UNUSED) { DBusMessage *msg = NULL; @@ -426,7 +519,7 @@ static int testMessageDict(const void *args ATTRIBUTE_UNUSED) } if (virDBusMessageEncode(msg, - "sa{si}s", + "(sa{si}s)", in_str1, 3, in_key1, in_int32a, @@ -438,7 +531,7 @@ static int testMessageDict(const void *args ATTRIBUTE_UNUSED) } if (virDBusMessageDecode(msg, - "sa{si}s", + "(sa{si}s)", &out_str1, 3, &out_key1, &out_int32a, @@ -471,6 +564,119 @@ static int testMessageDict(const void *args ATTRIBUTE_UNUSED) return ret; } +static int testMessageDictRef(const void *args ATTRIBUTE_UNUSED) +{ + DBusMessage *msg = NULL; + int ret = -1; + const char *in_str1 = "Hello"; + const char *in_strv1[] = { + "Fruit1", "Apple", + "Fruit2", "Orange", + "Fruit3", "Kiwi", + }; + const char *in_str2 = "World"; + char *out_str1 = NULL; + size_t out_nint32 = 0; + char **out_strv1 = NULL; + char *out_str2 = NULL; + + if (!(msg = dbus_message_new_method_call("org.libvirt.test", + "/org/libvirt/test", + "org.libvirt.test.astrochicken", + "cluck"))) { + VIR_DEBUG("Failed to allocate method call"); + goto cleanup; + } + + if (virDBusMessageEncode(msg, + "(sa&{ss}s)", + in_str1, + 3, in_strv1, + in_str2) < 0) { + VIR_DEBUG("Failed to encode arguments"); + goto cleanup; + } + + if (virDBusMessageDecode(msg, + "(sa&{ss}s)", + &out_str1, + &out_nint32, + &out_strv1, + &out_str2) < 0) { + VIR_DEBUG("Failed to decode arguments: '%s'", virGetLastErrorMessage()); + goto cleanup; + } + + + VERIFY_STR("str1", in_str1, out_str1, "%s"); + VERIFY_STR("strv1[0]", in_strv1[0], out_strv1[0], "%s"); + VERIFY_STR("strv1[1]", in_strv1[1], out_strv1[1], "%s"); + VERIFY_STR("strv1[2]", in_strv1[2], out_strv1[2], "%s"); + VERIFY_STR("strv1[3]", in_strv1[3], out_strv1[3], "%s"); + VERIFY_STR("strv1[4]", in_strv1[4], out_strv1[4], "%s"); + VERIFY_STR("strv1[5]", in_strv1[5], out_strv1[5], "%s"); + VERIFY_STR("str2", in_str2, out_str2, "%s"); + + ret = 0; + + cleanup: + VIR_FREE(out_str1); + VIR_FREE(out_str2); + if (out_strv1) { + VIR_FREE(out_strv1[0]); + VIR_FREE(out_strv1[1]); + VIR_FREE(out_strv1[2]); + VIR_FREE(out_strv1[3]); + VIR_FREE(out_strv1[4]); + VIR_FREE(out_strv1[5]); + } + VIR_FREE(out_strv1); + dbus_message_unref(msg); + return ret; +} + +static int testMessageEmptyDictRef(const void *args ATTRIBUTE_UNUSED) +{ + DBusMessage *msg = NULL; + int ret = -1; + const char *in_strv1[] = {}; + size_t out_nint32 = 0; + char **out_strv1 = NULL; + + if (!(msg = dbus_message_new_method_call("org.libvirt.test", + "/org/libvirt/test", + "org.libvirt.test.astrochicken", + "cluck"))) { + VIR_DEBUG("Failed to allocate method call"); + goto cleanup; + } + + if (virDBusMessageEncode(msg, + "a&{ss}", + 0, in_strv1) < 0) { + VIR_DEBUG("Failed to encode arguments"); + goto cleanup; + } + + if (virDBusMessageDecode(msg, + "a&{ss}", + &out_nint32, + &out_strv1) < 0) { + VIR_DEBUG("Failed to decode arguments: '%s'", virGetLastErrorMessage()); + goto cleanup; + } + + if (out_nint32 != 0) { + fprintf(stderr, "Unexpected dict entries\n"); + goto cleanup; + } + + ret = 0; + + cleanup: + dbus_message_unref(msg); + return ret; +} static int mymain(void) @@ -483,12 +689,20 @@ mymain(void) ret = -1; if (virtTestRun("Test message array ", testMessageArray, NULL) < 0) ret = -1; + if (virtTestRun("Test message array empty ref ", testMessageEmptyArrayRef, NULL) < 0) + ret = -1; + if (virtTestRun("Test message array single ref ", testMessageSingleArrayRef, NULL) < 0) + ret = -1; if (virtTestRun("Test message array ref ", testMessageArrayRef, NULL) < 0) ret = -1; if (virtTestRun("Test message struct ", testMessageStruct, NULL) < 0) ret = -1; if (virtTestRun("Test message dict ", testMessageDict, NULL) < 0) ret = -1; + if (virtTestRun("Test message dict empty ref ", testMessageEmptyDictRef, NULL) < 0) + ret = -1; + if (virtTestRun("Test message dict ref ", testMessageDictRef, NULL) < 0) + ret = -1; return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 1.9.3
Spawning the pkcheck program every time a permission check is required is hugely expensive on CPU. The pkcheck program is just a dumb wrapper for the DBus API, so rewrite the code to use the DBus API directly. This also simplifies error handling a bit. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- cfg.mk | 3 + po/POTFILES.in | 1 + src/util/virpolkit.c | 122 ++++++++--------- tests/Makefile.am | 9 +- tests/virpolkittest.c | 360 ++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 429 insertions(+), 66 deletions(-) create mode 100644 tests/virpolkittest.c diff --git a/cfg.mk b/cfg.mk index c7119e6..ed7123b 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1143,3 +1143,6 @@ exclude_file_name_regexp--sc_prohibit_mixed_case_abbreviations = \ exclude_file_name_regexp--sc_prohibit_empty_first_line = \ ^(README|daemon/THREADS\.txt|src/esx/README|docs/library.xen|tests/vmwareverdata/fusion-5.0.3.txt|tests/nodeinfodata/linux-raspberrypi/cpu/offline)$$ + +exclude_file_name_regexp--sc_prohibit_useless_translation = \ + ^tests/virpolkittest.c diff --git a/po/POTFILES.in b/po/POTFILES.in index 1a0b75e..b769dfe 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -236,6 +236,7 @@ src/xenapi/xenapi_utils.c src/xenconfig/xen_common.c src/xenconfig/xen_sxpr.c src/xenconfig/xen_xm.c +tests/virpolkittest.c tools/libvirt-guests.sh.in tools/virsh.c tools/virsh.h diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c index 620bfda..203bd6e 100644 --- a/src/util/virpolkit.c +++ b/src/util/virpolkit.c @@ -60,84 +60,76 @@ int virPolkitCheckAuth(const char *actionid, const char **details, bool allowInteraction) { - int status = -1; - bool authdismissed = 0; - bool supportsuid = 0; - char *pkout = NULL; - virCommandPtr cmd = NULL; + DBusConnection *sysbus; + DBusMessage *reply = NULL; + char **retdetails = NULL; + size_t nretdetails = 0; + int is_authorized; /* var-args requires int not bool */ + int is_challenge; /* var-args requires int not bool */ + bool is_dismissed = false; + size_t i; int ret = -1; - static bool polkitInsecureWarned = false; - - VIR_DEBUG("Checking PID %lld UID %d startTime %llu", - (long long)pid, (int)uid, startTime); - if (startTime == 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Start time is required for polkit auth")); - return -1; - } - - cmd = virCommandNewArgList(PKCHECK_PATH, "--action-id", actionid, NULL); - virCommandSetOutputBuffer(cmd, &pkout); - virCommandSetErrorBuffer(cmd, &pkout); - - virCommandAddArg(cmd, "--process"); -# ifdef PKCHECK_SUPPORTS_UID - supportsuid = 1; -# endif - if (supportsuid) { - virCommandAddArgFormat(cmd, "%lld,%llu,%lu", - (long long)pid, startTime, (unsigned long)uid); - } else { - if (!polkitInsecureWarned) { - VIR_WARN("No support for caller UID with pkcheck. This deployment is known to be insecure."); - polkitInsecureWarned = true; - } - virCommandAddArgFormat(cmd, "%lld,%llu", - (long long)pid, startTime); - } - if (allowInteraction) - virCommandAddArg(cmd, "--allow-user-interaction"); + if (!(sysbus = virDBusGetSystemBus())) + goto cleanup; - while (details && details[0] && details[1]) { - virCommandAddArgList(cmd, "--detail", details[0], details[1], NULL); - details += 2; - } + VIR_INFO("Checking PID %lld running as %d", + (long long) pid, uid); - if (virCommandRun(cmd, &status) < 0) + if (virDBusCallMethod(sysbus, + &reply, + NULL, + "org.freedesktop.PolicyKit1", + "/org/freedesktop/PolicyKit1/Authority", + "org.freedesktop.PolicyKit1.Authority", + "CheckAuthorization", + "(sa{sv})sa&{ss}us", + "unix-process", + 3, + "pid", "u", (unsigned int)pid, + "start-time", "t", startTime, + "uid", "i", (int)uid, + actionid, + virStringListLen(details) / 2, + details, + allowInteraction, + "" /* cancellation ID */) < 0) goto cleanup; - authdismissed = (pkout && strstr(pkout, "dismissed=true")); - if (status != 0) { - char *tmp = virProcessTranslateStatus(status); - VIR_DEBUG("Policy kit denied action %s from pid %lld, uid %d: %s", - actionid, (long long)pid, (int)uid, NULLSTR(tmp)); - VIR_FREE(tmp); - ret = -2; + if (virDBusMessageRead(reply, + "(bba&{ss})", + &is_authorized, + &is_challenge, + &nretdetails, + &retdetails) < 0) goto cleanup; - } - VIR_DEBUG("Policy allowed action %s from pid %lld, uid %d", - actionid, (long long)pid, (int)uid); + for (i = 0; i < (nretdetails / 2); i++) { + if (STREQ(retdetails[(i * 2)], "polkit.dismissed") && + STREQ(retdetails[(i * 2) + 1], "true")) + is_dismissed = true; + } - ret = 0; + VIR_DEBUG("is auth %d is challenge %d", + is_authorized, is_challenge); - cleanup: - if (ret < 0) { - virResetLastError(); - - if (authdismissed) { + if (is_authorized) { + ret = 0; + } else { + ret = -2; + if (is_dismissed) virReportError(VIR_ERR_AUTH_CANCELLED, "%s", - _("authentication cancelled by user")); - } else if (pkout && *pkout) { - virReportError(VIR_ERR_AUTH_FAILED, _("polkit: %s"), pkout); - } else { - virReportError(VIR_ERR_AUTH_FAILED, "%s", _("authentication failed")); - } + _("user cancelled authentication process")); + else if (is_challenge) + virReportError(VIR_ERR_AUTH_FAILED, "%s", + _("no agent is available to authenticate")); + else + virReportError(VIR_ERR_AUTH_FAILED, "%s", + _("access denied by policy")); } - virCommandFree(cmd); - VIR_FREE(pkout); + cleanup: + virStringFreeListCount(retdetails, nretdetails); return ret; } diff --git a/tests/Makefile.am b/tests/Makefile.am index d6c3cfb..9faa1c0 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -197,7 +197,9 @@ endif WITH_LIBVIRTD if WITH_DBUS test_programs += virdbustest \ - virsystemdtest + virpolkittest \ + virsystemdtest \ + $(NULL) endif WITH_DBUS if WITH_SECDRIVER_SELINUX @@ -1008,6 +1010,11 @@ virmockdbus_la_CFLAGS = $(AM_CFLAGS) $(DBUS_CFLAGS) virmockdbus_la_LDFLAGS = -module -avoid-version \ -rpath /evil/libtool/hack/to/force/shared/lib/creation +virpolkittest_SOURCES = \ + virpolkittest.c testutils.h testutils.c +virpolkittest_CFLAGS = $(AM_CFLAGS) $(DBUS_CFLAGS) +virpolkittest_LDADD = $(LDADDS) $(DBUS_LIBS) + virsystemdtest_SOURCES = \ virsystemdtest.c testutils.h testutils.c virsystemdtest_CFLAGS = $(AM_CFLAGS) $(DBUS_CFLAGS) diff --git a/tests/virpolkittest.c b/tests/virpolkittest.c new file mode 100644 index 0000000..4b3d6f0 --- /dev/null +++ b/tests/virpolkittest.c @@ -0,0 +1,360 @@ +/* + * Copyright (C) 2013, 2014 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Daniel P. Berrange <berrange@redhat.com> + */ + +#include <config.h> + +#include "testutils.h" + +#if defined(WITH_DBUS) && defined(__linux__) + +# include <stdlib.h> +# include <dbus/dbus.h> + +# include "virpolkit.h" +# include "virdbus.h" +# include "virlog.h" +# include "virmock.h" +# define VIR_FROM_THIS VIR_FROM_NONE + +VIR_LOG_INIT("tests.systemdtest"); + +/* Some interesting numbers */ +# define THE_PID 1458 +# define THE_TIME 11011000001 +# define THE_UID 1729 + +VIR_MOCK_IMPL_RET_ARGS(dbus_connection_send_with_reply_and_block, + DBusMessage *, + DBusConnection *, connection, + DBusMessage *, message, + int, timeout_milliseconds, + DBusError *, error) +{ + DBusMessage *reply = NULL; + const char *service = dbus_message_get_destination(message); + const char *member = dbus_message_get_member(message); + + VIR_MOCK_IMPL_INIT_REAL(dbus_connection_send_with_reply_and_block); + + if (STREQ(service, "org.freedesktop.PolicyKit1") && + STREQ(member, "CheckAuthorization")) { + char *type; + char *pidkey; + unsigned int pidval; + char *timekey; + unsigned long long timeval; + char *uidkey; + int uidval; + char *actionid; + char **details; + size_t detailslen; + int allowInteraction; + char *cancellationId; + const char **retdetails = NULL; + size_t retdetailslen = 0; + const char *retdetailscancelled[] = { + "polkit.dismissed", "true", + }; + int is_authorized = 1; + int is_challenge = 0; + + if (virDBusMessageRead(message, + "(sa{sv})sa&{ss}us", + &type, + 3, + &pidkey, "u", &pidval, + &timekey, "t", &timeval, + &uidkey, "i", &uidval, + &actionid, + &detailslen, + &details, + &allowInteraction, + &cancellationId) < 0) + goto error; + + if (STREQ(actionid, "org.libvirt.test.success")) { + is_authorized = 1; + is_challenge = 0; + } else if (STREQ(actionid, "org.libvirt.test.challenge")) { + is_authorized = 0; + is_challenge = 1; + } else if (STREQ(actionid, "org.libvirt.test.cancelled")) { + is_authorized = 0; + is_challenge = 0; + retdetails = retdetailscancelled; + retdetailslen = ARRAY_CARDINALITY(retdetailscancelled) / 2; + } else if (STREQ(actionid, "org.libvirt.test.details")) { + size_t i; + is_authorized = 0; + is_challenge = 0; + for (i = 0; i < detailslen / 2; i++) { + if (STREQ(details[i * 2], + "org.libvirt.test.person") && + STREQ(details[(i * 2) + 1], + "Fred")) { + is_authorized = 1; + is_challenge = 0; + } + } + } else { + is_authorized = 0; + is_challenge = 0; + } + + VIR_FREE(type); + VIR_FREE(pidkey); + VIR_FREE(timekey); + VIR_FREE(uidkey); + VIR_FREE(actionid); + VIR_FREE(cancellationId); + virStringFreeListCount(details, detailslen); + + if (virDBusCreateReply(&reply, + "(bba&{ss})", + is_authorized, + is_challenge, + retdetailslen, + retdetails) < 0) + goto error; + } else { + reply = dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN); + } + + return reply; + + error: + dbus_message_unref(reply); + return NULL; +} + + + +static int testPolkitAuthSuccess(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + + if (virPolkitCheckAuth("org.libvirt.test.success", + THE_PID, + THE_TIME, + THE_UID, + NULL, + true) < 0) + goto cleanup; + + ret = 0; + + cleanup: + return ret; +} + + +static int testPolkitAuthDenied(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + int rv; + virErrorPtr err; + + rv = virPolkitCheckAuth("org.libvirt.test.deny", + THE_PID, + THE_TIME, + THE_UID, + NULL, + true); + + if (rv == 0) { + fprintf(stderr, "Unexpected auth success\n"); + goto cleanup; + } else if (rv != -2) { + goto cleanup; + } + + err = virGetLastError(); + if (!err || !strstr(err->message, + _("access denied by policy"))) { + fprintf(stderr, "Incorrect error response\n"); + goto cleanup; + } + + ret = 0; + + cleanup: + return ret; +} + + +static int testPolkitAuthChallenge(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + int rv; + virErrorPtr err; + + rv = virPolkitCheckAuth("org.libvirt.test.challenge", + THE_PID, + THE_TIME, + THE_UID, + NULL, + true); + + if (rv == 0) { + fprintf(stderr, "Unexpected auth success\n"); + goto cleanup; + } else if (rv != -2) { + goto cleanup; + } + + err = virGetLastError(); + if (!err || !strstr(err->message, + _("no agent is available to authenticate"))) { + fprintf(stderr, "Incorrect error response\n"); + goto cleanup; + } + + ret = 0; + + cleanup: + return ret; +} + + +static int testPolkitAuthCancelled(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + int rv; + virErrorPtr err; + + rv = virPolkitCheckAuth("org.libvirt.test.cancelled", + THE_PID, + THE_TIME, + THE_UID, + NULL, + true); + + if (rv == 0) { + fprintf(stderr, "Unexpected auth success\n"); + goto cleanup; + } else if (rv != -2) { + goto cleanup; + } + + err = virGetLastError(); + if (!err || !strstr(err->message, + _("user cancelled authentication process"))) { + fprintf(stderr, "Incorrect error response\n"); + goto cleanup; + } + + ret = 0; + + cleanup: + return ret; +} + + +static int testPolkitAuthDetailsSuccess(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + const char *details[] = { + "org.libvirt.test.person", "Fred", + NULL, + }; + + if (virPolkitCheckAuth("org.libvirt.test.details", + THE_PID, + THE_TIME, + THE_UID, + details, + true) < 0) + goto cleanup; + + ret = 0; + + cleanup: + return ret; +} + + +static int testPolkitAuthDetailsDenied(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + int rv; + virErrorPtr err; + const char *details[] = { + "org.libvirt.test.person", "Joe", + NULL, + }; + + rv = virPolkitCheckAuth("org.libvirt.test.details", + THE_PID, + THE_TIME, + THE_UID, + details, + true); + + if (rv == 0) { + fprintf(stderr, "Unexpected auth success\n"); + goto cleanup; + } else if (rv != -2) { + goto cleanup; + } + + err = virGetLastError(); + if (!err || !strstr(err->message, + _("access denied by policy"))) { + fprintf(stderr, "Incorrect error response\n"); + goto cleanup; + } + + ret = 0; + + cleanup: + return ret; +} + + +static int +mymain(void) +{ + int ret = 0; + + if (virtTestRun("Polkit auth success ", testPolkitAuthSuccess, NULL) < 0) + ret = -1; + if (virtTestRun("Polkit auth deny ", testPolkitAuthDenied, NULL) < 0) + ret = -1; + if (virtTestRun("Polkit auth challenge ", testPolkitAuthChallenge, NULL) < 0) + ret = -1; + if (virtTestRun("Polkit auth cancel ", testPolkitAuthCancelled, NULL) < 0) + ret = -1; + if (virtTestRun("Polkit auth details success ", testPolkitAuthDetailsSuccess, NULL) < 0) + ret = -1; + if (virtTestRun("Polkit auth details deny ", testPolkitAuthDetailsDenied, NULL) < 0) + ret = -1; + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/virmockdbus.so") + +#else /* ! (WITH_DBUS && __linux__) */ +int +main(void) +{ + return EXIT_AM_SKIP; +} +#endif /* ! WITH_DBUS */ -- 1.9.3
On 10.09.2014 16:21, Daniel P. Berrange wrote:
Spawning the pkcheck program every time a permission check is required is hugely expensive on CPU. The pkcheck program is just a dumb wrapper for the DBus API, so rewrite the code to use the DBus API directly. This also simplifies error handling a bit.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- cfg.mk | 3 + po/POTFILES.in | 1 + src/util/virpolkit.c | 122 ++++++++--------- tests/Makefile.am | 9 +- tests/virpolkittest.c | 360 ++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 429 insertions(+), 66 deletions(-) create mode 100644 tests/virpolkittest.c
diff --git a/cfg.mk b/cfg.mk index c7119e6..ed7123b 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1143,3 +1143,6 @@ exclude_file_name_regexp--sc_prohibit_mixed_case_abbreviations = \
exclude_file_name_regexp--sc_prohibit_empty_first_line = \ ^(README|daemon/THREADS\.txt|src/esx/README|docs/library.xen|tests/vmwareverdata/fusion-5.0.3.txt|tests/nodeinfodata/linux-raspberrypi/cpu/offline)$$ + +exclude_file_name_regexp--sc_prohibit_useless_translation = \ + ^tests/virpolkittest.c diff --git a/po/POTFILES.in b/po/POTFILES.in index 1a0b75e..b769dfe 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -236,6 +236,7 @@ src/xenapi/xenapi_utils.c src/xenconfig/xen_common.c src/xenconfig/xen_sxpr.c src/xenconfig/xen_xm.c +tests/virpolkittest.c tools/libvirt-guests.sh.in tools/virsh.c tools/virsh.h diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c index 620bfda..203bd6e 100644 --- a/src/util/virpolkit.c +++ b/src/util/virpolkit.c @@ -60,84 +60,76 @@ int virPolkitCheckAuth(const char *actionid, const char **details, bool allowInteraction) { - int status = -1; - bool authdismissed = 0; - bool supportsuid = 0; - char *pkout = NULL; - virCommandPtr cmd = NULL; + DBusConnection *sysbus; + DBusMessage *reply = NULL; + char **retdetails = NULL; + size_t nretdetails = 0; + int is_authorized; /* var-args requires int not bool */ + int is_challenge; /* var-args requires int not bool */ + bool is_dismissed = false; + size_t i; int ret = -1; - static bool polkitInsecureWarned = false; - - VIR_DEBUG("Checking PID %lld UID %d startTime %llu", - (long long)pid, (int)uid, startTime);
- if (startTime == 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Start time is required for polkit auth")); - return -1; - } - - cmd = virCommandNewArgList(PKCHECK_PATH, "--action-id", actionid, NULL); - virCommandSetOutputBuffer(cmd, &pkout); - virCommandSetErrorBuffer(cmd, &pkout); - - virCommandAddArg(cmd, "--process"); -# ifdef PKCHECK_SUPPORTS_UID - supportsuid = 1; -# endif - if (supportsuid) { - virCommandAddArgFormat(cmd, "%lld,%llu,%lu", - (long long)pid, startTime, (unsigned long)uid); - } else { - if (!polkitInsecureWarned) { - VIR_WARN("No support for caller UID with pkcheck. This deployment is known to be insecure."); - polkitInsecureWarned = true; - } - virCommandAddArgFormat(cmd, "%lld,%llu", - (long long)pid, startTime); - } - if (allowInteraction) - virCommandAddArg(cmd, "--allow-user-interaction"); + if (!(sysbus = virDBusGetSystemBus())) + goto cleanup;
- while (details && details[0] && details[1]) { - virCommandAddArgList(cmd, "--detail", details[0], details[1], NULL); - details += 2; - } + VIR_INFO("Checking PID %lld running as %d", + (long long) pid, uid);
- if (virCommandRun(cmd, &status) < 0) + if (virDBusCallMethod(sysbus, + &reply, + NULL, + "org.freedesktop.PolicyKit1", + "/org/freedesktop/PolicyKit1/Authority", + "org.freedesktop.PolicyKit1.Authority", + "CheckAuthorization", + "(sa{sv})sa&{ss}us", + "unix-process", + 3, + "pid", "u", (unsigned int)pid, + "start-time", "t", startTime, + "uid", "i", (int)uid, + actionid, + virStringListLen(details) / 2, + details, + allowInteraction, + "" /* cancellation ID */) < 0) goto cleanup;
- authdismissed = (pkout && strstr(pkout, "dismissed=true")); - if (status != 0) { - char *tmp = virProcessTranslateStatus(status); - VIR_DEBUG("Policy kit denied action %s from pid %lld, uid %d: %s", - actionid, (long long)pid, (int)uid, NULLSTR(tmp)); - VIR_FREE(tmp); - ret = -2; + if (virDBusMessageRead(reply, + "(bba&{ss})", + &is_authorized, + &is_challenge, + &nretdetails, + &retdetails) < 0) goto cleanup; - }
- VIR_DEBUG("Policy allowed action %s from pid %lld, uid %d", - actionid, (long long)pid, (int)uid); + for (i = 0; i < (nretdetails / 2); i++) { + if (STREQ(retdetails[(i * 2)], "polkit.dismissed") && + STREQ(retdetails[(i * 2) + 1], "true")) + is_dismissed = true; + }
- ret = 0; + VIR_DEBUG("is auth %d is challenge %d", + is_authorized, is_challenge);
- cleanup: - if (ret < 0) { - virResetLastError(); - - if (authdismissed) { + if (is_authorized) { + ret = 0; + } else { + ret = -2; + if (is_dismissed) virReportError(VIR_ERR_AUTH_CANCELLED, "%s", - _("authentication cancelled by user")); - } else if (pkout && *pkout) { - virReportError(VIR_ERR_AUTH_FAILED, _("polkit: %s"), pkout); - } else { - virReportError(VIR_ERR_AUTH_FAILED, "%s", _("authentication failed")); - } + _("user cancelled authentication process")); + else if (is_challenge) + virReportError(VIR_ERR_AUTH_FAILED, "%s", + _("no agent is available to authenticate")); + else + virReportError(VIR_ERR_AUTH_FAILED, "%s", + _("access denied by policy")); }
- virCommandFree(cmd); - VIR_FREE(pkout); + cleanup: + virStringFreeListCount(retdetails, nretdetails); return ret; }
diff --git a/tests/Makefile.am b/tests/Makefile.am index d6c3cfb..9faa1c0 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -197,7 +197,9 @@ endif WITH_LIBVIRTD
if WITH_DBUS test_programs += virdbustest \ - virsystemdtest + virpolkittest \ + virsystemdtest \ + $(NULL) endif WITH_DBUS
if WITH_SECDRIVER_SELINUX @@ -1008,6 +1010,11 @@ virmockdbus_la_CFLAGS = $(AM_CFLAGS) $(DBUS_CFLAGS) virmockdbus_la_LDFLAGS = -module -avoid-version \ -rpath /evil/libtool/hack/to/force/shared/lib/creation
+virpolkittest_SOURCES = \ + virpolkittest.c testutils.h testutils.c +virpolkittest_CFLAGS = $(AM_CFLAGS) $(DBUS_CFLAGS) +virpolkittest_LDADD = $(LDADDS) $(DBUS_LIBS) + virsystemdtest_SOURCES = \ virsystemdtest.c testutils.h testutils.c virsystemdtest_CFLAGS = $(AM_CFLAGS) $(DBUS_CFLAGS) diff --git a/tests/virpolkittest.c b/tests/virpolkittest.c new file mode 100644 index 0000000..4b3d6f0 --- /dev/null +++ b/tests/virpolkittest.c @@ -0,0 +1,360 @@ +/* + * Copyright (C) 2013, 2014 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Daniel P. Berrange <berrange@redhat.com> + */ + +#include <config.h> + +#include "testutils.h" + +#if defined(WITH_DBUS) && defined(__linux__) + +# include <stdlib.h> +# include <dbus/dbus.h> + +# include "virpolkit.h" +# include "virdbus.h" +# include "virlog.h" +# include "virmock.h" +# define VIR_FROM_THIS VIR_FROM_NONE + +VIR_LOG_INIT("tests.systemdtest"); + +/* Some interesting numbers */ +# define THE_PID 1458 +# define THE_TIME 11011000001 +# define THE_UID 1729 + +VIR_MOCK_IMPL_RET_ARGS(dbus_connection_send_with_reply_and_block, + DBusMessage *, + DBusConnection *, connection, + DBusMessage *, message, + int, timeout_milliseconds, + DBusError *, error) +{ + DBusMessage *reply = NULL; + const char *service = dbus_message_get_destination(message); + const char *member = dbus_message_get_member(message); + + VIR_MOCK_IMPL_INIT_REAL(dbus_connection_send_with_reply_and_block);
After your latest patches, you need to update this: diff --git a/tests/virpolkittest.c b/tests/virpolkittest.c index 4b3d6f0..bc97529 100644 --- a/tests/virpolkittest.c +++ b/tests/virpolkittest.c @@ -51,7 +51,7 @@ VIR_MOCK_IMPL_RET_ARGS(dbus_connection_send_with_reply_and_block, const char *service = dbus_message_get_destination(message); const char *member = dbus_message_get_member(message); - VIR_MOCK_IMPL_INIT_REAL(dbus_connection_send_with_reply_and_block); + VIR_MOCK_REAL_INIT(dbus_connection_send_with_reply_and_block); if (STREQ(service, "org.freedesktop.PolicyKit1") && STREQ(member, "CheckAuthorization")) { Michal
On 10.09.2014 16:20, Daniel P. Berrange wrote:
This series improves the performance of the polkit driver by switching from use of the pk-check command, to the DBus APIs. As a convenient side effect, this means we are no longer vulnerable to CVE-2013-4311, on any polkit version, since we no longer need pk-check (which is what had the flaw).
In terms of performance, with access control checking turned on for all APIs, the time to list 100 VMs dropps from 2.7 secs to 1 sec on my machine. To improve on this further, we would need to find a way to parallelize the issuing of DBus calls for each VM, instead of serialize the access checks.
Daniel P. Berrange (7): Add common API for doing polkit authentication Add typesafe APIs for virIdentity attributes Convert callers to use typesafe APIs for setting identity attrs Convert callers to use typesafe APIs for getting identity attrs Convert remote daemon & acl code to use polkit API Support passing dict by reference for dbus messages Convert polkit code to use DBus API instead of CLI helper
cfg.mk | 3 + daemon/remote.c | 235 ++---------------------- include/libvirt/virterror.h | 2 + po/POTFILES.in | 2 + src/Makefile.am | 1 + src/access/viraccessdriverpolkit.c | 97 ++++------ src/libvirt_private.syms | 22 +++ src/rpc/virnetserverclient.c | 115 +++--------- src/util/virdbus.c | 274 +++++++++++++++++++--------- src/util/virerror.c | 2 + src/util/viridentity.c | 320 +++++++++++++++++++++++++++------ src/util/viridentity.h | 40 +++++ src/util/virpolkit.c | 255 ++++++++++++++++++++++++++ src/util/virpolkit.h | 34 ++++ src/util/virstring.c | 14 ++ src/util/virstring.h | 2 + tests/Makefile.am | 9 +- tests/virdbustest.c | 218 +++++++++++++++++++++- tests/virpolkittest.c | 360 +++++++++++++++++++++++++++++++++++++ 19 files changed, 1485 insertions(+), 520 deletions(-) create mode 100644 src/util/virpolkit.c create mode 100644 src/util/virpolkit.h create mode 100644 tests/virpolkittest.c
ACK series, but see my comment to 7/7. Michal
participants (3)
-
Daniel P. Berrange -
John Ferlan -
Michal Privoznik