[PATCH v7] run QEMU as non-root

[PATCH v7] 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>

---
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         |   55 ++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h   |    4 +++
 tools/libxl/libxl_types.idl    |    1 +
 6 files changed, 104 insertions(+)
 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..12eb104
--- /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) and 2) fail, 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..24c43df 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,33 @@ 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, char *username)
+{
+    struct passwd pwd, *user = NULL;
+    char *buf = NULL;
+    long buf_size;
+
+    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 -1;
+    }
+
+retry:
+    buf = libxl__realloc(gc, buf, buf_size);
+    errno = 0;
+    getpwnam_r(username, &pwd, buf, buf_size, &user);
+    if (user != NULL)
+        return 1;
+    if (errno == ERANGE) {
+        buf_size += 128;
+        goto retry;
+    }
+    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,
@@ -439,6 +468,7 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
     int i, connection, devid;
     uint64_t ram_size;
     const char *path, *chardev;
+    char *user = NULL;
 
     dm_args = flexarray_make(gc, 16, 1);
 
@@ -878,6 +908,31 @@ 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);
+        if (libxl__dm_runas_helper(gc, user) > 0)
+            goto end_search;
+
+        user = LIBXL_QEMU_USER_SHARED;
+        if (libxl__dm_runas_helper(gc, user) > 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) {
+            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 v7] run QEMU as non-root

[Fabio Fantoni]

Il 23/07/2015 19:08, Stefano Stabellini ha scritto:
> 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>

Thanks for this patch, now I'll test it.
I think can be good add also domU's xl cfg parameter for set custom user 
to use instead make possible only in libxl from external tools, is 
possible to add it?
For example in my case I use xl and I want run domU's qemu with user not 
shared but I want create only one user for each effective domU and not 
thousand of users.
Another comment below...

>
> ---
> 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         |   55 ++++++++++++++++++++++++++++++++++++++++
>   tools/libxl/libxl_internal.h   |    4 +++
>   tools/libxl/libxl_types.idl    |    1 +
>   6 files changed, 104 insertions(+)
>   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..12eb104
> --- /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) and 2) fail, 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..24c43df 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,33 @@ 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, char *username)
> +{
> +    struct passwd pwd, *user = NULL;
> +    char *buf = NULL;
> +    long buf_size;
> +
> +    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 -1;
> +    }
> +
> +retry:
> +    buf = libxl__realloc(gc, buf, buf_size);
> +    errno = 0;
> +    getpwnam_r(username, &pwd, buf, buf_size, &user);
> +    if (user != NULL)
> +        return 1;
> +    if (errno == ERANGE) {
> +        buf_size += 128;
> +        goto retry;
> +    }
> +    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,
> @@ -439,6 +468,7 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>       int i, connection, devid;
>       uint64_t ram_size;
>       const char *path, *chardev;
> +    char *user = NULL;
>   
>       dm_args = flexarray_make(gc, 16, 1);
>   
> @@ -878,6 +908,31 @@ 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;
> +        }

If user is defined don't check if it exist as it do with other 2 cases 
below, I think is good check if user exist also here.

> +
> +        user = libxl__sprintf(gc, "%s%d", LIBXL_QEMU_USER_BASE, guest_domid);
> +        if (libxl__dm_runas_helper(gc, user) > 0)
> +            goto end_search;
> +
> +        user = LIBXL_QEMU_USER_SHARED;
> +        if (libxl__dm_runas_helper(gc, user) > 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) {
> +            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),

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

[Wei Liu]

On Mon, Jul 27, 2015 at 03:19:56PM +0200, Fabio Fantoni wrote:
> Il 23/07/2015 19:08, Stefano Stabellini ha scritto:
> >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>
> 
> Thanks for this patch, now I'll test it.
> I think can be good add also domU's xl cfg parameter for set custom user to
> use instead make possible only in libxl from external tools, is possible to
> add it?

It looks trivial to me. The hardest part would be picking a name for
the new option and writing that down in manpage. :-)

Wei.

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

[Wei Liu]

On Thu, Jul 23, 2015 at 06:08:02PM +0100, Stefano Stabellini wrote:
[...]
> +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) and 2) fail, libxl will use a single user for
                     ^^^^^^^^^^^^^^
This is 2)

> +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..24c43df 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,33 @@ 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, char *username)

const char *

> +{
> +    struct passwd pwd, *user = NULL;
> +    char *buf = NULL;
> +    long buf_size;
> +
> +    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 -1;
> +    }
> +
> +retry:
> +    buf = libxl__realloc(gc, buf, buf_size);

This should be libxl__alloc and placed out of the loop?

> +    errno = 0;
> +    getpwnam_r(username, &pwd, buf, buf_size, &user);
> +    if (user != NULL)
> +        return 1;
> +    if (errno == ERANGE) {
> +        buf_size += 128;
> +        goto retry;
> +    }

Please use for / while to loop.

Also you might want to save and restore errno.

> +    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,
> @@ -439,6 +468,7 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>      int i, connection, devid;
>      uint64_t ram_size;
>      const char *path, *chardev;
> +    char *user = NULL;
>  
>      dm_args = flexarray_make(gc, 16, 1);
>  
> @@ -878,6 +908,31 @@ 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);
> +        if (libxl__dm_runas_helper(gc, user) > 0)
> +            goto end_search;

You haven't checked if libxl__dm_runas_helper returns -1. In that case
we should bail?

> +
> +        user = LIBXL_QEMU_USER_SHARED;
> +        if (libxl__dm_runas_helper(gc, user) > 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);
> +

Line too long.

Wei.

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

[Stefano Stabellini]

