[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [for-2.10 PATCH] 9pfs: local: fix fchmodat_nofollow() l
From: |
Greg Kurz |
Subject: |
Re: [Qemu-devel] [for-2.10 PATCH] 9pfs: local: fix fchmodat_nofollow() limitations |
Date: |
Wed, 9 Aug 2017 09:35:59 +0200 |
On Tue, 8 Aug 2017 14:14:18 -0500
Eric Blake <address@hidden> wrote:
> On 08/08/2017 12:28 PM, 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.
> >
> > 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
>
> Did your attempt at a kernel patch for AT_SYMLINK_NOFOLLOW ever get
> anywhere?
>
No.
> >
> > 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.
> >
> > Signed-off-by: Greg Kurz <address@hidden>
> > ---
> > hw/9pfs/9p-local.c | 44 ++++++++++++++++++++++++++++----------------
> > hw/9pfs/9p-util.h | 10 +++++++---
> > 2 files changed, 35 insertions(+), 19 deletions(-)
> >
> > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > index 6e478f4765ef..b178d627c764 100644
> > --- a/hw/9pfs/9p-local.c
> > +++ b/hw/9pfs/9p-local.c
> > @@ -333,30 +333,42 @@ update_map_file:
> >
> > static int fchmodat_nofollow(int dirfd, const char *name, mode_t mode)
> > {
> > + struct stat stbuf;
> > int fd, ret;
> > + char *proc_path;
> >
> > /* FIXME: this should be handled with fchmodat(AT_SYMLINK_NOFOLLOW).
> > - * Unfortunately, the linux kernel doesn't implement it yet. As an
> > - * alternative, let's open the file and use fchmod() instead. This
> > - * may fail depending on the permissions of the file, but it is the
> > - * best we can do to avoid TOCTTOU. We first try to open read-only
> > - * in case name points to a directory. If that fails, we try write-only
> > - * in case name doesn't point to a directory.
> > + * Unfortunately, the linux kernel doesn't implement it yet.
> > */
> > - fd = openat_file(dirfd, name, O_RDONLY, 0);
> > - if (fd == -1) {
> > - /* In case the file is writable-only and isn't a directory. */
> > - if (errno == EACCES) {
> > - fd = openat_file(dirfd, name, O_WRONLY, 0);
> > - }
> > - if (fd == -1 && errno == EISDIR) {
> > - errno = EACCES;
> > - }
> > + if (fstatat(dirfd, name, &stbuf, AT_SYMLINK_NOFOLLOW)) {
> > + return -1;
> > }
>
> Checking the file...
>
> > +
> > + if (S_ISLNK(stbuf.st_mode)) {
> > + errno = ELOOP;
> > + return -1;
> > + }
> > +
> > + fd = openat_file(dirfd, name, O_RDONLY | O_PATH, 0);
>
> ...and then opening the file is a TOCTTOU race (although it works most
> of the time and avoids the open where it is easy)...
>
Exactly. It is globally assumed in the 9p code that symbolic links are
resolved by the client. The earlier we detect the file is a symbolic
link, the earlier we can error out. The fstat()+S_ISLNK() is a deliberate
fast error path actually :)
> > if (fd == -1) {
> > return -1;
> > }
> > - ret = fchmod(fd, mode);
> > +
> > + ret = fstat(fd, &stbuf);
> > + if (ret) {
> > + goto out;
> > + }
>
> ...so you are double-checking that you got a non-symlink after all (your
> insurance against the race having done the wrong thing [but see below])...
>
Yes, this is the *real* check.
> > +
> > + 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);
>
> ...at which point you now have a valid file name that represents the
> file you wanted to chmod() in the first place (even if another rename
> occurs in the meantime, you are changing the mode tied to the
> non-symlink fd that you double-checked, which ends up behaving as if you
> had won the race and made the chmod() call before the rename).
>
Yes.
> > + g_free(proc_path);
> > +out:
> > close_preserve_errno(fd);
> > return ret;
>
> Might be worth littering some comments in the code explaining why you
> have to call both fstatat() and stat(), or perhaps you could drop the
> first fstatat() and just always do the open().
>
I like the idea of erroring out right away when a symbolic link is
detected. I'll add comments.
> Reading 'man open', it looks like O_PATH will chase symlinks UNLESS you
> also use O_NOFOLLOW. So I had to code up a simple test program to
> verify if things work...
>
> =============
> #define _GNU_SOURCE 1
> #include <sys/stat.h>
> #include <stdio.h>
> #include <errno.h>
> #include <unistd.h>
> #include <fcntl.h>
>
> int main(void) {
> struct stat st;
> int fd;
>
> remove("f");
> remove("l");
> if (creat("f", 0) < 0)
> return 1;
> if (symlink("f", "l") < 0)
> return 2;
>
> fd = open("l", O_RDONLY | O_PATH);
> if (fd < 0)
> return 3;
> if (fstat(fd, &st) < 0)
> return 4;
> if (S_ISLNK(st.st_mode)) {
> printf("got a link\n");
> } else {
> printf("dereferenced\n");
> }
> close(fd);
>
> fd = open("l", O_RDONLY | O_PATH | O_NOFOLLOW);
> if (fd < 0)
> return 5;
> if (fstat(fd, &st) < 0)
> return 6;
> if (S_ISLNK(st.st_mode)) {
> printf("got a link\n");
> } else {
> printf("dereferenced\n");
> }
> close(fd);
>
> remove("f");
> remove("l");
> return 0;
> }
> ============
> $ ./foo
> dereferenced
> got a link
>
> Ouch - you need to fix your patch to use open(O_NOFOLLOW | O_PATH).
>
openat_file() adds O_NOFOLLOW... I'll rename it to openat_file_nofollow()
in a separate patch for clarity.
> Furthermore, I'm worried that your patch is regressing commit 918112c0,
> when compiling for older platforms where either O_PATH does not exist,
> or where libc exposes the constant but the kernel is too old to
> understand it. (I guess the latter problem is already existing in our
> code base, so we really only need to worry about the former of compiling
> when the constant does not exist). So if supporting old platforms is
> desired, you MUST keep BOTH approaches intact, gated by whether O_PATH
> is defined to a non-zero value, rather than just assuming O_PATH works.
>
I don't necessarily want this to work for old platforms, but I'll fix the
potential build break like in commit 918112c0.
pgptGulNffZUd.pgp
Description: OpenPGP digital signature
Re: [Qemu-devel] [for-2.10 PATCH] 9pfs: local: fix fchmodat_nofollow() limitations, Eric Blake, 2017/08/08
- Re: [Qemu-devel] [for-2.10 PATCH] 9pfs: local: fix fchmodat_nofollow() limitations,
Greg Kurz <=