On Mon, Aug 05, 2024 at 09:21:35PM -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 > 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 whish 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. > > 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. > > 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 may now take > slightly longer to execute, but the extra time is independent of > client response behaviors, and is generally no worse than the time > already taken by the blk_exp_close_all_type() that disconnects all > clients that completed handshakes (since that code also has an > AIO_WAIT_WHILE_UNLOCKED). For a long-running server with lots of > clients rapidly connecting and disconnecting, the memory used to track > all client sockets can result in some memory overhead, but it is not a > leak; the next patch will further optimize that by cleaning up memory > as clients go away. At any rate, this patch in isolation is > sufficient to fix 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 in that patch to start 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.iva...@virtuozzo.com> > Fixes: CVE-2024-7409 > Signed-off-by: Eric Blake <ebl...@redhat.com> > --- > blockdev-nbd.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+)
Reviewed-by: Daniel P. Berrangé <berra...@redhat.com> With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