On Fri, 7 Aug 2015, Wei Liu wrote:
> On Thu, Jul 23, 2015 at 06:08:02PM +0100, Stefano Stabellini wrote:
> [...]
> > +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) and 2) fail, libxl will use a single user for
>                      ^^^^^^^^^^^^^^
> This is 2)

right

> > +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..24c43df 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,33 @@ 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, char *username)
> 
> const char *

ok


> > +{
> > +    struct passwd pwd, *user = NULL;
> > +    char *buf = NULL;
> > +    long buf_size;
> > +
> > +    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 -1;
> > +    }
> > +
> > +retry:
> > +    buf = libxl__realloc(gc, buf, buf_size);
> 
> This should be libxl__alloc and placed out of the loop?

But the point is that buf can be increase by 128 bytes chunks in
following iterations. How can I do that if I place it outside the loop?


> > +    errno = 0;
> > +    getpwnam_r(username, &pwd, buf, buf_size, &user);
> > +    if (user != NULL)
> > +        return 1;
> > +    if (errno == ERANGE) {
> > +        buf_size += 128;
> > +        goto retry;
> > +    }
> 
> Please use for / while to loop.

The goto retry loop is a very common patter for error handling, but I
can turn it into a loop if you are keen on it.


> Also you might want to save and restore errno.

Across the getpwnam_r call? Or across the loop? Or across the function
libxl__dm_runas_helper?


> > +    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,
> > @@ -439,6 +468,7 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
> >      int i, connection, devid;
> >      uint64_t ram_size;
> >      const char *path, *chardev;
> > +    char *user = NULL;
> >  
> >      dm_args = flexarray_make(gc, 16, 1);
> >  
> > @@ -878,6 +908,31 @@ 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);
> > +        if (libxl__dm_runas_helper(gc, user) > 0)
> > +            goto end_search;
> 
> You haven't checked if libxl__dm_runas_helper returns -1. In that case
> we should bail?

Yes, I think return NULL would be OK.


> > +
> > +        user = LIBXL_QEMU_USER_SHARED;
> > +        if (libxl__dm_runas_helper(gc, user) > 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);
> > +
> 
> Line too long.
> 
> Wei.
> 

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

[Ian Jackson]

Stefano Stabellini writes ("Re: [PATCH v7] run QEMU as non-root"):
> On Fri, 7 Aug 2015, Wei Liu wrote:
> > Please use for / while to loop.
> 
> The goto retry loop is a very common patter for error handling, but I
> can turn it into a loop if you are keen on it.

I'm afraid I agree with Wei here.

> > Also you might want to save and restore errno.
> 
> Across the getpwnam_r call? Or across the loop? Or across the function
> libxl__dm_runas_helper?

I don't understand either.  Also I don't understand why you set errno
to 0.
> 
Ian.

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

[Ian Campbell]

On Tue, 2015-09-29 at 18:07 +0100, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [PATCH v7] run QEMU as non-root"):
> > On Fri, 7 Aug 2015, Wei Liu wrote:
> > > Please use for / while to loop.
> > 
> > The goto retry loop is a very common patter for error handling, but I
> > can turn it into a loop if you are keen on it.
> 
> I'm afraid I agree with Wei here.
> 
> > > Also you might want to save and restore errno.
> > 
> > Across the getpwnam_r call? Or across the loop? Or across the function
> > libxl__dm_runas_helper?
> 
> I don't understand either.  Also I don't understand why you set errno
> to 0.

getpwnam(7) says:

RETURN VALUE
       The getpwnam() and getpwuid() functions return a pointer to a
       passwd structure, or NULL if the matching entry is not  found
       or  an error occurs.  If an error occurs, errno is set appro‐
       priately.  If one wants to check errno  after  the  call,  it
       should be set to zero before the call.

But this code is using the _r variants, on which the manpage later says:

       On  success,  getpwnam_r()  and getpwuid_r() return zero, and
       set *result to pwd.   If  no  matching  password  record  was
       found,  these  functions  return 0 and store NULL in *result.
       In case of error, an error number is returned,  and  NULL  is
       stored in *result.

So I guess the errno is either a leftover from not using _r in an earlier
version or from applying the first para above to the wrong functions.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1721 bytes --]

On Wed, 30 Sep 2015, Ian Campbell wrote:
> On Tue, 2015-09-29 at 18:07 +0100, Ian Jackson wrote:
> > Stefano Stabellini writes ("Re: [PATCH v7] run QEMU as non-root"):
> > > On Fri, 7 Aug 2015, Wei Liu wrote:
> > > > Please use for / while to loop.
> > >
> > > The goto retry loop is a very common patter for error handling, but I
> > > can turn it into a loop if you are keen on it.
> >
> > I'm afraid I agree with Wei here.
> >
> > > > Also you might want to save and restore errno.
> > >
> > > Across the getpwnam_r call? Or across the loop? Or across the function
> > > libxl__dm_runas_helper?
> >
> > I don't understand either.  Also I don't understand why you set errno
> > to 0.
>
> getpwnam(7) says:
>
> RETURN VALUE
>        The getpwnam() and getpwuid() functions return a pointer to a
>        passwd structure, or NULL if the matching entry is not  found
>        or  an error occurs.  If an error occurs, errno is set appro‐
>        priately.  If one wants to check errno  after  the  call,  it
>        should be set to zero before the call.
>
> But this code is using the _r variants, on which the manpage later says:
>
>        On  success,  getpwnam_r()  and getpwuid_r() return zero, and
>        set *result to pwd.   If  no  matching  password  record  was
>        found,  these  functions  return 0 and store NULL in *result.
>        In case of error, an error number is returned,  and  NULL  is
>        stored in *result.
>
> So I guess the errno is either a leftover from not using _r in an earlier
> version or from applying the first para above to the wrong functions.

Yes, I think it was probably a leftover from a previous version. I'll
remove it.

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel