[PULL 0/1] virtiofs queue (minor security)

[PULL 0/1] virtiofs queue (minor security)

[Dr. David Alan Gilbert (git)]

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

The following changes since commit c40ae5a3ee387b13116948cbfe7824f03311db7e:

  Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging (2021-03-03 16:55:15 +0000)

are available in the Git repository at:

  https://gitlab.com/dagrh/qemu.git tags/pull-virtiofs-20210304

for you to fetch changes up to e586edcb410543768ef009eaa22a2d9dd4a53846:

  virtiofs: drop remapped security.capability xattr as needed (2021-03-04 10:26:16 +0000)

----------------------------------------------------------------
virtiofs minor security fix

Fix xattrmap to drop remapped security.capability capabilities.

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

----------------------------------------------------------------
Dr. David Alan Gilbert (1):
      virtiofs: drop remapped security.capability xattr as needed

 docs/tools/virtiofsd.rst         |  4 +++
 tools/virtiofsd/passthrough_ll.c | 77 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 80 insertions(+), 1 deletion(-)

[PULL 1/1] virtiofs: drop remapped security.capability xattr as needed

[Dr. David Alan Gilbert (git)]

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

On Linux, the 'security.capability' xattr holds a set of
capabilities that can change when an executable is run, giving
a limited form of privilege escalation to those programs that
the writer of the file deemed worthy.

Any write causes the 'security.capability' xattr to be dropped,
stopping anyone from gaining privilege by modifying a blessed
file.

Fuse relies on the daemon to do this dropping, and in turn the
daemon relies on the host kernel to drop the xattr for it.  However,
with the addition of -o xattrmap, the xattr that the guest
stores its capabilities in is now not the same as the one that
the host kernel automatically clears.

Where the mapping changes 'security.capability', explicitly clear
the remapped name to preserve the same behaviour.

This bug is assigned CVE-2021-20263.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Vivek Goyal <vgoyal@redhat.com>
---
 docs/tools/virtiofsd.rst         |  4 ++
 tools/virtiofsd/passthrough_ll.c | 77 +++++++++++++++++++++++++++++++-
 2 files changed, 80 insertions(+), 1 deletion(-)

diff --git a/docs/tools/virtiofsd.rst b/docs/tools/virtiofsd.rst
index 866b7db3ee..00554c75bd 100644
--- a/docs/tools/virtiofsd.rst
+++ b/docs/tools/virtiofsd.rst
@@ -228,6 +228,10 @@ The 'map' type adds a number of separate rules to add **prepend** as a prefix
 to the matched **key** (or all attributes if **key** is empty).
 There may be at most one 'map' rule and it must be the last rule in the set.
 
+Note: When the 'security.capability' xattr is remapped, the daemon has to do
+extra work to remove it during many operations, which the host kernel normally
+does itself.
+
 xattr-mapping Examples
 ----------------------
 
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 58d24c0010..fc7e1b1e8e 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -148,6 +148,7 @@ struct lo_data {
     int posix_lock;
     int xattr;
     char *xattrmap;
+    char *xattr_security_capability;
     char *source;
     char *modcaps;
     double timeout;
@@ -217,6 +218,8 @@ static __thread bool cap_loaded = 0;
 
 static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st,
                                 uint64_t mnt_id);
+static int xattr_map_client(const struct lo_data *lo, const char *client_name,
+                            char **out_name);
 
 static int is_dot_or_dotdot(const char *name)
 {
@@ -356,6 +359,37 @@ out:
     return ret;
 }
 
