Re: [Qemu-devel] [PATCH 23/29] 9pfs: local: chmod: don't follow symlinks

[Jann Horn <jannh@google.com>]

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. :(