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 ?
> 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;
> }
>
>
>
pgpdACtO8ecxW.pgp
Description: OpenPGP digital signature
