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

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