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 > 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. > > 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.iva...@virtuozzo.com> > Fixes: CVE-2024-7409 > Signed-off-by: Eric Blake <ebl...@redhat.com> > --- > blockdev-nbd.c | 35 ++++++++++++++++++++++++++++++++++- > 1 file changed, 34 insertions(+), 1 deletion(-)
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 :|
