[RFC PATCH 0/5] virtiofsd: Add notion of unprivileged mode

[RFC PATCH 0/5] virtiofsd: Add notion of unprivileged mode

[Vivek Goyal]

Hi,

Daniel Berrange mentioned that having a unpriviliged mode in virtiofsd 
might be useful for certain use cases. Hence I decided to give it
a try.

This is RFC patch series to allow running virtiofsd as unpriviliged
user. This is still work in progress. I am posting it to get
some early feedback.

These patches are dependent on Stefan's patch series for sandbox=chroot.

https://www.redhat.com/archives/virtio-fs/2020-July/msg00078.html

I can now run virtiofsd as user "test" and also export a directory
into a VM running as user test.

This is ideally for the cases where user "test" inside VM will operate
on this virtiofs mount point. Any filesystem operations which can't
be done with the creds of "test" user on host, will fail.

Thanks
Vivek

Vivek Goyal (5):
  virtiofsd: Add notion of unprivileged mode
  virtiofsd: create lock/pid file in per user cache dir
  virtiofsd: open /proc/self/fd/ in sandbox=NONE mode
  virtiofsd: Open lo->source while setting up root in sandbox=NONE mode
  virtiofsd: Skip setup_capabilities() in sandbox=NONE mode

 tools/virtiofsd/fuse_virtio.c    | 40 ++++++++++++++++++++++++++++----
 tools/virtiofsd/passthrough_ll.c | 29 ++++++++++++++++++++---
 2 files changed, 61 insertions(+), 8 deletions(-)

-- 
2.25.4

[PATCH 1/5] virtiofsd: Add notion of unprivileged mode

[Vivek Goyal]

