[PATCH 0/3] virtiofsd capability changes and addition

[PATCH 0/3] virtiofsd capability changes and addition

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Hi,
  This is a set of changes relating to the capability restirctions
introduced in virtiofsd back in a59feb483b8.

The first one is a potentially important fix; the missing terminator
could mean extra capabilities are added based on junk on the stack;
although that's not been seen in practice.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>



Dr. David Alan Gilbert (3):
  virtiofsd: Terminate capability list
  virtiofsd: Check capability calls
  virtiofsd: Allow addition or removal of capabilities

 docs/tools/virtiofsd.rst         |  5 +++
 tools/virtiofsd/helper.c         |  2 +
 tools/virtiofsd/passthrough_ll.c | 68 +++++++++++++++++++++++++++++---
 3 files changed, 70 insertions(+), 5 deletions(-)

-- 
2.26.2

[PATCH 1/3] virtiofsd: Terminate capability list

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

capng_updatev is a varargs function that needs a -1 to terminate it,
but it was missing.

In practice what seems to have been happening is that it's added the
capabilities we asked for, then runs into junk on the stack, so if
we're unlucky it might be adding some more, but in reality it's
failing - but after adding the capabilities we asked for.

Fixes: a59feb483b8 ("virtiofsd: only retain file system capabilities")
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 2ce7c96085..e373e3b36e 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -2598,7 +2598,9 @@ static void setup_capabilities(void)
             CAP_SETGID,
             CAP_SETUID,
             CAP_MKNOD,
-            CAP_SETFCAP);
+            CAP_SETFCAP,
+            -1);
+
     capng_apply(CAPNG_SELECT_BOTH);
 
     cap.saved = capng_save_state();
-- 
2.26.2

Re: [PATCH 1/3] virtiofsd: Terminate capability list

[Stefan Hajnoczi]

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

On Thu, Jun 25, 2020 at 05:29:27PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> capng_updatev is a varargs function that needs a -1 to terminate it,
> but it was missing.
> 
> In practice what seems to have been happening is that it's added the
> capabilities we asked for, then runs into junk on the stack, so if
> we're unlucky it might be adding some more, but in reality it's
> failing - but after adding the capabilities we asked for.
> 
> Fixes: a59feb483b8 ("virtiofsd: only retain file system capabilities")
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  tools/virtiofsd/passthrough_ll.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 2/3] virtiofsd: Check capability calls

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Check the capability calls worked.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index e373e3b36e..99d562046a 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -2589,7 +2589,7 @@ static void setup_capabilities(void)
      */
     capng_setpid(syscall(SYS_gettid));
     capng_clear(CAPNG_SELECT_BOTH);
-    capng_updatev(CAPNG_ADD, CAPNG_PERMITTED | CAPNG_EFFECTIVE,
+    if (capng_updatev(CAPNG_ADD, CAPNG_PERMITTED | CAPNG_EFFECTIVE,
             CAP_CHOWN,
             CAP_DAC_OVERRIDE,
             CAP_DAC_READ_SEARCH,
@@ -2599,11 +2599,21 @@ static void setup_capabilities(void)
             CAP_SETUID,
             CAP_MKNOD,
             CAP_SETFCAP,
-            -1);
+            -1)) {
+        fuse_log(FUSE_LOG_ERR, "%s: capng_updatev failed\n", __func__);
+        exit(1);
+    }
 
-    capng_apply(CAPNG_SELECT_BOTH);
+    if (capng_apply(CAPNG_SELECT_BOTH)) {
+        fuse_log(FUSE_LOG_ERR, "%s: capng_apply failed\n", __func__);
+        exit(1);
+    }
 
     cap.saved = capng_save_state();
+    if (!cap.saved) {
+        fuse_log(FUSE_LOG_ERR, "%s: capng_save_state failed\n", __func__);
+        exit(1);
+    }
     pthread_mutex_unlock(&cap.mutex);
 }
 
-- 
2.26.2

Re: [PATCH 2/3] virtiofsd: Check capability calls

[Stefan Hajnoczi]

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

On Thu, Jun 25, 2020 at 05:29:28PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Check the capability calls worked.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  tools/virtiofsd/passthrough_ll.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 3/3] virtiofsd: Allow addition or removal of capabilities

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Allow capabilities to be added or removed from the allowed set for the
daemon; e.g.

default:
CapPrm: 00000000880000df
CapEff: 00000000880000df

-o modcaps=+sys_admin

CapPrm: 00000000882000df
CapEff: 00000000882000df