+/*
+ * The host kernel normally drops security.capability xattr's on
+ * any write, however if we're remapping xattr names we need to drop
+ * whatever the clients security.capability is actually stored as.
+ */
+static int drop_security_capability(const struct lo_data *lo, int fd)
+{
+    if (!lo->xattr_security_capability) {
+        /* We didn't remap the name, let the host kernel do it */
+        return 0;
+    }
+    if (!fremovexattr(fd, lo->xattr_security_capability)) {
+        /* All good */
+        return 0;
+    }
+
+    switch (errno) {
+    case ENODATA:
+        /* Attribute didn't exist, that's fine */
+        return 0;
+
+    case ENOTSUP:
+        /* FS didn't support attribute anyway, also fine */
+        return 0;
+
+    default:
+        /* Hmm other error */
+        return errno;
+    }
+}
+
 static void lo_map_init(struct lo_map *map)
 {
     map->elems = NULL;
@@ -737,6 +771,11 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
         uid_t uid = (valid & FUSE_SET_ATTR_UID) ? attr->st_uid : (uid_t)-1;
         gid_t gid = (valid & FUSE_SET_ATTR_GID) ? attr->st_gid : (gid_t)-1;
 
+        saverr = drop_security_capability(lo, ifd);
+        if (saverr) {
+            goto out_err;
+        }
+
         res = fchownat(ifd, "", uid, gid, AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);
         if (res == -1) {
             saverr = errno;
@@ -759,6 +798,14 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
             }
         }
 
