| Message ID | 148760173919.31154.7555675803159581620.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
On Mon, Feb 20, 2017 at 03:42:19PM +0100, Greg Kurz wrote: > The local_chmod() callback is vulnerable to symlink attacks because it > calls: > > (1) chmod() which follows symbolic links for all path elements > (2) local_set_xattr()->setxattr() which follows symbolic links for all > path elements > (3) local_set_mapped_file_attr() which calls in turn local_fopen() and > mkdir(), both functions following symbolic links for all path > elements but the rightmost one > > This patch converts local_chmod() to rely on open_nofollow() and > fchmod() to fix (1). > > It introduces a local_set_xattrat() replacement to local_set_xattr() > based on fsetxattrat() to fix (2), and a local_set_mapped_file_attrat() > replacement to local_set_mapped_file_attr() based on local_fopenat() > and mkdirat() to fix (3). No effort is made to factor out code because > both local_set_xattr() and local_set_mapped_file_attr() will be dropped > when all users have been converted to use the "at" versions. > > This partly fixes CVE-2016-9602. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/9pfs/9p-local.c | 163 ++++++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 150 insertions(+), 13 deletions(-) > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > index fb536fdb3082..56d7731f7ce1 100644 > --- a/hw/9pfs/9p-local.c > +++ b/hw/9pfs/9p-local.c > @@ -327,6 +327,87 @@ err_out: > return ret; > } > > +static int local_set_mapped_file_attrat(int dirfd, const char *name, > + FsCred *credp) > +{ > + FILE *fp; > + int ret; > + char buf[ATTR_MAX]; > + int uid = -1, gid = -1, mode = -1, rdev = -1; > + int map_dirfd; > + > + ret = mkdirat(dirfd, VIRTFS_META_DIR, 0700); > + if (ret < 0 && errno != EEXIST) { > + return -1; > + } > + > + map_dirfd = openat(dirfd, VIRTFS_META_DIR, > + O_RDONLY | O_DIRECTORY | O_NOFOLLOW); > + if (map_dirfd == -1) { > + return -1; > + } > + > + fp = local_fopenat(map_dirfd, name, "r"); > + if (!fp) { > + if (errno == ENOENT) { > + goto update_map_file; > + } else { > + close_preserve_errno(map_dirfd); > + return -1; > + } > + } > + memset(buf, 0, ATTR_MAX); > + while (fgets(buf, ATTR_MAX, fp)) { > + if (!strncmp(buf, "virtfs.uid", 10)) { > + uid = atoi(buf + 11); > + } else if (!strncmp(buf, "virtfs.gid", 10)) { > + gid = atoi(buf + 11); > + } else if (!strncmp(buf, "virtfs.mode", 11)) { > + mode = atoi(buf + 12); > + } else if (!strncmp(buf, "virtfs.rdev", 11)) { > + rdev = atoi(buf + 12); > + } > + memset(buf, 0, ATTR_MAX); > + } > + fclose(fp); > + > +update_map_file: > + fp = local_fopenat(map_dirfd, name, "w"); > + close_preserve_errno(map_dirfd); > + if (!fp) { > + return -1; > + } > + > + if (credp->fc_uid != -1) { > + uid = credp->fc_uid; > + } > + if (credp->fc_gid != -1) { > + gid = credp->fc_gid; > + } > + if (credp->fc_mode != -1) { > + mode = credp->fc_mode; > + } > + if (credp->fc_rdev != -1) { > + rdev = credp->fc_rdev; > + } > + > + if (uid != -1) { > + fprintf(fp, "virtfs.uid=%d\n", uid); > + } > + if (gid != -1) { > + fprintf(fp, "virtfs.gid=%d\n", gid); > + } > + if (mode != -1) { > + fprintf(fp, "virtfs.mode=%d\n", mode); > + } > + if (rdev != -1) { > + fprintf(fp, "virtfs.rdev=%d\n", rdev); > + } > + fclose(fp); > + > + return 0; > +} > + > static int local_set_xattr(const char *path, FsCred *credp) > { > int err; > @@ -362,6 +443,45 @@ static int local_set_xattr(const char *path, FsCred *credp) > return 0; > } > > +static int local_set_xattrat(int dirfd, const char *path, FsCred *credp) > +{ > + int err; > + > + if (credp->fc_uid != -1) { > + uint32_t tmp_uid = cpu_to_le32(credp->fc_uid); > + err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.uid", &tmp_uid, > + sizeof(uid_t), 0); > + if (err) { > + return err; > + } > + } > + if (credp->fc_gid != -1) { > + uint32_t tmp_gid = cpu_to_le32(credp->fc_gid); > + err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.gid", &tmp_gid, > + sizeof(gid_t), 0); > + if (err) { > + return err; > + } > + } > + if (credp->fc_mode != -1) { > + uint32_t tmp_mode = cpu_to_le32(credp->fc_mode); > + err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.mode", &tmp_mode, > + sizeof(mode_t), 0); > + if (err) { > + return err; > + } > + } > + if (credp->fc_rdev != -1) { > + uint64_t tmp_rdev = cpu_to_le64(credp->fc_rdev); > + err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.rdev", &tmp_rdev, > + sizeof(dev_t), 0); > + if (err) { > + return err; > + } > + } > + return 0; > +} > + > static int local_post_create_passthrough(FsContext *fs_ctx, const char *path, > FsCred *credp) > { > @@ -553,21 +673,38 @@ static ssize_t local_pwritev(FsContext *ctx, V9fsFidOpenState *fs, > > static int local_chmod(FsContext *fs_ctx, V9fsPath *fs_path, FsCred *credp) > { > - char *buffer; > int ret = -1; > - char *path = fs_path->data; > + int dirfd; > > - if (fs_ctx->export_flags & V9FS_SM_MAPPED) { > - buffer = rpath(fs_ctx, path); > - ret = local_set_xattr(buffer, credp); > - g_free(buffer); > - } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) { > - return local_set_mapped_file_attr(fs_ctx, path, credp); > - } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) || > - (fs_ctx->export_flags & V9FS_SM_NONE)) { > - buffer = rpath(fs_ctx, path); > - ret = chmod(buffer, credp->fc_mode); > - g_free(buffer); > + if (fs_ctx->export_flags & V9FS_SM_MAPPED || > + fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) { > + char *dirpath, *name; > + > + dirpath = g_path_get_dirname(fs_path->data); > + dirfd = local_opendir_nofollow(fs_ctx, dirpath); > + g_free(dirpath); > + if (dirfd == -1) { > + return -1; > + } > + > + name = g_path_get_basename(fs_path->data); > + if (fs_ctx->export_flags & V9FS_SM_MAPPED) { > + ret = local_set_xattrat(dirfd, name, credp); > + } else { > + ret = local_set_mapped_file_attrat(dirfd, name, credp); > + } > + g_free(name); > + close_preserve_errno(dirfd); > + } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH || > + fs_ctx->export_flags & V9FS_SM_NONE) { > + int fd; > + > + fd = local_open_nofollow(fs_ctx, fs_path->data, O_RDONLY, 0); > + if (fd == -1) { > + return -1; > + } > + ret = fchmod(fd, credp->fc_mode); > + close_preserve_errno(fd); As mentioned on IRC, not sure this is workable since chmod(2) must not require read permission on the file. This patch introduces failures when the file isn't readable. Stefan
On Thu, 23 Feb 2017 15:10:42 +0000 Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Mon, Feb 20, 2017 at 03:42:19PM +0100, Greg Kurz wrote: > > The local_chmod() callback is vulnerable to symlink attacks because it > > calls: > > > > (1) chmod() which follows symbolic links for all path elements > > (2) local_set_xattr()->setxattr() which follows symbolic links for all > > path elements > > (3) local_set_mapped_file_attr() which calls in turn local_fopen() and > > mkdir(), both functions following symbolic links for all path > > elements but the rightmost one > > > > This patch converts local_chmod() to rely on open_nofollow() and > > fchmod() to fix (1). > > > > It introduces a local_set_xattrat() replacement to local_set_xattr() > > based on fsetxattrat() to fix (2), and a local_set_mapped_file_attrat() > > replacement to local_set_mapped_file_attr() based on local_fopenat() > > and mkdirat() to fix (3). No effort is made to factor out code because > > both local_set_xattr() and local_set_mapped_file_attr() will be dropped > > when all users have been converted to use the "at" versions. > > > > This partly fixes CVE-2016-9602. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > hw/9pfs/9p-local.c | 163 ++++++++++++++++++++++++++++++++++++++++++++++++---- > > 1 file changed, 150 insertions(+), 13 deletions(-) > > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > > index fb536fdb3082..56d7731f7ce1 100644 > > --- a/hw/9pfs/9p-local.c > > +++ b/hw/9pfs/9p-local.c > > @@ -327,6 +327,87 @@ err_out: > > return ret; > > } > > > > +static int local_set_mapped_file_attrat(int dirfd, const char *name, > > + FsCred *credp) > > +{ > > + FILE *fp; > > + int ret; > > + char buf[ATTR_MAX]; > > + int uid = -1, gid = -1, mode = -1, rdev = -1; > > + int map_dirfd; > > + > > + ret = mkdirat(dirfd, VIRTFS_META_DIR, 0700); > > + if (ret < 0 && errno != EEXIST) { > > + return -1; > > + } > > + > > + map_dirfd = openat(dirfd, VIRTFS_META_DIR, > > + O_RDONLY | O_DIRECTORY | O_NOFOLLOW); > > + if (map_dirfd == -1) { > > + return -1; > > + } > > + > > + fp = local_fopenat(map_dirfd, name, "r"); > > + if (!fp) { > > + if (errno == ENOENT) { > > + goto update_map_file; > > + } else { > > + close_preserve_errno(map_dirfd); > > + return -1; > > + } > > + } > > + memset(buf, 0, ATTR_MAX); > > + while (fgets(buf, ATTR_MAX, fp)) { > > + if (!strncmp(buf, "virtfs.uid", 10)) { > > + uid = atoi(buf + 11); > > + } else if (!strncmp(buf, "virtfs.gid", 10)) { > > + gid = atoi(buf + 11); > > + } else if (!strncmp(buf, "virtfs.mode", 11)) { > > + mode = atoi(buf + 12); > > + } else if (!strncmp(buf, "virtfs.rdev", 11)) { > > + rdev = atoi(buf + 12); > > + } > > + memset(buf, 0, ATTR_MAX); > > + } > > + fclose(fp); > > + > > +update_map_file: > > + fp = local_fopenat(map_dirfd, name, "w"); > > + close_preserve_errno(map_dirfd); > > + if (!fp) { > > + return -1; > > + } > > + > > + if (credp->fc_uid != -1) { > > + uid = credp->fc_uid; > > + } > > + if (credp->fc_gid != -1) { > > + gid = credp->fc_gid; > > + } > > + if (credp->fc_mode != -1) { > > + mode = credp->fc_mode; > > + } > > + if (credp->fc_rdev != -1) { > > + rdev = credp->fc_rdev; > > + } > > + > > + if (uid != -1) { > > + fprintf(fp, "virtfs.uid=%d\n", uid); > > + } > > + if (gid != -1) { > > + fprintf(fp, "virtfs.gid=%d\n", gid); > > + } > > + if (mode != -1) { > > + fprintf(fp, "virtfs.mode=%d\n", mode); > > + } > > + if (rdev != -1) { > > + fprintf(fp, "virtfs.rdev=%d\n", rdev); > > + } > > + fclose(fp); > > + > > + return 0; > > +} > > + > > static int local_set_xattr(const char *path, FsCred *credp) > > { > > int err; > > @@ -362,6 +443,45 @@ static int local_set_xattr(const char *path, FsCred *credp) > > return 0; > > } > > > > +static int local_set_xattrat(int dirfd, const char *path, FsCred *credp) > > +{ > > + int err; > > + > > + if (credp->fc_uid != -1) { > > + uint32_t tmp_uid = cpu_to_le32(credp->fc_uid); > > + err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.uid", &tmp_uid, > > + sizeof(uid_t), 0); > > + if (err) { > > + return err; > > + } > > + } > > + if (credp->fc_gid != -1) { > > + uint32_t tmp_gid = cpu_to_le32(credp->fc_gid); > > + err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.gid", &tmp_gid, > > + sizeof(gid_t), 0); > > + if (err) { > > + return err; > > + } > > + } > > + if (credp->fc_mode != -1) { > > + uint32_t tmp_mode = cpu_to_le32(credp->fc_mode); > > + err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.mode", &tmp_mode, > > + sizeof(mode_t), 0); > > + if (err) { > > + return err; > > + } > > + } > > + if (credp->fc_rdev != -1) { > > + uint64_t tmp_rdev = cpu_to_le64(credp->fc_rdev); > > + err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.rdev", &tmp_rdev, > > + sizeof(dev_t), 0); > > + if (err) { > > + return err; > > + } > > + } > > + return 0; > > +} > > + > > static int local_post_create_passthrough(FsContext *fs_ctx, const char *path, > > FsCred *credp) > > { > > @@ -553,21 +673,38 @@ static ssize_t local_pwritev(FsContext *ctx, V9fsFidOpenState *fs, > > > > static int local_chmod(FsContext *fs_ctx, V9fsPath *fs_path, FsCred *credp) > > { > > - char *buffer; > > int ret = -1; > > - char *path = fs_path->data; > > + int dirfd; > > > > - if (fs_ctx->export_flags & V9FS_SM_MAPPED) { > > - buffer = rpath(fs_ctx, path); > > - ret = local_set_xattr(buffer, credp); > > - g_free(buffer); > > - } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) { > > - return local_set_mapped_file_attr(fs_ctx, path, credp); > > - } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) || > > - (fs_ctx->export_flags & V9FS_SM_NONE)) { > > - buffer = rpath(fs_ctx, path); > > - ret = chmod(buffer, credp->fc_mode); > > - g_free(buffer); > > + if (fs_ctx->export_flags & V9FS_SM_MAPPED || > > + fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) { > > + char *dirpath, *name; > > + > > + dirpath = g_path_get_dirname(fs_path->data); > > + dirfd = local_opendir_nofollow(fs_ctx, dirpath); > > + g_free(dirpath); > > + if (dirfd == -1) { > > + return -1; > > + } > > + > > + name = g_path_get_basename(fs_path->data); > > + if (fs_ctx->export_flags & V9FS_SM_MAPPED) { > > + ret = local_set_xattrat(dirfd, name, credp); > > + } else { > > + ret = local_set_mapped_file_attrat(dirfd, name, credp); > > + } > > + g_free(name); > > + close_preserve_errno(dirfd); > > + } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH || > > + fs_ctx->export_flags & V9FS_SM_NONE) { > > + int fd; > > + > > + fd = local_open_nofollow(fs_ctx, fs_path->data, O_RDONLY, 0); > > + if (fd == -1) { > > + return -1; > > + } > > + ret = fchmod(fd, credp->fc_mode); > > + close_preserve_errno(fd); > > As mentioned on IRC, not sure this is workable since chmod(2) must not > require read permission on the file. This patch introduces failures > when the file isn't readable. > Yeah :-\ This one is the more painful issue to address. I need a chmod() variant that would fail on symlinks instead of following them... and I'm kinda suffering to implement this in a race-free manner. Cc'ing Eric for insights. > Stefan
On 02/24/2017 04:34 AM, Greg Kurz wrote: > On Thu, 23 Feb 2017 15:10:42 +0000 > Stefan Hajnoczi <stefanha@gmail.com> wrote: > >> On Mon, Feb 20, 2017 at 03:42:19PM +0100, Greg Kurz wrote: >>> The local_chmod() callback is vulnerable to symlink attacks because it >>> calls: >>> >>> (1) chmod() which follows symbolic links for all path elements >>> (2) local_set_xattr()->setxattr() which follows symbolic links for all >>> path elements >>> (3) local_set_mapped_file_attr() which calls in turn local_fopen() and >>> mkdir(), both functions following symbolic links for all path >>> elements but the rightmost one >>> >>> + fd = local_open_nofollow(fs_ctx, fs_path->data, O_RDONLY, 0); >>> + if (fd == -1) { >>> + return -1; >>> + } >>> + ret = fchmod(fd, credp->fc_mode); >>> + close_preserve_errno(fd); >> >> As mentioned on IRC, not sure this is workable since chmod(2) must not >> require read permission on the file. This patch introduces failures >> when the file isn't readable. >> > > Yeah :-\ This one is the more painful issue to address. I need a > chmod() variant that would fail on symlinks instead of following > them... and I'm kinda suffering to implement this in a race-free > manner. BSD has lchmod(), but Linux does not. And Linux does not yet properly implement AT_SYMLINK_NOFOLLOW. (The BSD semantics are pretty cool: restricting mode bits on a symlink can create scenarios where some users can dereference the symlink but others get ENOENT failures - but I don't know of any effort to add those semantics to the Linux kernel) POSIX says support for AT_SYMLINK_NOFOLLOW is optional, precisely because POSIX does not require permissions on symlinks to matter, but the way it requires it is that AT_SYMLINK_NOFOLLOW is supposed to be a no-op for regular files, and cause an EOPNOTSUPP failure for symlinks. Unfortunately, the Linux man page says that Linux fails with ENOTSUP (which is identical to EOPNOTSUPP) any time AT_SYMLINK_NOFOLLOW is set, and not just if it is attempted on a symlink. And I just verified that poor behavior as follows: $ cat foo.c #include <stdio.h> #include <fcntl.h> #include <sys/stat.h> #include <unistd.h> #include <errno.h> int main(int argc, char **argv) { int ret = 1; if (symlink("a", "l") < 0) goto cleanup; if (fchmodat(AT_FDCWD, "l", 0600, AT_SYMLINK_NOFOLLOW) == 0) printf("kernel supports mode bits on symlinks\n"); else if (errno != EOPNOTSUPP) { printf("Oops, kernel failed with %m on symlink\n"); goto cleanup; } if (creat("f", 0600) < 0) goto cleanup; if (fchmodat(AT_FDCWD, "f", 0600, AT_SYMLINK_NOFOLLOW) < 0) { printf("Oops, kernel failed with %m on regular file\n"); goto cleanup; } ret = 0; cleanup: remove("l"); remove("f"); return ret; } $ ./foo Oops, kernel failed with Operation not supported on regular file If you were to get that kernel bug fixed, then you could use fchmodat() to do a safe chmod() which does not chase through a final symlink, and relative to a safe directory fd that you obtain for the rest of the path (as done elsewhere in the series). Use the CVE nature of the problem as leverage on the kernel developers, if that's what it takes.
On Fri, Feb 24, 2017 at 4:23 PM, Eric Blake <eblake@redhat.com> wrote: > On 02/24/2017 04:34 AM, Greg Kurz wrote: >> On Thu, 23 Feb 2017 15:10:42 +0000 >> Stefan Hajnoczi <stefanha@gmail.com> wrote: >> >>> On Mon, Feb 20, 2017 at 03:42:19PM +0100, Greg Kurz wrote: >>>> The local_chmod() callback is vulnerable to symlink attacks because it >>>> calls: >>>> >>>> (1) chmod() which follows symbolic links for all path elements >>>> (2) local_set_xattr()->setxattr() which follows symbolic links for all >>>> path elements >>>> (3) local_set_mapped_file_attr() which calls in turn local_fopen() and >>>> mkdir(), both functions following symbolic links for all path >>>> elements but the rightmost one >>>> > >>>> + fd = local_open_nofollow(fs_ctx, fs_path->data, O_RDONLY, 0); >>>> + if (fd == -1) { >>>> + return -1; >>>> + } >>>> + ret = fchmod(fd, credp->fc_mode); >>>> + close_preserve_errno(fd); >>> >>> As mentioned on IRC, not sure this is workable since chmod(2) must not >>> require read permission on the file. This patch introduces failures >>> when the file isn't readable. >>> >> >> Yeah :-\ This one is the more painful issue to address. I need a >> chmod() variant that would fail on symlinks instead of following >> them... and I'm kinda suffering to implement this in a race-free >> manner. > > BSD has lchmod(), but Linux does not. And Linux does not yet properly > implement AT_SYMLINK_NOFOLLOW. (The BSD semantics are pretty cool: > restricting mode bits on a symlink can create scenarios where some users > can dereference the symlink but others get ENOENT failures - but I don't > know of any effort to add those semantics to the Linux kernel) > > POSIX says support for AT_SYMLINK_NOFOLLOW is optional, precisely > because POSIX does not require permissions on symlinks to matter, but > the way it requires it is that AT_SYMLINK_NOFOLLOW is supposed to be a > no-op for regular files, and cause an EOPNOTSUPP failure for symlinks. > > Unfortunately, the Linux man page says that Linux fails with ENOTSUP > (which is identical to EOPNOTSUPP) any time AT_SYMLINK_NOFOLLOW is set, > and not just if it is attempted on a symlink. And unfortunately, that flags argument is not actually present in the real syscall. See this glibc code: int fchmodat (int fd, const char *file, mode_t mode, int flag) { if (flag & ~AT_SYMLINK_NOFOLLOW) return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL); #ifndef __NR_lchmod /* Linux so far has no lchmod syscall. */ if (flag & AT_SYMLINK_NOFOLLOW) return INLINE_SYSCALL_ERROR_RETURN_VALUE (ENOTSUP); #endif return INLINE_SYSCALL (fchmodat, 3, fd, file, mode); } and this kernel code: SYSCALL_DEFINE3(fchmodat, int, dfd, const char __user *, filename, umode_t, mode) { [...] } So to fix this, you'll probably have to add a new syscall fchmodat2() to the kernel, wire it up for all the architectures and get the various libc implementations to adopt that. That's going to be quite tedious. :(
On Fri, 24 Feb 2017 17:22:19 +0100 Jann Horn <jannh@google.com> wrote: > [...] > And unfortunately, that flags argument is not actually present in the > real syscall. > See this glibc code: > > int > fchmodat (int fd, const char *file, mode_t mode, int flag) > { > if (flag & ~AT_SYMLINK_NOFOLLOW) > return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL); > #ifndef __NR_lchmod /* Linux so far has no lchmod syscall. */ > if (flag & AT_SYMLINK_NOFOLLOW) > return INLINE_SYSCALL_ERROR_RETURN_VALUE (ENOTSUP); > #endif > > return INLINE_SYSCALL (fchmodat, 3, fd, file, mode); > } > > and this kernel code: > > SYSCALL_DEFINE3(fchmodat, int, dfd, const char __user *, filename, > umode_t, mode) > { > [...] > } > > So to fix this, you'll probably have to add a new syscall fchmodat2() > to the kernel, > wire it up for all the architectures and get the various libc > implementations to adopt > that. That's going to be quite tedious. :( Yeah, Eric and I had a discussion about that on irc. I'll start to work on the kernel part, at least. Indeed, adoption by the various libc is likely to take some time... When the syscalls are available in the kernel, maybe it is possible to implement something in the 9pfs code with the syscall() function. In the meantime, we'll have to live with a degraded version of fchmodat() based on openat()+fchmod(). This will fail if the file isn't accessible but it is better than allowing the guest to chmod() any file on the host.
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index fb536fdb3082..56d7731f7ce1 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -327,6 +327,87 @@ err_out: return ret; } +static int local_set_mapped_file_attrat(int dirfd, const char *name, + FsCred *credp) +{ + FILE *fp; + int ret; + char buf[ATTR_MAX]; + int uid = -1, gid = -1, mode = -1, rdev = -1; + int map_dirfd; + + ret = mkdirat(dirfd, VIRTFS_META_DIR, 0700); + if (ret < 0 && errno != EEXIST) { + return -1; + } + + map_dirfd = openat(dirfd, VIRTFS_META_DIR, + O_RDONLY | O_DIRECTORY | O_NOFOLLOW); + if (map_dirfd == -1) { + return -1; + } + + fp = local_fopenat(map_dirfd, name, "r"); + if (!fp) { + if (errno == ENOENT) { + goto update_map_file; + } else { + close_preserve_errno(map_dirfd); + return -1; + } + } + memset(buf, 0, ATTR_MAX); + while (fgets(buf, ATTR_MAX, fp)) { + if (!strncmp(buf, "virtfs.uid", 10)) { + uid = atoi(buf + 11); + } else if (!strncmp(buf, "virtfs.gid", 10)) { + gid = atoi(buf + 11); + } else if (!strncmp(buf, "virtfs.mode", 11)) { + mode = atoi(buf + 12); + } else if (!strncmp(buf, "virtfs.rdev", 11)) { + rdev = atoi(buf + 12); + } + memset(buf, 0, ATTR_MAX); + } + fclose(fp); + +update_map_file: + fp = local_fopenat(map_dirfd, name, "w"); + close_preserve_errno(map_dirfd); + if (!fp) { + return -1; + } + + if (credp->fc_uid != -1) { + uid = credp->fc_uid; + } + if (credp->fc_gid != -1) { + gid = credp->fc_gid; + } + if (credp->fc_mode != -1) { + mode = credp->fc_mode; + } + if (credp->fc_rdev != -1) { + rdev = credp->fc_rdev; + } + + if (uid != -1) { + fprintf(fp, "virtfs.uid=%d\n", uid); + } + if (gid != -1) { + fprintf(fp, "virtfs.gid=%d\n", gid); + } + if (mode != -1) { + fprintf(fp, "virtfs.mode=%d\n", mode); + } + if (rdev != -1) { + fprintf(fp, "virtfs.rdev=%d\n", rdev); + } + fclose(fp); + + return 0; +} + static int local_set_xattr(const char *path, FsCred *credp) { int err; @@ -362,6 +443,45 @@ static int local_set_xattr(const char *path, FsCred *credp) return 0; } +static int local_set_xattrat(int dirfd, const char *path, FsCred *credp) +{ + int err; + + if (credp->fc_uid != -1) { + uint32_t tmp_uid = cpu_to_le32(credp->fc_uid); + err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.uid", &tmp_uid, + sizeof(uid_t), 0); + if (err) { + return err; + } + } + if (credp->fc_gid != -1) { + uint32_t tmp_gid = cpu_to_le32(credp->fc_gid); + err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.gid", &tmp_gid, + sizeof(gid_t), 0); + if (err) { + return err; + } + } + if (credp->fc_mode != -1) { + uint32_t tmp_mode = cpu_to_le32(credp->fc_mode); + err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.mode", &tmp_mode, + sizeof(mode_t), 0); + if (err) { + return err; + } + } + if (credp->fc_rdev != -1) { + uint64_t tmp_rdev = cpu_to_le64(credp->fc_rdev); + err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.rdev", &tmp_rdev, + sizeof(dev_t), 0); + if (err) { + return err; + } + } + return 0; +} + static int local_post_create_passthrough(FsContext *fs_ctx, const char *path, FsCred *credp) { @@ -553,21 +673,38 @@ static ssize_t local_pwritev(FsContext *ctx, V9fsFidOpenState *fs, static int local_chmod(FsContext *fs_ctx, V9fsPath *fs_path, FsCred *credp) { - char *buffer; int ret = -1; - char *path = fs_path->data; + int dirfd; - if (fs_ctx->export_flags & V9FS_SM_MAPPED) { - buffer = rpath(fs_ctx, path); - ret = local_set_xattr(buffer, credp); - g_free(buffer); - } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) { - return local_set_mapped_file_attr(fs_ctx, path, credp); - } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) || - (fs_ctx->export_flags & V9FS_SM_NONE)) { - buffer = rpath(fs_ctx, path); - ret = chmod(buffer, credp->fc_mode); - g_free(buffer); + if (fs_ctx->export_flags & V9FS_SM_MAPPED || + fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) { + char *dirpath, *name; + + dirpath = g_path_get_dirname(fs_path->data); + dirfd = local_opendir_nofollow(fs_ctx, dirpath); + g_free(dirpath); + if (dirfd == -1) { + return -1; + } + + name = g_path_get_basename(fs_path->data); + if (fs_ctx->export_flags & V9FS_SM_MAPPED) { + ret = local_set_xattrat(dirfd, name, credp); + } else { + ret = local_set_mapped_file_attrat(dirfd, name, credp); + } + g_free(name); + close_preserve_errno(dirfd); + } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH || + fs_ctx->export_flags & V9FS_SM_NONE) { + int fd; + + fd = local_open_nofollow(fs_ctx, fs_path->data, O_RDONLY, 0); + if (fd == -1) { + return -1; + } + ret = fchmod(fd, credp->fc_mode); + close_preserve_errno(fd); } return ret; }
The local_chmod() callback is vulnerable to symlink attacks because it calls: (1) chmod() which follows symbolic links for all path elements (2) local_set_xattr()->setxattr() which follows symbolic links for all path elements (3) local_set_mapped_file_attr() which calls in turn local_fopen() and mkdir(), both functions following symbolic links for all path elements but the rightmost one This patch converts local_chmod() to rely on open_nofollow() and fchmod() to fix (1). It introduces a local_set_xattrat() replacement to local_set_xattr() based on fsetxattrat() to fix (2), and a local_set_mapped_file_attrat() replacement to local_set_mapped_file_attr() based on local_fopenat() and mkdirat() to fix (3). No effort is made to factor out code because both local_set_xattr() and local_set_mapped_file_attr() will be dropped when all users have been converted to use the "at" versions. This partly fixes CVE-2016-9602. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/9pfs/9p-local.c | 163 ++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 150 insertions(+), 13 deletions(-)