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.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> Thanks for a speedy review. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
