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 <gr...@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
signature.asc
Description: PGP signature
