As part of the QMP command nbd-server-start, the blockdev code was
creating a single global nbd_server object, and telling the qio code
to accept one or more client connections to the exposed listener
socket. But even though we tear down the listener socket during a
subsequent QMP nbd-server-stop, the qio code has handed off to a
coroutine that may be executing asynchronously with the server
shutdown, such that a client that connects to the socket before
nbd-server-stop but delays disconnect or completion of the NBD
handshake until after the followup QMP command can result in the
nbd_blockdev_client_closed() callback running after nbd_server has
already been torn down, causing a SEGV. Worse, if a second nbd_server
object is created (possibly on a different socket address), a late
client resolution from the first server can subtly interfere with the
second server. This can be abused by a malicious client that does not
know TLS secrets to cause qemu to SEGV after a legitimate client has
concluded storage migration to a destination qemu, which can be turned
into a denial of service attack even when qemu is set up to require
only TLS clients.
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; using frameworks like libvirt that ensure
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.
Fix this by passing a cookie through to each client connection
(whether or not that client actually knows the TLS details to
successfully negotiate); then increment the cookie every time a server
is torn down so that we can recognize any late client actions
lingering from an old server.
This patch does not address the fact that client sockets can be left
open indefinitely after the server is torn down (possibly creating a
resource leak, if a malicious client intentionally leaves lots of open
sockets paused partway through NBD negotiation); the next patch will
add some code to forcefully close any lingering clients as soon as
possible when the server is torn down.
Reported-by: Alexander Ivanov <alexander.iva...@virtuozzo.com>
Signed-off-by: Eric Blake <ebl...@redhat.com>
---
include/block/nbd.h | 3 ++-
blockdev-nbd.c | 17 ++++++++++++-----
nbd/server.c | 12 ++++++++----
qemu-nbd.c | 5 +++--
4 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 4e7bd6342f9..9c43bcf8a26 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -405,7 +405,8 @@ NBDExport *nbd_export_find(const char *name);
void nbd_client_new(QIOChannelSocket *sioc,
QCryptoTLSCreds *tlscreds,
const char *tlsauthz,
- void (*close_fn)(NBDClient *, bool));
+ void (*close_fn)(NBDClient *, uint32_t, bool),
+ uint32_t cookie);
void nbd_client_get(NBDClient *client);
void nbd_client_put(NBDClient *client);
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 213012435f4..1ddcbd7b247 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -30,6 +30,7 @@ typedef struct NBDServerData {
} NBDServerData;
static NBDServerData *nbd_server;
+static uint32_t nbd_cookie; /* Generation count of nbd_server */
static int qemu_nbd_connections = -1; /* Non-negative if this is qemu-nbd */
static void nbd_update_server_watch(NBDServerData *s);
@@ -49,23 +50,28 @@ int nbd_server_max_connections(void)
return nbd_server ? nbd_server->max_connections : qemu_nbd_connections;
}
-static void nbd_blockdev_client_closed(NBDClient *client, bool ignored)
+static void nbd_blockdev_client_closed(NBDClient *client, uint32_t cookie,
+ bool ignored)
{
nbd_client_put(client);
- assert(nbd_server->connections > 0);
- nbd_server->connections--;
- nbd_update_server_watch(nbd_server);
+ /* Ignore any (late) connection made under a previous server */
+ if (cookie == nbd_cookie) {
+ assert(nbd_server->connections > 0);
+ nbd_server->connections--;
+ nbd_update_server_watch(nbd_server);
+ }
}
static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc,
gpointer opaque)
{
+ assert(nbd_server);
nbd_server->connections++;
nbd_update_server_watch(nbd_server);
qio_channel_set_name(QIO_CHANNEL(cioc), "nbd-server");
nbd_client_new(cioc, nbd_server->tlscreds, nbd_server->tlsauthz,
- nbd_blockdev_client_closed);
+ nbd_blockdev_client_closed, nbd_cookie);
}
static void nbd_update_server_watch(NBDServerData *s)
@@ -89,6 +95,7 @@ static void nbd_server_free(NBDServerData *server)
object_unref(OBJECT(server->tlscreds));
}
g_free(server->tlsauthz);
+ nbd_cookie++;
g_free(server);
}
diff --git a/nbd/server.c b/nbd/server.c
index 892797bb111..7c37d9753f0 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -123,7 +123,8 @@ struct NBDMetaContexts {
struct NBDClient {
int refcount; /* atomic */
- void (*close_fn)(NBDClient *client, bool negotiated);
+ void (*close_fn)(NBDClient *client, uint32_t cookie, bool negotiated);
+ uint32_t close_cookie;
QemuMutex lock;
@@ -1621,7 +1622,7 @@ static void client_close(NBDClient *client, bool
negotiated)
/* Also tell the client, so that they release their reference. */
if (client->close_fn) {
- client->close_fn(client, negotiated);
+ client->close_fn(client, client->close_cookie, negotiated);
}
}
@@ -3207,12 +3208,14 @@ static coroutine_fn void nbd_co_client_start(void
*opaque)
/*
* Create a new client listener using the given channel @sioc.
* Begin servicing it in a coroutine. When the connection closes, call
- * @close_fn with an indication of whether the client completed negotiation.
+ * @close_fn with @cookie and an indication of whether the client completed
+ # negotiation.
*/
void nbd_client_new(QIOChannelSocket *sioc,
QCryptoTLSCreds *tlscreds,
const char *tlsauthz,
- void (*close_fn)(NBDClient *, bool))
+ void (*close_fn)(NBDClient *, uint32_t, bool),
+ uint32_t cookie)
{
NBDClient *client;
Coroutine *co;
@@ -3231,6 +3234,7 @@ void nbd_client_new(QIOChannelSocket *sioc,
client->ioc = QIO_CHANNEL(sioc);
object_ref(OBJECT(client->ioc));
client->close_fn = close_fn;
+ client->close_cookie = cookie;
co = qemu_coroutine_create(nbd_co_client_start, client);
qemu_coroutine_enter(co);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index d7b3ccab21c..3ad50eec18e 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -371,7 +371,8 @@ static int nbd_can_accept(void)
static void nbd_update_server_watch(void);
-static void nbd_client_closed(NBDClient *client, bool negotiated)
+static void nbd_client_closed(NBDClient *client, uint32_t ignored,
+ bool negotiated)
{
nb_fds--;
if (negotiated && nb_fds == 0 && !persistent && state == RUNNING) {
@@ -390,7 +391,7 @@ static void nbd_accept(QIONetListener *listener,
QIOChannelSocket *cioc,
nb_fds++;
nbd_update_server_watch();
- nbd_client_new(cioc, tlscreds, tlsauthz, nbd_client_closed);
+ nbd_client_new(cioc, tlscreds, tlsauthz, nbd_client_closed, 0);
}
static void nbd_update_server_watch(void)
--
2.45.2
