RE: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for Windows

[Shi, Guohuai]

> -----Original Message-----
> From: Greg Kurz <groug@kaod.org>
> Sent: 2022年5月11日 20:19
> To: Shi, Guohuai <Guohuai.Shi@windriver.com>
> Cc: Christian Schoenebeck <qemu_oss@crudebyte.com>; qemu-devel@nongnu.org; 
> Meng,
> Bin <Bin.Meng@windriver.com>; Bin Meng <bmeng.cn@gmail.com>
> Subject: Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend driver for
> Windows
> 
> [Please note: This e-mail is from an EXTERNAL e-mail address]
> 
> On Tue, 10 May 2022 15:35:10 +0000
> "Shi, Guohuai" <Guohuai.Shi@windriver.com> wrote:
> 
> > Let's force on the security issue:
> >
> 
> Please don't top post, especially when all previous comments were made inline,
> so that someone who steps in this thread now has a chance to catch-up.
> 
> > Firstly, this answer
> ( https://stackoverflow.com/questions/32138524/is-there-a-windows-equivalent-
> of-openat ) is useless for QEMU.
> > It uses Windows native API NtCreateFile() and accesses files by Windows 
> > handle.
> > But 9PFS is using Windows POSIX interface, handle can not be used in POSIX
> interface.
> > Actually, Windows provide similar APIs like
> GetFinalPathNameByHandle()/GetFileInformationByHandle().
> > It can also get file information by Windows handle.
> >
> > Windows POSIX interface do not support NO_FOLLOW flags, that means, Windows 
> > POSIX
> open() always translate symbolic link.
> >
> 
> This precludes any tentative to fix the issue at the QEMU level then.
> Maybe there are some knobs to control symlink behavior at the fs level but 
> this
> certainly requires windows knowledge that I don't have.
> 
> > So everything are finally point to one limitation: Windows POSIX interfaces 
> > do
> not support symbolic link and always translate link.
> >
> > For the security reason, I think it is reasonable to disable symbolic link 
> > support
> on Windows host for 9PFS.
> > I can re-work this patch to adding a symbolic link check during path-walk 
> > operation
> and stop it when get a symbolic link.
> >
> 
> This would be useless because of TOCTOU : a directory could be replaced by a 
> symlink
> between the check and the actual use of the file. O_NOFOLLOW provides the 
> atomicity
> needed to safely error out on symlinks. Since O_NOFOLLOW only makes sense for 
> the
> rightmost path element, paths from the client have to be broken down into a
> succession of *at() syscalls, one for each element.

For Windows file system, it would be OK.
Windows can not delete a opening file (this is different behavior between 
Windows file system driver and UNIX-like-inode-based file system).
So when 9PFS try to open the final file, the following steps will keep it safe:

1. open the final file by Windows NT APIs and keep the open handle.
2. open the final file by MinGW open().
3. close NT handle.

Windows file system does not allow delete/rename/move a opening file.
Even Windows provide "FILE_SHARE_DELETE" flag in its NT API CreateFile().
Windows allow to delete the opening file, but can not re-create same name.
The following steps will be failure on Windows:

1. Open a directory by CreateFile() with "FILE_SHARE_DELETE" flag and keep the 
handle open.
2. Remove the directory.
3. Re-create same name directory/file/links.

Windows will get failure on step #3.

So I think checking if there is a link in filename would be safety on Window 
host.

Best Regards,
Guohuai

