[PATCH v8] run QEMU as non-root

[PATCH v8] run QEMU as non-root

[Stefano Stabellini]

Try to use "xen-qemudepriv-domid$domid" first, then
"xen-qemudepriv-shared" and root if everything else fails.

The uids need to be manually created by the user or, more likely, by the
xen package maintainer.

Expose a device_model_user setting in libxl_domain_build_info, so that
opinionated callers, such as libvirt, can set any user they like. Do not
fall back to root if device_model_user is set.

QEMU is going to setuid and setgid to the user ID and the group ID of
the specified user, soon after initialization, before starting to deal
with any guest IO.

To actually secure QEMU when running in Dom0, we need at least to
deprivilege the privcmd and xenstore interfaces, this is just the first
step in that direction.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: wei.liu2@citrix.com
CC: Ian.Jackson@eu.citrix.com
CC: ian.campbell@citrix.com

---

Changes in v8:
- no need to pass the -runas option if the user requested for root
- return ERROR_FAIL from libxl__dm_runas_helper in case of errors
- return NULL from libxl__build_device_model_args_new if libxl__dm_runas_helper failed
- fix line too long
- remove setting errno
- replace retry goto loop, with a while loop
- const char * as argument to libxl__dm_runas_helper
- fix comment

Changes in v7:
- do not fall back to root if the user explicitly set
b_info->device_model_user.

Changes in v6:
- add device_model_user to libxl_domain_build_info
- improve doc
- improve wording in commit message

Changes in v5:
- improve wording in doc
- fix wording in warning message
- fix example in doc
- drop xen-qemudepriv-$domname

Changes in v4:
- rename qemu-deprivilege to qemu-deprivilege.txt
- add a note about qemu-deprivilege.txt to INSTALL
- instead of xen-qemudepriv-base + $domid, try xen-qemudepriv-domid$domid
- introduce libxl__dm_runas_helper to make the code nicer

Changes in v3:
- clarify doc
- handle errno == ERANGE
---
 INSTALL                        |    7 +++++
 docs/misc/qemu-deprivilege.txt |   31 +++++++++++++++++++
 tools/libxl/libxl.h            |    6 ++++
 tools/libxl/libxl_dm.c         |   67 +++++++++++++++++++++++++++++++++++++++-
 tools/libxl/libxl_internal.h   |    4 +++
 tools/libxl/libxl_types.idl    |    1 +
 6 files changed, 115 insertions(+), 1 deletion(-)
 create mode 100644 docs/misc/qemu-deprivilege.txt

diff --git a/INSTALL b/INSTALL
index a0f2e7b..fe83c20 100644
--- a/INSTALL
+++ b/INSTALL
@@ -297,6 +297,13 @@ systemctl enable xendomains.service
 systemctl enable xen-watchdog.service
 
 
+QEMU Deprivilege
+================
+It is recommended to run QEMU as non-root.
+See docs/misc/qemu-deprivilege.txt for an explanation on what you need
+to do at installation time to run QEMU as a dedicated user.
+
+
 History of options
 ==================
 
diff --git a/docs/misc/qemu-deprivilege.txt b/docs/misc/qemu-deprivilege.txt
new file mode 100644
index 0000000..6c63135
--- /dev/null
+++ b/docs/misc/qemu-deprivilege.txt
@@ -0,0 +1,31 @@
+For security reasons, libxl tries to pass a non-root username to QEMU as
+argument. During initialization QEMU calls setuid and setgid with the
+user ID and the group ID of the user passed as argument.
+Libxl looks for the following users in this order:
+
+1) a user named "xen-qemuuser-domid$domid", 
+Where $domid is the domid of the domain being created.
+This requires the reservation of 65535 uids from xen-qemuuser-domid1
+to xen-qemuuser-domid65535. To use this mechanism, you might want to
+create a large number of users at installation time. For example:
+
+for ((i=1; i<65536; i++))
+do
+    adduser --no-create-home --system xen-qemuuser-domid$i
+done
+
+You might want to consider passing --group to adduser to create a new
+group for each new user.
+
+
+2) a user named "xen-qemuuser-shared"
+As a fall back if both 1) fails, libxl will use a single user for
+all QEMU instances. The user is named xen-qemuuser-shared. This is
+less secure but still better than running QEMU as root. Using this is as
+simple as creating just one more user on your host:
+
+adduser --no-create-home --system xen-qemuuser-shared
+
+
+3) root
+As a last resort, libxl will start QEMU as root.
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index efc0617..3f4283f 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -192,6 +192,12 @@
  * is not present, instead of ERROR_INVAL.
  */
 #define LIBXL_HAVE_ERROR_DOMAIN_NOTFOUND 1
