Re: [Xen-devel] [PATCH v2 5/6] tools/dm_depriv: Add first cut RLIMITs

[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 5/6] tools/dm_depriv: Add first cut RLIMITs
> 
> Limit the ability of a potentially compromised QEMU to consume system
> resources.  Key limits:
>  - RLIMIT_FSIZE (file size): 256KiB
>  - RLIMIT_NPROC (after uid changes to a unique uid)
> 
> Probably unnecessary limits but why not:
>  - RLIMIT_CORE: 0
>  - RLIMIT_MSGQUEUE: 0
>  - RLIMIT_LOCKS: 0
>  - RLIMIT_MEMLOCK: 0
> 
> NB that we do not yet set RLIMIT_AS (total virtual memory) or
> RLIMIT_NOFILES (number of open files), since these require more care
> and/or more coordination with QEMU to implement.
> 
> 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_linux.c                    | 53 ++++++++++++++++++++
>  tools/tests/depriv/depriv-process-checker.sh | 38 ++++++++++++++
>  3 files changed, 97 insertions(+), 6 deletions(-)
> 
> diff --git a/docs/designs/qemu-deprivilege.md b/docs/designs/qemu-
> deprivilege.md
> index 58d5df6072..ca556005b5 100644
> --- a/docs/designs/qemu-deprivilege.md
> +++ b/docs/designs/qemu-deprivilege.md
> @@ -96,12 +96,6 @@ call:
> 
>  '''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.
> -
>  ### Basic RLIMITs
> 
>  '''Description''': A number of limits on the resources that a given
> @@ -126,6 +120,12 @@ Probably not necessary but why not:
> 
>  '''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.
> +
>  ### Further RLIMITs
> 
>  RLIMIT_AS limits the total amount of memory; but this includes the
> diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
> index 2eeac8df9a..a7ae5af30e 100644
> --- a/tools/libxl/libxl_linux.c
> +++ b/tools/libxl/libxl_linux.c
> @@ -16,6 +16,7 @@
>  #include "libxl_osdeps.h" /* must come before any other headers */
> 
>  #include "libxl_internal.h"
> +#include <sys/resource.h>
> 
>  int libxl__try_phy_backend(mode_t st_mode)
>  {
> @@ -307,9 +308,44 @@ int libxl__pci_topology_init(libxl__gc *gc,
>      return err;
>  }
> 
> +static struct {
> +    int resource;
> +    rlim_t limit;
> +} rlimits[] = {
> +    {
> +        .resource = RLIMIT_FSIZE,
> +        /* Big enough for log files, not big enough for a DoS */
> +        .limit = 256*1024,
> +    },
> +    {
> +        .resource = RLIMIT_NPROC,
> +        .limit = 0
> +    },
> +    {
> +        .resource = RLIMIT_CORE,
> +        .limit = 0
> +    },
> +    {
> +        .resource = RLIMIT_MSGQUEUE,
> +        .limit = 0
> +    },
> +    {
> +        .resource = RLIMIT_LOCKS,
> +        .limit = 0
> +    },
> +    {
> +        .resource = RLIMIT_MEMLOCK,
> +        .limit = 0
> +    },
> +    {
> +        .resource = -1

Is -1 guaranteed not to clash with any defined resource type?

> +    }
> +};
> +
>  void libxl__local_dm_preexec_restrict(libxl__gc *gc, int stderrfd)
>  {
>      int rc;
> +    unsigned i;
> 
>      /* Unshare mount and IPC namespaces.  These are unused by QEMU. */
>      rc = unshare(CLONE_NEWNS | CLONE_NEWIPC);
> @@ -319,6 +355,23 @@ void libxl__local_dm_preexec_restrict(libxl__gc *gc,
> int stderrfd)
>          write(stderrfd, msg, strlen(msg));
>          _exit(-1);
>      }
> +
> +    /* Set various "easy" rlimits */
> +    for (i=0; rlimits[i].resource != -1; i++) {

Couldn't figure out whether this is a coding style violation or not but I think 
'i=0' should be 'i = 0'

  Paul

> +        struct rlimit rlim;
> +
> +        rlim.rlim_cur = rlim.rlim_max = rlimits[i].limit;
> +
> +        rc = setrlimit(rlimits[i].resource, &rlim);
> +        if (rc < 0) {
> +            char *msg = GCSPRINTF("libxl: Setting rlimit %d to %lld
> failed with error %d\n",
> +                                  rlimits[i].resource,
> +                                  (unsigned long long)rlimits[i].limit,
> errno);
> +            write(stderrfd, msg, strlen(msg));
> +            _exit(-1);
> +        }
> +
> +    }
>  }
> 
>  /*
> diff --git a/tools/tests/depriv/depriv-process-checker.sh
> b/tools/tests/depriv/depriv-process-checker.sh
> index 7dc2573799..6a861bafa5 100755
> --- a/tools/tests/depriv/depriv-process-checker.sh
> +++ b/tools/tests/depriv/depriv-process-checker.sh
> @@ -98,6 +98,44 @@ for nsname in ipc mnt; do
>      fi
>  done
> 
> +# TEST: RLIMITs
> +#
> +# Read /proc/<dmpid>/limits
> +function check_rlimit() {
> +    limit_name=$1
> +    limit_string=$2
> +    tgt=$3
> +
> +    echo -n "rlimit $limit_name: "
> +    input=$(grep "^$limit_string" /proc/$dmpid/limits)
> +
> +    if [[ -z "$input" ]] ; then
> +     echo "Couldn't find limit $limit"
> +     echo FAILED
> +     failed="true"
> +     return
> +    fi
> +
> +    if [[ "$input" =~
> ^$limit_string[[:space:]]*([^[:space:]]+)[[:space:]]*([^[:space:]]+)[[:spa
> ce:]]*[^[:space:]]+ ]] ; then
> +     if [[ "${BASH_REMATCH[1]}" != $tgt ||
> +               "${BASH_REMATCH[2]}" != $tgt ]] ; then
> +         echo "FAILED"
> +         failed="true"
> +     else
> +         echo "PASSED"
> +     fi
> +    else
> +     echo "Couldn't parse /proc/<dmpid>/limits"
> +     echo "FAILED"
> +     failed="true"
> +    fi
> +}
> +check_rlimit FSIZE "Max file size" "262144"
> +check_rlimit NPROC "Max processes" 0
> +check_rlimit CORE "Max core file size" "0"
> +check_rlimit MSGQUEUE "Max msgqueue size" 0
> +check_rlimit LOCKS "Max file locks" 0
> +check_rlimit MEMLOCK "Max locked memory" 0
> 
>  if $failed ; then
>      exit 1
> --
> 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