[libvirt PATCH v2 03/10] util: generate a persistent system token

Daniel P. Berrangé posted 10 patches 4 years, 9 months ago
There is a newer version of this series
[libvirt PATCH v2 03/10] util: generate a persistent system token
Posted by Daniel P. Berrangé 4 years, 9 months ago
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.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/util/viridentity.c | 102 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)

diff --git a/src/util/viridentity.c b/src/util/viridentity.c
index 7da4ea12f5..8c939a507e 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,98 @@ 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;
+    VIR_AUTOCLOSE fd = -1;
+    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);
+        return NULL;
+    }
+
+    if (virSetCloseExec(fd) < 0) {
+        virReportSystemError(errno,
+                             _("Failed to set close-on-exec flag '%s'"),
+                             tokenfile);
+        return NULL;
+    }
+
+    if (virFileLock(fd, false, 0, 1, true) < 0) {
+        virReportSystemError(errno,
+                             _("Failed to lock system token '%s'"),
+                             tokenfile);
+        return NULL;
+    }
+
+    if (fstat(fd, &st) < 0) {
+        virReportSystemError(errno,
+                             _("Failed to check system token '%s'"),
+                             tokenfile);
+        return NULL;
+    }
+
+    /* Ok, we're the first one here, so we must populate it */
+    if (st.st_size == 0) {
+        if (!(token = virRandomToken(TOKEN_BYTES))) {
+            return NULL;
+        }
+        if (safewrite(fd, token, TOKEN_STRLEN) != TOKEN_STRLEN) {
+            virReportSystemError(errno,
+                                 _("Failed to write system token '%s'"),
+                                 tokenfile);
+            return NULL;
+        }
+    } else {
+        if (virFileReadLimFD(fd, TOKEN_STRLEN, &token) < 0) {
+            virReportSystemError(errno,
+                                 _("Failed to write system token '%s'"),
+                                 tokenfile);
+            return NULL;
+        }
+        if (strlen(token) != TOKEN_STRLEN) {
+            virReportSystemError(errno,
+                                 _("System token in %s was corrupt"),
+                                 tokenfile);
+            return NULL;
+        }
+    }
+
+    return g_steal_pointer(&token);
+}
+
+
 /**
  * virIdentityGetSystem:
  *
-- 
2.31.1

Re: [libvirt PATCH v2 03/10] util: generate a persistent system token
Posted by Michal Prívozník 4 years, 9 months ago
On 5/7/21 6:24 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.
> 
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/util/viridentity.c | 102 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 102 insertions(+)
> 
> diff --git a/src/util/viridentity.c b/src/util/viridentity.c
> index 7da4ea12f5..8c939a507e 100644
> --- a/src/util/viridentity.c
> +++ b/src/util/viridentity.c

> @@ -144,6 +154,98 @@ 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;
> +    VIR_AUTOCLOSE fd = -1;
> +    struct stat st;
> +

Sorry for not spotting this in v1, but @tokenfile can be NULL here, in
which case virIdentityConstructSystemTokenPath() already reported
accurate error. Something like: if (!tokenfile) return NULL;  is sufficient.

> +    fd = open(tokenfile, O_RDWR|O_APPEND|O_CREAT, 0600);
> +    if (fd < 0) {
> +        virReportSystemError(errno,
> +                             _("Unable to open system token %s"),
> +                             tokenfile);
> +        return NULL;
> +    }
> +


Also, I believe you will want to mock this function, because if you
don't then the viridentitytest starts failing after next patch.
Something among these lines:

https://gitlab.com/MichalPrivoznik/libvirt/-/commit/5594631df3b3512adecea0f5a0056611b324fa5d

Michal

Re: [libvirt PATCH v2 03/10] util: generate a persistent system token
Posted by Eric Blake 4 years, 9 months ago
On 5/7/21 11:24 AM, 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.

unprivileged

> 
> 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.
> 
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/util/viridentity.c | 102 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 102 insertions(+)
> 


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org