yeahuh / rpms / qemu-kvm

Forked from rpms/qemu-kvm 2 years ago
Clone

Blame SOURCES/kvm-nbd-server-Avoid-long-error-message-assertions-CVE-2.patch

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