[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PATCH 06/77] nbd/server: Avoid long error message assertions CVE-2020-1
From: |
Michael Roth |
Subject: |
[PATCH 06/77] nbd/server: Avoid long error message assertions CVE-2020-10761 |
Date: |
Thu, 3 Sep 2020 15:58:24 -0500 |
From: Eric Blake <eblake@redhat.com>
Ever since commit 36683283 (v2.8), the server code asserts that error
strings sent to the client are well-formed per the protocol by not
exceeding the maximum string length of 4096. At the time the server
first started sending error messages, the assertion could not be
triggered, because messages were completely under our control.
However, over the years, we have added latent scenarios where a client
could trigger the server to attempt an error message that would
include the client's information if it passed other checks first:
- requesting NBD_OPT_INFO/GO on an export name that is not present
(commit 0cfae925 in v2.12 echoes the name)
- requesting NBD_OPT_LIST/SET_META_CONTEXT on an export name that is
not present (commit e7b1948d in v2.12 echoes the name)
At the time, those were still safe because we flagged names larger
than 256 bytes with a different message; but that changed in commit
93676c88 (v4.2) when we raised the name limit to 4096 to match the NBD
string limit. (That commit also failed to change the magic number
4096 in nbd_negotiate_send_rep_err to the just-introduced named
constant.) So with that commit, long client names appended to server
text can now trigger the assertion, and thus be used as a denial of
service attack against a server. As a mitigating factor, if the
server requires TLS, the client cannot trigger the problematic paths
unless it first supplies TLS credentials, and such trusted clients are
less likely to try to intentionally crash the server.
We may later want to further sanitize the user-supplied strings we
place into our error messages, such as scrubbing out control
characters, but that is less important to the CVE fix, so it can be a
later patch to the new nbd_sanitize_name.
Consideration was given to changing the assertion in
nbd_negotiate_send_rep_verr to instead merely log a server error and
truncate the message, to avoid leaving a latent path that could
trigger a future CVE DoS on any new error message. However, this
merely complicates the code for something that is already (correctly)
flagging coding errors, and now that we are aware of the long message
pitfall, we are less likely to introduce such errors in the future,
which would make such error handling dead code.
Reported-by: Xueqiang Wei <xuwei@redhat.com>
CC: qemu-stable@nongnu.org
Fixes: https://bugzilla.redhat.com/1843684 CVE-2020-10761
Fixes: 93676c88d7
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20200610163741.3745251-2-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
(cherry picked from commit 5c4fe018c025740fef4a0a4421e8162db0c3eefd)
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
nbd/server.c | 23 ++++++++++++++++++++---
tests/qemu-iotests/143 | 4 ++++
tests/qemu-iotests/143.out | 2 ++
3 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index 02b1ed0801..20754e9ebc 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -217,7 +217,7 @@ nbd_negotiate_send_rep_verr(NBDClient *client, uint32_t
type,
msg = g_strdup_vprintf(fmt, va);
len = strlen(msg);
- assert(len < 4096);
+ assert(len < NBD_MAX_STRING_SIZE);
trace_nbd_negotiate_send_rep_err(msg);
ret = nbd_negotiate_send_rep_len(client, type, len, errp);
if (ret < 0) {
@@ -231,6 +231,19 @@ nbd_negotiate_send_rep_verr(NBDClient *client, uint32_t
type,
return 0;
}
+/*
+ * Return a malloc'd copy of @name suitable for use in an error reply.
+ */
+static char *
+nbd_sanitize_name(const char *name)
+{
+ if (strnlen(name, 80) < 80) {
+ return g_strdup(name);
+ }
+ /* XXX Should we also try to sanitize any control characters? */
+ return g_strdup_printf("%.80s...", name);
+}
+
/* Send an error reply.
* Return -errno on error, 0 on success. */
static int GCC_FMT_ATTR(4, 5)
@@ -595,9 +608,11 @@ static int nbd_negotiate_handle_info(NBDClient *client,
Error **errp)
exp = nbd_export_find(name);
if (!exp) {
+ g_autofree char *sane_name = nbd_sanitize_name(name);
+
return nbd_negotiate_send_rep_err(client, NBD_REP_ERR_UNKNOWN,
errp, "export '%s' not present",
- name);
+ sane_name);
}
/* Don't bother sending NBD_INFO_NAME unless client requested it */
@@ -995,8 +1010,10 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
meta->exp = nbd_export_find(export_name);
if (meta->exp == NULL) {
+ g_autofree char *sane_name = nbd_sanitize_name(export_name);
+
return nbd_opt_drop(client, NBD_REP_ERR_UNKNOWN, errp,
- "export '%s' not present", export_name);
+ "export '%s' not present", sane_name);
}
ret = nbd_opt_read(client, &nb_queries, sizeof(nb_queries), errp);
diff --git a/tests/qemu-iotests/143 b/tests/qemu-iotests/143
index f649b36195..d2349903b1 100755
--- a/tests/qemu-iotests/143
+++ b/tests/qemu-iotests/143
@@ -58,6 +58,10 @@ _send_qemu_cmd $QEMU_HANDLE \
$QEMU_IO_PROG -f raw -c quit \
"nbd+unix:///no_such_export?socket=$SOCK_DIR/nbd" 2>&1 \
| _filter_qemu_io | _filter_nbd
+# Likewise, with longest possible name permitted in NBD protocol
+$QEMU_IO_PROG -f raw -c quit \
+ "nbd+unix:///$(printf %4096d 1 | tr ' ' a)?socket=$SOCK_DIR/nbd" 2>&1 \
+ | _filter_qemu_io | _filter_nbd | sed 's/aaaa*aa/aa--aa/'
_send_qemu_cmd $QEMU_HANDLE \
"{ 'execute': 'quit' }" \
diff --git a/tests/qemu-iotests/143.out b/tests/qemu-iotests/143.out
index 1f4001c601..fc9c0a761f 100644
--- a/tests/qemu-iotests/143.out
+++ b/tests/qemu-iotests/143.out
@@ -5,6 +5,8 @@ QA output created by 143
{"return": {}}
qemu-io: can't open device nbd+unix:///no_such_export?socket=SOCK_DIR/nbd:
Requested export not available
server reported: export 'no_such_export' not present
+qemu-io: can't open device nbd+unix:///aa--aa1?socket=SOCK_DIR/nbd: Requested
export not available
+server reported: export 'aa--aa...' not present
{ 'execute': 'quit' }
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
--
2.17.1
- [PATCH 57/77] tpm: tpm_spapr: Exit on TPM backend failures, (continued)
- [PATCH 57/77] tpm: tpm_spapr: Exit on TPM backend failures, Michael Roth, 2020/09/03
- [PATCH 58/77] tests: tpm: Skip over pcrUpdateCounter byte in result comparison, Michael Roth, 2020/09/03
- [PATCH 59/77] qdev: Fix device_add DRIVER,help to print to monitor, Michael Roth, 2020/09/03
- [PATCH 60/77] virtio-balloon: Prevent guest from starting a report when we didn't request one, Michael Roth, 2020/09/03
- [PATCH 05/77] net: Do not include a newline in the id of -nic devices, Michael Roth, 2020/09/03
- [PATCH 61/77] virtio-balloon: Add locking to prevent possible race when starting hinting, Michael Roth, 2020/09/03
- [PATCH 63/77] linux-headers: update against Linux 5.7-rc3, Michael Roth, 2020/09/03
- [PATCH 65/77] virtio: list legacy-capable devices, Michael Roth, 2020/09/03
- [PATCH 67/77] intel_iommu: Use correct shift for 256 bits qi descriptor, Michael Roth, 2020/09/03
- [PATCH 69/77] libvhost-user: Report descriptor index on panic, Michael Roth, 2020/09/03
- [PATCH 06/77] nbd/server: Avoid long error message assertions CVE-2020-10761,
Michael Roth <=
- [PATCH 71/77] migration/block-dirty-bitmap: fix dirty_bitmap_mig_before_vm_start, Michael Roth, 2020/09/03
- [PATCH 64/77] virtio-balloon: Replace free page hinting references to 'report' with 'hint', Michael Roth, 2020/09/03
- [PATCH 66/77] virtio: verify that legacy support is not accidentally on, Michael Roth, 2020/09/03
- [PATCH 68/77] virtio-pci: Changed vdev to proxy for VirtIO PCI BAR callbacks., Michael Roth, 2020/09/03
- [PATCH 62/77] virtio-balloon: always indicate S_DONE when migration fails, Michael Roth, 2020/09/03
- [PATCH 72/77] block: Fix bdrv_aligned_p*v() for qiov_offset != 0, Michael Roth, 2020/09/03
- [PATCH 73/77] iotests/028: Add test for cross-base-EOF reads, Michael Roth, 2020/09/03
- [PATCH 74/77] nbd: Fix large trim/zero requests, Michael Roth, 2020/09/03
- [PATCH 70/77] Update OpenBIOS images to 7f28286f built from submodule., Michael Roth, 2020/09/03