Re: [PATCH v2 2/5] 9pfs: fix qemu_mknodat(S_IFSOCK) on macOS

[Greg Kurz <groug@kaod.org>]

On Wed, 27 Apr 2022 11:27:28 +0900
Akihiko Odaki <akihiko.odaki@gmail.com> wrote:

> On 2022/04/26 21:38, Greg Kurz wrote:

[..skip..]

> > 
> > I think Christian's explanation is clear enough. We don't guarantee
> > that v9fs_co_foo() calls run atomically. As a consequence, the client
> > might see transient states or be able to interact with an ongoing
> > request. And to answer your question, we have no specific rationale
> > on security with that.
> > 
> > I'm not sure what the concerns are but unless you come up with a
> > valid scenario [*] I don't see any reason to prevent this patch
> > to go forward.
> > 
> > [*] things like:
> >      - client escaping the shared directory
> >      - QEMU crashing
> >      - QEMU hogging host resources
> >      - client-side unprivileged user gaining elevated privleges
> >        in the guest
> 
> I was just not sure if such transient states are safe. The past 
> discussion was about the length of the non-atomic time window where a 
> path name is used to identify a particular file, but if such states are 
> not considered problematic, the length does not matter all and we can 
> confidently say the sequence of bind() and chmod() is safe.
> 
> Considering the transient states are tolerated in 9pfs, we need to 
> design this function to be tolerant with transient states as well. The 
> use of chmod() is not safe when we consider about transient states. A 
> malicious actor may replace the file at the path with a symlink which 
> may escape the shared directory and chmod() will naively follow it. 

You get a point here. Thanks for your tenacity ! :-)

> chmod() should be replaced with fchmodat_nofollow() or something similar.
> 

On a GNU/Linux system, this could be achieved by calling fchmod() on
the socket fd *before* calling bind() but I'm afraid this hack might
not work with a BSDish OS.

Replacing chmod() with fchmodat_nofollow(dirfd, addr.sun_path, mode)
won't make things atomic as above but at least it won't follow a
malicious symbolic link : mknod() on the client will fail with
ELOOP, which is fine when it comes to not breaking out of the shared
directory.

This brings up a new problem I hadn't realized before : the
fchmodat_nofollow() implementation in 9p-local.c is really
a linux only thing to cope with AT_SYMLINK_NOFOLLOW not being
supported with fchmodat(). It looks that this should move to
9p-util-linux.c and a proper version should be added for macOS
in 9p-util-darwin.c

Cheers,

--
Greg

> Regards,
> Akihiko Odaki
> 
> > 
> > Cheers,
> > 
> > --
> > Greg
> > 
> >> Regards,
> >> Akihiko Odaki
> > 
>