| Message ID | 20200729221410.147556-3-vgoyal@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtiofsd: Add notion of unprivileged mode | expand |
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
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
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);
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(-)