[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 07/29] 9pfs: local: introduce symlink-attack saf
From: |
Greg Kurz |
Subject: |
Re: [Qemu-devel] [PATCH 07/29] 9pfs: local: introduce symlink-attack safe xattr helpers |
Date: |
Thu, 23 Feb 2017 21:54:54 +0100 |
On Thu, 23 Feb 2017 13:44:41 +0000
Stefan Hajnoczi <address@hidden> wrote:
> On Mon, Feb 20, 2017 at 03:40:15PM +0100, Greg Kurz wrote:
> > +static ssize_t do_xattrat_op(int op_type, int dirfd, const char *path,
> > + const char *name, void *value, size_t size,
> > + int flags)
> > +{
> > + struct xattrat_data *data;
> > + pid_t pid;
> > + ssize_t ret = -1;
> > + int wstatus;
> > +
> > + data = mmap(NULL, sizeof(*data) + size, PROT_READ | PROT_WRITE,
> > + MAP_SHARED | MAP_ANONYMOUS, -1, 0);
> > + if (data == MAP_FAILED) {
> > + return -1;
> > + }
> > + data->ret = -1;
> > +
> > + pid = fork();
> > + if (pid < 0) {
> > + goto err_out;
> > + } else if (pid == 0) {
> > + if (fchdir(dirfd) == 0) {
> > + switch (op_type) {
> > + case XATTRAT_OP_GET:
> > + data->ret = lgetxattr(path, name, data->value, size);
> > + break;
> > + case XATTRAT_OP_LIST:
> > + data->ret = llistxattr(path, data->value, size);
> > + break;
> > + case XATTRAT_OP_SET:
> > + data->ret = lsetxattr(path, name, value, size, flags);
> > + break;
> > + case XATTRAT_OP_REMOVE:
> > + data->ret = lremovexattr(path, name);
> > + break;
> > + default:
> > + g_assert_not_reached();
> > + }
> > + }
> > + data->serrno = errno;
> > + _exit(0);
> > + }
> > + assert(waitpid(pid, &wstatus, 0) == pid && WIFEXITED(wstatus));
> > +
> > + ret = data->ret;
> > + if (ret < 0) {
> > + errno = data->serrno;
> > + goto err_out;
> > + }
> > + if (value) {
> > + memcpy(value, data->value, data->ret);
> > + }
> > +err_out:
> > + munmap_preserver_errno(data, sizeof(*data) + size);
> > + return ret;
> > +}
>
> Forking is ugly since QEMU is a multi-threaded program. We brainstormed
Yeah, forking is ugly and it completely ruins metadata performance (x30
slower in passthrough mode and x300 slower in mapped-xattr mode).
> alternatives on IRC like using /proc/self/fd/$fd to work around the
> missing getxattrat() API.
>
This should do the trick indeed. If we have to call getxattr()
on some untrusted $path that may be modified by the guest. We
can do:
dirfd = openat_nofollow($mount_fd, dirname($path))
filename = basename($path)
and then we can safely call:
lgetxattr("/proc/self/fd/$dirfd/$filename")
since "/proc/self/fd/$dirfd" is trusted.
> Stefan
pgpp6sZRH1yr9.pgp
Description: OpenPGP digital signature