On Tue, 8 Aug 2017 14:14:18 -0500 Eric Blake <ebl...@redhat.com> 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 <gr...@kaod.org> > > --- > > 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.
pgpFEQSjPVzj2.pgp
Description: OpenPGP digital signature