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

[Greg Kurz]

On Wed, 27 Apr 2022 14:32:53 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Mittwoch, 27. April 2022 12:18:10 CEST Greg Kurz wrote:
> > 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 ! :-)
> 
> Yep, I send a v4 with fchmodat_nofollow() instead of chmod(), thanks!
> 
> BTW, why is it actually allowed for client to create a symlink pointing 
> outside exported directory tree with security_model=passthrough/none? Did 
> anybody want that?
> 

The target argument to symlink() is merely a string that is stored in
the inode. It is only evaluated as a path at the time an action is
made on the link. Checking at symlink() time is thus useless.

Anyway, we're safe on this side since it's the client's job to
resolve links and we explicitly don't follow them in the server.

> > > 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.
> 
> As you already imagined, this is unfortunately not supported by any BSDs, 
> including macOS. I'll file a bug report with Apple though.
> 

I'm not sure if this is documented and supported behavior on linux either.

> > 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.
> 
> Current security_model=passthrough/none already has similar non-atomic 
> operations BTW, so this was not something new. E.g.:
> 
> static int local_symlink(FsContext *fs_ctx, const char *oldpath,
>                          V9fsPath *dir_path, const char *name, FsCred *credp)
> {
>     ...
>     } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
>                fs_ctx->export_flags & V9FS_SM_NONE) {
>         err = symlinkat(oldpath, dirfd, name);
>         if (err) {
>             goto out;
>         }
>         err = fchownat(dirfd, name, credp->fc_uid, credp->fc_gid,
>                        AT_SYMLINK_NOFOLLOW);
>     ...
> }
> 

Yes similar window but this is secure since fchownat() supports
AT_SYMLINK_NOFOLLOW.

> In general, if you care about a higher degree of security, I'd always 
> recommend to use security_model=mapped in the first place.
> 
> > 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
> 
> Like already agreed on the other thread, yes, that makes sense. But I think 
> this can be handled with a follow-up, separate from this series.
> 

Fair enough if you want to handle fchmodat_nofollow() later but you
must at least use fchmodat(AT_SYMLINK_NOFOLLOW) in this patch
instead of chmod().

> Best regards,
> Christian Schoenebeck
> 
>