+
+/* libxl_domain_build_info has device_model_user to specify the user to
+ * run the device model with. See docs/misc/qemu-deprivilege.txt.
+ */
+#define LIBXL_HAVE_DEVICE_MODEL_USER 1
+
 /*
  * libxl ABI compatibility
  *
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 0c6408d..ad93a09 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -19,6 +19,8 @@
 
 #include "libxl_internal.h"
 #include <xen/hvm/e820.h>
+#include <sys/types.h>
+#include <pwd.h>
 
 static const char *libxl_tapif_script(libxl__gc *gc)
 {
@@ -418,6 +420,36 @@ static char *dm_spice_options(libxl__gc *gc,
     return opt;
 }
 
+/* return 1 if the user was found, 0 if it was not, -1 on error */
+static int libxl__dm_runas_helper(libxl__gc *gc, const char *username)
+{
+    struct passwd pwd, *user = NULL;
+    char *buf = NULL;
+    long buf_size;
+    int ret;
+
+    buf_size = sysconf(_SC_GETPW_R_SIZE_MAX);
+    if (buf_size < 0) {
+        LOGE(ERROR, "sysconf(_SC_GETPW_R_SIZE_MAX) returned error %ld",
+                buf_size);
+        return ERROR_FAIL;
+    }
+
+    while (1) {
+        buf = libxl__realloc(gc, buf, buf_size);
+        ret = getpwnam_r(username, &pwd, buf, buf_size, &user);
+        if (ret == ERANGE) {
+            buf_size += 128;
+            continue;
+        }
+        if (ret != 0)
+            return ERROR_FAIL;
+        if (user != NULL)
+            return 1;
+        return 0;
+    }
+}
+
 static char ** libxl__build_device_model_args_new(libxl__gc *gc,
                                         const char *dm, int guest_domid,
                                         const libxl_domain_config *guest_config,
@@ -436,9 +468,10 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
     const char *keymap = dm_keymap(guest_config);
     char *machinearg;
     flexarray_t *dm_args;
-    int i, connection, devid;
+    int i, connection, devid, ret;
     uint64_t ram_size;
     const char *path, *chardev;
+    char *user = NULL;
 
     dm_args = flexarray_make(gc, 16, 1);
 
@@ -878,6 +911,38 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
         default:
             break;
         }
