diff options
author | Hitendra Prajapati <hprajapati@mvista.com> | 2024-09-03 13:38:34 +0530 |
---|---|---|
committer | Steve Sakoman <steve@sakoman.com> | 2024-09-07 05:38:17 -0700 |
commit | a952dec3e479f50a645faaf7258f5322c1767e42 (patch) | |
tree | 6f88d48d55d8a2121ab23f22cd1cce38ab93f711 | |
parent | 0ae3b2bd492d32ba401a447f09679ebaf10ddad4 (diff) | |
download | poky-a952dec3e479f50a645faaf7258f5322c1767e42.tar.gz |
qemu: fix CVE-2024-7409
A flaw was found in the QEMU NBD Server. This vulnerability allows a denial of service (DoS) attack
via improper synchronization during socket closure when a client keeps a socket open as the server
is taken offline.
Reference:
https://nvd.nist.gov/vuln/detail/CVE-2024-7409
Upstream Patches:
https://github.com/qemu/qemu/commit/fb1c2aaa981e0a2fa6362c9985f1296b74f055ac
https://github.com/qemu/qemu/commit/c8a76dbd90c2f48df89b75bef74917f90a59b623
https://gitlab.com/qemu-project/qemu/-/commit/b9b72cb3ce15b693148bd09cef7e50110566d8a0
https://gitlab.com/qemu-project/qemu/-/commit/3e7ef738c8462c45043a1d39f702a0990406a3b3
(From OE-Core rev: d84ab04dc66cb83638f96fcd2f4c67e67489c410)
Signed-off-by: Hitendra Prajapati <hprajapati@mvista.com>
Signed-off-by: Steve Sakoman <steve@sakoman.com>
-rw-r--r-- | meta/recipes-devtools/qemu/qemu.inc | 4 | ||||
-rw-r--r-- | meta/recipes-devtools/qemu/qemu/CVE-2024-7409-0001.patch | 162 | ||||
-rw-r--r-- | meta/recipes-devtools/qemu/qemu/CVE-2024-7409-0002.patch | 174 | ||||
-rw-r--r-- | meta/recipes-devtools/qemu/qemu/CVE-2024-7409-0003.patch | 122 | ||||
-rw-r--r-- | meta/recipes-devtools/qemu/qemu/CVE-2024-7409-0004.patch | 163 |
5 files changed, 625 insertions, 0 deletions
diff --git a/meta/recipes-devtools/qemu/qemu.inc b/meta/recipes-devtools/qemu/qemu.inc index 4747310ae4..4684e44524 100644 --- a/meta/recipes-devtools/qemu/qemu.inc +++ b/meta/recipes-devtools/qemu/qemu.inc | |||
@@ -109,6 +109,10 @@ SRC_URI = "https://download.qemu.org/${BPN}-${PV}.tar.xz \ | |||
109 | file://scsi-disk-ensure-block-size-is-non-zero-and-changes-limited-to-bits-8-15.patch \ | 109 | file://scsi-disk-ensure-block-size-is-non-zero-and-changes-limited-to-bits-8-15.patch \ |
110 | file://CVE-2023-42467.patch \ | 110 | file://CVE-2023-42467.patch \ |
111 | file://CVE-2023-6683.patch \ | 111 | file://CVE-2023-6683.patch \ |
112 | file://CVE-2024-7409-0001.patch \ | ||
113 | file://CVE-2024-7409-0002.patch \ | ||
114 | file://CVE-2024-7409-0003.patch \ | ||
115 | file://CVE-2024-7409-0004.patch \ | ||
112 | " | 116 | " |
113 | UPSTREAM_CHECK_REGEX = "qemu-(?P<pver>\d+(\.\d+)+)\.tar" | 117 | UPSTREAM_CHECK_REGEX = "qemu-(?P<pver>\d+(\.\d+)+)\.tar" |
114 | 118 | ||
diff --git a/meta/recipes-devtools/qemu/qemu/CVE-2024-7409-0001.patch b/meta/recipes-devtools/qemu/qemu/CVE-2024-7409-0001.patch new file mode 100644 index 0000000000..f4dad65097 --- /dev/null +++ b/meta/recipes-devtools/qemu/qemu/CVE-2024-7409-0001.patch | |||
@@ -0,0 +1,162 @@ | |||
1 | From fb1c2aaa981e0a2fa6362c9985f1296b74f055ac Mon Sep 17 00:00:00 2001 | ||
2 | From: Eric Blake <eblake@redhat.com> | ||
3 | Date: Wed, 7 Aug 2024 08:50:01 -0500 | ||
4 | Subject: [PATCH] nbd/server: Plumb in new args to nbd_client_add() | ||
5 | |||
6 | Upcoming patches to fix a CVE need to track an opaque pointer passed | ||
7 | in by the owner of a client object, as well as request for a time | ||
8 | limit on how fast negotiation must complete. Prepare for that by | ||
9 | changing the signature of nbd_client_new() and adding an accessor to | ||
10 | get at the opaque pointer, although for now the two servers | ||
11 | (qemu-nbd.c and blockdev-nbd.c) do not change behavior even though | ||
12 | they pass in a new default timeout value. | ||
13 | |||
14 | Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> | ||
15 | Signed-off-by: Eric Blake <eblake@redhat.com> | ||
16 | Message-ID: <20240807174943.771624-11-eblake@redhat.com> | ||
17 | Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> | ||
18 | [eblake: s/LIMIT/MAX_SECS/ as suggested by Dan] | ||
19 | Signed-off-by: Eric Blake <eblake@redhat.com> | ||
20 | |||
21 | CVE: CVE-2024-7409 | ||
22 | Upstream-Status: Backport [https://github.com/qemu/qemu/commit/fb1c2aaa981e0a2fa6362c9985f1296b74f055ac] | ||
23 | Signed-off-by: Hitendra Prajapati <hprajapati@mvista.com> | ||
24 | --- | ||
25 | blockdev-nbd.c | 6 ++++-- | ||
26 | include/block/nbd.h | 11 ++++++++++- | ||
27 | nbd/server.c | 20 +++++++++++++++++--- | ||
28 | qemu-nbd.c | 4 +++- | ||
29 | 4 files changed, 34 insertions(+), 7 deletions(-) | ||
30 | |||
31 | diff --git a/blockdev-nbd.c b/blockdev-nbd.c | ||
32 | index bdfa7ed3a..b9e8dc78f 100644 | ||
33 | --- a/blockdev-nbd.c | ||
34 | +++ b/blockdev-nbd.c | ||
35 | @@ -59,8 +59,10 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc, | ||
36 | nbd_update_server_watch(nbd_server); | ||
37 | |||
38 | qio_channel_set_name(QIO_CHANNEL(cioc), "nbd-server"); | ||
39 | - nbd_client_new(cioc, nbd_server->tlscreds, nbd_server->tlsauthz, | ||
40 | - nbd_blockdev_client_closed); | ||
41 | + /* TODO - expose handshake timeout as QMP option */ | ||
42 | + nbd_client_new(cioc, NBD_DEFAULT_HANDSHAKE_MAX_SECS, | ||
43 | + nbd_server->tlscreds, nbd_server->tlsauthz, | ||
44 | + nbd_blockdev_client_closed, NULL); | ||
45 | } | ||
46 | |||
47 | static void nbd_update_server_watch(NBDServerData *s) | ||
48 | diff --git a/include/block/nbd.h b/include/block/nbd.h | ||
49 | index 78d101b77..b71a29724 100644 | ||
50 | --- a/include/block/nbd.h | ||
51 | +++ b/include/block/nbd.h | ||
52 | @@ -27,6 +27,12 @@ | ||
53 | |||
54 | extern const BlockExportDriver blk_exp_nbd; | ||
55 | |||
56 | +/* | ||
57 | + * NBD_DEFAULT_HANDSHAKE_MAX_SECS: Number of seconds in which client must | ||
58 | + * succeed at NBD_OPT_GO before being forcefully dropped as too slow. | ||
59 | + */ | ||
60 | +#define NBD_DEFAULT_HANDSHAKE_MAX_SECS 10 | ||
61 | + | ||
62 | /* Handshake phase structs - this struct is passed on the wire */ | ||
63 | |||
64 | struct NBDOption { | ||
65 | @@ -338,9 +344,12 @@ AioContext *nbd_export_aio_context(NBDExport *exp); | ||
66 | NBDExport *nbd_export_find(const char *name); | ||
67 | |||
68 | void nbd_client_new(QIOChannelSocket *sioc, | ||
69 | + uint32_t handshake_max_secs, | ||
70 | QCryptoTLSCreds *tlscreds, | ||
71 | const char *tlsauthz, | ||
72 | - void (*close_fn)(NBDClient *, bool)); | ||
73 | + void (*close_fn)(NBDClient *, bool), | ||
74 | + void *owner); | ||
75 | +void *nbd_client_owner(NBDClient *client); | ||
76 | void nbd_client_get(NBDClient *client); | ||
77 | void nbd_client_put(NBDClient *client); | ||
78 | |||
79 | diff --git a/nbd/server.c b/nbd/server.c | ||
80 | index 4630dd732..12680c8dc 100644 | ||
81 | --- a/nbd/server.c | ||
82 | +++ b/nbd/server.c | ||
83 | @@ -121,9 +121,11 @@ struct NBDClient { | ||
84 | int refcount; | ||
85 | void (*close_fn)(NBDClient *client, bool negotiated); | ||
86 | |||
87 | + void *owner; | ||
88 | NBDExport *exp; | ||
89 | QCryptoTLSCreds *tlscreds; | ||
90 | char *tlsauthz; | ||
91 | + uint32_t handshake_max_secs; | ||
92 | QIOChannelSocket *sioc; /* The underlying data channel */ | ||
93 | QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */ | ||
94 | |||
95 | @@ -2703,6 +2705,7 @@ static coroutine_fn void nbd_co_client_start(void *opaque) | ||
96 | |||
97 | qemu_co_mutex_init(&client->send_lock); | ||
98 | |||
99 | + /* TODO - utilize client->handshake_max_secs */ | ||
100 | if (nbd_negotiate(client, &local_err)) { | ||
101 | if (local_err) { | ||
102 | error_report_err(local_err); | ||
103 | @@ -2715,14 +2718,17 @@ static coroutine_fn void nbd_co_client_start(void *opaque) | ||
104 | } | ||
105 | |||
106 | /* | ||
107 | - * Create a new client listener using the given channel @sioc. | ||
108 | + * Create a new client listener using the given channel @sioc and @owner. | ||
109 | * Begin servicing it in a coroutine. When the connection closes, call | ||
110 | - * @close_fn with an indication of whether the client completed negotiation. | ||
111 | + * @close_fn with an indication of whether the client completed negotiation | ||
112 | + * within @handshake_max_secs seconds (0 for unbounded). | ||
113 | */ | ||
114 | void nbd_client_new(QIOChannelSocket *sioc, | ||
115 | + uint32_t handshake_max_secs, | ||
116 | QCryptoTLSCreds *tlscreds, | ||
117 | const char *tlsauthz, | ||
118 | - void (*close_fn)(NBDClient *, bool)) | ||
119 | + void (*close_fn)(NBDClient *, bool), | ||
120 | + void *owner) | ||
121 | { | ||
122 | NBDClient *client; | ||
123 | Coroutine *co; | ||
124 | @@ -2734,12 +2740,20 @@ void nbd_client_new(QIOChannelSocket *sioc, | ||
125 | object_ref(OBJECT(client->tlscreds)); | ||
126 | } | ||
127 | client->tlsauthz = g_strdup(tlsauthz); | ||
128 | + client->handshake_max_secs = handshake_max_secs; | ||
129 | client->sioc = sioc; | ||
130 | object_ref(OBJECT(client->sioc)); | ||
131 | client->ioc = QIO_CHANNEL(sioc); | ||
132 | object_ref(OBJECT(client->ioc)); | ||
133 | client->close_fn = close_fn; | ||
134 | + client->owner = owner; | ||
135 | |||
136 | co = qemu_coroutine_create(nbd_co_client_start, client); | ||
137 | qemu_coroutine_enter(co); | ||
138 | } | ||
139 | + | ||
140 | +void * | ||
141 | +nbd_client_owner(NBDClient *client) | ||
142 | +{ | ||
143 | + return client->owner; | ||
144 | +} | ||
145 | diff --git a/qemu-nbd.c b/qemu-nbd.c | ||
146 | index c6c20df68..f48abf379 100644 | ||
147 | --- a/qemu-nbd.c | ||
148 | +++ b/qemu-nbd.c | ||
149 | @@ -363,7 +363,9 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc, | ||
150 | |||
151 | nb_fds++; | ||
152 | nbd_update_server_watch(); | ||
153 | - nbd_client_new(cioc, tlscreds, tlsauthz, nbd_client_closed); | ||
154 | + /* TODO - expose handshake timeout as command line option */ | ||
155 | + nbd_client_new(cioc, NBD_DEFAULT_HANDSHAKE_MAX_SECS, | ||
156 | + tlscreds, tlsauthz, nbd_client_closed, NULL); | ||
157 | } | ||
158 | |||
159 | static void nbd_update_server_watch(void) | ||
160 | -- | ||
161 | 2.25.1 | ||
162 | |||
diff --git a/meta/recipes-devtools/qemu/qemu/CVE-2024-7409-0002.patch b/meta/recipes-devtools/qemu/qemu/CVE-2024-7409-0002.patch new file mode 100644 index 0000000000..ccef8b36c5 --- /dev/null +++ b/meta/recipes-devtools/qemu/qemu/CVE-2024-7409-0002.patch | |||
@@ -0,0 +1,174 @@ | |||
1 | From c8a76dbd90c2f48df89b75bef74917f90a59b623 Mon Sep 17 00:00:00 2001 | ||
2 | From: Eric Blake <eblake@redhat.com> | ||
3 | Date: Tue, 6 Aug 2024 13:53:00 -0500 | ||
4 | Subject: [PATCH] nbd/server: CVE-2024-7409: Cap default max-connections to 100 | ||
5 | |||
6 | Allowing an unlimited number of clients to any web service is a recipe | ||
7 | for a rudimentary denial of service attack: the client merely needs to | ||
8 | open lots of sockets without closing them, until qemu no longer has | ||
9 | any more fds available to allocate. | ||
10 | |||
11 | For qemu-nbd, we default to allowing only 1 connection unless more are | ||
12 | explicitly asked for (-e or --shared); this was historically picked as | ||
13 | a nice default (without an explicit -t, a non-persistent qemu-nbd goes | ||
14 | away after a client disconnects, without needing any additional | ||
15 | follow-up commands), and we are not going to change that interface now | ||
16 | (besides, someday we want to point people towards qemu-storage-daemon | ||
17 | instead of qemu-nbd). | ||
18 | |||
19 | But for qemu proper, and the newer qemu-storage-daemon, the QMP | ||
20 | nbd-server-start command has historically had a default of unlimited | ||
21 | number of connections, in part because unlike qemu-nbd it is | ||
22 | inherently persistent until nbd-server-stop. Allowing multiple client | ||
23 | sockets is particularly useful for clients that can take advantage of | ||
24 | MULTI_CONN (creating parallel sockets to increase throughput), | ||
25 | although known clients that do so (such as libnbd's nbdcopy) typically | ||
26 | use only 8 or 16 connections (the benefits of scaling diminish once | ||
27 | more sockets are competing for kernel attention). Picking a number | ||
28 | large enough for typical use cases, but not unlimited, makes it | ||
29 | slightly harder for a malicious client to perform a denial of service | ||
30 | merely by opening lots of connections withot progressing through the | ||
31 | handshake. | ||
32 | |||
33 | This change does not eliminate CVE-2024-7409 on its own, but reduces | ||
34 | the chance for fd exhaustion or unlimited memory usage as an attack | ||
35 | surface. On the other hand, by itself, it makes it more obvious that | ||
36 | with a finite limit, we have the problem of an unauthenticated client | ||
37 | holding 100 fds opened as a way to block out a legitimate client from | ||
38 | being able to connect; thus, later patches will further add timeouts | ||
39 | to reject clients that are not making progress. | ||
40 | |||
41 | This is an INTENTIONAL change in behavior, and will break any client | ||
42 | of nbd-server-start that was not passing an explicit max-connections | ||
43 | parameter, yet expects more than 100 simultaneous connections. We are | ||
44 | not aware of any such client (as stated above, most clients aware of | ||
45 | MULTI_CONN get by just fine on 8 or 16 connections, and probably cope | ||
46 | with later connections failing by relying on the earlier connections; | ||
47 | libvirt has not yet been passing max-connections, but generally | ||
48 | creates NBD servers with the intent for a single client for the sake | ||
49 | of live storage migration; meanwhile, the KubeSAN project anticipates | ||
50 | a large cluster sharing multiple clients [up to 8 per node, and up to | ||
51 | 100 nodes in a cluster], but it currently uses qemu-nbd with an | ||
52 | explicit --shared=0 rather than qemu-storage-daemon with | ||
53 | nbd-server-start). | ||
54 | |||
55 | We considered using a deprecation period (declare that omitting | ||
56 | max-parameters is deprecated, and make it mandatory in 3 releases - | ||
57 | then we don't need to pick an arbitrary default); that has zero risk | ||
58 | of breaking any apps that accidentally depended on more than 100 | ||
59 | connections, and where such breakage might not be noticed under unit | ||
60 | testing but only under the larger loads of production usage. But it | ||
61 | does not close the denial-of-service hole until far into the future, | ||
62 | and requires all apps to change to add the parameter even if 100 was | ||
63 | good enough. It also has a drawback that any app (like libvirt) that | ||
64 | is accidentally relying on an unlimited default should seriously | ||
65 | consider their own CVE now, at which point they are going to change to | ||
66 | pass explicit max-connections sooner than waiting for 3 qemu releases. | ||
67 | Finally, if our changed default breaks an app, that app can always | ||
68 | pass in an explicit max-parameters with a larger value. | ||
69 | |||
70 | It is also intentional that the HMP interface to nbd-server-start is | ||
71 | not changed to expose max-connections (any client needing to fine-tune | ||
72 | things should be using QMP). | ||
73 | |||
74 | Suggested-by: Daniel P. Berrangé <berrange@redhat.com> | ||
75 | Signed-off-by: Eric Blake <eblake@redhat.com> | ||
76 | Message-ID: <20240807174943.771624-12-eblake@redhat.com> | ||
77 | Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> | ||
78 | [ericb: Expand commit message to summarize Dan's argument for why we | ||
79 | break corner-case back-compat behavior without a deprecation period] | ||
80 | Signed-off-by: Eric Blake <eblake@redhat.com> | ||
81 | |||
82 | CVE: CVE-2024-7409 | ||
83 | Upstream-Status: Backport [https://github.com/qemu/qemu/commit/c8a76dbd90c2f48df89b75bef74917f90a59b623] | ||
84 | Signed-off-by: Hitendra Prajapati <hprajapati@mvista.com> | ||
85 | --- | ||
86 | block/monitor/block-hmp-cmds.c | 3 ++- | ||
87 | blockdev-nbd.c | 8 ++++++++ | ||
88 | include/block/nbd.h | 7 +++++++ | ||
89 | qapi/block-export.json | 4 ++-- | ||
90 | 4 files changed, 19 insertions(+), 3 deletions(-) | ||
91 | |||
92 | diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c | ||
93 | index 2ac4aedff..32a666b5d 100644 | ||
94 | --- a/block/monitor/block-hmp-cmds.c | ||
95 | +++ b/block/monitor/block-hmp-cmds.c | ||
96 | @@ -411,7 +411,8 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict) | ||
97 | goto exit; | ||
98 | } | ||
99 | |||
100 | - nbd_server_start(addr, NULL, NULL, 0, &local_err); | ||
101 | + nbd_server_start(addr, NULL, NULL, NBD_DEFAULT_MAX_CONNECTIONS, | ||
102 | + &local_err); | ||
103 | qapi_free_SocketAddress(addr); | ||
104 | if (local_err != NULL) { | ||
105 | goto exit; | ||
106 | diff --git a/blockdev-nbd.c b/blockdev-nbd.c | ||
107 | index b9e8dc78f..4bd90bac1 100644 | ||
108 | --- a/blockdev-nbd.c | ||
109 | +++ b/blockdev-nbd.c | ||
110 | @@ -171,6 +171,10 @@ void nbd_server_start(SocketAddress *addr, const char *tls_creds, | ||
111 | |||
112 | void nbd_server_start_options(NbdServerOptions *arg, Error **errp) | ||
113 | { | ||
114 | + if (!arg->has_max_connections) { | ||
115 | + arg->max_connections = NBD_DEFAULT_MAX_CONNECTIONS; | ||
116 | + } | ||
117 | + | ||
118 | nbd_server_start(arg->addr, arg->tls_creds, arg->tls_authz, | ||
119 | arg->max_connections, errp); | ||
120 | } | ||
121 | @@ -183,6 +187,10 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr, | ||
122 | { | ||
123 | SocketAddress *addr_flat = socket_address_flatten(addr); | ||
124 | |||
125 | + if (!has_max_connections) { | ||
126 | + max_connections = NBD_DEFAULT_MAX_CONNECTIONS; | ||
127 | + } | ||
128 | + | ||
129 | nbd_server_start(addr_flat, tls_creds, tls_authz, max_connections, errp); | ||
130 | qapi_free_SocketAddress(addr_flat); | ||
131 | } | ||
132 | diff --git a/include/block/nbd.h b/include/block/nbd.h | ||
133 | index b71a29724..a31c34a8a 100644 | ||
134 | --- a/include/block/nbd.h | ||
135 | +++ b/include/block/nbd.h | ||
136 | @@ -33,6 +33,13 @@ extern const BlockExportDriver blk_exp_nbd; | ||
137 | */ | ||
138 | #define NBD_DEFAULT_HANDSHAKE_MAX_SECS 10 | ||
139 | |||
140 | +/* | ||
141 | + * NBD_DEFAULT_MAX_CONNECTIONS: Number of client sockets to allow at | ||
142 | + * once; must be large enough to allow a MULTI_CONN-aware client like | ||
143 | + * nbdcopy to create its typical number of 8-16 sockets. | ||
144 | + */ | ||
145 | +#define NBD_DEFAULT_MAX_CONNECTIONS 100 | ||
146 | + | ||
147 | /* Handshake phase structs - this struct is passed on the wire */ | ||
148 | |||
149 | struct NBDOption { | ||
150 | diff --git a/qapi/block-export.json b/qapi/block-export.json | ||
151 | index c1b92ce1c..181d7238f 100644 | ||
152 | --- a/qapi/block-export.json | ||
153 | +++ b/qapi/block-export.json | ||
154 | @@ -21,7 +21,7 @@ | ||
155 | # recreated on the fly while the NBD server is active. | ||
156 | # If missing, it will default to denying access (since 4.0). | ||
157 | # @max-connections: The maximum number of connections to allow at the same | ||
158 | -# time, 0 for unlimited. (since 5.2; default: 0) | ||
159 | +# time, 0 for unlimited. (since 5.2; default: 100) | ||
160 | # | ||
161 | # Since: 4.2 | ||
162 | ## | ||
163 | @@ -50,7 +50,7 @@ | ||
164 | # recreated on the fly while the NBD server is active. | ||
165 | # If missing, it will default to denying access (since 4.0). | ||
166 | # @max-connections: The maximum number of connections to allow at the same | ||
167 | -# time, 0 for unlimited. (since 5.2; default: 0) | ||
168 | +# time, 0 for unlimited. (since 5.2; default: 100) | ||
169 | # | ||
170 | # Returns: error if the server is already running. | ||
171 | # | ||
172 | -- | ||
173 | 2.25.1 | ||
174 | |||
diff --git a/meta/recipes-devtools/qemu/qemu/CVE-2024-7409-0003.patch b/meta/recipes-devtools/qemu/qemu/CVE-2024-7409-0003.patch new file mode 100644 index 0000000000..1d27f4712c --- /dev/null +++ b/meta/recipes-devtools/qemu/qemu/CVE-2024-7409-0003.patch | |||
@@ -0,0 +1,122 @@ | |||
1 | From b9b72cb3ce15b693148bd09cef7e50110566d8a0 Mon Sep 17 00:00:00 2001 | ||
2 | From: Eric Blake <eblake@redhat.com> | ||
3 | Date: Thu, 8 Aug 2024 16:05:08 -0500 | ||
4 | Subject: [PATCH] nbd/server: CVE-2024-7409: Drop non-negotiating clients | ||
5 | |||
6 | A client that opens a socket but does not negotiate is merely hogging | ||
7 | qemu's resources (an open fd and a small amount of memory); and a | ||
8 | malicious client that can access the port where NBD is listening can | ||
9 | attempt a denial of service attack by intentionally opening and | ||
10 | abandoning lots of unfinished connections. The previous patch put a | ||
11 | default bound on the number of such ongoing connections, but once that | ||
12 | limit is hit, no more clients can connect (including legitimate ones). | ||
13 | The solution is to insist that clients complete handshake within a | ||
14 | reasonable time limit, defaulting to 10 seconds. A client that has | ||
15 | not successfully completed NBD_OPT_GO by then (including the case of | ||
16 | where the client didn't know TLS credentials to even reach the point | ||
17 | of NBD_OPT_GO) is wasting our time and does not deserve to stay | ||
18 | connected. Later patches will allow fine-tuning the limit away from | ||
19 | the default value (including disabling it for doing integration | ||
20 | testing of the handshake process itself). | ||
21 | |||
22 | Note that this patch in isolation actually makes it more likely to see | ||
23 | qemu SEGV after nbd-server-stop, as any client socket still connected | ||
24 | when the server shuts down will now be closed after 10 seconds rather | ||
25 | than at the client's whims. That will be addressed in the next patch. | ||
26 | |||
27 | For a demo of this patch in action: | ||
28 | $ qemu-nbd -f raw -r -t -e 10 file & | ||
29 | $ nbdsh --opt-mode -c ' | ||
30 | H = list() | ||
31 | for i in range(20): | ||
32 | print(i) | ||
33 | H.insert(i, nbd.NBD()) | ||
34 | H[i].set_opt_mode(True) | ||
35 | H[i].connect_uri("nbd://localhost") | ||
36 | ' | ||
37 | $ kill $! | ||
38 | |||
39 | where later connections get to start progressing once earlier ones are | ||
40 | forcefully dropped for taking too long, rather than hanging. | ||
41 | |||
42 | Suggested-by: Daniel P. Berrangé <berrange@redhat.com> | ||
43 | Signed-off-by: Eric Blake <eblake@redhat.com> | ||
44 | Message-ID: <20240807174943.771624-13-eblake@redhat.com> | ||
45 | Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> | ||
46 | [eblake: rebase to changes earlier in series, reduce scope of timer] | ||
47 | Signed-off-by: Eric Blake <eblake@redhat.com> | ||
48 | |||
49 | CVE: CVE-2024-7409 | ||
50 | Upstream-Status: Backport [https://gitlab.com/qemu-project/qemu/-/commit/b9b72cb3ce15b693148bd09cef7e50110566d8a0] | ||
51 | Signed-off-by: Hitendra Prajapati <hprajapati@mvista.com> | ||
52 | --- | ||
53 | nbd/server.c | 28 +++++++++++++++++++++++++++- | ||
54 | nbd/trace-events | 1 + | ||
55 | 2 files changed, 28 insertions(+), 1 deletion(-) | ||
56 | |||
57 | diff --git a/nbd/server.c b/nbd/server.c | ||
58 | index 12680c8dc..1bb253726 100644 | ||
59 | --- a/nbd/server.c | ||
60 | +++ b/nbd/server.c | ||
61 | @@ -2698,22 +2698,48 @@ static void nbd_client_receive_next_request(NBDClient *client) | ||
62 | } | ||
63 | } | ||
64 | |||
65 | +static void nbd_handshake_timer_cb(void *opaque) | ||
66 | +{ | ||
67 | + QIOChannel *ioc = opaque; | ||
68 | + | ||
69 | + trace_nbd_handshake_timer_cb(); | ||
70 | + qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL); | ||
71 | +} | ||
72 | + | ||
73 | static coroutine_fn void nbd_co_client_start(void *opaque) | ||
74 | { | ||
75 | NBDClient *client = opaque; | ||
76 | Error *local_err = NULL; | ||
77 | + QEMUTimer *handshake_timer = NULL; | ||
78 | |||
79 | qemu_co_mutex_init(&client->send_lock); | ||
80 | |||
81 | - /* TODO - utilize client->handshake_max_secs */ | ||
82 | + /* | ||
83 | + * Create a timer to bound the time spent in negotiation. If the | ||
84 | + * timer expires, it is likely nbd_negotiate will fail because the | ||
85 | + * socket was shutdown. | ||
86 | + */ | ||
87 | + if (client->handshake_max_secs > 0) { | ||
88 | + handshake_timer = aio_timer_new(qemu_get_aio_context(), | ||
89 | + QEMU_CLOCK_REALTIME, | ||
90 | + SCALE_NS, | ||
91 | + nbd_handshake_timer_cb, | ||
92 | + client->sioc); | ||
93 | + timer_mod(handshake_timer, | ||
94 | + qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + | ||
95 | + client->handshake_max_secs * NANOSECONDS_PER_SECOND); | ||
96 | + } | ||
97 | + | ||
98 | if (nbd_negotiate(client, &local_err)) { | ||
99 | if (local_err) { | ||
100 | error_report_err(local_err); | ||
101 | } | ||
102 | + timer_free(handshake_timer); | ||
103 | client_close(client, false); | ||
104 | return; | ||
105 | } | ||
106 | |||
107 | + timer_free(handshake_timer); | ||
108 | nbd_client_receive_next_request(client); | ||
109 | } | ||
110 | |||
111 | diff --git a/nbd/trace-events b/nbd/trace-events | ||
112 | index c4919a2dd..553546f1f 100644 | ||
113 | --- a/nbd/trace-events | ||
114 | +++ b/nbd/trace-events | ||
115 | @@ -73,3 +73,4 @@ nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type, const char *n | ||
116 | nbd_co_receive_request_payload_received(uint64_t handle, uint32_t len) "Payload received: handle = %" PRIu64 ", len = %" PRIu32 | ||
117 | nbd_co_receive_align_compliance(const char *op, uint64_t from, uint32_t len, uint32_t align) "client sent non-compliant unaligned %s request: from=0x%" PRIx64 ", len=0x%" PRIx32 ", align=0x%" PRIx32 | ||
118 | nbd_trip(void) "Reading request" | ||
119 | +nbd_handshake_timer_cb(void) "client took too long to negotiate" | ||
120 | -- | ||
121 | 2.25.1 | ||
122 | |||
diff --git a/meta/recipes-devtools/qemu/qemu/CVE-2024-7409-0004.patch b/meta/recipes-devtools/qemu/qemu/CVE-2024-7409-0004.patch new file mode 100644 index 0000000000..ffdb1b0d94 --- /dev/null +++ b/meta/recipes-devtools/qemu/qemu/CVE-2024-7409-0004.patch | |||
@@ -0,0 +1,163 @@ | |||
1 | From 3e7ef738c8462c45043a1d39f702a0990406a3b3 Mon Sep 17 00:00:00 2001 | ||
2 | From: Eric Blake <eblake@redhat.com> | ||
3 | Date: Wed, 7 Aug 2024 12:23:13 -0500 | ||
4 | Subject: [PATCH] nbd/server: CVE-2024-7409: Close stray clients at server-stop | ||
5 | |||
6 | A malicious client can attempt to connect to an NBD server, and then | ||
7 | intentionally delay progress in the handshake, including if it does | ||
8 | not know the TLS secrets. Although the previous two patches reduce | ||
9 | this behavior by capping the default max-connections parameter and | ||
10 | killing slow clients, they did not eliminate the possibility of a | ||
11 | client waiting to close the socket until after the QMP nbd-server-stop | ||
12 | command is executed, at which point qemu would SEGV when trying to | ||
13 | dereference the NULL nbd_server global which is no longer present. | ||
14 | This amounts to a denial of service attack. Worse, if another NBD | ||
15 | server is started before the malicious client disconnects, I cannot | ||
16 | rule out additional adverse effects when the old client interferes | ||
17 | with the connection count of the new server (although the most likely | ||
18 | is a crash due to an assertion failure when checking | ||
19 | nbd_server->connections > 0). | ||
20 | |||
21 | For environments without this patch, the CVE can be mitigated by | ||
22 | ensuring (such as via a firewall) that only trusted clients can | ||
23 | connect to an NBD server. Note that using frameworks like libvirt | ||
24 | that ensure that TLS is used and that nbd-server-stop is not executed | ||
25 | while any trusted clients are still connected will only help if there | ||
26 | is also no possibility for an untrusted client to open a connection | ||
27 | but then stall on the NBD handshake. | ||
28 | |||
29 | Given the previous patches, it would be possible to guarantee that no | ||
30 | clients remain connected by having nbd-server-stop sleep for longer | ||
31 | than the default handshake deadline before finally freeing the global | ||
32 | nbd_server object, but that could make QMP non-responsive for a long | ||
33 | time. So intead, this patch fixes the problem by tracking all client | ||
34 | sockets opened while the server is running, and forcefully closing any | ||
35 | such sockets remaining without a completed handshake at the time of | ||
36 | nbd-server-stop, then waiting until the coroutines servicing those | ||
37 | sockets notice the state change. nbd-server-stop now has a second | ||
38 | AIO_WAIT_WHILE_UNLOCKED (the first is indirectly through the | ||
39 | blk_exp_close_all_type() that disconnects all clients that completed | ||
40 | handshakes), but forced socket shutdown is enough to progress the | ||
41 | coroutines and quickly tear down all clients before the server is | ||
42 | freed, thus finally fixing the CVE. | ||
43 | |||
44 | This patch relies heavily on the fact that nbd/server.c guarantees | ||
45 | that it only calls nbd_blockdev_client_closed() from the main loop | ||
46 | (see the assertion in nbd_client_put() and the hoops used in | ||
47 | nbd_client_put_nonzero() to achieve that); if we did not have that | ||
48 | guarantee, we would also need a mutex protecting our accesses of the | ||
49 | list of connections to survive re-entrancy from independent iothreads. | ||
50 | |||
51 | Although I did not actually try to test old builds, it looks like this | ||
52 | problem has existed since at least commit 862172f45c (v2.12.0, 2017) - | ||
53 | even back when that patch started using a QIONetListener to handle | ||
54 | listening on multiple sockets, nbd_server_free() was already unaware | ||
55 | that the nbd_blockdev_client_closed callback can be reached later by a | ||
56 | client thread that has not completed handshakes (and therefore the | ||
57 | client's socket never got added to the list closed in | ||
58 | nbd_export_close_all), despite that patch intentionally tearing down | ||
59 | the QIONetListener to prevent new clients. | ||
60 | |||
61 | Reported-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> | ||
62 | Fixes: CVE-2024-7409 | ||
63 | CC: qemu-stable@nongnu.org | ||
64 | Signed-off-by: Eric Blake <eblake@redhat.com> | ||
65 | Message-ID: <20240807174943.771624-14-eblake@redhat.com> | ||
66 | Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> | ||
67 | |||
68 | CVE: CVE-2024-7409 | ||
69 | Upstream-Status: Backport [https://gitlab.com/qemu-project/qemu/-/commit/3e7ef738c8462c45043a1d39f702a0990406a3b3] | ||
70 | Signed-off-by: Hitendra Prajapati <hprajapati@mvista.com> | ||
71 | --- | ||
72 | blockdev-nbd.c | 35 ++++++++++++++++++++++++++++++++++- | ||
73 | 1 file changed, 34 insertions(+), 1 deletion(-) | ||
74 | |||
75 | diff --git a/blockdev-nbd.c b/blockdev-nbd.c | ||
76 | index 4bd90bac1..c71ca38d2 100644 | ||
77 | --- a/blockdev-nbd.c | ||
78 | +++ b/blockdev-nbd.c | ||
79 | @@ -21,12 +21,18 @@ | ||
80 | #include "io/channel-socket.h" | ||
81 | #include "io/net-listener.h" | ||
82 | |||
83 | +typedef struct NBDConn { | ||
84 | + QIOChannelSocket *cioc; | ||
85 | + QLIST_ENTRY(NBDConn) next; | ||
86 | +} NBDConn; | ||
87 | + | ||
88 | typedef struct NBDServerData { | ||
89 | QIONetListener *listener; | ||
90 | QCryptoTLSCreds *tlscreds; | ||
91 | char *tlsauthz; | ||
92 | uint32_t max_connections; | ||
93 | uint32_t connections; | ||
94 | + QLIST_HEAD(, NBDConn) conns; | ||
95 | } NBDServerData; | ||
96 | |||
97 | static NBDServerData *nbd_server; | ||
98 | @@ -46,6 +52,14 @@ bool nbd_server_is_running(void) | ||
99 | |||
100 | static void nbd_blockdev_client_closed(NBDClient *client, bool ignored) | ||
101 | { | ||
102 | + NBDConn *conn = nbd_client_owner(client); | ||
103 | + | ||
104 | + assert(qemu_mutex_iothread_locked() && nbd_server); | ||
105 | + | ||
106 | + object_unref(OBJECT(conn->cioc)); | ||
107 | + QLIST_REMOVE(conn, next); | ||
108 | + g_free(conn); | ||
109 | + | ||
110 | nbd_client_put(client); | ||
111 | assert(nbd_server->connections > 0); | ||
112 | nbd_server->connections--; | ||
113 | @@ -55,14 +69,20 @@ static void nbd_blockdev_client_closed(NBDClient *client, bool ignored) | ||
114 | static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc, | ||
115 | gpointer opaque) | ||
116 | { | ||
117 | + NBDConn *conn = g_new0(NBDConn, 1); | ||
118 | + | ||
119 | + assert(qemu_mutex_iothread_locked() && nbd_server); | ||
120 | nbd_server->connections++; | ||
121 | + object_ref(OBJECT(cioc)); | ||
122 | + conn->cioc = cioc; | ||
123 | + QLIST_INSERT_HEAD(&nbd_server->conns, conn, next); | ||
124 | nbd_update_server_watch(nbd_server); | ||
125 | |||
126 | qio_channel_set_name(QIO_CHANNEL(cioc), "nbd-server"); | ||
127 | /* TODO - expose handshake timeout as QMP option */ | ||
128 | nbd_client_new(cioc, NBD_DEFAULT_HANDSHAKE_MAX_SECS, | ||
129 | nbd_server->tlscreds, nbd_server->tlsauthz, | ||
130 | - nbd_blockdev_client_closed, NULL); | ||
131 | + nbd_blockdev_client_closed, conn); | ||
132 | } | ||
133 | |||
134 | static void nbd_update_server_watch(NBDServerData *s) | ||
135 | @@ -76,12 +96,25 @@ static void nbd_update_server_watch(NBDServerData *s) | ||
136 | |||
137 | static void nbd_server_free(NBDServerData *server) | ||
138 | { | ||
139 | + NBDConn *conn, *tmp; | ||
140 | + | ||
141 | if (!server) { | ||
142 | return; | ||
143 | } | ||
144 | |||
145 | + /* | ||
146 | + * Forcefully close the listener socket, and any clients that have | ||
147 | + * not yet disconnected on their own. | ||
148 | + */ | ||
149 | qio_net_listener_disconnect(server->listener); | ||
150 | object_unref(OBJECT(server->listener)); | ||
151 | + QLIST_FOREACH_SAFE(conn, &server->conns, next, tmp) { | ||
152 | + qio_channel_shutdown(QIO_CHANNEL(conn->cioc), QIO_CHANNEL_SHUTDOWN_BOTH, | ||
153 | + NULL); | ||
154 | + } | ||
155 | + | ||
156 | + AIO_WAIT_WHILE(NULL, server->connections > 0); | ||
157 | + | ||
158 | if (server->tlscreds) { | ||
159 | object_unref(OBJECT(server->tlscreds)); | ||
160 | } | ||
161 | -- | ||
162 | 2.25.1 | ||
163 | |||