+        saverr = drop_security_capability(lo, truncfd);
+        if (saverr) {
+            if (!fi) {
+                close(truncfd);
+            }
+            goto out_err;
+        }
+
         if (kill_suidgid) {
             res = drop_effective_cap("FSETID", &cap_fsetid_dropped);
             if (res != 0) {
@@ -1784,6 +1831,13 @@ static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
         if (fd < 0) {
             return -fd;
         }
+        if (fi->flags & (O_TRUNC)) {
+            int err = drop_security_capability(lo, fd);
+            if (err) {
+                close(fd);
+                return err;
+            }
+        }
     }
 
     pthread_mutex_lock(&lo->mutex);
@@ -2191,6 +2245,12 @@ static void lo_write_buf(fuse_req_t req, fuse_ino_t ino,
              "lo_write_buf(ino=%" PRIu64 ", size=%zd, off=%lu kill_priv=%d)\n",
              ino, out_buf.buf[0].size, (unsigned long)off, fi->kill_priv);
 
+    res = drop_security_capability(lo_data(req), out_buf.buf[0].fd);
+    if (res) {
+        fuse_reply_err(req, res);
+        return;
+    }
+
     /*
      * If kill_priv is set, drop CAP_FSETID which should lead to kernel
      * clearing setuid/setgid on file. Note, for WRITE, we need to do
@@ -2432,6 +2492,7 @@ static void parse_xattrmap(struct lo_data *lo)
 {
     const char *map = lo->xattrmap;
     const char *tmp;
+    int ret;
 
     lo->xattr_map_nentries = 0;
     while (*map) {
@@ -2462,7 +2523,7 @@ static void parse_xattrmap(struct lo_data *lo)
              * the last entry.
              */
             parse_xattrmap_map(lo, map, sep);
-            return;
+            break;
         } else {
             fuse_log(FUSE_LOG_ERR,
                      "%s: Unexpected type;"
@@ -2531,6 +2592,19 @@ static void parse_xattrmap(struct lo_data *lo)
         fuse_log(FUSE_LOG_ERR, "Empty xattr map\n");
         exit(1);
     }
+
+    ret = xattr_map_client(lo, "security.capability",
+                           &lo->xattr_security_capability);
+    if (ret) {
+        fuse_log(FUSE_LOG_ERR, "Failed to map security.capability: %s\n",
+                strerror(ret));
+        exit(1);
+    }
+    if (!strcmp(lo->xattr_security_capability, "security.capability")) {
+        /* 1-1 mapping, don't need to do anything */
+        free(lo->xattr_security_capability);
+        lo->xattr_security_capability = NULL;
+    }
 }
 
 /*
@@ -3588,6 +3662,7 @@ static void fuse_lo_data_cleanup(struct lo_data *lo)
 
     free(lo->xattrmap);
     free_xattrmap(lo);
+    free(lo->xattr_security_capability);
     free(lo->source);
 }
 
-- 
2.29.2

Re: [PULL 1/1] virtiofs: drop remapped security.capability xattr as needed

[Dr. David Alan Gilbert]

Oops, forgot to copy in qemu-stable; since xattrmap is in 5.2.0
it probably should go into stable.
(Taking 1e08f16 'virtiofsd: Save error code early at the failure
callsite' makes it easier to backport)

Dave

* Dr. David Alan Gilbert (git) (dgilbert@redhat.com) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> On Linux, the 'security.capability' xattr holds a set of
> capabilities that can change when an executable is run, giving
> a limited form of privilege escalation to those programs that
> the writer of the file deemed worthy.
> 
> Any write causes the 'security.capability' xattr to be dropped,
> stopping anyone from gaining privilege by modifying a blessed
> file.
> 
> Fuse relies on the daemon to do this dropping, and in turn the
> daemon relies on the host kernel to drop the xattr for it.  However,
> with the addition of -o xattrmap, the xattr that the guest
> stores its capabilities in is now not the same as the one that
> the host kernel automatically clears.
> 
> Where the mapping changes 'security.capability', explicitly clear
> the remapped name to preserve the same behaviour.
> 
> This bug is assigned CVE-2021-20263.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  docs/tools/virtiofsd.rst         |  4 ++
>  tools/virtiofsd/passthrough_ll.c | 77 +++++++++++++++++++++++++++++++-
>  2 files changed, 80 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/tools/virtiofsd.rst b/docs/tools/virtiofsd.rst
> index 866b7db3ee..00554c75bd 100644
> --- a/docs/tools/virtiofsd.rst
> +++ b/docs/tools/virtiofsd.rst
> @@ -228,6 +228,10 @@ The 'map' type adds a number of separate rules to add **prepend** as a prefix
>  to the matched **key** (or all attributes if **key** is empty).
>  There may be at most one 'map' rule and it must be the last rule in the set.
>  
> +Note: When the 'security.capability' xattr is remapped, the daemon has to do
> +extra work to remove it during many operations, which the host kernel normally
> +does itself.
> +
>  xattr-mapping Examples
>  ----------------------
>  
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 58d24c0010..fc7e1b1e8e 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -148,6 +148,7 @@ struct lo_data {
>      int posix_lock;
>      int xattr;
>      char *xattrmap;
> +    char *xattr_security_capability;
>      char *source;
>      char *modcaps;
>      double timeout;
> @@ -217,6 +218,8 @@ static __thread bool cap_loaded = 0;
>  
>  static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st,
>                                  uint64_t mnt_id);
> +static int xattr_map_client(const struct lo_data *lo, const char *client_name,
> +                            char **out_name);
>  
>  static int is_dot_or_dotdot(const char *name)
>  {
> @@ -356,6 +359,37 @@ out:
>      return ret;
>  }
>  
> +/*
> + * The host kernel normally drops security.capability xattr's on
> + * any write, however if we're remapping xattr names we need to drop
> + * whatever the clients security.capability is actually stored as.
> + */
> +static int drop_security_capability(const struct lo_data *lo, int fd)
> +{
> +    if (!lo->xattr_security_capability) {
> +        /* We didn't remap the name, let the host kernel do it */
> +        return 0;
> +    }
> +    if (!fremovexattr(fd, lo->xattr_security_capability)) {
> +        /* All good */
> +        return 0;
> +    }
> +
> +    switch (errno) {
> +    case ENODATA:
> +        /* Attribute didn't exist, that's fine */
> +        return 0;
> +
> +    case ENOTSUP:
> +        /* FS didn't support attribute anyway, also fine */
> +        return 0;
> +
> +    default:
> +        /* Hmm other error */
> +        return errno;
> +    }
> +}
> +
>  static void lo_map_init(struct lo_map *map)
>  {
>      map->elems = NULL;
> @@ -737,6 +771,11 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
>          uid_t uid = (valid & FUSE_SET_ATTR_UID) ? attr->st_uid : (uid_t)-1;
>          gid_t gid = (valid & FUSE_SET_ATTR_GID) ? attr->st_gid : (gid_t)-1;
>  
> +        saverr = drop_security_capability(lo, ifd);
> +        if (saverr) {
> +            goto out_err;
> +        }
> +
>          res = fchownat(ifd, "", uid, gid, AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);
>          if (res == -1) {
>              saverr = errno;
> @@ -759,6 +798,14 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
>              }
>          }
>  
> +        saverr = drop_security_capability(lo, truncfd);
> +        if (saverr) {
> +            if (!fi) {
> +                close(truncfd);
> +            }
> +            goto out_err;
> +        }
> +
>          if (kill_suidgid) {
>              res = drop_effective_cap("FSETID", &cap_fsetid_dropped);
>              if (res != 0) {
> @@ -1784,6 +1831,13 @@ static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
>          if (fd < 0) {
>              return -fd;
>          }
> +        if (fi->flags & (O_TRUNC)) {
> +            int err = drop_security_capability(lo, fd);
> +            if (err) {
> +                close(fd);
> +                return err;
> +            }
> +        }
>      }
>  
>      pthread_mutex_lock(&lo->mutex);
> @@ -2191,6 +2245,12 @@ static void lo_write_buf(fuse_req_t req, fuse_ino_t ino,
>               "lo_write_buf(ino=%" PRIu64 ", size=%zd, off=%lu kill_priv=%d)\n",
>               ino, out_buf.buf[0].size, (unsigned long)off, fi->kill_priv);
>  
> +    res = drop_security_capability(lo_data(req), out_buf.buf[0].fd);
> +    if (res) {
> +        fuse_reply_err(req, res);
> +        return;
> +    }
> +
>      /*
>       * If kill_priv is set, drop CAP_FSETID which should lead to kernel
>       * clearing setuid/setgid on file. Note, for WRITE, we need to do
> @@ -2432,6 +2492,7 @@ static void parse_xattrmap(struct lo_data *lo)
>  {
>      const char *map = lo->xattrmap;
>      const char *tmp;
> +    int ret;
>  
>      lo->xattr_map_nentries = 0;
>      while (*map) {
> @@ -2462,7 +2523,7 @@ static void parse_xattrmap(struct lo_data *lo)
>               * the last entry.
>               */
>              parse_xattrmap_map(lo, map, sep);
> -            return;
> +            break;
>          } else {
>              fuse_log(FUSE_LOG_ERR,
>                       "%s: Unexpected type;"
> @@ -2531,6 +2592,19 @@ static void parse_xattrmap(struct lo_data *lo)
>          fuse_log(FUSE_LOG_ERR, "Empty xattr map\n");
>          exit(1);
>      }
> +
> +    ret = xattr_map_client(lo, "security.capability",
> +                           &lo->xattr_security_capability);
> +    if (ret) {
> +        fuse_log(FUSE_LOG_ERR, "Failed to map security.capability: %s\n",
> +                strerror(ret));
> +        exit(1);
> +    }
> +    if (!strcmp(lo->xattr_security_capability, "security.capability")) {
> +        /* 1-1 mapping, don't need to do anything */
> +        free(lo->xattr_security_capability);
> +        lo->xattr_security_capability = NULL;
> +    }
>  }
>  
>  /*
> @@ -3588,6 +3662,7 @@ static void fuse_lo_data_cleanup(struct lo_data *lo)
>  
>      free(lo->xattrmap);
>      free_xattrmap(lo);
> +    free(lo->xattr_security_capability);
>      free(lo->source);
>  }
>  
> -- 
> 2.29.2
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PULL 0/1] virtiofs queue (minor security)

[Peter Maydell]

On Thu, 4 Mar 2021 at 10:41, Dr. David Alan Gilbert (git)
<dgilbert@redhat.com> wrote:
>
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> The following changes since commit c40ae5a3ee387b13116948cbfe7824f03311db7e:
>
>   Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging (2021-03-03 16:55:15 +0000)
>
> are available in the Git repository at:
>
>   https://gitlab.com/dagrh/qemu.git tags/pull-virtiofs-20210304
>
> for you to fetch changes up to e586edcb410543768ef009eaa22a2d9dd4a53846:
>
>   virtiofs: drop remapped security.capability xattr as needed (2021-03-04 10:26:16 +0000)
>
> ----------------------------------------------------------------
> virtiofs minor security fix
>
> Fix xattrmap to drop remapped security.capability capabilities.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/6.0
for any user-visible changes.

-- PMM