On 04/03/17 11:21, Greg Kurz wrote:
> On Fri, 3 Mar 2017 17:43:49 -0600
> Eric Blake <ebl...@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.
