[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Mark Cave-Ayland
Subject: Re: [Qemu-devel] [PATCH] 9pfs: fix vulnerability in openat_dir() and local_unlinkat_common()
Date: Sat, 4 Mar 2017 13:55:47 +0000
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.6.0

On 04/03/17 11:21, Greg Kurz wrote:

> On Fri, 3 Mar 2017 17:43:49 -0600
> Eric Blake <address@hidden> 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
> #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 0
 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,



reply via email to

[Prev in Thread] Current Thread [Next in Thread]