[Qemu-devel] [PATCH] 9pfs: fix vulnerability in openat_dir() and local_unlinkat_common()

[Qemu-devel] [PATCH] 9pfs: fix vulnerability in openat_dir() and local_unlinkat_common()

[Greg Kurz]

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,

Re: [Qemu-devel] [PATCH] 9pfs: fix vulnerability in openat_dir() and local_unlinkat_common()

[Daniel P. Berrange]

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/ :|

Re: [Qemu-devel] [PATCH] 9pfs: fix vulnerability in openat_dir() and local_unlinkat_common()

[Mark Cave-Ayland]

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.

Re: [Qemu-devel] [PATCH] 9pfs: fix vulnerability in openat_dir() and local_unlinkat_common()

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 7163 bytes --]

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.
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH] 9pfs: fix vulnerability in openat_dir() and local_unlinkat_common()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1280 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] 9pfs: fix vulnerability in openat_dir() and local_unlinkat_common()

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 1486 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH] 9pfs: fix vulnerability in openat_dir() and local_unlinkat_common()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 5199 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] 9pfs: fix vulnerability in openat_dir() and local_unlinkat_common()

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 6154 bytes --]

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;
> }
> 
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH] 9pfs: fix vulnerability in openat_dir() and local_unlinkat_common()

[Mark Cave-Ayland]

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.

Re: [Qemu-devel] [PATCH] 9pfs: fix vulnerability in openat_dir() and local_unlinkat_common()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 734 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]