[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 23/29] 9pfs: local: chmod: don't follow symlinks
From: |
Greg Kurz |
Subject: |
Re: [Qemu-devel] [PATCH 23/29] 9pfs: local: chmod: don't follow symlinks |
Date: |
Fri, 24 Feb 2017 11:34:02 +0100 |
On Thu, 23 Feb 2017 15:10:42 +0000
Stefan Hajnoczi <address@hidden> 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 <address@hidden>
> > ---
> > 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
pgpvle3NtFGdo.pgp
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH 19/29] 9pfs: local: renameat: don't follow symlinks, (continued)
- [Qemu-devel] [PATCH 19/29] 9pfs: local: renameat: don't follow symlinks, Greg Kurz, 2017/02/20
- [Qemu-devel] [PATCH 20/29] 9pfs: local: rename: use renameat, Greg Kurz, 2017/02/20
- [Qemu-devel] [PATCH 21/29] 9pfs: local: improve error handling in link op, Greg Kurz, 2017/02/20
- [Qemu-devel] [PATCH 22/29] 9pfs: local: link: don't follow symlinks, Greg Kurz, 2017/02/20
- [Qemu-devel] [PATCH 23/29] 9pfs: local: chmod: don't follow symlinks, Greg Kurz, 2017/02/20
[Qemu-devel] [PATCH 24/29] 9pfs: local: chown: don't follow symlinks, Greg Kurz, 2017/02/20
[Qemu-devel] [PATCH 25/29] 9pfs: local: symlink: don't follow symlinks, Greg Kurz, 2017/02/20
[Qemu-devel] [PATCH 26/29] 9pfs: local: mknod: don't follow symlinks, Greg Kurz, 2017/02/20
[Qemu-devel] [PATCH 27/29] 9pfs: local: mkdir: don't follow symlinks, Greg Kurz, 2017/02/20