Re: [PATCH] nbd: Don't let client send oversize strings

[Eric Blake <eblake@redhat.com>]

On 9/29/19 1:49 PM, Maxim Levitsky wrote:
> On Fri, 2019-09-27 at 23:13 -0500, Eric Blake wrote:
>> Qemu as server currently won't accept export names larger than 256
>> bytes, so most uses of qemu as client have no reason to get anywhere
>> near the NBD spec maximum of a 4k limit per string.  However, we
>> didn't actually have any code that prevented the client from violating
>> the protocol, which, while useful for testing corner-case server
>> reactions, is probably not ideal.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>   include/block/nbd.h | 1 +
>>   nbd/client.c        | 8 ++++++++
>>   2 files changed, 9 insertions(+)
>>

>> +++ b/nbd/client.c
>> @@ -648,6 +648,10 @@ static int nbd_send_meta_query(QIOChannel *ioc, uint32_t opt,
>>       if (query) {
>>           query_len = strlen(query);
>>           data_len += sizeof(query_len) + query_len;
>> +        if (query_len > NBD_MAX_STRING_SIZE) {
>> +            error_setg(errp, "x_dirty_bitmap query too long to send to server");
> Is there a way not to do this here? I don't know nbd well to be honest,
> and it looks like this code currently is only called for x_dirty_bitmap but
> there could be more cases in the future.

I could make this an assert, and fix the callers to pass in valid 
lengths (callers pass in either "base:allocation" which fits, or a 
user-supplied x_dirty_bitmap, so validating at the point that hack is 
assigned is reasoanble).

> 
> 
> nbd_negotiate_simple_meta_context which seems to be the caller of this, already mentions
> a 'hack' about this :-(
> 
> Of course if you think that this is not worth the time, you can leave this as is.
> 
> 
>> +            return -1;
>> +        }
>>       } else {
>>           assert(opt == NBD_OPT_LIST_META_CONTEXT);
>>       }
>> @@ -1010,6 +1014,10 @@ int nbd_receive_negotiate(AioContext *aio_context, QIOChannel *ioc,
>>       bool base_allocation = info->base_allocation;
>>
>>       assert(info->name);
>> +    if (strlen(info->name) > NBD_MAX_STRING_SIZE) {
>> +        error_setg(errp, "name too long to send to server");
> Maybe 'export name'?

Sure.

> 
> 
>> +        return -EINVAL;
>> +    }
>>       trace_nbd_receive_negotiate_name(info->name);
>>
>>       result = nbd_start_negotiate(aio_context, ioc, tlscreds, hostname, outioc,
> 
> Why not to do the export name check when info->name is set, that is in nbd_client_connect?

I'll spin up a v2.

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