9pfs: local: symlink: don't follow symlinks

The local_symlink() callback is vulnerable to symlink attacks because it
calls:

(1) symlink() which follows symbolic links for all path elements but the
    rightmost one
(2) open(O_NOFOLLOW) which follows symbolic links for all path elements but
    the rightmost one
(3) local_set_xattr()->setxattr() which follows symbolic links for all
    path elements
(4) 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_symlink() to rely on opendir_nofollow() and
symlinkat() to fix (1), openat(O_NOFOLLOW) to fix (2), as well as
local_set_xattrat() and local_set_mapped_file_attrat() to fix (3) and
(4) respectively.

This partly fixes CVE-2016-9602.

Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

hw/9pfs/9p-local.c

@@ -978,102 +978,71 @@ static int local_symlink(FsContext *fs_ctx, const char *oldpath,
                          V9fsPath *dir_path, const char *name, FsCred *credp)
 {
     int err = -1;
-    int serrno = 0;
+    int dirfd;
-    char *newpath;
-    V9fsString fullname;
-    char *buffer = NULL;
 
-    v9fs_string_init(&fullname);
+    dirfd = local_opendir_nofollow(fs_ctx, dir_path->data);
-    v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
+    if (dirfd == -1) {
-    newpath = fullname.data;
+        return -1;
+    }
 
     /* Determine the security model */
+    if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
+        fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
+        int fd;
+        ssize_t oldpath_size, write_size;
+
+        fd = openat_file(dirfd, name, O_CREAT | O_EXCL | O_RDWR,
+                         SM_LOCAL_MODE_BITS);
+        if (fd == -1) {
+            goto out;
+        }
+        /* Write the oldpath (target) to the file. */
+        oldpath_size = strlen(oldpath);
+        do {
+            write_size = write(fd, (void *)oldpath, oldpath_size);
+        } while (write_size == -1 && errno == EINTR);
+        close_preserve_errno(fd);
+
+        if (write_size != oldpath_size) {
+            goto err_end;
+        }
+        /* Set cleint credentials in symlink's xattr */
+        credp->fc_mode = credp->fc_mode | S_IFLNK;
+
         if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
-        int fd;
+            err = local_set_xattrat(dirfd, name, credp);
-        ssize_t oldpath_size, write_size;
+        } else {
-        buffer = rpath(fs_ctx, newpath);
+            err = local_set_mapped_file_attrat(dirfd, name, credp);
-        fd = open(buffer, O_CREAT|O_EXCL|O_RDWR|O_NOFOLLOW, SM_LOCAL_MODE_BITS);
-        if (fd == -1) {
-            err = fd;
-            goto out;
         }
-        /* Write the oldpath (target) to the file. */
-        oldpath_size = strlen(oldpath);
-        do {
-            write_size = write(fd, (void *)oldpath, oldpath_size);
-        } while (write_size == -1 && errno == EINTR);
-
-        if (write_size != oldpath_size) {
-            serrno = errno;
-            close(fd);
-            err = -1;
-            goto err_end;
-        }
-        close(fd);
-        /* Set cleint credentials in symlink's xattr */
-        credp->fc_mode = credp->fc_mode|S_IFLNK;
-        err = local_set_xattr(buffer, credp);
         if (err == -1) {
-            serrno = errno;
             goto err_end;
         }
-    } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
+    } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
-        int fd;
+               fs_ctx->export_flags & V9FS_SM_NONE) {
-        ssize_t oldpath_size, write_size;
+        err = symlinkat(oldpath, dirfd, name);
-        buffer = rpath(fs_ctx, newpath);
-        fd = open(buffer, O_CREAT|O_EXCL|O_RDWR|O_NOFOLLOW, SM_LOCAL_MODE_BITS);
-        if (fd == -1) {
-            err = fd;
-            goto out;
-        }
-        /* Write the oldpath (target) to the file. */
-        oldpath_size = strlen(oldpath);
-        do {
-            write_size = write(fd, (void *)oldpath, oldpath_size);
-        } while (write_size == -1 && errno == EINTR);
-
-        if (write_size != oldpath_size) {
-            serrno = errno;
-            close(fd);
-            err = -1;
-            goto err_end;
-        }
-        close(fd);
-        /* Set cleint credentials in symlink's xattr */
-        credp->fc_mode = credp->fc_mode|S_IFLNK;
-        err = local_set_mapped_file_attr(fs_ctx, newpath, credp);
-        if (err == -1) {
-            serrno = errno;
-            goto err_end;
-        }
-    } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) ||
-               (fs_ctx->export_flags & V9FS_SM_NONE)) {
-        buffer = rpath(fs_ctx, newpath);
-        err = symlink(oldpath, buffer);
         if (err) {
             goto out;
         }
-        err = lchown(buffer, credp->fc_uid, credp->fc_gid);
+        err = fchownat(dirfd, name, credp->fc_uid, credp->fc_gid,
+                       AT_SYMLINK_NOFOLLOW);
         if (err == -1) {
             /*
              * If we fail to change ownership and if we are
              * using security model none. Ignore the error
              */
             if ((fs_ctx->export_flags & V9FS_SEC_MASK) != V9FS_SM_NONE) {
-                serrno = errno;
                 goto err_end;
-            } else
+            } else {
                 err = 0;
             }
         }
+    }
     goto out;
 
 err_end:
-    remove(buffer);
+    unlinkat_preserve_errno(dirfd, name, 0);
-    errno = serrno;
 out:
-    g_free(buffer);
+    close_preserve_errno(dirfd);
-    v9fs_string_free(&fullname);
     return err;
 }