[PATCH v2 0/2] Fix NBD CVE-2020-10761

[PATCH v2 0/2] Fix NBD CVE-2020-10761

[Eric Blake]

In qemu 4.2, I accidentally introduced the ability for an NBD client
obeying the specification to kill qemu as NBD server with an assertion
failure when the client requests an unusually long export name, as a
regression from the intended graceful server error message back to the
client.

In v2:
- use strnlen instead of strlen
- malloc a sane string rather than using a static buffer [Vladimir]
- enhance commit message
- tweak iotest to use different abbreviation than qemu-nbd
- add R-b on patch 2

Once this is reviewed, I'll then spin v2 of my NBD pull request.

Eric Blake (2):
  nbd/server: Avoid long error message assertions CVE-2020-10761
  block: Call attention to truncation of long NBD exports

 block.c                    |  7 +++++--
 block/nbd.c                | 21 +++++++++++++--------
 nbd/server.c               | 23 ++++++++++++++++++++---
 tests/qemu-iotests/143     |  4 ++++
 tests/qemu-iotests/143.out |  2 ++
 5 files changed, 44 insertions(+), 13 deletions(-)

-- 
2.27.0

[PATCH v2 1/2] nbd/server: Avoid long error message assertions CVE-2020-10761

[Eric Blake]

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>
---
 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 02b1ed080145..20754e9ebc3c 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 f649b3619501..b4acc4372542 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/aa.*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 1f4001c60131..fc9c0a761fa1 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

Re: [PATCH v2 1/2] nbd/server: Avoid long error message assertions CVE-2020-10761

[Vladimir Sementsov-Ogievskiy]

10.06.2020 19:37, Eric Blake wrote:
> 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>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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 02b1ed080145..20754e9ebc3c 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);

Cool! Somehow I forget about this feature, when writing my answer on v1.

> +
>           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 f649b3619501..b4acc4372542 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/aa.*aa/aa--aa/'

A bit more precise regexp: 's/a\{5,\}/aa--aa/'

> 
>   _send_qemu_cmd $QEMU_HANDLE \
>       "{ 'execute': 'quit' }" \
> diff --git a/tests/qemu-iotests/143.out b/tests/qemu-iotests/143.out
> index 1f4001c60131..fc9c0a761fa1 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"}}
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 1/2] nbd/server: Avoid long error message assertions CVE-2020-10761

[Eric Blake]

On 6/10/20 11:37 AM, Eric Blake wrote:

> 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.
> 

> +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);

Max pointed out off-list that this can take a valid UTF-8 name from the 
client and truncate it mid-character to make our reply NOT valid UTF-8, 
which is a (minor) violation of the NBD protocol.  We have not yet 
implemented strict UTF-8 enforcement in qemu (neither our client nor 
server code takes pains to only send UTF-8, nor validates that incoming 
strings are valid UTF-8); and while the server would previously echo 
non-UTF-8 (where the client violated protocol first), this is now a case 
where the server can be coerced into violating protocol first.  I guess 
I may end up doing a followup patch that adds incoming validation and in 
the process avoids chopping a multi-byte character, but that's just as 
easy to fold in with my question about sanitizing control characters.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH v2 2/2] block: Call attention to truncation of long NBD exports

[Eric Blake]

Commit 93676c88 relaxed our NBD client code to request export names up
to the NBD protocol maximum of 4096 bytes without NUL terminator, even
though the block layer can't store anything longer than 4096 bytes
including NUL terminator for display to the user.  Since this means
there are some export names where we have to truncate things, we can
at least try to make the truncation a bit more obvious for the user.
Note that in spite of the truncated display name, we can still
communicate with an NBD server using such a long export name; this was
deemed nicer than refusing to even connect to such a server (since the
server may not be under our control, and since determining our actual
length limits gets tricky when nbd://host:port/export and
nbd+unix:///export?socket=/path are themselves variable-length
expansions beyond the export name but count towards the block layer
name length).

