On Wed, 27 Apr 2022 11:27:28 +0900 Akihiko Odaki <akihiko.od...@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 > > >