-o modcaps=+sys_admin:-chown

CapPrm: 00000000882000de
CapEff: 00000000882000de

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 docs/tools/virtiofsd.rst         |  5 ++++
 tools/virtiofsd/helper.c         |  2 ++
 tools/virtiofsd/passthrough_ll.c | 50 ++++++++++++++++++++++++++++++--
 3 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/docs/tools/virtiofsd.rst b/docs/tools/virtiofsd.rst
index 378594c422..824e713491 100644
--- a/docs/tools/virtiofsd.rst
+++ b/docs/tools/virtiofsd.rst
@@ -54,6 +54,11 @@ Options
   * flock|no_flock -
     Enable/disable flock.  The default is ``no_flock``.
 
+  * modcaps=CAPLIST
+    Modify the list of capabilities allowed; CAPLIST is a colon separated
+    list of capabilities, each preceded by either + or -, e.g.
+    ''+sys_admin:-chown''.
+
   * log_level=LEVEL -
     Print only log messages matching LEVEL or more severe.  LEVEL is one of
     ``err``, ``warn``, ``info``, or ``debug``.  The default is ``info``.
diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c
index 00a1ef666a..3105b6c23a 100644
--- a/tools/virtiofsd/helper.c
+++ b/tools/virtiofsd/helper.c
@@ -174,6 +174,8 @@ void fuse_cmdline_help(void)
            "                               default: no_writeback\n"
            "    -o xattr|no_xattr          enable/disable xattr\n"
            "                               default: no_xattr\n"
