Message ID | 148856193073.554.6631259860971664030.stgit@bahia |
---|---|
State | New |
Headers | show |
On Fri, Mar 03, 2017 at 06:25:30PM +0100, Greg Kurz wrote: > We should pass O_NOFOLLOW otherwise openat() will follow symlinks and make > QEMU vulnerable. > > O_PATH was used as an optimization: the fd returned by openat_dir() is only > passed to openat() actually, so we don't really need to reach the underlying > filesystem. > > O_NOFOLLOW | O_PATH isn't an option: if name is a symlink, openat() will > return a fd, forcing us to do some other syscall to detect we have a > symlink. Also, O_PATH doesn't exist in glibc 2.13 and older. > > The only sane thing to do is hence to drop O_PATH, and only pass O_NOFOLLOW. > > While here, we also fix local_unlinkat_common() to use openat_dir() for > the same reasons (it was a leftover in the original patchset actually). > > This fixes CVE-2016-9602. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/9pfs/9p-local.c | 2 +- > hw/9pfs/9p-util.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > index 5db7104334d6..e31309a29c58 100644 > --- a/hw/9pfs/9p-local.c > +++ b/hw/9pfs/9p-local.c > @@ -959,7 +959,7 @@ static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name, > if (flags == AT_REMOVEDIR) { > int fd; > > - fd = openat(dirfd, name, O_RDONLY | O_DIRECTORY | O_PATH); > + fd = openat_dir(dirfd, name); > if (fd == -1) { > goto err_out; > } > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h > index 091f3ce88e15..4001d1fd8398 100644 > --- a/hw/9pfs/9p-util.h > +++ b/hw/9pfs/9p-util.h > @@ -22,7 +22,7 @@ static inline void close_preserve_errno(int fd) > > static inline int openat_dir(int dirfd, const char *name) > { > - return openat(dirfd, name, O_DIRECTORY | O_RDONLY | O_PATH); > + return openat(dirfd, name, O_DIRECTORY | O_RDONLY | O_NOFOLLOW); > } > > static inline int openat_file(int dirfd, const char *name, int flags, Reviewed-by: Daniel P. Berrange <berrange@redhat.com> Regards, Daniel
On 03/03/17 17:25, Greg Kurz wrote: > We should pass O_NOFOLLOW otherwise openat() will follow symlinks and make > QEMU vulnerable. > > O_PATH was used as an optimization: the fd returned by openat_dir() is only > passed to openat() actually, so we don't really need to reach the underlying > filesystem. > > O_NOFOLLOW | O_PATH isn't an option: if name is a symlink, openat() will > return a fd, forcing us to do some other syscall to detect we have a > symlink. Also, O_PATH doesn't exist in glibc 2.13 and older. > > The only sane thing to do is hence to drop O_PATH, and only pass O_NOFOLLOW. > > While here, we also fix local_unlinkat_common() to use openat_dir() for > the same reasons (it was a leftover in the original patchset actually). > > This fixes CVE-2016-9602. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/9pfs/9p-local.c | 2 +- > hw/9pfs/9p-util.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > index 5db7104334d6..e31309a29c58 100644 > --- a/hw/9pfs/9p-local.c > +++ b/hw/9pfs/9p-local.c > @@ -959,7 +959,7 @@ static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name, > if (flags == AT_REMOVEDIR) { > int fd; > > - fd = openat(dirfd, name, O_RDONLY | O_DIRECTORY | O_PATH); > + fd = openat_dir(dirfd, name); > if (fd == -1) { > goto err_out; > } > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h > index 091f3ce88e15..4001d1fd8398 100644 > --- a/hw/9pfs/9p-util.h > +++ b/hw/9pfs/9p-util.h > @@ -22,7 +22,7 @@ static inline void close_preserve_errno(int fd) > > static inline int openat_dir(int dirfd, const char *name) > { > - return openat(dirfd, name, O_DIRECTORY | O_RDONLY | O_PATH); > + return openat(dirfd, name, O_DIRECTORY | O_RDONLY | O_NOFOLLOW); > } > > static inline int openat_file(int dirfd, const char *name, int flags, Thanks Greg. This patch got me slightly further, but I now get another build failure: cc -I/home/build/src/qemu/git/qemu/hw/9pfs -Ihw/9pfs -I/home/build/src/qemu/git/qemu/tcg -I/home/build/src/qemu/git/qemu/tcg/i386 -I/home/build/src/qemu/git/qemu/linux-headers -I/home/build/src/qemu/git/qemu/linux-headers -I. -I/home/build/src/qemu/git/qemu -I/home/build/src/qemu/git/qemu/include -I/usr/include/pixman-1 -I/home/build/src/qemu/git/qemu/dtc/libfdt -Werror -pthread -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wendif-labels -Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration -Wold-style-definition -Wtype-limits -fstack-protector-all -I/usr/include/p11-kit-1 -I/usr/include/libpng12 -I/home/build/src/qemu/git/qemu/tests -MMD -MP -MT hw/9pfs/9p-local.o -MF hw/9pfs/9p-local.d -O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g -c -o hw/9pfs/9p-local.o hw/9pfs/9p-local.c hw/9pfs/9p-local.c: In function ‘local_set_cred_passthrough’: hw/9pfs/9p-local.c:352:40: error: ‘AT_EMPTY_PATH’ undeclared (first use in this function) hw/9pfs/9p-local.c:352:40: note: each undeclared identifier is reported only once for each function it appears in make: *** [hw/9pfs/9p-local.o] Error 1 A quick search suggested that I should be able to get around this by adding a #include in hw/9pfs/9p-local.c for "linux/fcntl.h" however adding that gives me a couple of redefinition errors: cc -I/home/build/src/qemu/git/qemu/hw/9pfs -Ihw/9pfs -I/home/build/src/qemu/git/qemu/tcg -I/home/build/src/qemu/git/qemu/tcg/i386 -I/home/build/src/qemu/git/qemu/linux-headers -I/home/build/src/qemu/git/qemu/linux-headers -I. -I/home/build/src/qemu/git/qemu -I/home/build/src/qemu/git/qemu/include -I/usr/include/pixman-1 -I/home/build/src/qemu/git/qemu/dtc/libfdt -Werror -pthread -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wendif-labels -Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration -Wold-style-definition -Wtype-limits -fstack-protector-all -I/usr/include/p11-kit-1 -I/usr/include/libpng12 -I/home/build/src/qemu/git/qemu/tests -MMD -MP -MT hw/9pfs/9p-local.o -MF hw/9pfs/9p-local.d -O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g -c -o hw/9pfs/9p-local.o hw/9pfs/9p-local.c In file included from /usr/include/x86_64-linux-gnu/asm/fcntl.h:1:0, from /usr/include/linux/fcntl.h:4, from hw/9pfs/9p-local.c:30: /usr/include/asm-generic/fcntl.h:127:8: error: redefinition of ‘struct f_owner_ex’ In file included from /usr/include/fcntl.h:34:0, from /home/build/src/qemu/git/qemu/include/qemu/osdep.h:79, from hw/9pfs/9p-local.c:14: /usr/include/x86_64-linux-gnu/bits/fcntl.h:203:8: note: originally defined here In file included from /usr/include/x86_64-linux-gnu/asm/fcntl.h:1:0, from /usr/include/linux/fcntl.h:4, from hw/9pfs/9p-local.c:30: /usr/include/asm-generic/fcntl.h:167:8: error: redefinition of ‘struct flock’ In file included from /usr/include/fcntl.h:34:0, from /home/build/src/qemu/git/qemu/include/qemu/osdep.h:79, from hw/9pfs/9p-local.c:14: /usr/include/x86_64-linux-gnu/bits/fcntl.h:167:8: note: originally defined here In file included from /usr/include/x86_64-linux-gnu/asm/fcntl.h:1:0, from /usr/include/linux/fcntl.h:4, from hw/9pfs/9p-local.c:30: /usr/include/asm-generic/fcntl.h:184:8: error: redefinition of ‘struct flock64’ In file included from /usr/include/fcntl.h:34:0, from /home/build/src/qemu/git/qemu/include/qemu/osdep.h:79, from hw/9pfs/9p-local.c:14: /usr/include/x86_64-linux-gnu/bits/fcntl.h:182:8: note: originally defined here make: *** [hw/9pfs/9p-local.o] Error 1 Any thoughts? Mark.
On 03/03/2017 11:25 AM, Greg Kurz wrote: > We should pass O_NOFOLLOW otherwise openat() will follow symlinks and make > QEMU vulnerable. > > O_PATH was used as an optimization: the fd returned by openat_dir() is only > passed to openat() actually, so we don't really need to reach the underlying > filesystem. > > O_NOFOLLOW | O_PATH isn't an option: if name is a symlink, openat() will > return a fd, forcing us to do some other syscall to detect we have a > symlink. Also, O_PATH doesn't exist in glibc 2.13 and older. But the very next use of openat(fd, ) should fail with EBADF if fd is not a directory, so you don't need any extra syscalls. I agree that we _need_ O_NOFOLLOW, but I'm not yet convinced that we must avoid O_PATH where it works. I'm in the middle of writing a test program to probe kernel behavior and demonstrate (at least to myself) whether there are scenarios where O_PATH makes it possible to open something where omitting it did not, while at the same time validating that O_NOFOLLOW doesn't cause problems if a symlink-fd is returned instead of a directory fd, based on our subsequent use of that fd in a *at call.
On Fri, 3 Mar 2017 17:54:35 +0000 Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > On 03/03/17 17:25, Greg Kurz wrote: > > > We should pass O_NOFOLLOW otherwise openat() will follow symlinks and make > > QEMU vulnerable. > > > > O_PATH was used as an optimization: the fd returned by openat_dir() is only > > passed to openat() actually, so we don't really need to reach the underlying > > filesystem. > > > > O_NOFOLLOW | O_PATH isn't an option: if name is a symlink, openat() will > > return a fd, forcing us to do some other syscall to detect we have a > > symlink. Also, O_PATH doesn't exist in glibc 2.13 and older. > > > > The only sane thing to do is hence to drop O_PATH, and only pass O_NOFOLLOW. > > > > While here, we also fix local_unlinkat_common() to use openat_dir() for > > the same reasons (it was a leftover in the original patchset actually). > > > > This fixes CVE-2016-9602. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > hw/9pfs/9p-local.c | 2 +- > > hw/9pfs/9p-util.h | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > > index 5db7104334d6..e31309a29c58 100644 > > --- a/hw/9pfs/9p-local.c > > +++ b/hw/9pfs/9p-local.c > > @@ -959,7 +959,7 @@ static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name, > > if (flags == AT_REMOVEDIR) { > > int fd; > > > > - fd = openat(dirfd, name, O_RDONLY | O_DIRECTORY | O_PATH); > > + fd = openat_dir(dirfd, name); > > if (fd == -1) { > > goto err_out; > > } > > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h > > index 091f3ce88e15..4001d1fd8398 100644 > > --- a/hw/9pfs/9p-util.h > > +++ b/hw/9pfs/9p-util.h > > @@ -22,7 +22,7 @@ static inline void close_preserve_errno(int fd) > > > > static inline int openat_dir(int dirfd, const char *name) > > { > > - return openat(dirfd, name, O_DIRECTORY | O_RDONLY | O_PATH); > > + return openat(dirfd, name, O_DIRECTORY | O_RDONLY | O_NOFOLLOW); > > } > > > > static inline int openat_file(int dirfd, const char *name, int flags, > > Thanks Greg. > > This patch got me slightly further, but I now get another build failure: > > > cc -I/home/build/src/qemu/git/qemu/hw/9pfs -Ihw/9pfs > -I/home/build/src/qemu/git/qemu/tcg > -I/home/build/src/qemu/git/qemu/tcg/i386 > -I/home/build/src/qemu/git/qemu/linux-headers > -I/home/build/src/qemu/git/qemu/linux-headers -I. > -I/home/build/src/qemu/git/qemu -I/home/build/src/qemu/git/qemu/include > -I/usr/include/pixman-1 -I/home/build/src/qemu/git/qemu/dtc/libfdt > -Werror -pthread -I/usr/include/glib-2.0 > -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -m64 -mcx16 -D_GNU_SOURCE > -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes > -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes > -fno-strict-aliasing -fno-common -fwrapv -Wendif-labels > -Wno-missing-include-dirs -Wempty-body -Wnested-externs > -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers > -Wold-style-declaration -Wold-style-definition -Wtype-limits > -fstack-protector-all -I/usr/include/p11-kit-1 > -I/usr/include/libpng12 -I/home/build/src/qemu/git/qemu/tests -MMD -MP > -MT hw/9pfs/9p-local.o -MF hw/9pfs/9p-local.d -O2 -U_FORTIFY_SOURCE > -D_FORTIFY_SOURCE=2 -g -c -o hw/9pfs/9p-local.o hw/9pfs/9p-local.c > hw/9pfs/9p-local.c: In function ‘local_set_cred_passthrough’: > hw/9pfs/9p-local.c:352:40: error: ‘AT_EMPTY_PATH’ undeclared (first use > in this function) > hw/9pfs/9p-local.c:352:40: note: each undeclared identifier is reported > only once for each function it appears in > make: *** [hw/9pfs/9p-local.o] Error 1 > Oops, my bad. In a previous version of the patchset, name could be an empty string on purpose... This isn't the case in the latest version but I forgot to drop AT_EMPTY_PATH. I'll send a patch, but you can simply remove it in the meantime to fix your build. > > A quick search suggested that I should be able to get around this by > adding a #include in hw/9pfs/9p-local.c for "linux/fcntl.h" however > adding that gives me a couple of redefinition errors: > > > cc -I/home/build/src/qemu/git/qemu/hw/9pfs -Ihw/9pfs > -I/home/build/src/qemu/git/qemu/tcg > -I/home/build/src/qemu/git/qemu/tcg/i386 > -I/home/build/src/qemu/git/qemu/linux-headers > -I/home/build/src/qemu/git/qemu/linux-headers -I. > -I/home/build/src/qemu/git/qemu -I/home/build/src/qemu/git/qemu/include > -I/usr/include/pixman-1 -I/home/build/src/qemu/git/qemu/dtc/libfdt > -Werror -pthread -I/usr/include/glib-2.0 > -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -m64 -mcx16 -D_GNU_SOURCE > -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes > -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes > -fno-strict-aliasing -fno-common -fwrapv -Wendif-labels > -Wno-missing-include-dirs -Wempty-body -Wnested-externs > -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers > -Wold-style-declaration -Wold-style-definition -Wtype-limits > -fstack-protector-all -I/usr/include/p11-kit-1 > -I/usr/include/libpng12 -I/home/build/src/qemu/git/qemu/tests -MMD -MP > -MT hw/9pfs/9p-local.o -MF hw/9pfs/9p-local.d -O2 -U_FORTIFY_SOURCE > -D_FORTIFY_SOURCE=2 -g -c -o hw/9pfs/9p-local.o hw/9pfs/9p-local.c > In file included from /usr/include/x86_64-linux-gnu/asm/fcntl.h:1:0, > from /usr/include/linux/fcntl.h:4, > from hw/9pfs/9p-local.c:30: > /usr/include/asm-generic/fcntl.h:127:8: error: redefinition of ‘struct > f_owner_ex’ > In file included from /usr/include/fcntl.h:34:0, > from /home/build/src/qemu/git/qemu/include/qemu/osdep.h:79, > from hw/9pfs/9p-local.c:14: > /usr/include/x86_64-linux-gnu/bits/fcntl.h:203:8: note: originally > defined here > In file included from /usr/include/x86_64-linux-gnu/asm/fcntl.h:1:0, > from /usr/include/linux/fcntl.h:4, > from hw/9pfs/9p-local.c:30: > /usr/include/asm-generic/fcntl.h:167:8: error: redefinition of ‘struct > flock’ > In file included from /usr/include/fcntl.h:34:0, > from /home/build/src/qemu/git/qemu/include/qemu/osdep.h:79, > from hw/9pfs/9p-local.c:14: > /usr/include/x86_64-linux-gnu/bits/fcntl.h:167:8: note: originally > defined here > In file included from /usr/include/x86_64-linux-gnu/asm/fcntl.h:1:0, > from /usr/include/linux/fcntl.h:4, > from hw/9pfs/9p-local.c:30: > /usr/include/asm-generic/fcntl.h:184:8: error: redefinition of ‘struct > flock64’ > In file included from /usr/include/fcntl.h:34:0, > from /home/build/src/qemu/git/qemu/include/qemu/osdep.h:79, > from hw/9pfs/9p-local.c:14: > /usr/include/x86_64-linux-gnu/bits/fcntl.h:182:8: note: originally > defined here > make: *** [hw/9pfs/9p-local.o] Error 1 > > > Any thoughts? > > Mark. >
On Fri, 3 Mar 2017 12:14:10 -0600 Eric Blake <eblake@redhat.com> wrote: > On 03/03/2017 11:25 AM, Greg Kurz wrote: > > We should pass O_NOFOLLOW otherwise openat() will follow symlinks and make > > QEMU vulnerable. > > > > O_PATH was used as an optimization: the fd returned by openat_dir() is only > > passed to openat() actually, so we don't really need to reach the underlying > > filesystem. > > > > O_NOFOLLOW | O_PATH isn't an option: if name is a symlink, openat() will > > return a fd, forcing us to do some other syscall to detect we have a > > symlink. Also, O_PATH doesn't exist in glibc 2.13 and older. > > But the very next use of openat(fd, ) should fail with EBADF if fd is > not a directory, so you don't need any extra syscalls. I agree that we > _need_ O_NOFOLLOW, but I'm not yet convinced that we must avoid O_PATH > where it works. > You may have a point indeed. > I'm in the middle of writing a test program to probe kernel behavior and > demonstrate (at least to myself) whether there are scenarios where > O_PATH makes it possible to open something where omitting it did not, > while at the same time validating that O_NOFOLLOW doesn't cause problems > if a symlink-fd is returned instead of a directory fd, based on our > subsequent use of that fd in a *at call. > I must leave right now, but please share the result of your experiment. Thanks for your support on helping to fix 9p, Eric :) Cheers. -- Greg
On 03/03/2017 12:14 PM, Eric Blake wrote: > On 03/03/2017 11:25 AM, Greg Kurz wrote: >> We should pass O_NOFOLLOW otherwise openat() will follow symlinks and make >> QEMU vulnerable. >> >> O_PATH was used as an optimization: the fd returned by openat_dir() is only >> passed to openat() actually, so we don't really need to reach the underlying >> filesystem. >> >> O_NOFOLLOW | O_PATH isn't an option: if name is a symlink, openat() will >> return a fd, forcing us to do some other syscall to detect we have a >> symlink. Also, O_PATH doesn't exist in glibc 2.13 and older. > > But the very next use of openat(fd, ) should fail with EBADF if fd is or ENOTDIR, actually > not a directory, so you don't need any extra syscalls. I agree that we > _need_ O_NOFOLLOW, but I'm not yet convinced that we must avoid O_PATH > where it works. > > I'm in the middle of writing a test program to probe kernel behavior and > demonstrate (at least to myself) whether there are scenarios where > O_PATH makes it possible to open something where omitting it did not, > while at the same time validating that O_NOFOLLOW doesn't cause problems > if a symlink-fd is returned instead of a directory fd, based on our > subsequent use of that fd in a *at call. My test case is below. Note that based on my testing, I think you want a v2 of this patch, which does: #ifndef O_PATH #define O_PATH 0 #endif static inline int openat_dir(int dirfd, const char *name) { - return openat(dirfd, name, O_DIRECTORY | O_RDONLY | O_PATH); + return openat(dirfd, name, O_DIRECTORY | O_RDONLY | O_NOFOLLOW | O_PATH); } #define _GNU_SOURCE 1 #include <stdio.h> #include <fcntl.h> #include <sys/stat.h> #include <unistd.h> #include <errno.h> #include <stdlib.h> int main(int argc, char **argv) { int i = 0; int ret = 1; int fd; struct stat st; if (mkdir("d", 0700) < 0) { printf("giving up, please try again once 'd' is removed\n"); return ret; } /* Create a playground for testing with. */ i = 1; if (creat("d/file", 0600) < 0) goto cleanup; i = 2; if (mkdir("d/subdir", 0700) < 0) goto cleanup; i = 3; if (creat("d/subdir/subfile", 0600) < 0) goto cleanup; i = 4; if (chmod("d/subdir", 0100) < 0) goto cleanup; i = 5; if (symlink("file", "d/link-file") < 0) goto cleanup; i = 6; if (symlink("subdir", "d/link-subdir") < 0) goto cleanup; /* Sanity: We can stat a child file with just search permissions, * whether via a directory or symlink-to-directory */ i = 7; if (stat("d/subdir/subfile", &st) < 0) goto cleanup; i = 8; if (stat("d/link-subdir/subfile", &st) < 0) goto cleanup; /* Since the directory is not readable, we can't get a normal fd */ fd = open("d/subdir", O_DIRECTORY | O_NOFOLLOW | O_RDONLY); if (fd != -1) { printf("unexpected success opening non-readable dir\n"); ret = 2; goto cleanup; } /* But we can get at it with O_PATH */ i = 9; fd = open("d/subdir", O_DIRECTORY | O_NOFOLLOW | O_RDONLY | O_PATH); if (fd < 0) goto cleanup; /* And use it in *at functions */ i = 10; if (fstatat(fd, "subfile", &st, 0) < 0) goto cleanup; i = 11; if (close(fd) < 0) goto cleanup; /* Note that O_DIRECTORY fails on symlinks with O_PATH... */ i = 12; fd = open("d/link-subdir", O_DIRECTORY | O_NOFOLLOW | O_RDONLY | O_PATH); if (fd != -1) { printf("unexpected success on symlink-dir with O_DIRECTORY\n"); ret = 2; goto cleanup; } /* or on symlinks to files regardless of O_PATH... */ i = 13; fd = open("d/link-file", O_DIRECTORY | O_NOFOLLOW | O_RDONLY | O_PATH); if (fd != -1) { printf("unexpected success on symlink-file with O_DIRECTORY|O_PATH\n"); ret = 2; goto cleanup; } i = 14; fd = open("d/link-file", O_DIRECTORY | O_NOFOLLOW | O_RDONLY); if (fd != -1) { printf("unexpected success on symlink-file with just O_DIRECTORY\n"); ret = 2; goto cleanup; } /* but O_PATH without O_DIRECTORY opens a symlink fd */ i = 15; fd = open("d/link-subdir", O_NOFOLLOW | O_RDONLY | O_PATH); if (fd < 0) goto cleanup; /* However, that symlink fd is not usable in *at */ i = 16; if (fstatat(fd, "subfile", &st, 0) == 0) { printf("unexpected success using symlink fd in fstatat\n"); ret = 2; goto cleanup; } if (errno != EBADF && errno != ENOTDIR) goto cleanup; i = 17; if (close(fd) < 0) goto cleanup; printf("All tests passed\n"); ret = 0; cleanup: if (ret == 1) { perror("unexpected failure"); printf("encountered when i=%d\n", i); } system("chmod -R u+rwx d; rm -rf d"); return ret; }
On Fri, 3 Mar 2017 17:43:49 -0600 Eric Blake <eblake@redhat.com> wrote: > On 03/03/2017 12:14 PM, Eric Blake wrote: > > On 03/03/2017 11:25 AM, Greg Kurz wrote: > >> We should pass O_NOFOLLOW otherwise openat() will follow symlinks and make > >> QEMU vulnerable. > >> > >> O_PATH was used as an optimization: the fd returned by openat_dir() is only > >> passed to openat() actually, so we don't really need to reach the underlying > >> filesystem. > >> > >> O_NOFOLLOW | O_PATH isn't an option: if name is a symlink, openat() will > >> return a fd, forcing us to do some other syscall to detect we have a > >> symlink. Also, O_PATH doesn't exist in glibc 2.13 and older. > > > > But the very next use of openat(fd, ) should fail with EBADF if fd is > > or ENOTDIR, actually > > > not a directory, so you don't need any extra syscalls. I agree that we > > _need_ O_NOFOLLOW, but I'm not yet convinced that we must avoid O_PATH > > where it works. > > > > I'm in the middle of writing a test program to probe kernel behavior and > > demonstrate (at least to myself) whether there are scenarios where > > O_PATH makes it possible to open something where omitting it did not, > > while at the same time validating that O_NOFOLLOW doesn't cause problems > > if a symlink-fd is returned instead of a directory fd, based on our > > subsequent use of that fd in a *at call. > > My test case is below. Note that based on my testing, I think you want > a v2 of this patch, which does: > Yeah, #12 and #13 in your test case show that we're safe because O_DIRECTORY causes openat() to fail with EISDIR right away (we won't have to worry about an hypothetical symlink-fd). > #ifndef O_PATH > #define O_PATH 0 > #endif > It is acceptable to ignore O_PATH here because we have O_DIRECTORY, and we know openat_dir() will hence fail. But this code sits in a header file, and we probably don't want O_PATH to be silently converted to 0 in other potential cases where it is used without O_DIRECTORY. I'd rather do something like the following then: #ifdef O_PATH #define OPENAT_DIR_O_PATH O_PATH #else #define OPENAT_DIR_O_PATH 0 #endif Makes sense ? > static inline int openat_dir(int dirfd, const char *name) > { > - return openat(dirfd, name, O_DIRECTORY | O_RDONLY | O_PATH); > + return openat(dirfd, name, O_DIRECTORY | O_RDONLY | O_NOFOLLOW | > O_PATH); > } > > > > #define _GNU_SOURCE 1 > #include <stdio.h> > #include <fcntl.h> > #include <sys/stat.h> > #include <unistd.h> > #include <errno.h> > #include <stdlib.h> > > int main(int argc, char **argv) > { > int i = 0; > int ret = 1; > int fd; > struct stat st; > > if (mkdir("d", 0700) < 0) { > printf("giving up, please try again once 'd' is removed\n"); > return ret; > } > > /* Create a playground for testing with. */ > i = 1; > if (creat("d/file", 0600) < 0) > goto cleanup; > i = 2; > if (mkdir("d/subdir", 0700) < 0) > goto cleanup; > i = 3; > if (creat("d/subdir/subfile", 0600) < 0) > goto cleanup; > i = 4; > if (chmod("d/subdir", 0100) < 0) > goto cleanup; > i = 5; > if (symlink("file", "d/link-file") < 0) > goto cleanup; > i = 6; > if (symlink("subdir", "d/link-subdir") < 0) > goto cleanup; > > /* Sanity: We can stat a child file with just search permissions, > * whether via a directory or symlink-to-directory */ > i = 7; > if (stat("d/subdir/subfile", &st) < 0) > goto cleanup; > i = 8; > if (stat("d/link-subdir/subfile", &st) < 0) > goto cleanup; > > /* Since the directory is not readable, we can't get a normal fd */ > fd = open("d/subdir", O_DIRECTORY | O_NOFOLLOW | O_RDONLY); > if (fd != -1) { > printf("unexpected success opening non-readable dir\n"); > ret = 2; > goto cleanup; > } > /* But we can get at it with O_PATH */ > i = 9; > fd = open("d/subdir", O_DIRECTORY | O_NOFOLLOW | O_RDONLY | O_PATH); > if (fd < 0) > goto cleanup; > /* And use it in *at functions */ > i = 10; > if (fstatat(fd, "subfile", &st, 0) < 0) > goto cleanup; > i = 11; > if (close(fd) < 0) > goto cleanup; > > /* Note that O_DIRECTORY fails on symlinks with O_PATH... */ > i = 12; > fd = open("d/link-subdir", O_DIRECTORY | O_NOFOLLOW | O_RDONLY | > O_PATH); > if (fd != -1) { > printf("unexpected success on symlink-dir with O_DIRECTORY\n"); > ret = 2; > goto cleanup; > } > /* or on symlinks to files regardless of O_PATH... */ > i = 13; > fd = open("d/link-file", O_DIRECTORY | O_NOFOLLOW | O_RDONLY | O_PATH); > if (fd != -1) { > printf("unexpected success on symlink-file with > O_DIRECTORY|O_PATH\n"); > ret = 2; > goto cleanup; > } > i = 14; > fd = open("d/link-file", O_DIRECTORY | O_NOFOLLOW | O_RDONLY); > if (fd != -1) { > printf("unexpected success on symlink-file with just > O_DIRECTORY\n"); > ret = 2; > goto cleanup; > } > /* but O_PATH without O_DIRECTORY opens a symlink fd */ > i = 15; > fd = open("d/link-subdir", O_NOFOLLOW | O_RDONLY | O_PATH); > if (fd < 0) > goto cleanup; > /* However, that symlink fd is not usable in *at */ > i = 16; > if (fstatat(fd, "subfile", &st, 0) == 0) { > printf("unexpected success using symlink fd in fstatat\n"); > ret = 2; > goto cleanup; > } > if (errno != EBADF && errno != ENOTDIR) > goto cleanup; > i = 17; > if (close(fd) < 0) > goto cleanup; > > printf("All tests passed\n"); > ret = 0; > > cleanup: > if (ret == 1) { > perror("unexpected failure"); > printf("encountered when i=%d\n", i); > } > system("chmod -R u+rwx d; rm -rf d"); > return ret; > } > > >
On 03/04/2017 05:21 AM, Greg Kurz wrote: > On Fri, 3 Mar 2017 17:43:49 -0600 > Eric Blake <eblake@redhat.com> wrote: > > It is acceptable to ignore O_PATH here because we have O_DIRECTORY, and > we know openat_dir() will hence fail. But this code sits in a header > file, and we probably don't want O_PATH to be silently converted to 0 in > other potential cases where it is used without O_DIRECTORY. > > I'd rather do something like the following then: > > #ifdef O_PATH > #define OPENAT_DIR_O_PATH O_PATH > #else > #define OPENAT_DIR_O_PATH 0 > #endif > > Makes sense ? Yes, that's reasonable.
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index 5db7104334d6..e31309a29c58 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -959,7 +959,7 @@ static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name, if (flags == AT_REMOVEDIR) { int fd; - fd = openat(dirfd, name, O_RDONLY | O_DIRECTORY | O_PATH); + fd = openat_dir(dirfd, name); if (fd == -1) { goto err_out; } diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h index 091f3ce88e15..4001d1fd8398 100644 --- a/hw/9pfs/9p-util.h +++ b/hw/9pfs/9p-util.h @@ -22,7 +22,7 @@ static inline void close_preserve_errno(int fd) static inline int openat_dir(int dirfd, const char *name) { - return openat(dirfd, name, O_DIRECTORY | O_RDONLY | O_PATH); + return openat(dirfd, name, O_DIRECTORY | O_RDONLY | O_NOFOLLOW); } static inline int openat_file(int dirfd, const char *name, int flags,
We should pass O_NOFOLLOW otherwise openat() will follow symlinks and make QEMU vulnerable. O_PATH was used as an optimization: the fd returned by openat_dir() is only passed to openat() actually, so we don't really need to reach the underlying filesystem. O_NOFOLLOW | O_PATH isn't an option: if name is a symlink, openat() will return a fd, forcing us to do some other syscall to detect we have a symlink. Also, O_PATH doesn't exist in glibc 2.13 and older. The only sane thing to do is hence to drop O_PATH, and only pass O_NOFOLLOW. While here, we also fix local_unlinkat_common() to use openat_dir() for the same reasons (it was a leftover in the original patchset actually). This fixes CVE-2016-9602. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/9pfs/9p-local.c | 2 +- hw/9pfs/9p-util.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)