[PATCH for-9.1 v4 0/7] CVE-2024-7409

[PATCH for-9.1 v4 0/7] CVE-2024-7409

[Eric Blake]

v3 was here:
https://lists.gnu.org/archive/html/qemu-devel/2024-08/msg00818.html

since then:
 - re-add a minor patch from v2 (now patch 1)
 - refactor how the client opaque pointer is handled (patch 2)
 - add two new patches to prevent malicious clients from consuming
   inordinate resources: change the default max-connections from
   unlimited to capped at 100 (patch 3), and add code to kill any
   client that takes longer than 10 seconds after connect to reach
   NBD_OPT_GO (patch 4) [Dan]
 - squash the connection list handling into a single patch (5) [Dan]
 - two new additional patches for reverting back to 9.0 behavior for
   integration testing purposes; I'm okay if these last two miss 9.1

Eric Blake (7):
  nbd: Minor style fixes
  nbd/server: Plumb in new args to nbd_client_add()
  nbd/server: CVE-2024-7409: Change default max-connections to 100
  nbd/server: CVE-2024-7409: Drop non-negotiating clients
  nbd/server: CVE-2024-7409: Close stray client sockets at shutdown
  qemu-nbd: Allow users to adjust handshake limit
  nbd/server: Allow users to adjust handshake limit in QMP

 docs/tools/qemu-nbd.rst        |  5 +++
 qapi/block-export.json         | 18 +++++++---
 include/block/nbd.h            | 20 +++++++++--
 block/monitor/block-hmp-cmds.c |  3 +-
 blockdev-nbd.c                 | 62 +++++++++++++++++++++++++++++++---
 nbd/server.c                   | 51 +++++++++++++++++++++++++---
 qemu-nbd.c                     | 44 ++++++++++++++++--------
 nbd/trace-events               |  1 +
 8 files changed, 173 insertions(+), 31 deletions(-)

-- 
2.45.2

[PATCH v4 1/7] nbd: Minor style fixes

[Eric Blake]

Touch up a comment with the wrong type name, and an over-long line,
both noticed while working on upcoming patches.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 2 +-
 qemu-nbd.c   | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 892797bb111..ecd9366ba64 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1972,7 +1972,7 @@ static void nbd_export_request_shutdown(BlockExport *blk_exp)

     blk_exp_ref(&exp->common);
     /*
-     * TODO: Should we expand QMP NbdServerRemoveNode enum to allow a
+     * TODO: Should we expand QMP BlockExportRemoveMode enum to allow a
      * close mode that stops advertising the export to new clients but
      * still permits existing clients to run to completion? Because of
      * that possibility, nbd_export_close() can be called more than
diff --git a/qemu-nbd.c b/qemu-nbd.c
index d7b3ccab21c..8e104ef22c3 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -588,7 +588,8 @@ int main(int argc, char **argv)
     pthread_t client_thread;
     const char *fmt = NULL;
     Error *local_err = NULL;
-    BlockdevDetectZeroesOptions detect_zeroes = BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
+    BlockdevDetectZeroesOptions detect_zeroes =
+        BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
     QDict *options = NULL;
     const char *export_name = NULL; /* defaults to "" later for server mode */
     const char *export_description = NULL;
-- 
2.45.2

Re: [PATCH v4 1/7] nbd: Minor style fixes

[Daniel P. Berrangé]

in $subject  s/style/style & typo/

On Wed, Aug 07, 2024 at 12:43:27PM -0500, Eric Blake wrote:
> Touch up a comment with the wrong type name, and an over-long line,
> both noticed while working on upcoming patches.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  nbd/server.c | 2 +-
>  qemu-nbd.c   | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)

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

[PATCH v4 2/7] nbd/server: Plumb in new args to nbd_client_add()

[Eric Blake]

Upcoming patches to fix a CVE need to track an opaque pointer passed
in by the owner of a client object, as well as reequest for a time
limit on how fast negotiation must complete.  Prepare for that by
changing the signature of nbd_client_new() and adding an accessor to
get at the opaque pointer, although for now the two servers
(qemu-nbd.c and blockdev-nbd.c) do not change behavior.

Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/nbd.h | 11 ++++++++++-
 blockdev-nbd.c      |  6 ++++--
 nbd/server.c        | 20 +++++++++++++++++---
 qemu-nbd.c          |  4 +++-
 4 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 4e7bd6342f9..5fe14786414 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -33,6 +33,12 @@ typedef struct NBDMetaContexts NBDMetaContexts;

 extern const BlockExportDriver blk_exp_nbd;

+/*
+ * NBD_DEFAULT_HANDSHAKE_LIMIT: Number of seconds in which client must
+ * succeed at NBD_OPT_GO before being forcefully dropped as too slow.
+ */
+#define NBD_DEFAULT_HANDSHAKE_LIMIT 10
+
 /* Handshake phase structs - this struct is passed on the wire */

 typedef struct NBDOption {
@@ -403,9 +409,12 @@ AioContext *nbd_export_aio_context(NBDExport *exp);
 NBDExport *nbd_export_find(const char *name);

 void nbd_client_new(QIOChannelSocket *sioc,
+                    uint32_t handshake_limit,
                     QCryptoTLSCreds *tlscreds,
                     const char *tlsauthz,
-                    void (*close_fn)(NBDClient *, bool));
+                    void (*close_fn)(NBDClient *, bool),
+                    void *owner);
+void *nbd_client_owner(NBDClient *client);
 void nbd_client_get(NBDClient *client);
 void nbd_client_put(NBDClient *client);

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 213012435f4..11f878b6db3 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -64,8 +64,10 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc,
     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);
+    /* TODO - expose handshake limit as QMP option */
+    nbd_client_new(cioc, NBD_DEFAULT_HANDSHAKE_LIMIT,
+                   nbd_server->tlscreds, nbd_server->tlsauthz,
+                   nbd_blockdev_client_closed, NULL);
 }

 static void nbd_update_server_watch(NBDServerData *s)
