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

[Greg Kurz]

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

pgpHiQXq0m_LW.pgp
Description: OpenPGP digital signature