On 02.08.24 04:32, Eric Blake wrote:
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>
---
[..]
-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) {
creating a getter nbd_client_get_cookie(client), and use it instead of passing
together with client, will simplify the patch a lot. [*]
Hmm.. don't we need some atomic accessors for nbd_cookie? and for
nbs_server->connections.. The function is called from client, which live in
coroutine and maybe in another thread? At least client code do atomic accesses of
client->refcount..
+ assert(nbd_server->connections > 0);
+ nbd_server->connections--;
+ nbd_update_server_watch(nbd_server);
+ }
}
[..]
@@ -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);
[*] passing client->close_cokkie together with client itself looks like we lack
a getter for .close_cookie
}
}
[..]
Hmm, instead of cookies and additional NBDConn objects in the next patch, could
we simply have a list of connected NBDClient objects in NBDServer and link to
NBDServer in NBDClient? (Ok we actually don't have NBDServer, but NBDServerData
in qemu, and several global variables in qemu-nbd, so some refactoring is
needed, to put common state to NBDServer, and add clients list to it)
This way, in nbd_server_free we'll just call client_close() in a loop. And in
client_close we'll have nbd_server_client_detach(client->server, client), instead
of client->close_fn(...). And server is freed only after all clients are closed.
And client never try to detach from another server.
This way, we also may implement several NBD servers working simultaneously if
we want.
--
Best regards,
Vladimir
