|
|
902636 |
From f49ff2ed5675f1d0cddc404842e9d6e4e572d5a7 Mon Sep 17 00:00:00 2001
|
|
|
902636 |
From: Eric Blake <eblake@redhat.com>
|
|
|
902636 |
Date: Wed, 10 Jun 2020 18:32:01 -0400
|
|
|
902636 |
Subject: [PATCH 1/2] nbd/server: Avoid long error message assertions
|
|
|
902636 |
CVE-2020-10761
|
|
|
902636 |
|
|
|
902636 |
RH-Author: Eric Blake <eblake@redhat.com>
|
|
|
902636 |
Message-id: <20200610183202.3780750-2-eblake@redhat.com>
|
|
|
902636 |
Patchwork-id: 97494
|
|
|
902636 |
O-Subject: [RHEL-AV-8.2.1 qemu-kvm PATCH 1/2] nbd/server: Avoid long error message assertions CVE-2020-10761
|
|
|
902636 |
Bugzilla: 1845384
|
|
|
902636 |
RH-Acked-by: Sergio Lopez Pascual <slp@redhat.com>
|
|
|
902636 |
RH-Acked-by: Max Reitz <mreitz@redhat.com>
|
|
|
902636 |
RH-Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
|
|
|
902636 |
|
|
|
902636 |
Ever since commit 36683283 (v2.8), the server code asserts that error
|
|
|
902636 |
strings sent to the client are well-formed per the protocol by not
|
|
|
902636 |
exceeding the maximum string length of 4096. At the time the server
|
|
|
902636 |
first started sending error messages, the assertion could not be
|
|
|
902636 |
triggered, because messages were completely under our control.
|
|
|
902636 |
However, over the years, we have added latent scenarios where a client
|
|
|
902636 |
could trigger the server to attempt an error message that would
|
|
|
902636 |
include the client's information if it passed other checks first:
|
|
|
902636 |
|
|
|
902636 |
- requesting NBD_OPT_INFO/GO on an export name that is not present
|
|
|
902636 |
(commit 0cfae925 in v2.12 echoes the name)
|
|
|
902636 |
|
|
|
902636 |
- requesting NBD_OPT_LIST/SET_META_CONTEXT on an export name that is
|
|
|
902636 |
not present (commit e7b1948d in v2.12 echoes the name)
|
|
|
902636 |
|
|
|
902636 |
At the time, those were still safe because we flagged names larger
|
|
|
902636 |
than 256 bytes with a different message; but that changed in commit
|
|
|
902636 |
93676c88 (v4.2) when we raised the name limit to 4096 to match the NBD
|
|
|
902636 |
string limit. (That commit also failed to change the magic number
|
|
|
902636 |
4096 in nbd_negotiate_send_rep_err to the just-introduced named
|
|
|
902636 |
constant.) So with that commit, long client names appended to server
|
|
|
902636 |
text can now trigger the assertion, and thus be used as a denial of
|
|
|
902636 |
service attack against a server. As a mitigating factor, if the
|
|
|
902636 |
server requires TLS, the client cannot trigger the problematic paths
|
|
|
902636 |
unless it first supplies TLS credentials, and such trusted clients are
|
|
|
902636 |
less likely to try to intentionally crash the server.
|
|
|
902636 |
|
|
|
902636 |
We may later want to further sanitize the user-supplied strings we
|
|
|
902636 |
place into our error messages, such as scrubbing out control
|
|
|
902636 |
characters, but that is less important to the CVE fix, so it can be a
|
|
|
902636 |
later patch to the new nbd_sanitize_name.
|
|
|
902636 |
|
|
|
902636 |
Consideration was given to changing the assertion in
|
|
|
902636 |
nbd_negotiate_send_rep_verr to instead merely log a server error and
|
|
|
902636 |
truncate the message, to avoid leaving a latent path that could
|
|
|
902636 |
trigger a future CVE DoS on any new error message. However, this
|
|
|
902636 |
merely complicates the code for something that is already (correctly)
|
|
|
902636 |
flagging coding errors, and now that we are aware of the long message
|
|
|
902636 |
pitfall, we are less likely to introduce such errors in the future,
|
|
|
902636 |
which would make such error handling dead code.
|
|
|
902636 |
|
|
|
902636 |
Reported-by: Xueqiang Wei <xuwei@redhat.com>
|
|
|
902636 |
CC: qemu-stable@nongnu.org
|
|
|
902636 |
Fixes: https://bugzilla.redhat.com/1843684 CVE-2020-10761
|
|
|
902636 |
Fixes: 93676c88d7
|
|
|
902636 |
Signed-off-by: Eric Blake <eblake@redhat.com>
|
|
|
902636 |
Message-Id: <20200610163741.3745251-2-eblake@redhat.com>
|
|
|
902636 |
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
|
|
|
902636 |
(cherry picked from commit 5c4fe018c025740fef4a0a4421e8162db0c3eefd)
|
|
|
902636 |
Signed-off-by: Eric Blake <eblake@redhat.com>
|
|
|
902636 |
Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.com>
|
|
|
902636 |
---
|
|
|
902636 |
nbd/server.c | 23 ++++++++++++++++++++---
|
|
|
902636 |
tests/qemu-iotests/143 | 4 ++++
|
|
|
902636 |
tests/qemu-iotests/143.out | 2 ++
|
|
|
902636 |
3 files changed, 26 insertions(+), 3 deletions(-)
|
|
|
902636 |
|
|
|
902636 |
diff --git a/nbd/server.c b/nbd/server.c
|
|
|
902636 |
index 24ebc1a805..d5b9df092c 100644
|
|
|
902636 |
--- a/nbd/server.c
|
|
|
902636 |
+++ b/nbd/server.c
|
|
|
902636 |
@@ -217,7 +217,7 @@ nbd_negotiate_send_rep_verr(NBDClient *client, uint32_t type,
|
|
|
902636 |
|
|
|
902636 |
msg = g_strdup_vprintf(fmt, va);
|
|
|
902636 |
len = strlen(msg);
|
|
|
902636 |
- assert(len < 4096);
|
|
|
902636 |
+ assert(len < NBD_MAX_STRING_SIZE);
|
|
|
902636 |
trace_nbd_negotiate_send_rep_err(msg);
|
|
|
902636 |
ret = nbd_negotiate_send_rep_len(client, type, len, errp);
|
|
|
902636 |
if (ret < 0) {
|
|
|
902636 |
@@ -231,6 +231,19 @@ nbd_negotiate_send_rep_verr(NBDClient *client, uint32_t type,
|
|
|
902636 |
return 0;
|
|
|
902636 |
}
|
|
|
902636 |
|
|
|
902636 |
+/*
|
|
|
902636 |
+ * Return a malloc'd copy of @name suitable for use in an error reply.
|
|
|
902636 |
+ */
|
|
|
902636 |
+static char *
|
|
|
902636 |
+nbd_sanitize_name(const char *name)
|
|
|
902636 |
+{
|
|
|
902636 |
+ if (strnlen(name, 80) < 80) {
|
|
|
902636 |
+ return g_strdup(name);
|
|
|
902636 |
+ }
|
|
|
902636 |
+ /* XXX Should we also try to sanitize any control characters? */
|
|
|
902636 |
+ return g_strdup_printf("%.80s...", name);
|
|
|
902636 |
+}
|
|
|
902636 |
+
|
|
|
902636 |
/* Send an error reply.
|
|
|
902636 |
* Return -errno on error, 0 on success. */
|
|
|
902636 |
static int GCC_FMT_ATTR(4, 5)
|
|
|
902636 |
@@ -595,9 +608,11 @@ static int nbd_negotiate_handle_info(NBDClient *client, Error **errp)
|
|
|
902636 |
|
|
|
902636 |
exp = nbd_export_find(name);
|
|
|
902636 |
if (!exp) {
|
|
|
902636 |
+ g_autofree char *sane_name = nbd_sanitize_name(name);
|
|
|
902636 |
+
|
|
|
902636 |
return nbd_negotiate_send_rep_err(client, NBD_REP_ERR_UNKNOWN,
|
|
|
902636 |
errp, "export '%s' not present",
|
|
|
902636 |
- name);
|
|
|
902636 |
+ sane_name);
|
|
|
902636 |
}
|
|
|
902636 |
|
|
|
902636 |
/* Don't bother sending NBD_INFO_NAME unless client requested it */
|
|
|
902636 |
@@ -995,8 +1010,10 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
|
|
|
902636 |
|
|
|
902636 |
meta->exp = nbd_export_find(export_name);
|
|
|
902636 |
if (meta->exp == NULL) {
|
|
|
902636 |
+ g_autofree char *sane_name = nbd_sanitize_name(export_name);
|
|
|
902636 |
+
|
|
|
902636 |
return nbd_opt_drop(client, NBD_REP_ERR_UNKNOWN, errp,
|
|
|
902636 |
- "export '%s' not present", export_name);
|
|
|
902636 |
+ "export '%s' not present", sane_name);
|
|
|
902636 |
}
|
|
|
902636 |
|
|
|
902636 |
ret = nbd_opt_read(client, &nb_queries, sizeof(nb_queries), errp);
|
|
|
902636 |
diff --git a/tests/qemu-iotests/143 b/tests/qemu-iotests/143
|
|
|
902636 |
index f649b36195..d2349903b1 100755
|
|
|
902636 |
--- a/tests/qemu-iotests/143
|
|
|
902636 |
+++ b/tests/qemu-iotests/143
|
|
|
902636 |
@@ -58,6 +58,10 @@ _send_qemu_cmd $QEMU_HANDLE \
|
|
|
902636 |
$QEMU_IO_PROG -f raw -c quit \
|
|
|
902636 |
"nbd+unix:///no_such_export?socket=$SOCK_DIR/nbd" 2>&1 \
|
|
|
902636 |
| _filter_qemu_io | _filter_nbd
|
|
|
902636 |
+# Likewise, with longest possible name permitted in NBD protocol
|
|
|
902636 |
+$QEMU_IO_PROG -f raw -c quit \
|
|
|
902636 |
+ "nbd+unix:///$(printf %4096d 1 | tr ' ' a)?socket=$SOCK_DIR/nbd" 2>&1 \
|
|
|
902636 |
+ | _filter_qemu_io | _filter_nbd | sed 's/aaaa*aa/aa--aa/'
|
|
|
902636 |
|
|
|
902636 |
_send_qemu_cmd $QEMU_HANDLE \
|
|
|
902636 |
"{ 'execute': 'quit' }" \
|
|
|
902636 |
diff --git a/tests/qemu-iotests/143.out b/tests/qemu-iotests/143.out
|
|
|
902636 |
index 1f4001c601..fc9c0a761f 100644
|
|
|
902636 |
--- a/tests/qemu-iotests/143.out
|
|
|
902636 |
+++ b/tests/qemu-iotests/143.out
|
|
|
902636 |
@@ -5,6 +5,8 @@ QA output created by 143
|
|
|
902636 |
{"return": {}}
|
|
|
902636 |
qemu-io: can't open device nbd+unix:///no_such_export?socket=SOCK_DIR/nbd: Requested export not available
|
|
|
902636 |
server reported: export 'no_such_export' not present
|
|
|
902636 |
+qemu-io: can't open device nbd+unix:///aa--aa1?socket=SOCK_DIR/nbd: Requested export not available
|
|
|
902636 |
+server reported: export 'aa--aa...' not present
|
|
|
902636 |
{ 'execute': 'quit' }
|
|
|
902636 |
{"return": {}}
|
|
|
902636 |
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
|
|
|
902636 |
--
|
|
|
902636 |
2.27.0
|
|
|
902636 |
|