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

[Eric Blake <eblake@redhat.com>]

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