Reported-by: Xueqiang Wei <xuwei@redhat.com>
Fixes: https://bugzilla.redhat.com/1843684
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c     |  7 +++++--
 block/nbd.c | 21 +++++++++++++--------
 2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/block.c b/block.c
index 8416376c9b71..6dbcb7e083ea 100644
--- a/block.c
+++ b/block.c
@@ -6809,8 +6809,11 @@ void bdrv_refresh_filename(BlockDriverState *bs)
         pstrcpy(bs->filename, sizeof(bs->filename), bs->exact_filename);
     } else {
         QString *json = qobject_to_json(QOBJECT(bs->full_open_options));
-        snprintf(bs->filename, sizeof(bs->filename), "json:%s",
-                 qstring_get_str(json));
+        if (snprintf(bs->filename, sizeof(bs->filename), "json:%s",
+                     qstring_get_str(json)) >= sizeof(bs->filename)) {
+            /* Give user a hint if we truncated things. */
+            strcpy(bs->filename + sizeof(bs->filename) - 4, "...");
+        }
         qobject_unref(json);
     }
 }
diff --git a/block/nbd.c b/block/nbd.c
index 4ac23c8f6299..eed160c5cda1 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -1984,6 +1984,7 @@ static void nbd_refresh_filename(BlockDriverState *bs)
 {
     BDRVNBDState *s = bs->opaque;
     const char *host = NULL, *port = NULL, *path = NULL;
+    size_t len = 0;

     if (s->saddr->type == SOCKET_ADDRESS_TYPE_INET) {
         const InetSocketAddress *inet = &s->saddr->u.inet;
@@ -1996,17 +1997,21 @@ static void nbd_refresh_filename(BlockDriverState *bs)
     } /* else can't represent as pseudo-filename */

     if (path && s->export) {
-        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd+unix:///%s?socket=%s", s->export, path);
+        len = snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+                       "nbd+unix:///%s?socket=%s", s->export, path);
     } else if (path && !s->export) {
-        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd+unix://?socket=%s", path);
+        len = snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+                       "nbd+unix://?socket=%s", path);
     } else if (host && s->export) {
-        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd://%s:%s/%s", host, port, s->export);
+        len = snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+                       "nbd://%s:%s/%s", host, port, s->export);
     } else if (host && !s->export) {
-        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd://%s:%s", host, port);
+        len = snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+                       "nbd://%s:%s", host, port);
+    }
+    if (len > sizeof(bs->exact_filename)) {
+        /* Name is too long to represent exactly, so leave it empty. */
+        bs->exact_filename[0] = '\0';
     }
 }

-- 
2.27.0

Re: [PATCH v2 2/2] block: Call attention to truncation of long NBD exports

[Eric Blake]

On 6/10/20 11:37 AM, Eric Blake wrote:
> Commit 93676c88 relaxed our NBD client code to request export names up
> to the NBD protocol maximum of 4096 bytes without NUL terminator, even
> though the block layer can't store anything longer than 4096 bytes
> including NUL terminator for display to the user.  Since this means
> there are some export names where we have to truncate things, we can
> at least try to make the truncation a bit more obvious for the user.
> Note that in spite of the truncated display name, we can still
> communicate with an NBD server using such a long export name; this was
> deemed nicer than refusing to even connect to such a server (since the
> server may not be under our control, and since determining our actual
> length limits gets tricky when nbd://host:port/export and
> nbd+unix:///export?socket=/path are themselves variable-length
> expansions beyond the export name but count towards the block layer
> name length).
> 

> +++ b/block/nbd.c

>       } else if (host && !s->export) {
> -        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
> -                 "nbd://%s:%s", host, port);
> +        len = snprintf(bs->exact_filename, sizeof(bs->exact_filename),
> +                       "nbd://%s:%s", host, port);
> +    }
> +    if (len > sizeof(bs->exact_filename)) {

Max pointed out off-list that this is off-by-one: by not using >=, I am 
failing to detect overflow when exactly one byte is truncated.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org