Re: [Xen-devel] [PATCH v2 4/6] tools/dm_restrict: Unshare mount and IPC namespaces on Linux

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf
> Of George Dunlap
> Sent: 21 September 2018 18:04
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Anthony Perard <anthony.perard@xxxxxxxxxx>; Ian Jackson
> <Ian.Jackson@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>
> Subject: [Xen-devel] [PATCH v2 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@xxxxxxxxxx>
> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> ---
> 
> CC: Ian Jackson <ian.jackson@xxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Anthony Perard <anthony.perard@xxxxxxxxxx>
> ---
>  docs/designs/qemu-deprivilege.md             | 12 ++++++------
>  tools/libxl/libxl_dm.c                       |  2 ++
>  tools/libxl/libxl_freebsd.c                  |  5 +++++
>  tools/libxl/libxl_internal.h                 |  7 +++++++
>  tools/libxl/libxl_linux.c                    | 14 ++++++++++++++
>  tools/libxl/libxl_netbsd.c                   |  5 +++++
>  tools/tests/depriv/depriv-process-checker.sh | 18 ++++++++++++++++++
>  7 files changed, 57 insertions(+), 6 deletions(-)
> 
> diff --git a/docs/designs/qemu-deprivilege.md b/docs/designs/qemu-
> deprivilege.md
> index df5bb07d7c..58d5df6072 100644
> --- a/docs/designs/qemu-deprivilege.md
> +++ b/docs/designs/qemu-deprivilege.md
> @@ -75,12 +75,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''': Enter QEMU into its own mount & IPC namespaces.
> @@ -102,6 +96,12 @@ call:
> 
>  '''Tested''': Not tested

Again.. should this not become '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.
> +
>  ### 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 df6fb64bee..6733514370 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -2391,6 +2391,8 @@ retry_transaction:
>          goto out_close;
>      if (!rc) { /* inner child */
>          setsid();
> +        if (libxl_defbool_val(b_info->dm_restrict))
> +            libxl__local_dm_preexec_restrict(gc, logfile_w);
>          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..aed2a2b115 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;
>  }
> +
> +void libxl__local_dm_preexec_restrict(libxl__gc *gc, int stderrfd)
> +{
> +    return;
> +}
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 802382c704..6e5ff977a6 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -3782,6 +3782,13 @@ 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.
> + * Must call _exit(-1) on failure, printing an error message to
> + * stderrfd if available.
> + */
> +_hidden void libxl__local_dm_preexec_restrict(libxl__gc *gc, int
> stderrfd);
> +
>  /* Stubdom device models. */
> 
>  typedef struct {
> diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
> index 6ef0abc693..2eeac8df9a 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;
>  }
> 
> +void libxl__local_dm_preexec_restrict(libxl__gc *gc, int stderrfd)
> +{
> +    int rc;
> +
> +    /* Unshare mount and IPC namespaces.  These are unused by QEMU. */
> +    rc = unshare(CLONE_NEWNS | CLONE_NEWIPC);
> +    if (rc < 0) {
> +        char *msg = GCSPRINTF("libxl: Mount and IPC namespace unfailed
> with error %d\n",
> +                              errno);
> +        write(stderrfd, msg, strlen(msg));
> +        _exit(-1);
> +    }
> +}
> +
>  /*
>   * 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;
> +}
> diff --git a/tools/tests/depriv/depriv-process-checker.sh
> b/tools/tests/depriv/depriv-process-checker.sh
> index cb240fd0ed..7dc2573799 100755
> --- a/tools/tests/depriv/depriv-process-checker.sh
> +++ b/tools/tests/depriv/depriv-process-checker.sh
> @@ -81,6 +81,24 @@ else
>      echo "SKIPPED (XEN_RUN_DIR undefined)"
>  fi
> 
> +# TEST: Namespace unsharing
> +#
> +# Read /proc/<dmpid>/ns/<namespace> and make sure it's not equal to
> +# the current processes' value
> +for nsname in ipc mnt; do
> +    echo -n "Unshare namespace $nsname: "
> +    dmns=$(readlink /proc/$dmpid/ns/$nsname)
> +    myns=$(readlink /proc/self/ns/$nsname)
> +
> +    if [[ "$dmns" == "$myns" ]] ; then
> +     echo "FAILED"
> +     failed="true"
> +    else
> +     echo "PASSED"
> +    fi
> +done
> +
> +
>  if $failed ; then
>      exit 1
>  else
> --
> 2.18.0
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel