From: Vivek Goyal <vgoyal@redhat.com> To: "Daniel P. Berrangé" <berrange@redhat.com> Cc: virtio-fs@redhat.com, vromanso@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, dgilbert@redhat.com Subject: Re: [PATCH 2/5] virtiofsd: create lock/pid file in per user cache dir Date: Thu, 30 Jul 2020 10:10:20 -0400 [thread overview] Message-ID: <20200730141020.GA149245@redhat.com> (raw) In-Reply-To: <20200730085937.GA3477223@redhat.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Vivek Goyal <vgoyal@redhat.com> To: "Daniel P. Berrangé" <berrange@redhat.com> Cc: virtio-fs@redhat.com, vromanso@redhat.com, qemu-devel@nongnu.org Subject: Re: [Virtio-fs] [PATCH 2/5] virtiofsd: create lock/pid file in per user cache dir Date: Thu, 30 Jul 2020 10:10:20 -0400 [thread overview] Message-ID: <20200730141020.GA149245@redhat.com> (raw) In-Reply-To: <20200730085937.GA3477223@redhat.com> 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
next prev parent reply other threads:[~2020-07-30 14:11 UTC|newest] Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-29 22:14 [RFC PATCH 0/5] virtiofsd: Add notion of unprivileged mode Vivek Goyal 2020-07-29 22:14 ` [Virtio-fs] " Vivek Goyal 2020-07-29 22:14 ` [PATCH 1/5] " Vivek Goyal 2020-07-29 22:14 ` [Virtio-fs] " Vivek Goyal 2020-07-29 22:14 ` [PATCH 2/5] virtiofsd: create lock/pid file in per user cache dir Vivek Goyal 2020-07-29 22:14 ` [Virtio-fs] " Vivek Goyal 2020-07-30 8:59 ` Daniel P. Berrangé 2020-07-30 8:59 ` [Virtio-fs] " Daniel P. Berrangé 2020-07-30 14:10 ` Vivek Goyal [this message] 2020-07-30 14:10 ` Vivek Goyal 2020-07-30 14:00 ` Daniel Walsh 2020-07-29 22:14 ` [PATCH 3/5] virtiofsd: open /proc/self/fd/ in sandbox=NONE mode Vivek Goyal 2020-07-29 22:14 ` [Virtio-fs] " Vivek Goyal 2020-07-29 22:14 ` [PATCH 4/5] virtiofsd: Open lo->source while setting up root " Vivek Goyal 2020-07-29 22:14 ` [Virtio-fs] " Vivek Goyal 2020-07-29 22:14 ` [PATCH 5/5] virtiofsd: Skip setup_capabilities() " Vivek Goyal 2020-07-29 22:14 ` [Virtio-fs] " Vivek Goyal
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200730141020.GA149245@redhat.com \ --to=vgoyal@redhat.com \ --cc=berrange@redhat.com \ --cc=dgilbert@redhat.com \ --cc=qemu-devel@nongnu.org \ --cc=stefanha@redhat.com \ --cc=virtio-fs@redhat.com \ --cc=vromanso@redhat.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.