> 
> > Best Regards,
> > Guohuai
> >
> > > -----Original Message-----
> > > From: Greg Kurz <groug@kaod.org>
> > > Sent: 2022年5月10日 22:35
> > > To: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > Cc: qemu-devel@nongnu.org; Meng, Bin <Bin.Meng@windriver.com>; Bin
> > > Meng <bmeng.cn@gmail.com>; Shi, Guohuai <Guohuai.Shi@windriver.com>
> > > Subject: Re: [PATCH 5/9] hw/9pfs: Add a 'local' file system backend
> > > driver for Windows
> > >
> > > [Please note: This e-mail is from an EXTERNAL e-mail address]
> > >
> > > On Tue, 10 May 2022 16:04:28 +0200
> > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > >
> > > > On Dienstag, 10. Mai 2022 15:40:06 CEST Greg Kurz wrote:
> > > > > On Tue, 10 May 2022 13:54:46 +0200
> > > > >
> > > > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > > > > On Dienstag, 10. Mai 2022 12:18:33 CEST Christian Schoenebeck wrote:
> > > > > > > On Dienstag, 10. Mai 2022 04:17:44 CEST Shi, Guohuai wrote:
> > > > > > > [...]
> > > > > > >
> > > > > > > > > > > > I tend to agree with Christian's remarks that this
> > > > > > > > > > > > patch is too big and that the choice of
> > > > > > > > > > > > introducing right away a new implementation of
> > > > > > > > > > > > 9p-local for windows hosts is too bold to start
> > > > > > > > > > > > with. We need to clearly understand what's
> > > > > > > > > > > > diverging between windows and linux in order to
> > > > > > > > > > > > make such a decision. You should first try to
> > > > > > > > > > > > introduce the required abstractions to cope with these 
> > > > > > > > > > > > differences,
> so that we can review.
> > > > > > > > > > >
> > > > > > > > > > > Here is the basic introductions of 9PFS for Windows 
> > > > > > > > > > > development:
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Windows always returns -1 when try to call open()
> > > > > > > > > > > for a directory.
> > > > > > > > > > > Windows (actually MinGW library) only allows
> > > > > > > > > > > opendir() for a directory.
> > > > > > >
> > > > > > > That missing behaviour could be implemented in
> > > > > > > 9p-util-win.c, similar to the missing behaviours of
> > > > > > > mknodat() for macOS which did not support a bunch of things
> > > > > > > like creating a UNIX socket file and
> > > more:
> > > > > > >
> > > > > > > https://github.com/qemu/qemu/commit/055ab89327bab83f1bd07e9d
> > > > > > > e07f
> > > > > > > 7628643d
> > > > > > > 3d8d> >
> > > > > > > > > > Does MinGW have dirfd() ?
> > > > > > > > >
> > > > > > > > > No.
> > > > > > > > > MinGW does not open any directory.
> > > > > > > > > Here is opendir() source code of MinGW:
> > > > > > > > > https://github.com/mirror/mingw-w64/blob/master/mingw-w6
> > > > > > > > > 4-cr
> > > > > > > > > t/misc/d
> > > > > > > > > iren
> > > > > > > > > t.
> > > > > > > > > c#L42
> > > > > > > > >
> > > > > > > > > So MinGW do not have a fd associated to a directory.
> > > > > > > > >
> > > > > > > > > > > Windows does not support APIs like "*at" (openat(),
> > > > > > > > > > > renameat(),
> > > > > > > > > > > etc.)
> > > > > > >
> > > > > > > Like already suggested before on your previous RFC version,
> > > > > > > it is possible to use the same workaround as we are using
> > > > > > > for macOS hosts already (which
> > > > > > >
> > > > > > > was missing mknodat()):
> > > > > > >   pthread_fchdir_np(...)
> > > > > > >   mknod(...)
> > > > > > >
> > > > > > >
> > > > > > > https://github.com/qemu/qemu/blob/master/hw/9pfs/9p-util-darwin.
> > > > > > > c#L84
> > > > > > >
> > > > > > > So on Windows it would be viable to:
> > > > > > >   chdir(...)
> > > > > > >   open(...)
> > > > > > >
> > > > > > > The same approach could be used for any missing *at()
> > > > > > > function for Windows.
> > > > > >
> > > > > > Problem though is that the chdir() functions on Windows all
> > > > > > seem to have process-wide effect, we would need to change the
> > > > > > current directory only for the current thread, because
> > > > > > filesystem access of 9p server is multi-threaded.
> > > > > >
> > > > > > Protecting the chdir(); foo(); calls by a process wide global
> > > > > > mutex isn't very appealing either. :/
> > > > >
> > > > > And it wouldn't be safe anyway because I'm pretty sure that the
> > > > > rest of the QEMU code assumes that the current directory is invariant,
> e.g.
> > > > > user could be very confused by 'drive_add file=./foo.img' not working.
> > > > >
> > > > > BTW duckduckgo gives:
> > > > >
> > > > > https://stackoverflow.com/questions/32138524/is-there-a-windows-
> > > > > equi
> > > > > valent-o
> > > > > f-openat
> > > > >
> > > > > So yes it seems to be technically possible to implement *at()
> > > > > functions on windows. This is the only way to avoid
> > > > > CVE-2016-9602 in the QEMU process.
> > > >
> > > > +1
> > > >
> > > > > Another option is to use the proxy backend : this offloads all
> > > > > fs visit accesses to an external process running
> > > > > virtfs-proxy-helper, that runs privileged and chroot() into the
> > > > > shared directory so that it can safely use path based syscalls.
> > > >
> > > > As a very last resort, maybe. But just for the other two guys to know 
> > > > upfront:
> > > > the proxy backend is very slow and not in good shape. There were
> > > > plans to deprecate the proxy backend therefore, as it's more or less 
> > > > dead.
> > > >
> > >
> > > Yeah as mentioned before, the way to go now would be to come with a
> > > vhost-user implementation like virtiofsd. This would address all
> > > perf problems we have with proxy since the client would directly
> > > talk to the external process. This should also provide better perf
> > > than the local backend since it wouldn't have to do do the "at*()"
> > > dance thanks to chroot().
> > >
> > > > > > > > > > Ouch...
> > > > > > > > > >
> > > > > > > > > > > So 9PFS can not use any openat() for opening a sub
> > > > > > > > > > > file or directory in 9P
> > > > > > > > >
> > > > > > > > > mount
> > > > > > > > >
> > > > > > > > > > directory.
> > > > > > > > > >
> > > > > > > > > > > This commit use merge_fs_path() to build up full
> > > > > > > > > > > visit filename by string
> > > > > > > > >
> > > > > > > > > concatenation.
> > > > > > > > >
> > > > > > > > > > > I know that may have a risk of security, but Windows
> > > > > > > > > > > does fully support POSIX
> > > > > > >
> > > > > > > You will not find anybody merging code that's inherently insecure.
> > > > > > >
> > > > > > > > > > I understand from your various answers that symlinks
> > > > > > > > > > aren't currently supported by window's POSIX API. Is this 
> > > > > > > > > > forever ?
> > > > > > > > > > Google do mentions symlinks in windows 10. What's the
> > > > > > > > > > story there ? How do they behave ? How would they be
> > > > > > > > > > exposed to the client ? Be aware that, even if the
> > > > > > > > > > client cannot create symlinks, an existing symlink
> > > > > > > > > > could be used to escape
> > > with rename().
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > If the code "may have a risk of security" then it must
> > > > > > > > > > be fixed or avoided in some way before being merged 
> > > > > > > > > > upstream.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Other thing that comes to mind is that windows hosts
> > > > > > > > > > should maybe use the mapped or mapped-file security
> > > > > > > > > > modes visit visit since they emulate symlinks with a
> > > > > > > > > > simple file hidden in the VIRTFS_META_DIR directory.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Cheers,
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > --
> > > > > > > > > > Greg
> > > > > > > > >
> > > > > > > > > Windows native API support symbolic link file start from
> > > > > > > > > Windows
> > > > > > > > > Vista:
> > > > > > > > > https://docs.microsoft.com/en-us/windows/win32/api/winba
> > > > > > > > > se/n
> > > > > > > > > f-winbas
> > > > > > > > > e-cr
> > > > > > > > > ea
> > > > > > > > > tes ymboliclinka
> > > > > > > > >
> > > > > > > > > I mean Windows POSIX APIs do not support symbolic link
> > > > > > > > > (MinGW use
> > > > > > > > > Win32
> > > > > > > > > POSIX APIs) So we can not create symbolic link by MinGW.
> > > > > > >
> > > > > > > A function with POSIX signature could be added to
> > > > > > > 9p-util-win.c which would call the native Windows function to 
> > > > > > > create
> symlinks.
> > > > > > >
> > > > > > > > > Anyway, there is another solution: re-work whole 9PFS code:
> > > > > > > > > not only 9p-local.c, but also every file in 9p driver.
> > > > > > > > > Replace every MinGW/POSIX APIs (e.g. open, lseek, read,
> > > > > > > > > write, close), by Windows Native APIs (e.g. open ->
> > > > > > > > > CreateFile, lseek -> SetFilePointer, read -> ReadFile,
> > > > > > > > > write
> > > > > > > > > -> WriteFile, close -> CloseHandle, etc.) Then 9P can
> > > > > > > > > -> use
> > > > > > > > > Windows symbolic link feature.
> > > > > > > > > However, I do think it is a good idea to replace everything.
> > > > > > > >
> > > > > > > > TYPO: it NOT is a good idea to replace everything.
> > > > > > >
> > > > > > > Right, that does not make sense. The way to go is adding and
> > > > > > > implementing missing system functions with POSIX signatures
> > > > > > > and POSIX behaviour for Windows. Not turning the entire code
> > > > > > > base upside down.
> > > > > > >
> > > > > > > Best regards,
> > > > > > > Christian Schoenebeck
> > > >
> > > >
> >