+           "    -o modcaps=CAPLIST         Modify the list of capabilities\n"
+           "                               e.g. -o modcaps=+sys_admin:-chown\n"
            "    --rlimit-nofile=<num>      set maximum number of file descriptors\n"
            "                               (0 leaves rlimit unchanged)\n"
            "                               default: min(1000000, fs.file-max - 16384)\n"
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 99d562046a..9d2cbc70ca 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -145,6 +145,7 @@ struct lo_data {
     int posix_lock;
     int xattr;
     char *source;
+    char *modcaps;
     double timeout;
     int cache;
     int timeout_set;
@@ -170,6 +171,7 @@ static const struct fuse_opt lo_opts[] = {
     { "no_posix_lock", offsetof(struct lo_data, posix_lock), 0 },
     { "xattr", offsetof(struct lo_data, xattr), 1 },
     { "no_xattr", offsetof(struct lo_data, xattr), 0 },
+    { "modcaps=%s", offsetof(struct lo_data, modcaps), 0 },
     { "timeout=%lf", offsetof(struct lo_data, timeout), 0 },
     { "timeout=", offsetof(struct lo_data, timeout_set), 1 },
     { "cache=none", offsetof(struct lo_data, cache), CACHE_NONE },
@@ -2571,7 +2573,7 @@ static void setup_mounts(const char *source)
 /*
  * Only keep whitelisted capabilities that are needed for file system operation
  */
-static void setup_capabilities(void)
+static void setup_capabilities(struct lo_data *lo)
 {
     pthread_mutex_lock(&cap.mutex);
     capng_restore_state(&cap.saved);
@@ -2604,6 +2606,50 @@ static void setup_capabilities(void)
         exit(1);
     }
 
+    /*
+     * The modcaps option is a colon separated list of caps,
+     * each preceded by either + or -.
+     */
+    while (lo->modcaps) {
+        capng_act_t action;
+        int cap;
+
+        char *next = strchr(lo->modcaps, ':');
+        if (next) {
+            *next = '\0';
+            next++;
+        }
+
+        switch (lo->modcaps[0]) {
+        case '+':
+            action = CAPNG_ADD;
+            break;
+
+        case '-':
+            action = CAPNG_DROP;
+            break;
+
+        default:
+            fuse_log(FUSE_LOG_ERR,
+                     "%s: Expecting '+'/'-' in modcaps but found '%c'\n",
+                     __func__, lo->modcaps[0]);
+            exit(1);
+        }
+        cap = capng_name_to_capability(lo->modcaps + 1);
+        if (cap < 0) {
+            fuse_log(FUSE_LOG_ERR, "%s: Unknown capability '%s'\n", __func__,
+                     lo->modcaps);
+            exit(1);
+        }
+        if (capng_update(action, CAPNG_PERMITTED | CAPNG_EFFECTIVE, cap)) {
+            fuse_log(FUSE_LOG_ERR, "%s: capng_update failed for '%s'\n",
+                     __func__, lo->modcaps);
+            exit(1);
+        }
+
+        lo->modcaps = next;
+    }
+
     if (capng_apply(CAPNG_SELECT_BOTH)) {
         fuse_log(FUSE_LOG_ERR, "%s: capng_apply failed\n", __func__);
         exit(1);
@@ -2627,7 +2673,7 @@ static void setup_sandbox(struct lo_data *lo, struct fuse_session *se,
     setup_namespaces(lo, se);
     setup_mounts(lo->source);
     setup_seccomp(enable_syslog);
-    setup_capabilities();
+    setup_capabilities(lo);
 }
 
 /* Set the maximum number of open file descriptors */
-- 
2.26.2

Re: [PATCH 3/3] virtiofsd: Allow addition or removal of capabilities

[Stefan Hajnoczi]

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

On Thu, Jun 25, 2020 at 05:29:29PM +0100, Dr. David Alan Gilbert (git) wrote:
> +    /*
> +     * The modcaps option is a colon separated list of caps,
> +     * each preceded by either + or -.
> +     */
> +    while (lo->modcaps) {
> +        capng_act_t action;
> +        int cap;
> +
> +        char *next = strchr(lo->modcaps, ':');
> +        if (next) {
> +            *next = '\0';
> +            next++;
> +        }
> +
> +        switch (lo->modcaps[0]) {
> +        case '+':
> +            action = CAPNG_ADD;
> +            break;
> +
> +        case '-':
> +            action = CAPNG_DROP;
> +            break;
> +
> +        default:
> +            fuse_log(FUSE_LOG_ERR,
> +                     "%s: Expecting '+'/'-' in modcaps but found '%c'\n",
> +                     __func__, lo->modcaps[0]);
> +            exit(1);
> +        }
> +        cap = capng_name_to_capability(lo->modcaps + 1);
> +        if (cap < 0) {
> +            fuse_log(FUSE_LOG_ERR, "%s: Unknown capability '%s'\n", __func__,
> +                     lo->modcaps);
> +            exit(1);
> +        }
> +        if (capng_update(action, CAPNG_PERMITTED | CAPNG_EFFECTIVE, cap)) {
> +            fuse_log(FUSE_LOG_ERR, "%s: capng_update failed for '%s'\n",
> +                     __func__, lo->modcaps);
> +            exit(1);
> +        }
> +
> +        lo->modcaps = next;

How about passing char *modcaps into this function so that lo->modcaps
isn't modified by the parsing loop? That seems a bit cleaner and if we
ever decide to free lo->modcaps it will work as expected.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] virtiofsd: Allow addition or removal of capabilities

[Dr. David Alan Gilbert]

* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> On Thu, Jun 25, 2020 at 05:29:29PM +0100, Dr. David Alan Gilbert (git) wrote:
> > +    /*
> > +     * The modcaps option is a colon separated list of caps,
> > +     * each preceded by either + or -.
> > +     */
> > +    while (lo->modcaps) {
> > +        capng_act_t action;
> > +        int cap;
> > +
> > +        char *next = strchr(lo->modcaps, ':');
> > +        if (next) {
> > +            *next = '\0';
> > +            next++;
> > +        }
> > +
> > +        switch (lo->modcaps[0]) {
> > +        case '+':
> > +            action = CAPNG_ADD;
> > +            break;
> > +
> > +        case '-':
> > +            action = CAPNG_DROP;
> > +            break;
> > +
> > +        default:
> > +            fuse_log(FUSE_LOG_ERR,
> > +                     "%s: Expecting '+'/'-' in modcaps but found '%c'\n",
> > +                     __func__, lo->modcaps[0]);
> > +            exit(1);
> > +        }
> > +        cap = capng_name_to_capability(lo->modcaps + 1);
> > +        if (cap < 0) {
> > +            fuse_log(FUSE_LOG_ERR, "%s: Unknown capability '%s'\n", __func__,
> > +                     lo->modcaps);
> > +            exit(1);
> > +        }
> > +        if (capng_update(action, CAPNG_PERMITTED | CAPNG_EFFECTIVE, cap)) {
> > +            fuse_log(FUSE_LOG_ERR, "%s: capng_update failed for '%s'\n",
> > +                     __func__, lo->modcaps);
> > +            exit(1);
> > +        }
> > +
> > +        lo->modcaps = next;
> 
> How about passing char *modcaps into this function so that lo->modcaps
> isn't modified by the parsing loop? That seems a bit cleaner and if we
> ever decide to free lo->modcaps it will work as expected.

Yep, can do.

Dave

> Stefan


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK