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

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

[Eric Blake]

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

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 316fd705a9e4..fcabdf0f37c3 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -232,6 +232,7 @@ enum {
  * going larger would require an audit of more code to make sure we
  * aren't overflowing some other buffer. */
 #define NBD_MAX_NAME_SIZE 256
+#define NBD_MAX_STRING_SIZE 4096

 /* Two types of reply structures */
 #define NBD_SIMPLE_REPLY_MAGIC      0x67446698
diff --git a/nbd/client.c b/nbd/client.c
index f6733962b49b..3f21722dd914 100644
--- a/nbd/client.c
+++ 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");
+            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");
+        return -EINVAL;
+    }
     trace_nbd_receive_negotiate_name(info->name);

     result = nbd_start_negotiate(aio_context, ioc, tlscreds, hostname, outioc,
-- 
2.21.0

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

[Maxim Levitsky]

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(+)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 316fd705a9e4..fcabdf0f37c3 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -232,6 +232,7 @@ enum {
>   * going larger would require an audit of more code to make sure we
>   * aren't overflowing some other buffer. */
>  #define NBD_MAX_NAME_SIZE 256
> +#define NBD_MAX_STRING_SIZE 4096
> 
>  /* Two types of reply structures */
>  #define NBD_SIMPLE_REPLY_MAGIC      0x67446698
> diff --git a/nbd/client.c b/nbd/client.c
> index f6733962b49b..3f21722dd914 100644
> --- a/nbd/client.c
> +++ 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.


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'?


> +        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?


Best regards,
	Maxim Levitsky

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

[Eric Blake]

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