Re: [PATCH v4 4/6] tools/dm_restrict: Unshare mount and IPC namespaces on Linux

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> Of George Dunlap
> Sent: 05 November 2018 18:07
> To: xen-devel@lists.xenproject.org
> Cc: Anthony Perard <anthony.perard@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Wei Liu <wei.liu2@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>
> Subject: [Xen-devel] [PATCH v4 4/6] tools/dm_restrict: Unshare mount and
> IPC namespaces on Linux
> 
> QEMU running under Xen doesn't need mount or IPC functionality.
> Create and enter separate namespaces for each of these before
> executing QEMU, so that in the event that other restrictions fail, the
> process won't be able to even name system mount points or exsting
> non-file-based IPC descriptors to attempt to attack them.
> 
> Unsharing is something a process can only do to itself (it would
> seem); so add an os-specific "dm_preexec_restrict()" hook just before
> we exec() the device model.
> 
> Also add checks to depriv-process-checker.sh to verify that dm is
> running in a new namespace (or at least, a different one than the
> caller).
> 
> Suggested-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
> Changes since v3:
> - Fix some more style issues
> 
> Changes since v2:
> - Return an error rather than calling exit()
> - Use LOGE() and print to the current stderr fd, rather than
>   printing to the new stderr fd via write()
> - Use r for external return values rather than rc.
> 
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Anthony Perard <anthony.perard@citrix.com>
> ---
>  docs/designs/qemu-deprivilege.md | 12 ++++++------
>  tools/libxl/libxl_dm.c           |  5 +++++
>  tools/libxl/libxl_freebsd.c      |  5 +++++
>  tools/libxl/libxl_internal.h     |  5 +++++
>  tools/libxl/libxl_linux.c        | 14 ++++++++++++++
>  tools/libxl/libxl_netbsd.c       |  5 +++++
>  6 files changed, 40 insertions(+), 6 deletions(-)
> 
> diff --git a/docs/designs/qemu-deprivilege.md b/docs/designs/qemu-
> deprivilege.md
> index 0395bbbb40..a461ebbadd 100644
> --- a/docs/designs/qemu-deprivilege.md
> +++ b/docs/designs/qemu-deprivilege.md
> @@ -78,12 +78,6 @@ Then adds the following to the qemu command-line:
> 
>  '''Tested''': Not tested
> 
> -## Restrictions / improvements still to do
> -
> -This lists potential restrictions still to do.  It is meant to be
> -listed in order of ease of implementation, with low-hanging fruit
> -first.
> -
>  ## Namespaces for unused functionality (Linux only)
> 
>  '''Description''': QEMU doesn't use the functionality associated with
> @@ -111,6 +105,12 @@ call:
> 
>  [qemu-namespaces]: https://lists.gnu.org/archive/html/qemu-devel/2017-
> 10/msg04723.html
> 
> +# Restrictions / improvements still to do
> +
> +This lists potential restrictions still to do.  It is meant to be
> +listed in order of ease of implementation, with low-hanging fruit
> +first.
> +
>  ### Basic RLIMITs
> 
>  '''Description''': A number of limits on the resources that a given
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index ad3efcc783..278cfd6e6e 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -2393,6 +2393,11 @@ retry_transaction:
>          goto out_close;
>      if (!rc) { /* inner child */
>          setsid();
> +        if (libxl_defbool_val(b_info->dm_restrict)) {
> +            rc = libxl__local_dm_preexec_restrict(gc);
> +            if (rc)
> +                _exit(-1);
> +        }
>          libxl__exec(gc, null, logfile_w, logfile_w, dm, args, envs);
>      }
> 
> diff --git a/tools/libxl/libxl_freebsd.c b/tools/libxl/libxl_freebsd.c
> index 6442ccec72..f7ef4a8910 100644
> --- a/tools/libxl/libxl_freebsd.c
> +++ b/tools/libxl/libxl_freebsd.c
> @@ -245,3 +245,8 @@ int libxl__pci_topology_init(libxl__gc *gc,
>  {
>      return ERROR_NI;
>  }
> +
> +int libxl__local_dm_preexec_restrict(libxl__gc *gc)
> +{
> +    return 0;
> +}
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index ff889385fe..e498435e16 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -3774,6 +3774,11 @@ struct libxl__dm_spawn_state {
> 
>  _hidden void libxl__spawn_local_dm(libxl__egc *egc,
> libxl__dm_spawn_state*);
> 
> +/*
> + * Called after forking but before executing the local devicemodel.
> + */
> +_hidden int libxl__local_dm_preexec_restrict(libxl__gc *gc);
> +
>  /* Stubdom device models. */
> 
>  typedef struct {
> diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
> index 6ef0abc693..c7a345f4bb 100644
> --- a/tools/libxl/libxl_linux.c
> +++ b/tools/libxl/libxl_linux.c
> @@ -307,6 +307,20 @@ int libxl__pci_topology_init(libxl__gc *gc,
>      return err;
>  }
> 
> +int libxl__local_dm_preexec_restrict(libxl__gc *gc)
> +{
> +    int r;
> +
> +    /* Unshare mount and IPC namespaces.  These are unused by QEMU. */
> +    r = unshare(CLONE_NEWNS | CLONE_NEWIPC);
> +    if (r) {
> +        LOGE(ERROR, "libxl: Mount and IPC namespace unfailed");
> +        return ERROR_FAIL;
> +    }
> +
> +    return 0;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
> index 2edfb00641..dce3f1fdce 100644
> --- a/tools/libxl/libxl_netbsd.c
> +++ b/tools/libxl/libxl_netbsd.c
> @@ -124,3 +124,8 @@ int libxl__pci_topology_init(libxl__gc *gc,
>  {
>      return ERROR_NI;
>  }
> +
> +void libxl__local_dm_preexec_restrict(libxl__gc *gc, int stderrfd)
> +{
> +    return;
> +}

This is a void function whereas the caller always appears to expect an int return value, regardless of OS.

  Paul

> --
> 2.19.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel