Re: [PATCH v3 1/2] nbd: CVE-2024-7409: Close stray client sockets at server shutdown

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

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

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