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.ivanov@virtuozzo.com>
Signed-off-by: Eric Blake <eblake@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

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.ivanov@virtuozzo.com>
> Signed-off-by: Eric Blake <eblake@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

On Fri, Aug 02, 2024 at 06:00:32PM GMT, Vladimir Sementsov-Ogievskiy wrote:
> > -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);

The client code already jumps through hoops to make sure
nbd_blockdev_client_closed() is the last thing called, and that
nbd_client_put() is only reached from the main thread (any coroutines
fired off a one-shot bh); but I added asserts in v3 to make it clear
I'm relying on the synchronous nature of coroutines yielding only at
known points and the code executing only in the main thread as the
reason why we don't need explicit locking here.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

On Fri, Aug 02, 2024 at 06:00:32PM GMT, Vladimir Sementsov-Ogievskiy wrote:
> On 02.08.24 04:32, Eric Blake wrote:

> [..]
> 
> > -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. [*]

I may be able to avoid the cookie altogether if I can add an
AIO_WAIT_WHILE(, nbd_server->connections > 0) after forcefully closing
all of the client sockets (nbd_client_new _should_ progress pretty
rapidly towards eventually calling nbd_blockdev_client_closed once the
socket is closed) - but that still requires patch 2 to keep a list of
open clients.

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

Whether the cookie be a uint32_t or the void* server object itself, it
is opaque to the client, but the server needs to track something.


> 
> >       }
> >   }
> > 
> 
> [..]
> 
> 
> 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.

Yes, we do eventually want to get to the point of being able to open
parallel NBD servers on different ports simultaneously, at which point
having a client remember which server it is associated makes sense (so
at a bare minimum, pass in a void* instead of a uint32_t to
nbd_client_new).  And given that we can have an NBD server with more
than one export, and with exports running in different threads (if
multithread io is enabled), I probably also need to add missing
locking to protect nbd_server (whether or not it stays global or we
eventually reach the point of having parallel servers on separate
ports).

Looks like I have work cut out for me before posting a v3.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

On Thu, Aug 01, 2024 at 08:32:06PM GMT, 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.

Now assigned CVE-2024-7409; I'll make sure that is mentioned in the
commit message, whether a v2 is needed or whether this gets a positive
review as-is modulo the reference.

I have not (yet?) determined how long the bug has existed, but note
that the code in question appears unchanged since at least qemu 6.0
(from 2021), and probably predates that, so it is long-standing.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org