[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: |
Jann Horn |
Subject: |
Re: [Qemu-devel] [PATCH 23/29] 9pfs: local: chmod: don't follow symlinks |
Date: |
Fri, 24 Feb 2017 17:22:19 +0100 |
On Fri, Feb 24, 2017 at 4:23 PM, Eric Blake <address@hidden> wrote:
> On 02/24/2017 04:34 AM, Greg Kurz wrote:
>> 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
>>>>
>
>>>> + 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. :(
- [Qemu-devel] [PATCH 20/29] 9pfs: local: rename: use renameat, (continued)
[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
[Qemu-devel] [PATCH 28/29] 9pfs: local: open2: don't follow symlinks, Greg Kurz, 2017/02/20