diff --git a/nbd/server.c b/nbd/server.c
index ecd9366ba64..31b77bf0d4f 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -124,12 +124,14 @@ struct NBDMetaContexts {
 struct NBDClient {
     int refcount; /* atomic */
     void (*close_fn)(NBDClient *client, bool negotiated);
+    void *owner;

     QemuMutex lock;

     NBDExport *exp;
     QCryptoTLSCreds *tlscreds;
     char *tlsauthz;
+    uint32_t handshake_limit;
     QIOChannelSocket *sioc; /* The underlying data channel */
     QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */

@@ -3191,6 +3193,7 @@ static coroutine_fn void nbd_co_client_start(void *opaque)

     qemu_co_mutex_init(&client->send_lock);

+    /* TODO - utilize client->handshake_limit */
     if (nbd_negotiate(client, &local_err)) {
         if (local_err) {
             error_report_err(local_err);
@@ -3205,14 +3208,17 @@ static coroutine_fn void nbd_co_client_start(void *opaque)
 }

 /*
- * Create a new client listener using the given channel @sioc.
+ * Create a new client listener using the given channel @sioc and @owner.
  * 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 an indication of whether the client completed negotiation
+ * within @handshake_limit seconds (0 for unbounded).
  */
 void nbd_client_new(QIOChannelSocket *sioc,
+                    uint32_t handshake_limit,
                     QCryptoTLSCreds *tlscreds,
                     const char *tlsauthz,
-                    void (*close_fn)(NBDClient *, bool))
+                    void (*close_fn)(NBDClient *, bool),
+                    void *owner)
 {
     NBDClient *client;
     Coroutine *co;
@@ -3225,13 +3231,21 @@ void nbd_client_new(QIOChannelSocket *sioc,
         object_ref(OBJECT(client->tlscreds));
     }
     client->tlsauthz = g_strdup(tlsauthz);
+    client->handshake_limit = handshake_limit;
     client->sioc = sioc;
     qio_channel_set_delay(QIO_CHANNEL(sioc), false);
     object_ref(OBJECT(client->sioc));
     client->ioc = QIO_CHANNEL(sioc);
     object_ref(OBJECT(client->ioc));
     client->close_fn = close_fn;
+    client->owner = owner;

     co = qemu_coroutine_create(nbd_co_client_start, client);
     qemu_coroutine_enter(co);
 }
+
+void *
+nbd_client_owner(NBDClient *client)
+{
+    return client->owner;
+}
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 8e104ef22c3..7bf86a6566b 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -390,7 +390,9 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc,

     nb_fds++;
     nbd_update_server_watch();
-    nbd_client_new(cioc, tlscreds, tlsauthz, nbd_client_closed);
+    /* TODO - expose handshake limit as command line option */
+    nbd_client_new(cioc, NBD_DEFAULT_HANDSHAKE_LIMIT,
+                   tlscreds, tlsauthz, nbd_client_closed, NULL);
 }

 static void nbd_update_server_watch(void)
-- 
2.45.2

Re: [PATCH v4 2/7] nbd/server: Plumb in new args to nbd_client_add()

[Daniel P. Berrangé]

On Wed, Aug 07, 2024 at 12:43:28PM -0500, Eric Blake wrote:
> Upcoming patches to fix a CVE need to track an opaque pointer passed
> in by the owner of a client object, as well as reequest for a time
> limit on how fast negotiation must complete.  Prepare for that by
> changing the signature of nbd_client_new() and adding an accessor to
> get at the opaque pointer, although for now the two servers
> (qemu-nbd.c and blockdev-nbd.c) do not change behavior.
> 
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/block/nbd.h | 11 ++++++++++-
>  blockdev-nbd.c      |  6 ++++--
>  nbd/server.c        | 20 +++++++++++++++++---
>  qemu-nbd.c          |  4 +++-
>  4 files changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 4e7bd6342f9..5fe14786414 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -33,6 +33,12 @@ typedef struct NBDMetaContexts NBDMetaContexts;
> 
>  extern const BlockExportDriver blk_exp_nbd;
> 
> +/*
> + * NBD_DEFAULT_HANDSHAKE_LIMIT: Number of seconds in which client must
> + * succeed at NBD_OPT_GO before being forcefully dropped as too slow.
> + */
> +#define NBD_DEFAULT_HANDSHAKE_LIMIT 10

Suggest

s/NBD_DEFAULT_HANDSHAKE_LIMIT/NBD_DEFAULT_HANDSHAKE_MAX_SECS/


> +
>  /* Handshake phase structs - this struct is passed on the wire */
> 
>  typedef struct NBDOption {
> @@ -403,9 +409,12 @@ AioContext *nbd_export_aio_context(NBDExport *exp);
>  NBDExport *nbd_export_find(const char *name);
> 
>  void nbd_client_new(QIOChannelSocket *sioc,
> +                    uint32_t handshake_limit,

s/handshake_limit/handshake_max_secs/

to make the units of the parameter self-documenting.

Since this is a non-functional suggestion

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

Re: [PATCH v4 2/7] nbd/server: Plumb in new args to nbd_client_add()

[Eric Blake]

On Wed, Aug 07, 2024 at 06:58:36PM GMT, Daniel P. Berrangé wrote:
> On Wed, Aug 07, 2024 at 12:43:28PM -0500, Eric Blake wrote:
> > Upcoming patches to fix a CVE need to track an opaque pointer passed
> > in by the owner of a client object, as well as reequest for a time

s/reequest/request/

> > limit on how fast negotiation must complete.  Prepare for that by
> > changing the signature of nbd_client_new() and adding an accessor to
> > get at the opaque pointer, although for now the two servers
> > (qemu-nbd.c and blockdev-nbd.c) do not change behavior.
> > 
> > Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> >  include/block/nbd.h | 11 ++++++++++-
> >  blockdev-nbd.c      |  6 ++++--
> >  nbd/server.c        | 20 +++++++++++++++++---
> >  qemu-nbd.c          |  4 +++-
> >  4 files changed, 34 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/block/nbd.h b/include/block/nbd.h
> > index 4e7bd6342f9..5fe14786414 100644
> > --- a/include/block/nbd.h
> > +++ b/include/block/nbd.h
> > @@ -33,6 +33,12 @@ typedef struct NBDMetaContexts NBDMetaContexts;
> > 
> >  extern const BlockExportDriver blk_exp_nbd;
> > 
> > +/*
> > + * NBD_DEFAULT_HANDSHAKE_LIMIT: Number of seconds in which client must
> > + * succeed at NBD_OPT_GO before being forcefully dropped as too slow.
> > + */
> > +#define NBD_DEFAULT_HANDSHAKE_LIMIT 10
> 
> Suggest
> 
> s/NBD_DEFAULT_HANDSHAKE_LIMIT/NBD_DEFAULT_HANDSHAKE_MAX_SECS/

I like it.

> 
> 
> > +
> >  /* Handshake phase structs - this struct is passed on the wire */
> > 
> >  typedef struct NBDOption {
> > @@ -403,9 +409,12 @@ AioContext *nbd_export_aio_context(NBDExport *exp);
> >  NBDExport *nbd_export_find(const char *name);
> > 
> >  void nbd_client_new(QIOChannelSocket *sioc,
> > +                    uint32_t handshake_limit,
> 
> s/handshake_limit/handshake_max_secs/
> 
> to make the units of the parameter self-documenting.
> 
> Since this is a non-functional suggestion
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Will adjust the series with the fallout.

At this point, I'm leaning towards queuing 1-5 in an upcoming pull
request, but leaving 6-7 for the 9.2 development cycle.

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

[PATCH v4 3/7] nbd/server: CVE-2024-7409: Change default max-connections to 100

[Eric Blake]

Allowing an unlimited number of clients to any web service is a recipe
for a rudimentary denial of service attack: the client merely needs to
open lots of sockets without closing them, until qemu no longer has
any more fds available to allocate.

For qemu-nbd, we default to allowing only 1 connection unless more are
explicitly asked for (-e or --shared); this was historically picked as
a nice default (without an explicit -t, a non-persistent qemu-nbd goes
away after a client disconnects, without needing any additional
follow-up commands), and we are not going to change that interface now
(besides, someday we want to point people towards qemu-storage-daemon
instead of qemu-nbd).

But for qemu proper, the QMP nbd-server-start command has historically
had a default of unlimited number of connections, in part because
unlike qemu-nbd it is inherently persistent.  Allowing multiple client
sockets is particularly useful for clients that can take advantage of
MULTI_CONN (creating parallel sockets to increase throughput),
although known clients that do so (such as libnbd's nbdcopy) typically
use only 8 or 16 connections (the benefits of scaling diminish once
more sockets are competing for kernel attention).  Picking a number
large enough for typical use cases, but not unlimited, makes it
slightly harder for a malicious client to perform a denial of service
merely by opening lots of connections withot progressing through the
handshake.

This change does not eliminate CVE-2024-7409 on its own, but reduces
the chance for fd exhaustion or unlimited memory usage as an attack
surface.  On the other hand, by itself, it makes it more obvious that
with a finite limit, we have the problem of an unauthenticated client
holding 100 fds opened as a way to block out a legitimate client from
being able to connect; thus, later patches will further add timeouts
to reject clients that are not making progress.

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qapi/block-export.json         | 4 ++--
 include/block/nbd.h            | 7 +++++++
 block/monitor/block-hmp-cmds.c | 3 ++-
 blockdev-nbd.c                 | 8 ++++++++
 4 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/qapi/block-export.json b/qapi/block-export.json
index 665d5fd0262..ce33fe378df 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -28,7 +28,7 @@
 # @max-connections: The maximum number of connections to allow at the
 #     same time, 0 for unlimited.  Setting this to 1 also stops the
 #     server from advertising multiple client support (since 5.2;
-#     default: 0)
+#     default: 100)
 #
 # Since: 4.2
 ##
@@ -63,7 +63,7 @@
 # @max-connections: The maximum number of connections to allow at the
 #     same time, 0 for unlimited.  Setting this to 1 also stops the
 #     server from advertising multiple client support (since 5.2;
-#     default: 0).
+#     default: 100).
 #
 # Errors:
 #     - if the server is already running
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 5fe14786414..fd5044359dc 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -39,6 +39,13 @@ extern const BlockExportDriver blk_exp_nbd;
  */
 #define NBD_DEFAULT_HANDSHAKE_LIMIT 10

+/*
+ * NBD_DEFAULT_MAX_CONNECTIONS: Number of client sockets to allow at
+ * once; must be large enough to allow a MULTI_CONN-aware client like
+ * nbdcopy to create its typical number of 8-16 sockets.
+ */
+#define NBD_DEFAULT_MAX_CONNECTIONS 100
+
 /* Handshake phase structs - this struct is passed on the wire */

 typedef struct NBDOption {
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index d954bec6f1e..bdf2eb50b68 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -402,7 +402,8 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
         goto exit;
     }

-    nbd_server_start(addr, NULL, NULL, 0, &local_err);
+    nbd_server_start(addr, NULL, NULL, NBD_DEFAULT_MAX_CONNECTIONS,
+                     &local_err);
     qapi_free_SocketAddress(addr);
     if (local_err != NULL) {
         goto exit;
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 11f878b6db3..19c57897819 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -170,6 +170,10 @@ void nbd_server_start(SocketAddress *addr, const char *tls_creds,

 void nbd_server_start_options(NbdServerOptions *arg, Error **errp)
 {
+    if (!arg->has_max_connections) {
+        arg->max_connections = NBD_DEFAULT_MAX_CONNECTIONS;
+    }
+
     nbd_server_start(arg->addr, arg->tls_creds, arg->tls_authz,
                      arg->max_connections, errp);
 }
@@ -182,6 +186,10 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr,
 {
     SocketAddress *addr_flat = socket_address_flatten(addr);

+    if (!has_max_connections) {
+        max_connections = NBD_DEFAULT_MAX_CONNECTIONS;
+    }
+
     nbd_server_start(addr_flat, tls_creds, tls_authz, max_connections, errp);
     qapi_free_SocketAddress(addr_flat);
 }
-- 
2.45.2

Re: [PATCH v4 3/7] nbd/server: CVE-2024-7409: Change default max-connections to 100

[Daniel P. Berrangé]

On Wed, Aug 07, 2024 at 12:43:29PM -0500, Eric Blake wrote:
> Allowing an unlimited number of clients to any web service is a recipe
> for a rudimentary denial of service attack: the client merely needs to
> open lots of sockets without closing them, until qemu no longer has
> any more fds available to allocate.
> 
> For qemu-nbd, we default to allowing only 1 connection unless more are
> explicitly asked for (-e or --shared); this was historically picked as
> a nice default (without an explicit -t, a non-persistent qemu-nbd goes
> away after a client disconnects, without needing any additional
> follow-up commands), and we are not going to change that interface now
> (besides, someday we want to point people towards qemu-storage-daemon
> instead of qemu-nbd).
> 
> But for qemu proper, the QMP nbd-server-start command has historically
> had a default of unlimited number of connections, in part because
> unlike qemu-nbd it is inherently persistent.  Allowing multiple client
> sockets is particularly useful for clients that can take advantage of
> MULTI_CONN (creating parallel sockets to increase throughput),
> although known clients that do so (such as libnbd's nbdcopy) typically
> use only 8 or 16 connections (the benefits of scaling diminish once
> more sockets are competing for kernel attention).  Picking a number
> large enough for typical use cases, but not unlimited, makes it
> slightly harder for a malicious client to perform a denial of service
> merely by opening lots of connections withot progressing through the
> handshake.
> 
> This change does not eliminate CVE-2024-7409 on its own, but reduces
> the chance for fd exhaustion or unlimited memory usage as an attack
> surface.  On the other hand, by itself, it makes it more obvious that
> with a finite limit, we have the problem of an unauthenticated client
> holding 100 fds opened as a way to block out a legitimate client from
> being able to connect; thus, later patches will further add timeouts
> to reject clients that are not making progress.
> 
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  qapi/block-export.json         | 4 ++--
>  include/block/nbd.h            | 7 +++++++
>  block/monitor/block-hmp-cmds.c | 3 ++-
>  blockdev-nbd.c                 | 8 ++++++++
>  4 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/qapi/block-export.json b/qapi/block-export.json
> index 665d5fd0262..ce33fe378df 100644
> --- a/qapi/block-export.json
> +++ b/qapi/block-export.json
> @@ -28,7 +28,7 @@
>  # @max-connections: The maximum number of connections to allow at the
>  #     same time, 0 for unlimited.  Setting this to 1 also stops the
>  #     server from advertising multiple client support (since 5.2;
> -#     default: 0)
> +#     default: 100)
>  #
>  # Since: 4.2
>  ##
> @@ -63,7 +63,7 @@
>  # @max-connections: The maximum number of connections to allow at the
>  #     same time, 0 for unlimited.  Setting this to 1 also stops the
>  #     server from advertising multiple client support (since 5.2;
> -#     default: 0).
> +#     default: 100).
>  #
>  # Errors:
>  #     - if the server is already running

This is considered a backwards compatibility break.
An intentional one.

Our justification is that we believe it is the least worst option
to mitigate the DoS vector. Lets explore the reasoning for that
belief...

An alternative would be to deprecate the absence of 'max-connections',
and after the deprecation period, make it in into a mandatory
parameter, forcing apps to make a decision. This would be strictly
compliant with our QAPI change policy.

How does that differ in impact from changing the defaults...

  * Changed default
     - Small risk of breaking app if needing > 100 concurrent conns
     - Immediately closes the DoS hole for all apps using QEMU

  * Deprecation & eventual change to mandatory
     - Zero risk of breaking apps today
     - Forces all apps to be changed to pass max-connections
       in 3 releases time
     - Does NOT close the DoS hole until the 3rd future release
       from now.

If we took the deprecation route, arguably any app which isn't
already setting max-connections would need to have a CVE filed
against it *today*, and thus effectively apps would be forced
into immediately setting max-connections, despite our deprecaiton
grace period supposedly giving them 3 releases to adjust.

If we took the changed default route and broke an app, it would
again have to be changed to set max-connections to a value that
satisfies its use case.


So....

If we take the deprecation route, we create work for /all/ app
developers using NBD to update their code now to fix the CVE.

If we take the changed defaults route, and our 100 value choice
is sufficient, then no apps need changing.

If we take the changed defaults route, and our 100 value choice
is NOT sufficient, then most apps still don't need changing, but
perhaps 1 or 2 do need changing.

The unpleasant bit is that if 100 is insufficient, an app
maintainer or user might not become aware of this until they
have been in production for 12 months and suddenly hit a
rare load spike.


Overall though, changing the defaults still looks likely to
be the least disruptive option for fixing the DoS, providing
our choice of 100 is a good one.

I think system emulators are likely OK given common use
cases for NBD in context of a running VM (migration and
backup related).

I've got a nagging concern someone could be doing something
adventurous with QSD though. eg using it to serve content
(perhaps readonly) to a huge pool of clients ?

Probably that's just a risk we have to take, as any app
relying on the max-connections default is vulernable.

There are no good answers AFAICT. Only bad answers. This
one feels least bad to me.

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

Re: [PATCH v4 3/7] nbd/server: CVE-2024-7409: Change default max-connections to 100

[Eric Blake]

On Wed, Aug 07, 2024 at 07:24:56PM GMT, Daniel P. Berrangé wrote:
> On Wed, Aug 07, 2024 at 12:43:29PM -0500, Eric Blake wrote:
> > Allowing an unlimited number of clients to any web service is a recipe
> > for a rudimentary denial of service attack: the client merely needs to
> > open lots of sockets without closing them, until qemu no longer has
> > any more fds available to allocate.
> > 
> > For qemu-nbd, we default to allowing only 1 connection unless more are
> > explicitly asked for (-e or --shared); this was historically picked as
> > a nice default (without an explicit -t, a non-persistent qemu-nbd goes
> > away after a client disconnects, without needing any additional
> > follow-up commands), and we are not going to change that interface now
> > (besides, someday we want to point people towards qemu-storage-daemon
> > instead of qemu-nbd).
> > 
> > But for qemu proper, the QMP nbd-server-start command has historically
> > had a default of unlimited number of connections, in part because
> > unlike qemu-nbd it is inherently persistent.  Allowing multiple client
> > sockets is particularly useful for clients that can take advantage of
> > MULTI_CONN (creating parallel sockets to increase throughput),
> > although known clients that do so (such as libnbd's nbdcopy) typically
> > use only 8 or 16 connections (the benefits of scaling diminish once
> > more sockets are competing for kernel attention).  Picking a number
> > large enough for typical use cases, but not unlimited, makes it
> > slightly harder for a malicious client to perform a denial of service
> > merely by opening lots of connections withot progressing through the
> > handshake.
> > 
> > This change does not eliminate CVE-2024-7409 on its own, but reduces
> > the chance for fd exhaustion or unlimited memory usage as an attack
> > surface.  On the other hand, by itself, it makes it more obvious that
> > with a finite limit, we have the problem of an unauthenticated client
> > holding 100 fds opened as a way to block out a legitimate client from
> > being able to connect; thus, later patches will further add timeouts
> > to reject clients that are not making progress.
> > 
> > Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> >  qapi/block-export.json         | 4 ++--
> >  include/block/nbd.h            | 7 +++++++
> >  block/monitor/block-hmp-cmds.c | 3 ++-
> >  blockdev-nbd.c                 | 8 ++++++++
> >  4 files changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/qapi/block-export.json b/qapi/block-export.json
> > index 665d5fd0262..ce33fe378df 100644
> > --- a/qapi/block-export.json
> > +++ b/qapi/block-export.json
> > @@ -28,7 +28,7 @@
> >  # @max-connections: The maximum number of connections to allow at the
> >  #     same time, 0 for unlimited.  Setting this to 1 also stops the
> >  #     server from advertising multiple client support (since 5.2;
> > -#     default: 0)
> > +#     default: 100)
> >  #
> >  # Since: 4.2
> >  ##
> > @@ -63,7 +63,7 @@
> >  # @max-connections: The maximum number of connections to allow at the
> >  #     same time, 0 for unlimited.  Setting this to 1 also stops the
> >  #     server from advertising multiple client support (since 5.2;
> > -#     default: 0).
> > +#     default: 100).
> >  #
> >  # Errors:
> >  #     - if the server is already running
> 
> This is considered a backwards compatibility break.
> An intentional one.
> 
> Our justification is that we believe it is the least worst option
> to mitigate the DoS vector. Lets explore the reasoning for that
> belief...

I'll probably crib quite a bit of this analysis into the commit message.

> 
> An alternative would be to deprecate the absence of 'max-connections',
> and after the deprecation period, make it in into a mandatory
> parameter, forcing apps to make a decision. This would be strictly
> compliant with our QAPI change policy.
> 
> How does that differ in impact from changing the defaults...
> 
>   * Changed default
>      - Small risk of breaking app if needing > 100 concurrent conns
>      - Immediately closes the DoS hole for all apps using QEMU

Reduces, rather than closes.  QEMU is no longer vulnerable to running
out of fds (likely) or memory (less likely, since fds are more
limited); but once there is a cap, the malicious client can still tie
up 100 fds and block any other connections from proceeding.

Presumably, though, in the bigger picture, if you have difficulties
connecting your legitimate client (because a client has tied up max
connections), your investigation will probably lead you to block the
bad client by firewall; so our reduced DoS hole is easier to recover
from than the original DoS of no resources.

> 
>   * Deprecation & eventual change to mandatory
>      - Zero risk of breaking apps today
>      - Forces all apps to be changed to pass max-connections
>        in 3 releases time
>      - Does NOT close the DoS hole until the 3rd future release
>        from now.
> 
> If we took the deprecation route, arguably any app which isn't
> already setting max-connections would need to have a CVE filed
> against it *today*, and thus effectively apps would be forced
> into immediately setting max-connections, despite our deprecaiton
> grace period supposedly giving them 3 releases to adjust.

libvirt is in this boat: a grep of libvirt.git shows it is not yet
passing max-connections (of any value), in part because until only
recently, it was still targetting older qemu that did not support
max-connections at all (and it's easier to skip a parameter globally
than to implement a feature detection bit to parse the QAPI
introspection to see which releases support it).

> 
> If we took the changed default route and broke an app, it would
> again have to be changed to set max-connections to a value that
> satisfies its use case.
> 
> 
> So....
> 
> If we take the deprecation route, we create work for /all/ app
> developers using NBD to update their code now to fix the CVE.
> 
> If we take the changed defaults route, and our 100 value choice
> is sufficient, then no apps need changing.
> 
> If we take the changed defaults route, and our 100 value choice
> is NOT sufficient, then most apps still don't need changing, but
> perhaps 1 or 2 do need changing.

Are we in agreement that 100 is generally sufficient?  Again, my
benchmark here is nbdcopy, which defaults to 4 sockets when the server
advertises MULTI_CONN; I don't know if Rich has tried testing it with
more than 16 connections, but his benchmarks also show that doubling
connections has diminishing gains.

I also know that Change Block Tracking takes advantage of multiple
sockets (one reading the qemu:dirty-bitmap:XXX data, the other(s)
using that data to drive which portions of the disk they read); but
again, I couldn't quickly find any CBT implementation that was using
100 or more connections.

> 
> The unpleasant bit is that if 100 is insufficient, an app
> maintainer or user might not become aware of this until they
> have been in production for 12 months and suddenly hit a
> rare load spike.
> 
> 
> Overall though, changing the defaults still looks likely to
> be the least disruptive option for fixing the DoS, providing
> our choice of 100 is a good one.
> 
> I think system emulators are likely OK given common use
> cases for NBD in context of a running VM (migration and
> backup related).
> 
> I've got a nagging concern someone could be doing something
> adventurous with QSD though. eg using it to serve content
> (perhaps readonly) to a huge pool of clients ?

Yeah, I could see how a large fanout of disk-images all backed by a
single read-only base image served over NBD could result in lots of
legitimate client connections.

In the KubeSAN project, we are achieving multiple-node read-write
access of a shared VG by having secondary nodes connect via nbd.ko
(configured for 8 sockets per client node) to a qemu-nbd server on the
primary node - but a quick grep of that source shows that it has an
explicit 'qemu-nbd --shared=0' for unlimited, so that one is
unimpacted by this change (not to mention this change is to qemu QMP
nbd-server-start, not to qemu-nbd --shared=N).

> 
> Probably that's just a risk we have to take, as any app
> relying on the max-connections default is vulernable.
> 
> There are no good answers AFAICT. Only bad answers. This
> one feels least bad to me.
> 
> 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 :|
> 

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

[PATCH v4 4/7] nbd/server: CVE-2024-7409: Drop non-negotiating clients

[Eric Blake]

A client that opens a socket but does not negotiate is merely hogging
qemu's resources (an open fd and a small amount of memory); and a
malicious client that can access the port where NBD is listening can
attempt a denial of service attack by intentionally opening and
abandoning lots of unfinished connections.  The previous patch put a
default bound on the number of such ongoing connections, but once that
limit is hit, no more clients can connect (including legitimate ones).
The solution is to insist that clients complete handshake within a
reasonable time limit, defaulting to 10 seconds.  A client that has
not successfully completed NBD_OPT_GO by then (including the case of
where the client didn't know TLS credentials to even reach the point
of NBD_OPT_GO) is wasting our time and does not deserve to stay
connected.  Later patches will allow fine-tuning the limit away from
the default value (including disabling it for doing integration
testing of the handshake process itself).

Note that this patch in isolation actually makes it more likely to see
qemu SEGV after nbd-server-stop, as any client socket still connected
when the server shuts down will now be closed after 10 seconds rather
than at the client's whims.  That will be addressed in the next patch.

For a demo of this patch in action:
$ qemu-nbd -f raw -r -t -e 10 file &
$ nbdsh --opt-mode -c '
H = list()
for i in range(20):
  print(i)
  H.insert(i, nbd.NBD())
  H[i].set_opt_mode(True)
  H[i].connect_uri("nbd://localhost")
'

where later connections get to start progressing once earlier ones are
forcefully dropped for taking too long, rather than hanging.

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c     | 31 ++++++++++++++++++++++++++++++-
 nbd/trace-events |  1 +
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/nbd/server.c b/nbd/server.c
index 31b77bf0d4f..a470052d957 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -132,6 +132,7 @@ struct NBDClient {
     QCryptoTLSCreds *tlscreds;
     char *tlsauthz;
     uint32_t handshake_limit;
+    QEMUTimer *handshake_timer;
     QIOChannelSocket *sioc; /* The underlying data channel */
     QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */

@@ -3186,6 +3187,14 @@ static void nbd_client_receive_next_request(NBDClient *client)
     }
 }

+static void nbd_handshake_timer_cb(void *opaque)
+{
+    QIOChannel *ioc = opaque;
+
+    trace_nbd_handshake_timer_cb();
+    qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
+}
+
 static coroutine_fn void nbd_co_client_start(void *opaque)
 {
     NBDClient *client = opaque;
@@ -3193,15 +3202,35 @@ static coroutine_fn void nbd_co_client_start(void *opaque)

     qemu_co_mutex_init(&client->send_lock);

-    /* TODO - utilize client->handshake_limit */
+    /*
+     * Create a timer to bound the time spent in negotiation. If the
+     * timer expires, it is likely nbd_negotiate will fail because the
+     * socket was shutdown.
+     */
+    client->handshake_timer = aio_timer_new(qemu_get_aio_context(),
+                                            QEMU_CLOCK_REALTIME,
+                                            SCALE_NS,
+                                            nbd_handshake_timer_cb,
+                                            client->sioc);
+    if (client->handshake_limit > 0) {
+        timer_mod(client->handshake_timer,
+                  qemu_clock_get_ns(QEMU_CLOCK_REALTIME) +
+                  client->handshake_limit * NANOSECONDS_PER_SECOND);
+    }
+
     if (nbd_negotiate(client, &local_err)) {
         if (local_err) {
             error_report_err(local_err);
         }
+        timer_free(client->handshake_timer);
+        client->handshake_timer = NULL;
         client_close(client, false);
         return;
     }

+    timer_free(client->handshake_timer);
+    client->handshake_timer = NULL;
+
     WITH_QEMU_LOCK_GUARD(&client->lock) {
         nbd_client_receive_next_request(client);
     }
diff --git a/nbd/trace-events b/nbd/trace-events
index 00ae3216a11..cbd0a4ab7e4 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -76,6 +76,7 @@ nbd_co_receive_request_payload_received(uint64_t cookie, uint64_t len) "Payload
 nbd_co_receive_ext_payload_compliance(uint64_t from, uint64_t len) "client sent non-compliant write without payload flag: from=0x%" PRIx64 ", len=0x%" PRIx64
 nbd_co_receive_align_compliance(const char *op, uint64_t from, uint64_t len, uint32_t align) "client sent non-compliant unaligned %s request: from=0x%" PRIx64 ", len=0x%" PRIx64 ", align=0x%" PRIx32
 nbd_trip(void) "Reading request"
+nbd_handshake_timer_cb(void) "client took too long to negotiate"

 # client-connection.c
 nbd_connect_thread_sleep(uint64_t timeout) "timeout %" PRIu64
-- 
2.45.2

Re: [PATCH v4 4/7] nbd/server: CVE-2024-7409: Drop non-negotiating clients

[Daniel P. Berrangé]

On Wed, Aug 07, 2024 at 12:43:30PM -0500, Eric Blake wrote:
> A client that opens a socket but does not negotiate is merely hogging
> qemu's resources (an open fd and a small amount of memory); and a
> malicious client that can access the port where NBD is listening can
> attempt a denial of service attack by intentionally opening and
> abandoning lots of unfinished connections.  The previous patch put a
> default bound on the number of such ongoing connections, but once that
> limit is hit, no more clients can connect (including legitimate ones).
> The solution is to insist that clients complete handshake within a
> reasonable time limit, defaulting to 10 seconds.  A client that has
> not successfully completed NBD_OPT_GO by then (including the case of
> where the client didn't know TLS credentials to even reach the point
> of NBD_OPT_GO) is wasting our time and does not deserve to stay
> connected.  Later patches will allow fine-tuning the limit away from
> the default value (including disabling it for doing integration
> testing of the handshake process itself).
> 
> Note that this patch in isolation actually makes it more likely to see
> qemu SEGV after nbd-server-stop, as any client socket still connected
> when the server shuts down will now be closed after 10 seconds rather
> than at the client's whims.  That will be addressed in the next patch.
> 
> For a demo of this patch in action:
> $ qemu-nbd -f raw -r -t -e 10 file &
> $ nbdsh --opt-mode -c '
> H = list()
> for i in range(20):
>   print(i)
>   H.insert(i, nbd.NBD())
>   H[i].set_opt_mode(True)
>   H[i].connect_uri("nbd://localhost")
> '
> 
> where later connections get to start progressing once earlier ones are
> forcefully dropped for taking too long, rather than hanging.
> 
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  nbd/server.c     | 31 ++++++++++++++++++++++++++++++-
>  nbd/trace-events |  1 +
>  2 files changed, 31 insertions(+), 1 deletion(-)

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

[PATCH v4 5/7] nbd/server: CVE-2024-7409: Close stray client sockets at shutdown

[Eric Blake]

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

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
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 blockdev-nbd.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 19c57897819..4e38ff46747 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 limit as QMP option */
     nbd_client_new(cioc, NBD_DEFAULT_HANDSHAKE_LIMIT,
                    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.45.2

Re: [PATCH v4 5/7] nbd/server: CVE-2024-7409: Close stray client sockets at shutdown

[Daniel P. Berrangé]

On Wed, Aug 07, 2024 at 12:43:31PM -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 which 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.
> 
> 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
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  blockdev-nbd.c | 35 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)

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

Re: [PATCH v4 5/7] nbd/server: CVE-2024-7409: Close stray client sockets at shutdown

[Eric Blake]

On Wed, Aug 07, 2024 at 07:29:25PM GMT, Daniel P. Berrangé wrote:
> On Wed, Aug 07, 2024 at 12:43:31PM -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

I need to touch that sentence up to match the earlier patches.  (The
curse of rebasing late at night...)

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

I _think_ the effect is most likely to be an assertion failure
(nbd_server->connections > 0), since we recommend compiling qemu with
assertions enabled.  But "most likely" is not the same as "certainty".

> > 
> > 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
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> >  blockdev-nbd.c | 35 ++++++++++++++++++++++++++++++++++-
> >  1 file changed, 34 insertions(+), 1 deletion(-)
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Thanks for a speedy review.

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

[PATCH v4 6/7] qemu-nbd: Allow users to adjust handshake limit

[Eric Blake]

Although defaulting the handshake limit to 10 seconds was a nice QoI
change to weed out intentionally slow clients, it can interfere with
integration testing done with manual NBD_OPT commands over 'nbdsh
--opt-mode'.  Expose a command line option to allow the user to alter
the timeout away from the default.  This option is unlikely to be used
in enough scenarios to warrant a short option letter.

Signed-off-by: Eric Blake <eblake@redhat.com>

---

I'm not sure if this is 9.1 material.  It is a new feature
(user-visible command line option) implemented after soft freeze; on
the other hand, it allows one to recover the behavior that existed
prior to plugging the CVE which may be useful in integration testing.
---
 docs/tools/qemu-nbd.rst |  5 +++++
 qemu-nbd.c              | 41 ++++++++++++++++++++++++++---------------
 2 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/docs/tools/qemu-nbd.rst b/docs/tools/qemu-nbd.rst
index 329f44d9895..f55c10eb39c 100644
--- a/docs/tools/qemu-nbd.rst
+++ b/docs/tools/qemu-nbd.rst
@@ -154,6 +154,11 @@ driver options if :option:`--image-opts` is specified.
   Set the NBD volume export description, as a human-readable
   string.

+.. option:: --handshake-limit=N
+
+  Set the timeout for a client to successfully complete its handshake
+  to N seconds (default 10), or 0 for no limit.
+
 .. option:: -L, --list

   Connect as a client and list all details about the exports exposed by
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 7bf86a6566b..4287a595d92 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -57,19 +57,20 @@
 #define HAVE_NBD_DEVICE 0
 #endif

-#define SOCKET_PATH                "/var/lock/qemu-nbd-%s"
-#define QEMU_NBD_OPT_CACHE         256
-#define QEMU_NBD_OPT_AIO           257
-#define QEMU_NBD_OPT_DISCARD       258
-#define QEMU_NBD_OPT_DETECT_ZEROES 259
-#define QEMU_NBD_OPT_OBJECT        260
-#define QEMU_NBD_OPT_TLSCREDS      261
-#define QEMU_NBD_OPT_IMAGE_OPTS    262
-#define QEMU_NBD_OPT_FORK          263
-#define QEMU_NBD_OPT_TLSAUTHZ      264
-#define QEMU_NBD_OPT_PID_FILE      265
-#define QEMU_NBD_OPT_SELINUX_LABEL 266
-#define QEMU_NBD_OPT_TLSHOSTNAME   267
+#define SOCKET_PATH                  "/var/lock/qemu-nbd-%s"
+#define QEMU_NBD_OPT_CACHE           256
+#define QEMU_NBD_OPT_AIO             257
+#define QEMU_NBD_OPT_DISCARD         258
+#define QEMU_NBD_OPT_DETECT_ZEROES   259
+#define QEMU_NBD_OPT_OBJECT          260
+#define QEMU_NBD_OPT_TLSCREDS        261
+#define QEMU_NBD_OPT_IMAGE_OPTS      262
+#define QEMU_NBD_OPT_FORK            263
+#define QEMU_NBD_OPT_TLSAUTHZ        264
+#define QEMU_NBD_OPT_PID_FILE        265
+#define QEMU_NBD_OPT_SELINUX_LABEL   266
+#define QEMU_NBD_OPT_TLSHOSTNAME     267
+#define QEMU_NBD_OPT_HANDSHAKE_LIMIT 268

 #define MBR_SIZE 512

@@ -80,6 +81,7 @@ static int nb_fds;
 static QIONetListener *server;
 static QCryptoTLSCreds *tlscreds;
 static const char *tlsauthz;
+static int handshake_limit = NBD_DEFAULT_HANDSHAKE_LIMIT;

 static void usage(const char *name)
 {
@@ -101,6 +103,7 @@ static void usage(const char *name)
 "  -v, --verbose             display extra debugging information\n"
 "  -x, --export-name=NAME    expose export by name (default is empty string)\n"
 "  -D, --description=TEXT    export a human-readable description\n"
+"      --handshake-limit=N   limit client's handshake to N seconds (default 10)\n"
 "\n"
 "Exposing part of the image:\n"
 "  -o, --offset=OFFSET       offset into the image\n"
@@ -390,8 +393,7 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc,

     nb_fds++;
     nbd_update_server_watch();
-    /* TODO - expose handshake limit as command line option */
-    nbd_client_new(cioc, NBD_DEFAULT_HANDSHAKE_LIMIT,
+    nbd_client_new(cioc, handshake_limit,
                    tlscreds, tlsauthz, nbd_client_closed, NULL);
 }

@@ -569,6 +571,8 @@ int main(int argc, char **argv)
         { "object", required_argument, NULL, QEMU_NBD_OPT_OBJECT },
         { "export-name", required_argument, NULL, 'x' },
         { "description", required_argument, NULL, 'D' },
+        { "handshake-limit", required_argument, NULL,
+          QEMU_NBD_OPT_HANDSHAKE_LIMIT },
         { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },
         { "tls-hostname", required_argument, NULL, QEMU_NBD_OPT_TLSHOSTNAME },
         { "tls-authz", required_argument, NULL, QEMU_NBD_OPT_TLSAUTHZ },
@@ -815,6 +819,13 @@ int main(int argc, char **argv)
         case QEMU_NBD_OPT_SELINUX_LABEL:
             selinux_label = optarg;
             break;
+        case QEMU_NBD_OPT_HANDSHAKE_LIMIT:
+            if (qemu_strtoi(optarg, NULL, 0, &handshake_limit) < 0 ||
+                handshake_limit < 0) {
+                error_report("Invalid handshake limit '%s'", optarg);
+                exit(EXIT_FAILURE);
+            }
+            break;
         }
     }

-- 
2.45.2

[PATCH v4 7/7] nbd/server: Allow users to adjust handshake limit in QMP

[Eric Blake]

Although defaulting the handshake limit to 10 seconds was a nice QoI
change to weed out intentionally slow clients, it can interfere with
integration testing done with manual NBD_OPT commands over 'nbdsh
--opt-mode'.  Expose a QMP knob to allow the user to alter the timeout
away from the default.

Signed-off-by: Eric Blake <eblake@redhat.com>

---

I'm not sure if this is 9.1 material.  It is a new feature
(user-visible QMP addition) implemented after soft freeze; on the
other hand, it allows one to recover the behavior that existed prior
to plugging the CVE which may be useful in integration testing.
---
 qapi/block-export.json         | 14 ++++++++++++--
 include/block/nbd.h            |  2 +-
 block/monitor/block-hmp-cmds.c |  2 +-
 blockdev-nbd.c                 | 19 ++++++++++++++-----
 4 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/qapi/block-export.json b/qapi/block-export.json
index ce33fe378df..b485b380f1a 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -30,13 +30,18 @@
 #     server from advertising multiple client support (since 5.2;
 #     default: 100)
 #
+# @handshake-limit: Time limit, in seconds, at which a client that
+#     has not completed the negotiation handshake will be disconnected,
+#     or 0 for no limit (since 9.1; default: 10).
+#
 # Since: 4.2
 ##
 { 'struct': 'NbdServerOptions',
   'data': { 'addr': 'SocketAddress',
             '*tls-creds': 'str',
             '*tls-authz': 'str',
-            '*max-connections': 'uint32' } }
+            '*max-connections': 'uint32',
+            '*handshake-limit': 'uint32' } }

 ##
 # @nbd-server-start:
@@ -65,6 +70,10 @@
 #     server from advertising multiple client support (since 5.2;
 #     default: 100).
 #
+# @handshake-limit: Time limit, in seconds, at which a client that
+#     has not completed the negotiation handshake will be disconnected,
+#     or 0 for no limit (since 9.1; default: 10).
+#
 # Errors:
 #     - if the server is already running
 #
@@ -74,7 +83,8 @@
   'data': { 'addr': 'SocketAddressLegacy',
             '*tls-creds': 'str',
             '*tls-authz': 'str',
-            '*max-connections': 'uint32' },
+            '*max-connections': 'uint32',
+            '*handshake-limit': 'uint32' },
   'allow-preconfig': true }

 ##
diff --git a/include/block/nbd.h b/include/block/nbd.h
index fd5044359dc..22071aea797 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -430,7 +430,7 @@ bool nbd_server_is_running(void);
 int nbd_server_max_connections(void);
 void nbd_server_start(SocketAddress *addr, const char *tls_creds,
                       const char *tls_authz, uint32_t max_connections,
-                      Error **errp);
+                      uint32_t handshake_limit, Error **errp);
 void nbd_server_start_options(NbdServerOptions *arg, Error **errp);

 /* nbd_read
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index bdf2eb50b68..a258e029f78 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -403,7 +403,7 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
     }

     nbd_server_start(addr, NULL, NULL, NBD_DEFAULT_MAX_CONNECTIONS,
-                     &local_err);
+                     NBD_DEFAULT_HANDSHAKE_LIMIT, &local_err);
     qapi_free_SocketAddress(addr);
     if (local_err != NULL) {
         goto exit;
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 4e38ff46747..ad21bd1412d 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -30,6 +30,7 @@ typedef struct NBDServerData {
     QIONetListener *listener;
     QCryptoTLSCreds *tlscreds;
     char *tlsauthz;
+    uint32_t handshake_limit;
     uint32_t max_connections;
     uint32_t connections;
     QLIST_HEAD(, NBDConn) conns;
@@ -84,8 +85,7 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc,
     nbd_update_server_watch(nbd_server);

     qio_channel_set_name(QIO_CHANNEL(cioc), "nbd-server");
-    /* TODO - expose handshake limit as QMP option */
-    nbd_client_new(cioc, NBD_DEFAULT_HANDSHAKE_LIMIT,
+    nbd_client_new(cioc, nbd_server->handshake_limit,
                    nbd_server->tlscreds, nbd_server->tlsauthz,
                    nbd_blockdev_client_closed, conn);
 }
@@ -160,7 +160,7 @@ static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp)

 void nbd_server_start(SocketAddress *addr, const char *tls_creds,
                       const char *tls_authz, uint32_t max_connections,
-                      Error **errp)
+                      uint32_t handshake_limit, Error **errp)
 {
     if (nbd_server) {
         error_setg(errp, "NBD server already running");
@@ -169,6 +169,7 @@ void nbd_server_start(SocketAddress *addr, const char *tls_creds,

     nbd_server = g_new0(NBDServerData, 1);
     nbd_server->max_connections = max_connections;
+    nbd_server->handshake_limit = handshake_limit;
     nbd_server->listener = qio_net_listener_new();

     qio_net_listener_set_name(nbd_server->listener,
@@ -206,15 +207,19 @@ void nbd_server_start_options(NbdServerOptions *arg, Error **errp)
     if (!arg->has_max_connections) {
         arg->max_connections = NBD_DEFAULT_MAX_CONNECTIONS;
     }
+    if (!arg->has_handshake_limit) {
+        arg->handshake_limit = NBD_DEFAULT_HANDSHAKE_LIMIT;
+    }

     nbd_server_start(arg->addr, arg->tls_creds, arg->tls_authz,
-                     arg->max_connections, errp);
+                     arg->max_connections, arg->handshake_limit, errp);
 }

 void qmp_nbd_server_start(SocketAddressLegacy *addr,
                           const char *tls_creds,
                           const char *tls_authz,
                           bool has_max_connections, uint32_t max_connections,
+                          bool has_handshake_limit, uint32_t handshake_limit,
                           Error **errp)
 {
     SocketAddress *addr_flat = socket_address_flatten(addr);
@@ -222,8 +227,12 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr,
     if (!has_max_connections) {
         max_connections = NBD_DEFAULT_MAX_CONNECTIONS;
     }
+    if (!has_handshake_limit) {
+        handshake_limit = NBD_DEFAULT_HANDSHAKE_LIMIT;
+    }

-    nbd_server_start(addr_flat, tls_creds, tls_authz, max_connections, errp);
+    nbd_server_start(addr_flat, tls_creds, tls_authz, max_connections,
+                     handshake_limit, errp);
     qapi_free_SocketAddress(addr_flat);
 }

-- 
2.45.2

Re: [PATCH for-9.1 v4 0/7] CVE-2024-7409

[Denis V. Lunev]

On 8/7/24 19:43, Eric Blake wrote:
> v3 was here:
> https://lists.gnu.org/archive/html/qemu-devel/2024-08/msg00818.html
>
> since then:
>   - re-add a minor patch from v2 (now patch 1)
>   - refactor how the client opaque pointer is handled (patch 2)
>   - add two new patches to prevent malicious clients from consuming
>     inordinate resources: change the default max-connections from
>     unlimited to capped at 100 (patch 3), and add code to kill any
>     client that takes longer than 10 seconds after connect to reach
>     NBD_OPT_GO (patch 4) [Dan]
>   - squash the connection list handling into a single patch (5) [Dan]
>   - two new additional patches for reverting back to 9.0 behavior for
>     integration testing purposes; I'm okay if these last two miss 9.1
>
> Eric Blake (7):
>    nbd: Minor style fixes
>    nbd/server: Plumb in new args to nbd_client_add()
>    nbd/server: CVE-2024-7409: Change default max-connections to 100
>    nbd/server: CVE-2024-7409: Drop non-negotiating clients
>    nbd/server: CVE-2024-7409: Close stray client sockets at shutdown
>    qemu-nbd: Allow users to adjust handshake limit
>    nbd/server: Allow users to adjust handshake limit in QMP
>
>   docs/tools/qemu-nbd.rst        |  5 +++
>   qapi/block-export.json         | 18 +++++++---
>   include/block/nbd.h            | 20 +++++++++--
>   block/monitor/block-hmp-cmds.c |  3 +-
>   blockdev-nbd.c                 | 62 +++++++++++++++++++++++++++++++---
>   nbd/server.c                   | 51 +++++++++++++++++++++++++---
>   qemu-nbd.c                     | 44 ++++++++++++++++--------
>   nbd/trace-events               |  1 +
>   8 files changed, 173 insertions(+), 31 deletions(-)
>
should this go to stable too? 7.5 score is high enough.
We have had CVE-2024-4467 
<https://security-tracker.debian.org/tracker/CVE-2024-4467> with 7.8 
merged to stables too.

Den