On 5/4/21 7:43 PM, Daniel P. Berrangé wrote:
> When creating the system identity set the system token. The system
> token is currently stored in a local path
>
> /var/run/libvirt/common/system.token
>
> Obviously with only traditional UNIX DAC in effect, this is largely
> security through obscurity, if the client is running at the same
> privilege level as the daemon. It does, however, reliably distinguish
> an unprivilegd client from the system daemons.
>
> With a MAC system like SELinux though, or possible use of containers,
> access can be further restricted.
>
> A possible future improvement for Linux would be to populate the
> kernel keyring with a secret for libvirt daemons to share.
>
> Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> ---
> src/util/viridentity.c | 107 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 107 insertions(+)
>
> diff --git a/src/util/viridentity.c b/src/util/viridentity.c
> index 7da4ea12f5..065db06e49 100644
> --- a/src/util/viridentity.c
> +++ b/src/util/viridentity.c
> @@ -22,6 +22,7 @@
> #include <config.h>
>
> #include <unistd.h>
> +#include <fcntl.h>
> #if WITH_SELINUX
> # include <selinux/selinux.h>
> #endif
> @@ -32,11 +33,14 @@
> #include "viridentity.h"
> #include "virlog.h"
> #include "virobject.h"
> +#include "virrandom.h"
> #include "virthread.h"
> #include "virutil.h"
> #include "virstring.h"
> #include "virprocess.h"
> #include "virtypedparam.h"
> +#include "virfile.h"
> +#include "configmake.h"
>
> #define VIR_FROM_THIS VIR_FROM_IDENTITY
>
> @@ -54,7 +58,10 @@ struct _virIdentity {
>
> G_DEFINE_TYPE(virIdentity, vir_identity, G_TYPE_OBJECT)
>
> +static char *virIdentityEnsureSystemToken(void);
> +
> static virThreadLocal virIdentityCurrent;
> +static char *systemToken;
>
> static void virIdentityFinalize(GObject *obj);
>
> @@ -73,6 +80,9 @@ static int virIdentityOnceInit(void)
> return -1;
> }
>
> + if (!(systemToken = virIdentityEnsureSystemToken()))
> + return -1;
> +
> return 0;
> }
>
> @@ -144,6 +154,103 @@ int virIdentitySetCurrent(virIdentity *ident)
> }
>
>
> +#define TOKEN_BYTES 16
> +#define TOKEN_STRLEN (TOKEN_BYTES * 2)
> +
> +static char *
> +virIdentityConstructSystemTokenPath(void)
> +{
> + g_autofree char *commondir = NULL;
> + if (geteuid() == 0) {
> + commondir = g_strdup(RUNSTATEDIR "/libvirt/common");
> + } else {
> + g_autofree char *rundir = virGetUserRuntimeDirectory();
> + commondir = g_strdup_printf("%s/common", rundir);
> + }
> +
> + if (g_mkdir_with_parents(commondir, 0700) < 0) {
> + virReportSystemError(errno,
> + _("Cannot create daemon common directory '%s'"),
> + commondir);
> + return NULL;
> + }
> +
> + return g_strdup_printf("%s/system.token", commondir);
> +}
> +
> +
> +static char *
> +virIdentityEnsureSystemToken(void)
> +{
> + g_autofree char *tokenfile = virIdentityConstructSystemTokenPath();
> + g_autofree char *token = NULL;
> + int fd = -1;
Or VIR_AUTOCLOSE fd = -1 and drop those explicit close calls at the end.
> + struct stat st;
> +
> + fd = open(tokenfile, O_RDWR|O_APPEND|O_CREAT, 0600);
> + if (fd < 0) {
> + virReportSystemError(errno,
> + _("Unable to open system token %s"),
> + tokenfile);
> + goto error;
> + }
> +
> + if (virSetCloseExec(fd) < 0) {
I know we have this pattern in many areas, but it's inherently racy.
What's stopping us from passing O_CLOEXEC to open()? IIUC, O_CLOEXEC
will exist on mingw where it's just an alias to O_NOINHERIT.
> + virReportSystemError(errno,
> + _("Failed to set close-on-exec flag '%s'"),
> + tokenfile);
> + goto error;
> + }
Michal
