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
signature.asc
Description: OpenPGP digital signature
