Re: [PATCH v4 5/7] nbd/server: CVE-2024-7409: Close stray client sockets at shutdown

[Eric Blake]

On Wed, Aug 07, 2024 at 07:29:25PM GMT, Daniel P. Berrangé wrote:
> On Wed, Aug 07, 2024 at 12:43:31PM -0500, Eric Blake wrote:
> > A malicious client can attempt to connect to an NBD server, and then
> > intentionally delay progress in the handshake, including if it does
> > not know the TLS secrets.  Although this behavior can be bounded by
> > the max-connections parameter, the QMP nbd-server-start currently
> > defaults to unlimited incoming client connections.  Worse, if the

I need to touch that sentence up to match the earlier patches.  (The
curse of rebasing late at night...)

> > client waits to close the socket until after the QMP nbd-server-stop
> > command is executed, qemu will then SEGV when trying to dereference
> > the NULL nbd_server global which is no longer present, which amounts
> > to a denial of service attack.  If another NBD server is started
> > before the malicious client disconnects, I cannot rule out additional
> > adverse effects when the old client interferes with the connection
> > count of the new server.

I _think_ the effect is most likely to be an assertion failure
(nbd_server->connections > 0), since we recommend compiling qemu with
assertions enabled.  But "most likely" is not the same as "certainty".

> > 
> > For environments without this patch, the CVE can be mitigated by
> > ensuring (such as via a firewall) that only trusted clients can
> > connect to an NBD server.  Note that using frameworks like libvirt
> > that ensure that TLS is used and that nbd-server-stop is not executed
> > while any trusted clients are still connected will only help if there
> > is also no possibility for an untrusted client to open a connection
> > but then stall on the NBD handshake.
> > 
> > Given the previous patches, it would be possible to guarantee that no
> > clients remain connected by having nbd-server-stop sleep for longer
> > than the default handshake deadline before finally freeing the global
> > nbd_server object, but that could make QMP non-responsive for a long
> > time.  So intead, this patch fixes the problem by tracking all client
> > sockets opened while the server is running, and forcefully closing any
> > such sockets remaining without a completed handshake at the time of
> > nbd-server-stop, then waiting until the coroutines servicing those
> > sockets notice the state change.  nbd-server-stop now has a second
> > AIO_WAIT_WHILE_UNLOCKED (the first is indirectly through the
> > blk_exp_close_all_type() that disconnects all clients that completed
> > handshakes), but forced socket shutdown is enough to progress the
> > coroutines and quickly tear down all clients before the server is
> > freed, thus finally fixing the CVE.
> > 
> > This patch relies heavily on the fact that nbd/server.c guarantees
> > that it only calls nbd_blockdev_client_closed() from the main loop
> > (see the assertion in nbd_client_put() and the hoops used in
> > nbd_client_put_nonzero() to achieve that); if we did not have that
> > guarantee, we would also need a mutex protecting our accesses of the
> > list of connections to survive re-entrancy from independent iothreads.
> > 
> > Although I did not actually try to test old builds, it looks like this
> > problem has existed since at least commit 862172f45c (v2.12.0, 2017) -
> > even back when that patch started using a QIONetListener to handle
> > listening on multiple sockets, nbd_server_free() was already unaware
> > that the nbd_blockdev_client_closed callback can be reached later by a
> > client thread that has not completed handshakes (and therefore the
> > client's socket never got added to the list closed in
> > nbd_export_close_all), despite that patch intentionally tearing down
> > the QIONetListener to prevent new clients.
> > 
> > Reported-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> > Fixes: CVE-2024-7409
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> >  blockdev-nbd.c | 35 ++++++++++++++++++++++++++++++++++-
> >  1 file changed, 34 insertions(+), 1 deletion(-)
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Thanks for a speedy review.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org