At startup if we are running as non-root user, then internall set
unpriviliged mode set. Also add a notion of sandbox NONE and set
that internally in unprivileged mode. setting up namespaces and
chroot() fails when one does not have privileges.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index e2fbc614fd..cd91c4a831 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -147,11 +147,13 @@ enum {
 enum {
     SANDBOX_NAMESPACE,
     SANDBOX_CHROOT,
+    SANDBOX_NONE,
 };
 
 struct lo_data {
     pthread_mutex_t mutex;
     int sandbox;
+    bool unprivileged;
     int debug;
     int writeback;
     int flock;
@@ -3288,6 +3290,12 @@ int main(int argc, char *argv[])
     lo_map_init(&lo.dirp_map);
     lo_map_init(&lo.fd_map);
 
+    if (geteuid() != 0) {
+       lo.unprivileged = true;
+       lo.sandbox = SANDBOX_NONE;
+       fuse_log(FUSE_LOG_DEBUG, "Running in unprivileged passthrough mode.\n");
+    }
+
     if (fuse_parse_cmdline(&args, &opts) != 0) {
         goto err_out1;
     }
-- 
2.25.4

[PATCH 2/5] virtiofsd: create lock/pid file in per user cache dir

[Vivek Goyal]

Right now we create lock/pid file in /usr/local/var/... and unprivliged
user does not have access to create files there.

So create this file in per user cache dir as queried as specified
by environment variable XDG_RUNTIME_DIR.

Note: "su $USER" does not update XDG_RUNTIME_DIR and it still points to
root user's director. So for now I create a directory /tmp/$UID to save
lock/pid file. Dan pointed out that it can be a problem if a malicious
app already has /tmp/$UID created. So we probably need to get rid of this.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/fuse_virtio.c | 40 ++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 6b21a93841..f763a70ba5 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -972,13 +972,43 @@ static bool fv_socket_lock(struct fuse_session *se)
     g_autofree gchar *pidfile = NULL;
     g_autofree gchar *dir = NULL;
     Error *local_err = NULL;
+    gboolean unprivileged = false;
 
-    dir = qemu_get_local_state_pathname("run/virtiofsd");
+    if (geteuid() != 0)
+        unprivileged = true;
 
-    if (g_mkdir_with_parents(dir, S_IRWXU) < 0) {
-        fuse_log(FUSE_LOG_ERR, "%s: Failed to create directory %s: %s",
-                 __func__, dir, strerror(errno));
-        return false;
+    /*
+     * Unpriviliged users don't have access to /usr/local/var. Hence
+     * store lock/pid file in per user directory. Use environment
+     * variable XDG_RUNTIME_DIR.
+     * If one logs into the system as root and then does "su" then
+     * XDG_RUNTIME_DIR still points to root user directory. In that
+     * case create a directory for user in /tmp/$UID
+     */
+    if (unprivileged) {
+        gchar *user_dir = NULL;
+        gboolean create_dir = false;
+        user_dir = g_strdup(g_get_user_runtime_dir());
+        if (!user_dir || g_str_has_suffix(user_dir, "/0")) {
+            user_dir = g_strdup_printf("/tmp/%d", geteuid());
+            create_dir = true;
+        }
+
+        if (create_dir && g_mkdir_with_parents(user_dir, S_IRWXU) < 0) {
+            fuse_log(FUSE_LOG_ERR, "%s: Failed to create directory %s: %s",
+                     __func__, user_dir, strerror(errno));
+            g_free(user_dir);
+            return false;
+        }
+        dir = g_strdup(user_dir);
+        g_free(user_dir);
+    } else {
+        dir = qemu_get_local_state_pathname("run/virtiofsd");
+        if (g_mkdir_with_parents(dir, S_IRWXU) < 0) {
+            fuse_log(FUSE_LOG_ERR, "%s: Failed to create directory %s: %s",
+                     __func__, dir, strerror(errno));
+            return false;
+        }
     }
 
     sk_name = g_strdup(se->vu_socket_path);
-- 
2.25.4

Re: [PATCH 2/5] virtiofsd: create lock/pid file in per user cache dir

[Daniel P. Berrangé]

On Wed, Jul 29, 2020 at 06:14:07PM -0400, Vivek Goyal wrote:
> Right now we create lock/pid file in /usr/local/var/... and unprivliged
> user does not have access to create files there.
> 
> So create this file in per user cache dir as queried as specified
> by environment variable XDG_RUNTIME_DIR.
> 
> Note: "su $USER" does not update XDG_RUNTIME_DIR and it still points to
> root user's director. So for now I create a directory /tmp/$UID to save
> lock/pid file. Dan pointed out that it can be a problem if a malicious
> app already has /tmp/$UID created. So we probably need to get rid of this.

IMHO use of "su $USER" is simply user error and we don't need to
care about workarounds. They will see the startup fail due to
EPERM on /run/user/0 directory, and then they'll have to fix
their command to use "su - $USER" to setup a clean environment.


> +    /*
> +     * Unpriviliged users don't have access to /usr/local/var. Hence
> +     * store lock/pid file in per user directory. Use environment
> +     * variable XDG_RUNTIME_DIR.
> +     * If one logs into the system as root and then does "su" then
> +     * XDG_RUNTIME_DIR still points to root user directory. In that
> +     * case create a directory for user in /tmp/$UID
> +     */
> +    if (unprivileged) {
> +        gchar *user_dir = NULL;
> +        gboolean create_dir = false;
> +        user_dir = g_strdup(g_get_user_runtime_dir());
> +        if (!user_dir || g_str_has_suffix(user_dir, "/0")) {
> +            user_dir = g_strdup_printf("/tmp/%d", geteuid());
> +            create_dir = true;
> +        }

As above, I don't think we need to have this fallback code to deal
with something that is just user error.

Also, g_get_user_runtime_dir() is guaranteed to return non-NULL.

> +
> +        if (create_dir && g_mkdir_with_parents(user_dir, S_IRWXU) < 0) {
> +            fuse_log(FUSE_LOG_ERR, "%s: Failed to create directory %s: %s",
> +                     __func__, user_dir, strerror(errno));
> +            g_free(user_dir);
> +            return false;
> +        }
> +        dir = g_strdup(user_dir);

Don't we also want to be appending "virtiofsd" to this directory path
like we do in the privileged case ?


> +        g_free(user_dir);
> +    } else {
> +        dir = qemu_get_local_state_pathname("run/virtiofsd");
> +        if (g_mkdir_with_parents(dir, S_IRWXU) < 0) {
> +            fuse_log(FUSE_LOG_ERR, "%s: Failed to create directory %s: %s",
> +                     __func__, dir, strerror(errno));
> +            return false;
> +        }
>      }
>  
>      sk_name = g_strdup(se->vu_socket_path);
> -- 
> 2.25.4
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/5] virtiofsd: create lock/pid file in per user cache dir

[Vivek Goyal]

On Thu, Jul 30, 2020 at 09:59:37AM +0100, Daniel P. Berrangé wrote:
> On Wed, Jul 29, 2020 at 06:14:07PM -0400, Vivek Goyal wrote:
> > Right now we create lock/pid file in /usr/local/var/... and unprivliged
> > user does not have access to create files there.
> > 
> > So create this file in per user cache dir as queried as specified
> > by environment variable XDG_RUNTIME_DIR.
> > 
> > Note: "su $USER" does not update XDG_RUNTIME_DIR and it still points to
> > root user's director. So for now I create a directory /tmp/$UID to save
> > lock/pid file. Dan pointed out that it can be a problem if a malicious
> > app already has /tmp/$UID created. So we probably need to get rid of this.
> 
> IMHO use of "su $USER" is simply user error and we don't need to
> care about workarounds. They will see the startup fail due to
> EPERM on /run/user/0 directory, and then they'll have to fix
> their command to use "su - $USER" to setup a clean environment.

I tried "su - $USER". That clears the old XDG_RUNTIME_DIR but does
not set new one. So now we have an empty XDG_RUNTIME_DIR env variable.
But good thing is that now g_get_user_runtime_dir() returns
"/home/$USER/.cache" and we can store user specific temp files there.

So I agree that I will get rid of all the logic to create /tmp/$USER.
"su $USER" will not be a supported path.

> 
> 
> > +    /*
> > +     * Unpriviliged users don't have access to /usr/local/var. Hence
> > +     * store lock/pid file in per user directory. Use environment
> > +     * variable XDG_RUNTIME_DIR.
> > +     * If one logs into the system as root and then does "su" then
> > +     * XDG_RUNTIME_DIR still points to root user directory. In that
> > +     * case create a directory for user in /tmp/$UID
> > +     */
> > +    if (unprivileged) {
> > +        gchar *user_dir = NULL;
> > +        gboolean create_dir = false;
> > +        user_dir = g_strdup(g_get_user_runtime_dir());
> > +        if (!user_dir || g_str_has_suffix(user_dir, "/0")) {
> > +            user_dir = g_strdup_printf("/tmp/%d", geteuid());
> > +            create_dir = true;
> > +        }
> 
> As above, I don't think we need to have this fallback code to deal
> with something that is just user error.
> 
> Also, g_get_user_runtime_dir() is guaranteed to return non-NULL.

Thanks. I will get rid of (!user_dir) case.

> 
> > +
> > +        if (create_dir && g_mkdir_with_parents(user_dir, S_IRWXU) < 0) {
> > +            fuse_log(FUSE_LOG_ERR, "%s: Failed to create directory %s: %s",
> > +                     __func__, user_dir, strerror(errno));
> > +            g_free(user_dir);
> > +            return false;
> > +        }
> > +        dir = g_strdup(user_dir);
> 
> Don't we also want to be appending "virtiofsd" to this directory path
> like we do in the privileged case ?

Yes. I forgot to append "virtiofsd" dir. Will do.

Thanks
Vivek

[PATCH 3/5] virtiofsd: open /proc/self/fd/ in sandbox=NONE mode

[Vivek Goyal]

We need /proc/self/fd descriptor even in sandbox=NONE mode.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index cd91c4a831..76ef891105 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -2969,6 +2969,15 @@ static void setup_capabilities(char *modcaps_in)
     pthread_mutex_unlock(&cap.mutex);
 }
 
+static void setup_none(struct lo_data *lo)
+{
+    lo->proc_self_fd = open("/proc/self/fd", O_PATH);
+    if (lo->proc_self_fd == -1) {
+        fuse_log(FUSE_LOG_ERR, "open(\"/proc/self/fd\", O_PATH): %m\n");
+        exit(1);
+    }
+}
+
 /*
  * Use chroot as a weaker sandbox for environments where the process is
  * launched without CAP_SYS_ADMIN.
@@ -3014,8 +3023,10 @@ static void setup_sandbox(struct lo_data *lo, struct fuse_session *se,
     if (lo->sandbox == SANDBOX_NAMESPACE) {
         setup_namespaces(lo, se);
         setup_mounts(lo->source);
-    } else {
+    } else if (lo->sandbox == SANDBOX_CHROOT) {
         setup_chroot(lo);
+    } else {
+        setup_none(lo);
     }
 
     setup_seccomp(enable_syslog);
-- 
2.25.4

[PATCH 4/5] virtiofsd: Open lo->source while setting up root in sandbox=NONE mode

[Vivek Goyal]

In sandbox=NONE mode, lo->source points to the directory which is being
exported. We have not done any chroot()/pivot_root(). So open lo->source.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 76ef891105..a6fa816b6c 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -3209,7 +3209,10 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root)
     int fd, res;
     struct stat stat;
 
-    fd = open("/", O_PATH);
+    if (lo->sandbox == SANDBOX_NONE)
+        fd = open(lo->source, O_PATH);
+    else
+        fd = open("/", O_PATH);
     if (fd == -1) {
         fuse_log(FUSE_LOG_ERR, "open(%s, O_PATH): %m\n", lo->source);
         exit(1);
-- 
2.25.4

[PATCH 5/5] virtiofsd: Skip setup_capabilities() in sandbox=NONE mode

[Vivek Goyal]

While running as unpriviliged user setup_capabilities() fails for me.
So we are doing some operation which requires priviliges. For now
simply skip it in sandbox=NONE mode.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index a6fa816b6c..1a0b24cbf2 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -3030,7 +3030,8 @@ static void setup_sandbox(struct lo_data *lo, struct fuse_session *se,
     }
 
     setup_seccomp(enable_syslog);
-    setup_capabilities(g_strdup(lo->modcaps));
+    if (lo->sandbox != SANDBOX_NONE)
+       setup_capabilities(g_strdup(lo->modcaps));
 }
 
 /* Set the maximum number of open file descriptors */
-- 
2.25.4