[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Stable-8.2.7 40/53] nbd/server: CVE-2024-7409: Close stray clients at s
From: |
Michael Tokarev |
Subject: |
[Stable-8.2.7 40/53] nbd/server: CVE-2024-7409: Close stray clients at server-stop |
Date: |
Fri, 6 Sep 2024 09:54:10 +0300 |
From: Eric Blake <eblake@redhat.com>
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 the previous two patches reduce
this behavior by capping the default max-connections parameter and
killing slow clients, they did not eliminate the possibility of a
client waiting to close the socket until after the QMP nbd-server-stop
command is executed, at which point qemu would SEGV when trying to
dereference the NULL nbd_server global which is no longer present.
This amounts to a denial of service attack. Worse, 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 (although the most likely
is a crash due to an assertion failure when checking
nbd_server->connections > 0).
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
CC: qemu-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-ID: <20240807174943.771624-14-eblake@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
(cherry picked from commit 3e7ef738c8462c45043a1d39f702a0990406a3b3)
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 24ba5382db..f73409ae49 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -21,12 +21,18 @@
#include "io/channel-socket.h"
#include "io/net-listener.h"
+typedef struct NBDConn {
+ QIOChannelSocket *cioc;
+ QLIST_ENTRY(NBDConn) next;
+} NBDConn;
+
typedef struct NBDServerData {
QIONetListener *listener;
QCryptoTLSCreds *tlscreds;
char *tlsauthz;
uint32_t max_connections;
uint32_t connections;
+ QLIST_HEAD(, NBDConn) conns;
} NBDServerData;
static NBDServerData *nbd_server;
@@ -51,6 +57,14 @@ int nbd_server_max_connections(void)
static void nbd_blockdev_client_closed(NBDClient *client, bool ignored)
{
+ NBDConn *conn = nbd_client_owner(client);
+
+ assert(qemu_in_main_thread() && nbd_server);
+
+ object_unref(OBJECT(conn->cioc));
+ QLIST_REMOVE(conn, next);
+ g_free(conn);
+
nbd_client_put(client);
assert(nbd_server->connections > 0);
nbd_server->connections--;
@@ -60,14 +74,20 @@ static void nbd_blockdev_client_closed(NBDClient *client,
bool ignored)
static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc,
gpointer opaque)
{
+ NBDConn *conn = g_new0(NBDConn, 1);
+
+ assert(qemu_in_main_thread() && nbd_server);
nbd_server->connections++;
+ object_ref(OBJECT(cioc));
+ conn->cioc = cioc;
+ QLIST_INSERT_HEAD(&nbd_server->conns, conn, next);
nbd_update_server_watch(nbd_server);
qio_channel_set_name(QIO_CHANNEL(cioc), "nbd-server");
/* TODO - expose handshake timeout as QMP option */
nbd_client_new(cioc, NBD_DEFAULT_HANDSHAKE_MAX_SECS,
nbd_server->tlscreds, nbd_server->tlsauthz,
- nbd_blockdev_client_closed, NULL);
+ nbd_blockdev_client_closed, conn);
}
static void nbd_update_server_watch(NBDServerData *s)
@@ -81,12 +101,25 @@ static void nbd_update_server_watch(NBDServerData *s)
static void nbd_server_free(NBDServerData *server)
{
+ NBDConn *conn, *tmp;
+
if (!server) {
return;
}
+ /*
+ * Forcefully close the listener socket, and any clients that have
+ * not yet disconnected on their own.
+ */
qio_net_listener_disconnect(server->listener);
object_unref(OBJECT(server->listener));
+ QLIST_FOREACH_SAFE(conn, &server->conns, next, tmp) {
+ qio_channel_shutdown(QIO_CHANNEL(conn->cioc),
QIO_CHANNEL_SHUTDOWN_BOTH,
+ NULL);
+ }
+
+ AIO_WAIT_WHILE_UNLOCKED(NULL, server->connections > 0);
+
if (server->tlscreds) {
object_unref(OBJECT(server->tlscreds));
}
--
2.39.2
- [Stable-8.2.7 27/53] target/arm: Handle denormals correctly for FMOPA (widening), (continued)
- [Stable-8.2.7 27/53] target/arm: Handle denormals correctly for FMOPA (widening), Michael Tokarev, 2024/09/06
- [Stable-8.2.7 32/53] vvfat: Fix bug in writing to middle of file, Michael Tokarev, 2024/09/06
- [Stable-8.2.7 34/53] vvfat: Fix wrong checks for cluster mappings invariant, Michael Tokarev, 2024/09/06
- [Stable-8.2.7 30/53] target/i386: Fix VSIB decode, Michael Tokarev, 2024/09/06
- [Stable-8.2.7 33/53] vvfat: Fix usage of `info.file.offset`, Michael Tokarev, 2024/09/06
- [Stable-8.2.7 37/53] nbd/server: Plumb in new args to nbd_client_add(), Michael Tokarev, 2024/09/06
- [Stable-8.2.7 36/53] iotests: Add `vvfat` tests, Michael Tokarev, 2024/09/06
- [Stable-8.2.7 38/53] nbd/server: CVE-2024-7409: Cap default max-connections to 100, Michael Tokarev, 2024/09/06
- [Stable-8.2.7 35/53] vvfat: Fix reading files with non-continuous clusters, Michael Tokarev, 2024/09/06
- [Stable-8.2.7 44/53] target/i386: Do not apply REX to MMX operands, Michael Tokarev, 2024/09/06
- [Stable-8.2.7 40/53] nbd/server: CVE-2024-7409: Close stray clients at server-stop,
Michael Tokarev <=
- [Stable-8.2.7 39/53] nbd/server: CVE-2024-7409: Drop non-negotiating clients, Michael Tokarev, 2024/09/06
- [Stable-8.2.7 43/53] block/blkio: use FUA flag on write zeroes only if supported, Michael Tokarev, 2024/09/06
- [Stable-8.2.7 42/53] hw/core/ptimer: fix timer zero period condition for freq > 1GHz, Michael Tokarev, 2024/09/06
- [Stable-8.2.7 41/53] nbd/server: CVE-2024-7409: Avoid use-after-free when closing server, Michael Tokarev, 2024/09/06
- [Stable-8.2.7 46/53] module: Prevent crash by resetting local_err in module_load_qom_all(), Michael Tokarev, 2024/09/06
- [Stable-8.2.7 47/53] target/hexagon: don't look for static glib, Michael Tokarev, 2024/09/06
- [Stable-8.2.7 45/53] target/arm: Clear high SVE elements in handle_vec_simd_wshli, Michael Tokarev, 2024/09/06
- [Stable-8.2.7 48/53] target/sparc: Restrict STQF to sparcv9, Michael Tokarev, 2024/09/06
- [Stable-8.2.7 49/53] crypto/tlscredspsk: Free username on finalize, Michael Tokarev, 2024/09/06
- [Stable-8.2.7 53/53] target/hppa: Fix PSW V-bit packaging in cpu_hppa_get for hppa64, Michael Tokarev, 2024/09/06