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

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 2184 bytes --]

On Wed, 9 Aug 2017 10:01:14 -0500
Eric Blake <eblake@redhat.com> wrote:

> 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)'
> 

Yes, will do.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]