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

[PATCH 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.  Given that the DoS security hole can be mitigated by
requiring TLS (and a client with TLS credentials is less likely to
play such games), the plan is to make the issue public today and send
a pull request through my NBD tree on Tuesday.

We may still want to revisit whether the block layer caps display
names to 4095 bytes, or whether it should track a malloc'd name even
when that name exceeds 4k.

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               | 28 +++++++++++++++++++++++++---
 tests/qemu-iotests/143     |  4 ++++
 tests/qemu-iotests/143.out |  2 ++
 5 files changed, 49 insertions(+), 13 deletions(-)

-- 
2.27.0

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

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               | 28 +++++++++++++++++++++++++---
 tests/qemu-iotests/143     |  4 ++++
 tests/qemu-iotests/143.out |  2 ++
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 02b1ed080145..ec130303586d 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,27 @@ nbd_negotiate_send_rep_verr(NBDClient *client, uint32_t type,
     return 0;
 }

+/*
+ * Truncate a potentially-long user-supplied string into something
+ * more suitable for an error reply.
+ */
+static const char *
+nbd_truncate_name(const char *name)
+{
+#define SANE_LENGTH 80
+    static char buf[SANE_LENGTH + 3 + 1]; /* Trailing '...', NUL */
+
+    if (strlen(name) < SANE_LENGTH) {
+        return name;
+    }
+    memcpy(buf, name, SANE_LENGTH);
+    buf[SANE_LENGTH] = '.';
+    buf[SANE_LENGTH + 1] = '.';
+    buf[SANE_LENGTH + 2] = '.';
+    buf[SANE_LENGTH + 3] = '\0';
+    return buf;
+}
+
 /* Send an error reply.
  * Return -errno on error, 0 on success. */
 static int GCC_FMT_ATTR(4, 5)
@@ -597,7 +618,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, Error **errp)
     if (!exp) {
         return nbd_negotiate_send_rep_err(client, NBD_REP_ERR_UNKNOWN,
                                           errp, "export '%s' not present",
-                                          name);
+                                          nbd_truncate_name(name));
     }

     /* Don't bother sending NBD_INFO_NAME unless client requested it */
@@ -996,7 +1017,8 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
     meta->exp = nbd_export_find(export_name);
     if (meta->exp == NULL) {
         return nbd_opt_drop(client, NBD_REP_ERR_UNKNOWN, errp,
-                            "export '%s' not present", export_name);
+                            "export '%s' not present",
+                            nbd_truncate_name(export_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..b0b1cff86cb6 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..be1f3a625458 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 1/2] nbd/server: Avoid long error message assertions CVE-2020-10761

[Eric Blake]

On 6/8/20 1:26 PM, 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)

Note that this patch does NOT scrub the client's export name for control 
characters.  Then again, the qcow2 file format does not (currently) 
prohibit control characters in bitmap or internal snapshot names, and 
'qemu-img info' blindly outputs there too.  We may want to do followup 
patches that further scrub qemu error messages to avoid scenarios where 
a user can attempt to coerce qemu into producing an error message 
containing control characters.

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

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

[Vladimir Sementsov-Ogievskiy]

08.06.2020 21:26, 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.
> 
> 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               | 28 +++++++++++++++++++++++++---
>   tests/qemu-iotests/143     |  4 ++++
>   tests/qemu-iotests/143.out |  2 ++
>   3 files changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 02b1ed080145..ec130303586d 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,27 @@ nbd_negotiate_send_rep_verr(NBDClient *client, uint32_t type,
>       return 0;
>   }
> 
> +/*
> + * Truncate a potentially-long user-supplied string into something
> + * more suitable for an error reply.
> + */
> +static const char *
> +nbd_truncate_name(const char *name)
> +{
> +#define SANE_LENGTH 80
> +    static char buf[SANE_LENGTH + 3 + 1]; /* Trailing '...', NUL */

s/NUL/NULL/

Hmm. It may break if we use it in parallel in two coroutines or threads.. Not sure, is it possible now, neither of course will it be possible in future.

I'd avoid creating functions returning  instead use g_strdup_printf(), like

char *tmp = g_strdup_printf("%.80s...", name);

   ( OR, if you want explicit constant: g_strdup_printf("%.*s...", SANE_LENGTH, name) )

... report error ...

g_free(tmp)

Using g_strdup_printf also is safer as we don't need to care about buf size.

> +
> +    if (strlen(name) < SANE_LENGTH) {
> +        return name;
> +    }
> +    memcpy(buf, name, SANE_LENGTH);
> +    buf[SANE_LENGTH] = '.';
> +    buf[SANE_LENGTH + 1] = '.';
> +    buf[SANE_LENGTH + 2] = '.';
> +    buf[SANE_LENGTH + 3] = '\0';

one-line suggestion:

   sprintf(buf, "%.80s...", name);

OR

   sprintf(buf, "%.*s...", SANE_LENGTH, name);

> +    return buf;
> +}
> +
>   /* Send an error reply.
>    * Return -errno on error, 0 on success. */
>   static int GCC_FMT_ATTR(4, 5)
> @@ -597,7 +618,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, Error **errp)
>       if (!exp) {
>           return nbd_negotiate_send_rep_err(client, NBD_REP_ERR_UNKNOWN,
>                                             errp, "export '%s' not present",
> -                                          name);
> +                                          nbd_truncate_name(name));
>       }
> 
>       /* Don't bother sending NBD_INFO_NAME unless client requested it */
> @@ -996,7 +1017,8 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
>       meta->exp = nbd_export_find(export_name);
>       if (meta->exp == NULL) {
>           return nbd_opt_drop(client, NBD_REP_ERR_UNKNOWN, errp,
> -                            "export '%s' not present", export_name);
> +                            "export '%s' not present",
> +                            nbd_truncate_name(export_name));
>       }
> 

