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

[Akihiko Odaki]

On 2022/04/22 23:06, Christian Schoenebeck wrote:
> On Freitag, 22. April 2022 04:43:40 CEST Akihiko Odaki wrote:
> > On 2022/04/22 0:07, Christian Schoenebeck wrote:
> > > mknod() on macOS does not support creating sockets, so divert to
> > > call sequence socket(), bind() and chmod() respectively if S_IFSOCK
> > > was passed with mode argument.
> > >
> > > Link: https://lore.kernel.org/qemu-devel/17933734.zYzKuhC07K@silver/
> > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > Reviewed-by: Will Cohen <wwcohen@gmail.com>
> > > ---
> > >
> > >    hw/9pfs/9p-util-darwin.c | 27 ++++++++++++++++++++++++++-
> > >    1 file changed, 26 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
> > > index e24d09763a..39308f2a45 100644
> > > --- a/hw/9pfs/9p-util-darwin.c
> > > +++ b/hw/9pfs/9p-util-darwin.c
> > > @@ -74,6 +74,27 @@ int fsetxattrat_nofollow(int dirfd, const char
> > > *filename, const char *name,>
> > >     */
> > >    
> > >    #if defined CONFIG_PTHREAD_FCHDIR_NP
> > >
> > > +static int create_socket_file_at_cwd(const char *filename, mode_t mode) {
> > > +    int fd, err;
> > > +    struct sockaddr_un addr = {
> > > +        .sun_family = AF_UNIX
> > > +    };
> > > +
> > > +    fd = socket(PF_UNIX, SOCK_DGRAM, 0);
> > > +    if (fd == -1) {
> > > +        return fd;
> > > +    }
> > > +    snprintf(addr.sun_path, sizeof(addr.sun_path), "./%s", filename);
> >
> > It would result in an incorrect path if the path does not fit in
> > addr.sun_path. It should report an explicit error instead.
>
> Looking at its header file, 'sun_path' is indeed defined on macOS with an
> oddly small size of only 104 bytes. So yes, I should explicitly handle that
> error case.
>
> I'll post a v3.
>
> > > +    err = bind(fd, (struct sockaddr *) &addr, sizeof(addr));
> > > +    if (err == -1) {
> > > +        goto out;
> >
> > You may close(fd) as soon as bind() returns (before checking the
> > returned value) and eliminate goto.
>
> Yeah, I thought about that alternative, but found it a bit ugly, and probably
> also counter-productive in case this function might get extended with more
> error pathes in future. Not that I would insist on the current solution
> though.

I'm happy with the explanation. Thanks.

> > > +    }
> > > +    err = chmod(addr.sun_path, mode);
> >
> > I'm not sure if it is fine to have a time window between bind() and
> > chmod(). Do you have some rationale?
>
> Good question. QEMU's 9p server is multi-threaded; all 9p requests come in
> serialized and the 9p server controller portion (9p.c) is only running on QEMU
> main thread, but the actual filesystem driver calls are then dispatched to
> QEMU worker threads and therefore running concurrently at this point:
>
> https://wiki.qemu.org/Documentation/9p#Threads_and_Coroutines
>
> Similar situation on Linux 9p client side: it handles access to a mounted 9p
> filesystem concurrently, requests are then serialized by 9p driver on Linux
> and sent over wire to 9p server (host).
>
> So yes, there might be implications by that short time windows. But could that
> be exploited on macOS hosts in practice?
>
> The socket file would have mode srwxr-xr-x for a short moment.
>
> For security_model=mapped* this should not be a problem.
>
> For security_model=none|passhrough, in theory, maybe? But how likely is that?
> If you are using a Linux client for instance, trying to brute-force opening
> the socket file, the client would send several 9p commands (Twalk, Tgetattr,
> Topen, probably more). The time window of the two commands above should be
> much smaller than that and I would expect one of the 9p commands to error out
> in between.
>
> What would be a viable approach to avoid this issue on macOS?

It is unlikely that a naive brute-force approach will succeed to 
exploit. The more concerning scenario is that the attacker uses the 
knowledge of the underlying implementation of macOS to cause resource 
contention to widen the window. Whether an exploitation is viable 
depends on how much time you spend digging XNU.

However, I'm also not sure if it really *has* a race condition. Looking 
at v9fs_co_mknod(), it sequentially calls s->ops->mknod() and 
s->ops->lstat(). It also results in an entity called "path name based 
fid" in the code, which inherently cannot identify a file when it is 
renamed or recreated.

If there is some rationale it is safe, it may also be applied to the 
sequence of bind() and chmod(). Can anyone explain the sequence of 
s->ops->mknod() and s->ops->lstat() or path name based fid in general?

Regards,
Akihiko Odaki

> > > +out:
> > > +    close(fd);
> > > +    return err;
> > > +}
> > > +
> > >
> > >    int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t
> > >    dev)
> > >    {
> > >    
> > >        int preserved_errno, err;
> > >
> > > @@ -93,7 +114,11 @@ int qemu_mknodat(int dirfd, const char *filename,
> > > mode_t mode, dev_t dev)>
> > >        if (pthread_fchdir_np(dirfd) < 0) {
> > >        
> > >            return -1;
> > >        
> > >        }
> > >
> > > -    err = mknod(filename, mode, dev);
> > > +    if (S_ISSOCK(mode)) {
> > > +        err = create_socket_file_at_cwd(filename, mode);
> > > +    } else {
> > > +        err = mknod(filename, mode, dev);
> > > +    }
> > >
> > >        preserved_errno = errno;
> > >        /* Stop using the thread-local cwd */
> > >        pthread_fchdir_np(-1);