qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [for-2.10 PATCH v2] 9pfs: local: fix fchmodat_nofollow(


From: Eric Blake
Subject: Re: [Qemu-devel] [for-2.10 PATCH v2] 9pfs: local: fix fchmodat_nofollow() limitations
Date: Wed, 9 Aug 2017 10:01:14 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1

On 08/09/2017 09:55 AM, Eric Blake wrote:
> On 08/09/2017 09:23 AM, Greg Kurz wrote:
>> This function has to ensure it doesn't follow a symlink that could be used
>> to escape the virtfs directory. This could be easily achieved if fchmodat()
>> on linux honored the AT_SYMLINK_NOFOLLOW flag as described in POSIX, but
>> it doesn't.
> 
> Might be worth including a URL of the LKML discussion on the last
> version of that patch attempt.
> 
>>
>> The current implementation covers most use-cases, but it notably fails if:
>> - the target path has access rights equal to 0000 (openat() returns EPERM),
>>   => once you've done chmod(0000) on a file, you can never chmod() again
>> - the target path is UNIX domain socket (openat() returns ENXIO)
>>   => bind() of UNIX domain sockets fails if the file is on 9pfs
>>
>> The solution is to use O_PATH: openat() now succeeds in both cases, and we
>> can ensure the path isn't a symlink with fstat(). The associated entry in
>> "/proc/self/fd" can hence be safely passed to the regular chmod() syscall.
> 
> Hey - should we point this out as a viable solution to the glibc folks,
> since their current user-space emulation of AT_SYMLINK_NOFOLLOW is broken?
> 


> Nope, unsafe when O_PATH_9P_UTIL is 0.  This needs to be more like:
> 
> /* Now we handle racing symlinks.  On kernels without O_PATH, we will
>  * fail on some corner cases, but that's better than dereferencing a
>  * symlink that was injected during the TOCTTOU between our initial
>  * fstatat() and openat_file().
>  */
> if (O_PATH_9P_UTIL) {
>     fstat, S_ISLINK, proc_path, chmod()
> } else {
>     fchmod()
> }

For that matter, I think you also want to avoid the O_WRONLY fallback
when O_PATH works, as in:

>> -    fd = openat_file(dirfd, name, O_RDONLY, 0);
>> +    fd = openat_file(dirfd, name, O_RDONLY | O_PATH_9P_UTIL, 0);
>>      if (fd == -1) {

changing this to 'if (fd == -1 && !O_PATH_9P_UTIL)'

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]