[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [for-2.10 PATCH v2] 9pfs: local: fix fchmodat_nofollow(
From: |
Greg Kurz |
Subject: |
Re: [Qemu-devel] [for-2.10 PATCH v2] 9pfs: local: fix fchmodat_nofollow() limitations |
Date: |
Wed, 9 Aug 2017 17:22:33 +0200 |
On Wed, 9 Aug 2017 09:55:32 -0500
Eric Blake <address@hidden> wrote:
> On 08/09/2017 09:23 AM, Greg Kurz wrote:
> > This function has to ensure it doesn't follow a symlink that could be used
> > to escape the virtfs directory. This could be easily achieved if fchmodat()
> > on linux honored the AT_SYMLINK_NOFOLLOW flag as described in POSIX, but
> > it doesn't.
>
> Might be worth including a URL of the LKML discussion on the last
> version of that patch attempt.
>
Ok.
> >
> > The current implementation covers most use-cases, but it notably fails if:
> > - the target path has access rights equal to 0000 (openat() returns EPERM),
> >
> > => once you've done chmod(0000) on a file, you can never chmod() again
> > - the target path is UNIX domain socket (openat() returns ENXIO)
> > => bind() of UNIX domain sockets fails if the file is on 9pfs
> >
> > The solution is to use O_PATH: openat() now succeeds in both cases, and we
> > can ensure the path isn't a symlink with fstat(). The associated entry in
> > "/proc/self/fd" can hence be safely passed to the regular chmod() syscall.
>
> Hey - should we point this out as a viable solution to the glibc folks,
> since their current user-space emulation of AT_SYMLINK_NOFOLLOW is broken?
>
Probably. What's the best way to do that ?
> >
> > The previous behavior is kept for older systems that don't have O_PATH.
> >
> > Signed-off-by: Greg Kurz <address@hidden>
> > ---
> > v2: - renamed OPENAT_DIR_O_PATH to O_PATH_9P_UTIL and use it as a
> > replacement
> > for O_PATH to avoid build breaks on O_PATH-less systems
> > - keep current behavior for O_PATH-less systems
> > - added comments
> > - TODO in 2.11: add _nofollow suffix to openat_dir() and openat_file()
> > ---
> > hw/9pfs/9p-local.c | 41 +++++++++++++++++++++++++++++++++--------
> > hw/9pfs/9p-util.h | 24 +++++++++++++++---------
> > 2 files changed, 48 insertions(+), 17 deletions(-)
> >
> > + /* First, we clear non-racing symlinks out of the way. */
> > + if (fstatat(dirfd, name, &stbuf, AT_SYMLINK_NOFOLLOW)) {
> > + return -1;
> > + }
> > + if (S_ISLNK(stbuf.st_mode)) {
> > + errno = ELOOP;
>
> I don't know if ELOOP is the best errno value to use here, but I don't
> have any better suggestions so I'm okay with it.
>
> > + return -1;
> > + }
> > +
> > + /* Access modes are ignored when O_PATH is supported. We try O_RDONLY
> > and
> > + * O_WRONLY for old-systems that don't support O_PATH.
> > */
> > - fd = openat_file(dirfd, name, O_RDONLY, 0);
> > + fd = openat_file(dirfd, name, O_RDONLY | O_PATH_9P_UTIL, 0);
> > if (fd == -1) {
> > /* In case the file is writable-only and isn't a directory. */
> > if (errno == EACCES) {
> > @@ -356,7 +366,22 @@ static int fchmodat_nofollow(int dirfd, const char
> > *name, mode_t mode)
> > if (fd == -1) {
> > return -1;
> > }
> > - ret = fchmod(fd, mode);
> > +
> > + /* Now we handle racing symlinks. */
> > + ret = fstat(fd, &stbuf);
> > + if (ret) {
> > + goto out;
> > + }
> > + if (S_ISLNK(stbuf.st_mode)) {
> > + errno = ELOOP;
> > + ret = -1;
> > + goto out;
> > + }
> > +
> > + proc_path = g_strdup_printf("/proc/self/fd/%d", fd);
> > + ret = chmod(proc_path, mode);
>
> Nope, unsafe when O_PATH_9P_UTIL is 0. This needs to be more like:
>
> /* Now we handle racing symlinks. On kernels without O_PATH, we will
> * fail on some corner cases, but that's better than dereferencing a
> * symlink that was injected during the TOCTTOU between our initial
> * fstatat() and openat_file().
> */
> if (O_PATH_9P_UTIL) {
> fstat, S_ISLINK, proc_path, chmod()
> } else {
> fchmod()
> }
>
Oops, you're right. I'll fix that.
pgpyabJZ0uXd6.pgp
Description: OpenPGP digital signature