From f49ff2ed5675f1d0cddc404842e9d6e4e572d5a7 Mon Sep 17 00:00:00 2001

From: Eric Blake <eblake@redhat.com>

Date: Wed, 10 Jun 2020 18:32:01 -0400

Subject: [PATCH 1/2] nbd/server: Avoid long error message assertions

 CVE-2020-10761

RH-Author: Eric Blake <eblake@redhat.com>

Message-id: <20200610183202.3780750-2-eblake@redhat.com>

Patchwork-id: 97494

O-Subject: [RHEL-AV-8.2.1 qemu-kvm PATCH 1/2] nbd/server: Avoid long error message assertions CVE-2020-10761

Bugzilla: 1845384

RH-Acked-by: Sergio Lopez Pascual <slp@redhat.com>

RH-Acked-by: Max Reitz <mreitz@redhat.com>

RH-Acked-by: Stefan Hajnoczi <stefanha@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: Eric Blake <eblake@redhat.com>

Signed-off-by: Eduardo Lima (Etrunko) <etrunko@redhat.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 24ebc1a805..d5b9df092c 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.27.0