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

["Daniel P. Berrangé" <berrange@redhat.com>]

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.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>


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 :|