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,
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
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
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 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 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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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;
}
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
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 04/03/17 11:21, Greg Kurz wrote:
> 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 ?
I can confirm that the revised version of the original patch below fixes
the build issue for me.
Given that I don't have any configurations set up for 9pfs then I don't
have an easy way to verify the security aspects of the patch but it
looks like Eric's tests have verified this.
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 01467d2..96c1736 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 091f3ce..c26ed1b 100644
--- a/hw/9pfs/9p-util.h
+++ b/hw/9pfs/9p-util.h
@@ -20,9 +20,16 @@ static inline void close_preserve_errno(int fd)
errno = serrno;
}
+#ifdef O_PATH
+#define OPENAT_DIR_O_PATH O_PATH
+#else
+#define OPENAT_DIR_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 | OPENAT_DIR_O_PATH |
+ O_NOFOLLOW);
}
static inline int openat_file(int dirfd, const char *name, int flags,
ATB,
Mark.
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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
© 2016 - 2024 Red Hat, Inc.