[Qemu-devel] [PULL 0/6] 9pfs fixes for 2.9 2017-03-06

[Qemu-devel] [PULL 0/6] 9pfs fixes for 2.9 2017-03-06

[Greg Kurz]

The following changes since commit 56b51708e9e22809d2a78f38d0ac84bb3f3fca92:

  Merge remote-tracking branch 'remotes/dgibson/tags/ppc-for-2.9-20170306' into staging (2017-03-06 13:06:30 +0000)

are available in the git repository at:

  https://github.com/gkurz/qemu.git tags/fixes-for-2.9

for you to fetch changes up to b003fc0d8aa5e7060dbf7e5862b8013c73857c7f:

  9pfs: fix vulnerability in openat_dir() and local_unlinkat_common() (2017-03-06 17:34:01 +0100)

----------------------------------------------------------------
Fixes issues that got merged with the latest pull request:
- missing O_NOFOLLOW flag for CVE-2016-960
- build break with older glibc that don't have O_PATH and AT_EMPTY_PATH
- various bugs reported by Coverity

----------------------------------------------------------------
Greg Kurz (6):
      9pfs: fix bogus fd check in local_remove()
      9pfs: fix fd leak in local_opendir()
      9pfs: fail local_statfs() earlier
      9pfs: don't use AT_EMPTY_PATH in local_set_cred_passthrough()
      9pfs: fix O_PATH build break with older glibc versions
      9pfs: fix vulnerability in openat_dir() and local_unlinkat_common()

 hw/9pfs/9p-local.c | 10 +++++++---
 hw/9pfs/9p-util.h  |  8 +++++++-
 2 files changed, 14 insertions(+), 4 deletions(-)
-- 
2.7.4

[Qemu-devel] [PULL 1/6] 9pfs: fix bogus fd check in local_remove()

[Greg Kurz]

This was spotted by Coverity as a fd leak. This is certainly true, but also
local_remove() would always return without doing anything, unless the fd is
zero, which is very unlikely.

(Coverity issue CID1371732)

Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 hw/9pfs/9p-local.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index f22a3c3654db..5db7104334d6 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1008,7 +1008,7 @@ static int local_remove(FsContext *ctx, const char *path)
     int err = -1;
 
     dirfd = local_opendir_nofollow(ctx, dirpath);