+
+        if (b_info->device_model_user) {
+            user = b_info->device_model_user;
+            goto end_search;
+        }
+
+        user = libxl__sprintf(gc, "%s%d", LIBXL_QEMU_USER_BASE, guest_domid);
+        ret = libxl__dm_runas_helper(gc, user);
+        if (ret < 0)
+            return NULL;
+        if (ret > 0)
+            goto end_search;
+
+        user = LIBXL_QEMU_USER_SHARED;
+        ret = libxl__dm_runas_helper(gc, user);
+        if (ret < 0)
+            return NULL;
+        if (ret > 0) {
+            LOG(WARN, "Could not find user %s%d, falling back to %s",
+                    LIBXL_QEMU_USER_BASE, guest_domid, LIBXL_QEMU_USER_SHARED);
+            goto end_search;
+        }
+
+        user = NULL;
+        LOG(WARN, "Could not find user %s, starting QEMU as root",
+            LIBXL_QEMU_USER_SHARED);
+
+end_search:
+        if (user != NULL && strcmp(user, "root")) {
+            flexarray_append(dm_args, "-runas");
+            flexarray_append(dm_args, user);
+        }
     }
     flexarray_append(dm_args, NULL);
     return (char **) flexarray_contents(dm_args);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 8eb38aa..7d0af40 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3692,6 +3692,10 @@ static inline void libxl__update_config_vtpm(libxl__gc *gc,
  */
 void libxl__bitmap_copy_best_effort(libxl__gc *gc, libxl_bitmap *dptr,
                                     const libxl_bitmap *sptr);
+
+#define LIBXL_QEMU_USER_PREFIX "xen-qemuuser"
+#define LIBXL_QEMU_USER_BASE   LIBXL_QEMU_USER_PREFIX"-domid"
+#define LIBXL_QEMU_USER_SHARED LIBXL_QEMU_USER_PREFIX"-shared"
 #endif
 
 /*
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 023b21e..16d5ddc 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -395,6 +395,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
     ("device_model",     string),
     ("device_model_ssidref", uint32),
     ("device_model_ssid_label", string),
+    ("device_model_user", string),
 
     # extra parameters pass directly to qemu, NULL terminated
     ("extra",            libxl_string_list),
-- 
1.7.10.4

Re: [PATCH v8] run QEMU as non-root

[Ian Campbell]

On Wed, 2015-09-30 at 16:45 +0100, Stefano Stabellini wrote:

> QEMU is going to setuid and setgid to the user ID and the group ID of
> the specified user, soon after initialization, before starting to deal
> with any guest IO.

Can you confirm that QEMU will bail if the user given via -runas doesn't
exist. IOW if the user gives b_info->device_model_user != NULL we will
correctly end up bailing if that specific user doesn't exist rather than
running as root?

If that is the case then:

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Wasn't there some code to plumb this into xl at one point? Did that get
dropped along the way?

> [...]
> +        user = NULL;
> +        LOG(WARN, "Could not find user %s, starting QEMU as root",
> +            LIBXL_QEMU_USER_SHARED);
> +
> +end_search:
> +        if (user != NULL && strcmp(user, "root")) {

This strcmp struck me as odd given the user = NULL just above, but this is
for the case where the user explicitly requested root, right?

> +            flexarray_append(dm_args, "-runas");
> +            flexarray_append(dm_args, user);
> +        }

Re: [PATCH v8] run QEMU as non-root

[Stefano Stabellini]

On Fri, 2 Oct 2015, Ian Campbell wrote:
> On Wed, 2015-09-30 at 16:45 +0100, Stefano Stabellini wrote:
> 
> > QEMU is going to setuid and setgid to the user ID and the group ID of
> > the specified user, soon after initialization, before starting to deal
> > with any guest IO.
> 
> Can you confirm that QEMU will bail if the user given via -runas doesn't
> exist.

It prints the error

User "blah" doesn't exist

and exits.


> IOW if the user gives b_info->device_model_user != NULL we will
> correctly end up bailing if that specific user doesn't exist rather than
> running as root?

Yes


> If that is the case then:
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Thanks


> Wasn't there some code to plumb this into xl at one point? Did that get
> dropped along the way?

device_model_user is added to the idl by this patch, I think that is
enough, right?


> > [...]
> > +        user = NULL;
> > +        LOG(WARN, "Could not find user %s, starting QEMU as root",
> > +            LIBXL_QEMU_USER_SHARED);
> > +
> > +end_search:
> > +        if (user != NULL && strcmp(user, "root")) {
> 
> This strcmp struck me as odd given the user = NULL just above, but this is
> for the case where the user explicitly requested root, right?

Yes, that's right


> > +            flexarray_append(dm_args, "-runas");
> > +            flexarray_append(dm_args, user);
> > +        }

Re: [PATCH v8] run QEMU as non-root

[Ian Campbell]

On Mon, 2015-10-05 at 16:53 +0100, Stefano Stabellini wrote:
> > Wasn't there some code to plumb this into xl at one point? Did that get
> > dropped along the way?
> 
> device_model_user is added to the idl by this patch, I think that is
> enough, right?

Depends what you mean by "enough", it adds it to the _libxl_ API, which is
sufficient for the patch to be useful but it doesn't actually cause the
option to be available to users of xl via a cfg file, which arguably it
should.

To make it available to xl users patches to tools/libxl/xl_cmdimpl.c would
be needed (plus docs changes)

Ian.

Re: [PATCH v8] run QEMU as non-root

[Stefano Stabellini]

On Mon, 5 Oct 2015, Ian Campbell wrote:
> On Mon, 2015-10-05 at 16:53 +0100, Stefano Stabellini wrote:
> > > Wasn't there some code to plumb this into xl at one point? Did that get
> > > dropped along the way?
> > 
> > device_model_user is added to the idl by this patch, I think that is
> > enough, right?
> 
> Depends what you mean by "enough", it adds it to the _libxl_ API, which is
> sufficient for the patch to be useful but it doesn't actually cause the
> option to be available to users of xl via a cfg file, which arguably it
> should.
> 
> To make it available to xl users patches to tools/libxl/xl_cmdimpl.c would
> be needed (plus docs changes)

I see. No, xl_cmdimpl.c support for the option was never written, as it
was introduced principally for libvirt's benefit.

Re: [PATCH v8] run QEMU as non-root

[Ian Campbell]

On Tue, 2015-10-06 at 14:13 +0100, Stefano Stabellini wrote:
> On Mon, 5 Oct 2015, Ian Campbell wrote:
> > On Mon, 2015-10-05 at 16:53 +0100, Stefano Stabellini wrote:
> > > > Wasn't there some code to plumb this into xl at one point? Did that
> > > > get
> > > > dropped along the way?
> > > 
> > > device_model_user is added to the idl by this patch, I think that is
> > > enough, right?
> > 
> > Depends what you mean by "enough", it adds it to the _libxl_ API, which
> > is
> > sufficient for the patch to be useful but it doesn't actually cause the
> > option to be available to users of xl via a cfg file, which arguably it
> > should.
> > 
> > To make it available to xl users patches to tools/libxl/xl_cmdimpl.c
> > would
> > be needed (plus docs changes)
> 
> I see. No, xl_cmdimpl.c support for the option was never written, as it
> was introduced principally for libvirt's benefit.

Ah, I was wondering how this had been tested...

Ian.

Re: [PATCH v8] run QEMU as non-root

[Jim Fehlig]

Ian Campbell wrote:
> On Tue, 2015-10-06 at 14:13 +0100, Stefano Stabellini wrote:
>> On Mon, 5 Oct 2015, Ian Campbell wrote:
>>> On Mon, 2015-10-05 at 16:53 +0100, Stefano Stabellini wrote:
>>>>> Wasn't there some code to plumb this into xl at one point? Did that
>>>>> get
>>>>> dropped along the way?
>>>> device_model_user is added to the idl by this patch, I think that is
>>>> enough, right?
>>> Depends what you mean by "enough", it adds it to the _libxl_ API, which
>>> is
>>> sufficient for the patch to be useful but it doesn't actually cause the
>>> option to be available to users of xl via a cfg file, which arguably it
>>> should.
>>>
>>> To make it available to xl users patches to tools/libxl/xl_cmdimpl.c
>>> would
>>> be needed (plus docs changes)
>> I see. No, xl_cmdimpl.c support for the option was never written, as it
>> was introduced principally for libvirt's benefit.
> 
> Ah, I was wondering how this had been tested...

Currently, there is no libivrt code to using this. But surely users would like
to specify the qemu user in their xl config right?

Regards,
Jim

Re: [PATCH v8] run QEMU as non-root

[Ian Campbell]

On Tue, 2015-10-06 at 09:29 -0600, Jim Fehlig wrote:

> Currently, there is no libivrt code to using this. But surely users would like
> to specify the qemu user in their xl config right?

Surely. Stefano can you take care of this in a followup please.

NB I went to apply this patch for now but there were several conflicts with
something in staging.

Ian.