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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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.

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