Hmm, maybe instead of assertion, shrink message in nbd_negotiate_send_rep_verr() too?
This will save us from forgotten (or future) uses of the function.

Shrinking name is better, as it provides better message on result. But generally shrink
all two long messages in nbd_negotiate_send_rep_verr() (maybe, together with error_report())
seems a good thing for me.

>       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..b0b1cff86cb6 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..be1f3a625458 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 1/2] nbd/server: Avoid long error message assertions CVE-2020-10761

[Eric Blake]

On 6/10/20 3:57 AM, Vladimir Sementsov-Ogievskiy wrote:
> 08.06.2020 21:26, 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.
>>
>> 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               | 28 +++++++++++++++++++++++++---
>>   tests/qemu-iotests/143     |  4 ++++
>>   tests/qemu-iotests/143.out |  2 ++
>>   3 files changed, 31 insertions(+), 3 deletions(-)
>>
>> diff --git a/nbd/server.c b/nbd/server.c
>> index 02b1ed080145..ec130303586d 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,27 @@ nbd_negotiate_send_rep_verr(NBDClient *client, 
>> uint32_t type,
>>       return 0;
>>   }
>>
>> +/*
>> + * Truncate a potentially-long user-supplied string into something
>> + * more suitable for an error reply.
>> + */
>> +static const char *
>> +nbd_truncate_name(const char *name)
>> +{
>> +#define SANE_LENGTH 80
>> +    static char buf[SANE_LENGTH + 3 + 1]; /* Trailing '...', NUL */
> 
> s/NUL/NULL/

NULL is the pointer (typically 4 or 8 bytes); NUL is the character 
(exactly one byte in all multi-byte-encodings like UTF-8, or 
sizeof(wchar_t) when using wide characters).

> 
> Hmm. It may break if we use it in parallel in two coroutines or 
> threads.. Not sure, is it possible now, neither of course will it be 
> possible in future.

After some testing (including adding some temporary sleep() into the 
code), it looks like 'qemu-nbd -e 2' is currently serialized (we don't 
start responding to a second client until we are done negotiating with 
the first); on that grounds, we are not risking that information leaks 
from one client to another.  But you are correct that it is not obvious, 
and that if we do have a situation where two threads can try to allow an 
NBD connection, then this static buffer could leak information from one 
client to another.  So I'll need to post a v2.

> 
> I'd avoid creating functions returning  instead use g_strdup_printf(), like
> 
> char *tmp = g_strdup_printf("%.80s...", name);
> 
>    ( OR, if you want explicit constant: g_strdup_printf("%.*s...", 
> SANE_LENGTH, name) )
> 
> ... report error ...
> 
> g_free(tmp)
> 
> Using g_strdup_printf also is safer as we don't need to care about buf 
> size.

malloc'ing the buffer is not too bad; error messages are not the hot 
path. I'll change it along those lines for v2.

>> @@ -996,7 +1017,8 @@ static int nbd_negotiate_meta_queries(NBDClient 
>> *client,
>>       meta->exp = nbd_export_find(export_name);
>>       if (meta->exp == NULL) {
>>           return nbd_opt_drop(client, NBD_REP_ERR_UNKNOWN, errp,
>> -                            "export '%s' not present", export_name);
>> +                            "export '%s' not present",
>> +                            nbd_truncate_name(export_name));
>>       }
>>
> 
> Hmm, maybe instead of assertion, shrink message in 
> nbd_negotiate_send_rep_verr() too?
> This will save us from forgotten (or future) uses of the function.

Truncating in nbd_negotiate_send_rep_verr is arbitrary; it does not have 
the context of what makes sense to truncate.  With an artificially short 
length, and a client request for "longname_from_the_client", the 
difference would be between:

export "longnam..." not present
export "longname_from_the_cl...

As a user, I prefer the first form, but truncating in 
nbd_negotiate_send_rep_verr is the second form.

> 
> Shrinking name is better, as it provides better message on result. But 
> generally shrink
> all two long messages in nbd_negotiate_send_rep_verr() (maybe, together 
> with error_report())
> seems a good thing for me.

What would error_report() accomplish?  Logging on the server that we are 
truncating a message sent back to the client doesn't help the client. 
This is not a case of protocol violation, and a server sending an error 
message to the client is not out of the ordinary.

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

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

[Vladimir Sementsov-Ogievskiy]

10.06.2020 16:39, Eric Blake wrote:
> On 6/10/20 3:57 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 08.06.2020 21:26, 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.
>>>
>>> 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               | 28 +++++++++++++++++++++++++---
>>>   tests/qemu-iotests/143     |  4 ++++
>>>   tests/qemu-iotests/143.out |  2 ++
>>>   3 files changed, 31 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/nbd/server.c b/nbd/server.c
>>> index 02b1ed080145..ec130303586d 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,27 @@ nbd_negotiate_send_rep_verr(NBDClient *client, uint32_t type,
>>>       return 0;
>>>   }
>>>
>>> +/*
>>> + * Truncate a potentially-long user-supplied string into something
>>> + * more suitable for an error reply.
>>> + */
>>> +static const char *
>>> +nbd_truncate_name(const char *name)
>>> +{
>>> +#define SANE_LENGTH 80
>>> +    static char buf[SANE_LENGTH + 3 + 1]; /* Trailing '...', NUL */
>>
>> s/NUL/NULL/
> 
> NULL is the pointer (typically 4 or 8 bytes); NUL is the character (exactly one byte in all multi-byte-encodings like UTF-8, or sizeof(wchar_t) when using wide characters).
> 
>>
>> Hmm. It may break if we use it in parallel in two coroutines or threads.. Not sure, is it possible now, neither of course will it be possible in future.
> 
> After some testing (including adding some temporary sleep() into the code), it looks like 'qemu-nbd -e 2' is currently serialized (we don't start responding to a second client until we are done negotiating with the first); on that grounds, we are not risking that information leaks from one client to another.  But you are correct that it is not obvious, and that if we do have a situation where two threads can try to allow an NBD connection, then this static buffer could leak information from one client to another.  So I'll need to post a v2.
> 
>>
>> I'd avoid creating functions returning  instead use g_strdup_printf(), like
>>
>> char *tmp = g_strdup_printf("%.80s...", name);
>>
>>    ( OR, if you want explicit constant: g_strdup_printf("%.*s...", SANE_LENGTH, name) )
>>
>> ... report error ...
>>
>> g_free(tmp)
>>
>> Using g_strdup_printf also is safer as we don't need to care about buf size.
> 
> malloc'ing the buffer is not too bad; error messages are not the hot path. I'll change it along those lines for v2.
> 
>>> @@ -996,7 +1017,8 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
>>>       meta->exp = nbd_export_find(export_name);
>>>       if (meta->exp == NULL) {
>>>           return nbd_opt_drop(client, NBD_REP_ERR_UNKNOWN, errp,
>>> -                            "export '%s' not present", export_name);
>>> +                            "export '%s' not present",
>>> +                            nbd_truncate_name(export_name));
>>>       }
>>>
>>
>> Hmm, maybe instead of assertion, shrink message in nbd_negotiate_send_rep_verr() too?
>> This will save us from forgotten (or future) uses of the function.
> 
> Truncating in nbd_negotiate_send_rep_verr is arbitrary; it does not have the context of what makes sense to truncate.  With an artificially short length, and a client request for "longname_from_the_client", the difference would be between:
> 
> export "longnam..." not present
> export "longname_from_the_cl...
> 
> As a user, I prefer the first form, but truncating in nbd_negotiate_send_rep_verr is the second form.
> 
>>
>> Shrinking name is better, as it provides better message on result. But generally shrink
>> all two long messages in nbd_negotiate_send_rep_verr() (maybe, together with error_report())
>> seems a good thing for me.
> 
> What would error_report() accomplish?  Logging on the server that we are truncating a message sent back to the client doesn't help the client. This is not a case of protocol violation, and a server sending an error message to the client is not out of the ordinary.
> 

I mean the following:

If we have too long message in nbd_negotiate_send_rep_verr() it's a bug, and assertion is correct. But in this particular case, It seems better to use error_report() instead of assertion and shrink the message for client, just to avoid similar CVEs in future. So if the situation will come again (we call nbd_negotiate_send_rep_verr() from some new place, forgetting to check length of user-provided part of the message) it will not be a CVE but just user will have not-well-formed message and we'll have errror-report in server log.

-- 
Best regards,
Vladimir

[PATCH 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>
---
 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 2/2] block: Call attention to truncation of long NBD exports

[Vladimir Sementsov-Ogievskiy]

08.06.2020 21:26, 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).
> 
> 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, "...");
> +        }

Is  4096 really enough for json in normal cases?

>           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';
>       }
>   }
> 


-- 
Best regards,
Vladimir

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

[Eric Blake]

On 6/10/20 4:24 AM, Vladimir Sementsov-Ogievskiy wrote:
> 08.06.2020 21:26, 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).
>>
>> 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, "...");
>> +        }
> 
> Is  4096 really enough for json in normal cases?

By its very nature, a json string tends be longer than a counterpart URI 
string representing the same information (when such an explicit name 
exists) because of the extra characters burned in adding "key":value 
pairs wrapping the data that was compact in explicit form.  But 4k is 
still quite a lot, and the only cases I've seen where names don't fit in 
JSON form is where the user was explicitly trying to break things with 
corner-case testing, rather than what you get with day-to-day use.

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