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

[Eric Blake <eblake@redhat.com>]

[-- 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 --]