-    if (dirfd) {
+    if (dirfd == -1) {
         goto out;
     }
 
-- 
2.7.4

[Qemu-devel] [PULL 2/6] 9pfs: fix fd leak in local_opendir()

[Greg Kurz]

Coverity issue CID1371731

Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/9pfs/9p-local.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 5db7104334d6..09f6a46d61b8 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -435,6 +435,7 @@ static int local_opendir(FsContext *ctx,
 
     stream = fdopendir(dirfd);
     if (!stream) {
+        close(dirfd);
         return -1;
     }
     fs->dir.stream = stream;
-- 
2.7.4

[Qemu-devel] [PULL 3/6] 9pfs: fail local_statfs() earlier

[Greg Kurz]

If we cannot open the given path, we can return right away instead of
passing -1 to fstatfs() and close(). This will make Coverity happy.

(Coverity issue CID1371729)

Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Daniel P. berrange <berrange@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/9pfs/9p-local.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 09f6a46d61b8..6d16c4a06587 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1053,6 +1053,9 @@ static int local_statfs(FsContext *s, V9fsPath *fs_path, struct statfs *stbuf)
     int fd, ret;
 
     fd = local_open_nofollow(s, fs_path->data, O_RDONLY, 0);
+    if (fd == -1) {
+        return -1;
+    }
     ret = fstatfs(fd, stbuf);
     close_preserve_errno(fd);
     return ret;
-- 
2.7.4

[Qemu-devel] [PULL 4/6] 9pfs: don't use AT_EMPTY_PATH in local_set_cred_passthrough()

[Greg Kurz]

The name argument can never be an empty string, and dirfd always point to
the containing directory of the file name. AT_EMPTY_PATH is hence useless
here. Also it breaks build with glibc version 2.13 and older.

It is actually an oversight of a previous tentative patch to implement this
function. We can safely drop it.

Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Greg Kurz <groug@kaod.org>
Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 hw/9pfs/9p-local.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 6d16c4a06587..0ca4c94ee4a8 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -349,7 +349,7 @@ static int local_set_cred_passthrough(FsContext *fs_ctx, int dirfd,
                                       const char *name, FsCred *credp)
 {
     if (fchownat(dirfd, name, credp->fc_uid, credp->fc_gid,
-                 AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH) < 0) {
+                 AT_SYMLINK_NOFOLLOW) < 0) {
         /*
          * If we fail to change ownership and if we are
          * using security model none. Ignore the error
-- 
2.7.4

[Qemu-devel] [PULL 5/6] 9pfs: fix O_PATH build break with older glibc versions

[Greg Kurz]

When O_PATH is used with O_DIRECTORY, it only acts as an optimization: the
openat() syscall simply finds the name in the VFS, and doesn't trigger the
underlying filesystem.

On systems that don't define O_PATH, because they have glibc version 2.13
or older for example, we can safely omit it. We don't want to deactivate
O_PATH globally though, in case it is used without O_DIRECTORY. The is done
with a dedicated macro.

Systems without O_PATH may thus fail to resolve names that involve
unreadable directories, compared to newer systems succeeding, but such
corner case failure is our only option on those older systems to avoid
the security hole of chasing symlinks inappropriately.

Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Eric Blake <eblake@redhat.com>
(added last paragraph to changelog as suggested by Eric Blake)
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-util.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
index 091f3ce88e15..cb7b2072d3ac 100644
--- a/hw/9pfs/9p-util.h
+++ b/hw/9pfs/9p-util.h
@@ -22,7 +22,12 @@ 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);
+#ifdef O_PATH
+#define OPENAT_DIR_O_PATH O_PATH
+#else
+#define OPENAT_DIR_O_PATH 0
+#endif
+    return openat(dirfd, name, O_DIRECTORY | O_RDONLY | OPENAT_DIR_O_PATH);
 }
 
 static inline int openat_file(int dirfd, const char *name, int flags,
-- 
2.7.4

[Qemu-devel] [PULL 6/6] 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.

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>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 hw/9pfs/9p-local.c | 2 +-
 hw/9pfs/9p-util.h  | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 0ca4c94ee4a8..45e9a1f9b0ca 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -960,7 +960,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 cb7b2072d3ac..517027c52032 100644
--- a/hw/9pfs/9p-util.h
+++ b/hw/9pfs/9p-util.h
@@ -27,7 +27,8 @@ static inline int openat_dir(int dirfd, const char *name)
 #else
 #define OPENAT_DIR_O_PATH 0
 #endif
-    return openat(dirfd, name, O_DIRECTORY | O_RDONLY | OPENAT_DIR_O_PATH);
+    return openat(dirfd, name,
+                  O_DIRECTORY | O_RDONLY | O_NOFOLLOW | OPENAT_DIR_O_PATH);
 }
 
 static inline int openat_file(int dirfd, const char *name, int flags,
-- 
2.7.4

Re: [Qemu-devel] [PULL 0/6] 9pfs fixes for 2.9 2017-03-06

[Mark Cave-Ayland]

On 06/03/17 17:54, Greg Kurz wrote:

> The following changes since commit 56b51708e9e22809d2a78f38d0ac84bb3f3fca92:
> 
>   Merge remote-tracking branch 'remotes/dgibson/tags/ppc-for-2.9-20170306' into staging (2017-03-06 13:06:30 +0000)
> 
> are available in the git repository at:
> 
>   https://github.com/gkurz/qemu.git tags/fixes-for-2.9
> 
> for you to fetch changes up to b003fc0d8aa5e7060dbf7e5862b8013c73857c7f:
> 
>   9pfs: fix vulnerability in openat_dir() and local_unlinkat_common() (2017-03-06 17:34:01 +0100)
> 
> ----------------------------------------------------------------
> Fixes issues that got merged with the latest pull request:
> - missing O_NOFOLLOW flag for CVE-2016-960
> - build break with older glibc that don't have O_PATH and AT_EMPTY_PATH
> - various bugs reported by Coverity
> 
> ----------------------------------------------------------------
> Greg Kurz (6):
>       9pfs: fix bogus fd check in local_remove()
>       9pfs: fix fd leak in local_opendir()
>       9pfs: fail local_statfs() earlier
>       9pfs: don't use AT_EMPTY_PATH in local_set_cred_passthrough()
>       9pfs: fix O_PATH build break with older glibc versions
>       9pfs: fix vulnerability in openat_dir() and local_unlinkat_common()
> 
>  hw/9pfs/9p-local.c | 10 +++++++---
>  hw/9pfs/9p-util.h  |  8 +++++++-
>  2 files changed, 14 insertions(+), 4 deletions(-)

Greg - just to confirm that I've done a local checkout of the above tag
and it fixes the build for me. Thanks for taking the time to sort this
one out even though it's an issue caused by an older OS.


ATB,

Mark.

Re: [Qemu-devel] [PULL 0/6] 9pfs fixes for 2.9 2017-03-06

[Peter Maydell]

On 6 March 2017 at 17:54, Greg Kurz <groug@kaod.org> wrote:
> The following changes since commit 56b51708e9e22809d2a78f38d0ac84bb3f3fca92:
>
>   Merge remote-tracking branch 'remotes/dgibson/tags/ppc-for-2.9-20170306' into staging (2017-03-06 13:06:30 +0000)
>
> are available in the git repository at:
>
>   https://github.com/gkurz/qemu.git tags/fixes-for-2.9
>
> for you to fetch changes up to b003fc0d8aa5e7060dbf7e5862b8013c73857c7f:
>
>   9pfs: fix vulnerability in openat_dir() and local_unlinkat_common() (2017-03-06 17:34:01 +0100)
>
> ----------------------------------------------------------------
> Fixes issues that got merged with the latest pull request:
> - missing O_NOFOLLOW flag for CVE-2016-960
> - build break with older glibc that don't have O_PATH and AT_EMPTY_PATH
> - various bugs reported by Coverity
>
> ----------------------------------------------------------------
> Greg Kurz (6):
>       9pfs: fix bogus fd check in local_remove()
>       9pfs: fix fd leak in local_opendir()
>       9pfs: fail local_statfs() earlier
>       9pfs: don't use AT_EMPTY_PATH in local_set_cred_passthrough()
>       9pfs: fix O_PATH build break with older glibc versions
>       9pfs: fix vulnerability in openat_dir() and local_unlinkat_common()
>

Applied, thanks.

-- PMM