Re: [PATCH v3 3/4] nbd: Don't send oversize strings

[Vladimir Sementsov-Ogievskiy]

14.11.2019 5:46, Eric Blake wrote:
> Qemu as server currently won't accept export names larger than 256
> bytes, nor create dirty bitmap names longer than 1023 bytes, so most
> uses of qemu as client or server have no reason to get anywhere near
> the NBD spec maximum of a 4k limit per string.
> 
> However, we weren't actually enforcing things, ignoring when the
> remote side violates the protocol on input, and also having several
> code paths where we send oversize strings on output (for example,
> qemu-nbd --description could easily send more than 4k).  Tighten
> things up as follows:
> 
> client:
> - Perform bounds check on export name and dirty bitmap request prior
>    to handing it to server
> - Validate that copied server replies are not too long (ignoring
>    NBD_INFO_* replies that are not copied is not too bad)
> server:
> - Perform bounds check on export name and description prior to
>    advertising it to client
> - Reject client name or metadata query that is too long
> - Adjust things to allow full 4k name limit rather than previous
>    256 byte limit
> 
> Signed-off-by: Eric Blake <address@hidden>

Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

Still, same doubt about putting over-sized strings into error messages. If you
decide to drop them, keep my r-b.

======

Not very simple to review, as I need to check that all assertions will not fire.
Hope you don't mind me doing it here)

1.
nbd_send_meta_query checks export and query

    nbd_negotiate_simple_meta_context ^ info->name, 
info->x_dirty_bitmap/"base:allocation"

       nbd_receive_negotiate ^ info=info
           (info->name proved by assert)

          nbd_client_connect ^ info=s->info                                     
                   OK
              (s->info.x_dirty_bitmap = g_strdup(s->x_dirty_bitmap);
               s->info.name = g_strdup(s->export ?: "");
               s->export is set only in nbd_process_options() and checked in it
               s->x_dirty_bitmap too)

          nbd_client_thread & info= local info, with name = "" and 
x_dirty_bitmap = NULL           OK


    nbd_list_meta_contexts ^ info->name, NULL/"qemu:"

       nbd_receive_export_list ^ info=&array[i]
           (array[count - 1].name = name;, name comes from nbd_receive_list, 
where it is checked)  OK

2.
nbd_receive_negotiate check info->name
     (see 1.)

3.
nbd_negotiate_send_rep_list check exp->name and exp->description

    nbd_negotiate_handle_list                                                   
                   OK
        (does QTAILQ_FOREACH(exp, &exports, next)
         new exports comes from nbd_export_new() which checks name and desc (by 
assertions))

4.
nbd_negotiate_handle_info checks exp->description                               
                  OK
     (exp comes from nbd_export_find(name), from global exports, so it's OK)


5.
nbd_negotiate_send_meta_context checks  context

     nbd_negotiate_meta_queries ^ context = 
"base:allocation"/meta->exp->export_bitmap_context     OK
     (meta is filled inside the function, meta->exp = 
nbd_export_find(export_name),

     and export_bitmap_context is set and asserted in  nbd_export_new

6.
nbd_export_new checks name and desc and bitmap (export_bitmap_context is 
obvious)
     bitmap is found before assertion (which proves that name is shorter than 
1024)

     qmp_nbd_server_add ^ name NULL bitmap                                      
                   OK
         name is checked

     main in qemu-nbd.c ^ export_name export_description bitmap                 
                   OK
         both export_name and export_description are checked.

> ---
>   include/block/nbd.h |  8 ++++----
>   block/nbd.c         | 10 ++++++++++
>   blockdev-nbd.c      |  5 +++++
>   nbd/client.c        | 18 +++++++++++++++---
>   nbd/server.c        | 20 +++++++++++++++-----
>   qemu-nbd.c          |  9 +++++++++
>   6 files changed, 58 insertions(+), 12 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index c306423dc85c..7f46932d80f1 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -227,11 +227,11 @@ enum {
>   #define NBD_MAX_BUFFER_SIZE (32 * 1024 * 1024)
> 
>   /*
> - * Maximum size of an export name. The NBD spec requires a minimum of
> - * 256 and recommends that servers support up to 4096; all users use
> - * malloc so we can bump this constant without worry.
> + * Maximum size of a protocol string (export name, meta context name,
> + * etc.).  Use malloc rather than stack allocation for storage of a
> + * string.
>    */
> -#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/block/nbd.c b/block/nbd.c
> index 123976171cf4..5f18f78a9471 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -1832,6 +1832,10 @@ static int nbd_process_options(BlockDriverState *bs, 
> QDict *options,
>       }
> 
>       s->export = g_strdup(qemu_opt_get(opts, "export"));
> +    if (s->export && strlen(s->export) > NBD_MAX_STRING_SIZE) {
> +        error_setg(errp, "export name too long to send to server");
> +        goto error;
> +    }
> 
>       s->tlscredsid = g_strdup(qemu_opt_get(opts, "tls-creds"));
>       if (s->tlscredsid) {
> @@ -1849,6 +1853,11 @@ static int nbd_process_options(BlockDriverState *bs, 
> QDict *options,
>       }
> 
>       s->x_dirty_bitmap = g_strdup(qemu_opt_get(opts, "x-dirty-bitmap"));
> +    if (s->x_dirty_bitmap && strlen(s->x_dirty_bitmap) > 
> NBD_MAX_STRING_SIZE) {
> +        error_setg(errp, "x-dirty-bitmap query too long to send to server");
> +        goto error;
> +    }
> +
>       s->reconnect_delay = qemu_opt_get_number(opts, "reconnect-delay", 0);
> 
>       ret = 0;
> @@ -1859,6 +1868,7 @@ static int nbd_process_options(BlockDriverState *bs, 
> QDict *options,
>           qapi_free_SocketAddress(s->saddr);
>           g_free(s->export);
>           g_free(s->tlscredsid);
> +        g_free(s->x_dirty_bitmap);
>       }
>       qemu_opts_del(opts);
>       return ret;
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index 6a8b206e1d74..8c20baa4a4b9 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -162,6 +162,11 @@ void qmp_nbd_server_add(const char *device, bool 
> has_name, const char *name,
>           name = device;
>       }
> 
> +    if (strlen(name) > NBD_MAX_STRING_SIZE) {
> +        error_setg(errp, "export name '%s' too long", name);
> +        return;
> +    }
> +
>       if (nbd_export_find(name)) {
>           error_setg(errp, "NBD server already has export named '%s'", name);
>           return;
> diff --git a/nbd/client.c b/nbd/client.c
> index f6733962b49b..ba173108baab 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -289,8 +289,8 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, 
> char **description,
>           return -1;
>       }
>       len -= sizeof(namelen);
> -    if (len < namelen) {
> -        error_setg(errp, "incorrect option name length");
> +    if (len < namelen || namelen > NBD_MAX_STRING_SIZE) {
> +        error_setg(errp, "incorrect name length in server's list response");
>           nbd_send_opt_abort(ioc);
>           return -1;
>       }
> @@ -303,6 +303,12 @@ static int nbd_receive_list(QIOChannel *ioc, char 
> **name, char **description,
>       local_name[namelen] = '\0';
>       len -= namelen;
>       if (len) {
> +        if (len > NBD_MAX_STRING_SIZE) {
> +            error_setg(errp, "incorrect description length in server's "
> +                       "list response");
> +            nbd_send_opt_abort(ioc);
> +            return -1;
> +        }
>           local_desc = g_malloc(len + 1);
>           if (nbd_read(ioc, local_desc, len, "export description", errp) < 0) 
> {
>               nbd_send_opt_abort(ioc);
> @@ -479,6 +485,10 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t 
> opt,
>               break;
> 
>           default:
> +            /*
> +             * Not worth the bother to check if NBD_INFO_NAME or
> +             * NBD_INFO_DESCRIPTION exceed NBD_MAX_STRING_SIZE.
> +             */
>               trace_nbd_opt_info_unknown(type, nbd_info_lookup(type));
>               if (nbd_drop(ioc, len, errp) < 0) {
>                   error_prepend(errp, "Failed to read info payload: ");
> @@ -645,9 +655,11 @@ static int nbd_send_meta_query(QIOChannel *ioc, uint32_t 
> opt,
>       char *p;
> 
>       data_len = sizeof(export_len) + export_len + sizeof(queries);
> +    assert(export_len <= NBD_MAX_STRING_SIZE);
>       if (query) {
>           query_len = strlen(query);
>           data_len += sizeof(query_len) + query_len;
> +        assert(query_len <= NBD_MAX_STRING_SIZE);
>       } else {
>           assert(opt == NBD_OPT_LIST_META_CONTEXT);
>       }
> @@ -1009,7 +1021,7 @@ int nbd_receive_negotiate(AioContext *aio_context, 
> QIOChannel *ioc,
>       bool zeroes;
>       bool base_allocation = info->base_allocation;
> 
> -    assert(info->name);
> +    assert(info->name && strlen(info->name) <= NBD_MAX_STRING_SIZE);
>       trace_nbd_receive_negotiate_name(info->name);
> 
>       result = nbd_start_negotiate(aio_context, ioc, tlscreds, hostname, 
> outioc,
> diff --git a/nbd/server.c b/nbd/server.c
> index c63b76b22735..d28123c562be 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -321,7 +321,7 @@ static int nbd_opt_skip(NBDClient *client, size_t size, 
> Error **errp)
>   /* nbd_opt_read_name
>    *
>    * Read a string with the format:
> - *   uint32_t len     (<= NBD_MAX_NAME_SIZE)
> + *   uint32_t len     (<= NBD_MAX_STRING_SIZE)
>    *   len bytes string (not 0-terminated)
>    *
>    * On success, @name will be allocated.
> @@ -344,7 +344,7 @@ static int nbd_opt_read_name(NBDClient *client, char 
> **name, uint32_t *length,
>       }
>       len = cpu_to_be32(len);
> 
> -    if (len > NBD_MAX_NAME_SIZE) {
> +    if (len > NBD_MAX_STRING_SIZE) {
>           return nbd_opt_invalid(client, errp,
>                                  "Invalid name length: %" PRIu32, len);
>       }
> @@ -379,6 +379,7 @@ static int nbd_negotiate_send_rep_list(NBDClient *client, 
> NBDExport *exp,
>       trace_nbd_negotiate_send_rep_list(name, desc);
>       name_len = strlen(name);
>       desc_len = strlen(desc);
> +    assert(name_len <= NBD_MAX_STRING_SIZE && desc_len <= 
> NBD_MAX_STRING_SIZE);
>       len = name_len + desc_len + sizeof(len);
>       ret = nbd_negotiate_send_rep_len(client, NBD_REP_SERVER, len, errp);
>       if (ret < 0) {
> @@ -445,7 +446,7 @@ static int nbd_negotiate_handle_export_name(NBDClient 
> *client, bool no_zeroes,
>           [10 .. 133]   reserved     (0) [unless no_zeroes]
>        */
>       trace_nbd_negotiate_handle_export_name();
> -    if (client->optlen > NBD_MAX_NAME_SIZE) {
> +    if (client->optlen > NBD_MAX_STRING_SIZE) {
>           error_setg(errp, "Bad length received");
>           return -EINVAL;
>       }
> @@ -613,6 +614,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, 
> Error **errp)
>       if (exp->description) {
>           size_t len = strlen(exp->description);
> 
> +        assert(len <= NBD_MAX_STRING_SIZE);
>           rc = nbd_negotiate_send_info(client, NBD_INFO_DESCRIPTION,
>                                        len, exp->description, errp);
>           if (rc < 0) {
> @@ -757,6 +759,7 @@ static int nbd_negotiate_send_meta_context(NBDClient 
> *client,
>           {.iov_base = (void *)context, .iov_len = strlen(context)}
>       };
> 
> +    assert(iov[1].iov_len <= NBD_MAX_STRING_SIZE);
>       if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
>           context_id = 0;
>       }
> @@ -905,7 +908,7 @@ static int nbd_meta_qemu_query(NBDClient *client, 
> NBDExportMetaContexts *meta,
>    * Parse namespace name and call corresponding function to parse body of the
>    * query.
>    *
> - * The only supported namespace now is 'base'.
> + * The only supported namespaces are 'base' and 'qemu'.
>    *
>    * The function aims not wasting time and memory to read long unknown 
> namespace
>    * names.
> @@ -931,6 +934,10 @@ static int nbd_negotiate_meta_query(NBDClient *client,
>       }
>       len = cpu_to_be32(len);
> 
> +    if (len > NBD_MAX_STRING_SIZE) {
> +        trace_nbd_negotiate_meta_query_skip("length too long");
> +        return nbd_opt_skip(client, len, errp);
> +    }
>       if (len < ns_len) {
>           trace_nbd_negotiate_meta_query_skip("length too short");
>           return nbd_opt_skip(client, len, errp);
> @@ -1492,7 +1499,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, 
> uint64_t dev_offset,
>        * access since the export could be available before migration handover.
>        * ctx was acquired in the caller.
>        */
> -    assert(name);
> +    assert(name && strlen(name) <= NBD_MAX_STRING_SIZE);
>       ctx = bdrv_get_aio_context(bs);
>       bdrv_invalidate_cache(bs, NULL);
> 
> @@ -1518,6 +1525,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, 
> uint64_t dev_offset,
>       assert(dev_offset <= INT64_MAX);
>       exp->dev_offset = dev_offset;
>       exp->name = g_strdup(name);
> +    assert(!desc || strlen(desc) <= NBD_MAX_STRING_SIZE);
>       exp->description = g_strdup(desc);
>       exp->nbdflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_FLUSH |
>                        NBD_FLAG_SEND_FUA | NBD_FLAG_SEND_CACHE);
> @@ -1564,8 +1572,10 @@ NBDExport *nbd_export_new(BlockDriverState *bs, 
> uint64_t dev_offset,
> 
>           bdrv_dirty_bitmap_set_busy(bm, true);
>           exp->export_bitmap = bm;
> +        assert(strlen(bitmap) <= BDRV_BITMAP_MAX_NAME_SIZE);
>           exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s",
>                                                        bitmap);
> +        assert(strlen(exp->export_bitmap_context) < NBD_MAX_STRING_SIZE);
>       }
> 
>       exp->close = close;
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index caacf0ed7379..108a51f7eb01 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -833,9 +833,18 @@ int main(int argc, char **argv)
>               break;
>           case 'x':
>               export_name = optarg;
> +            if (strlen(export_name) > NBD_MAX_STRING_SIZE) {
> +                error_report("export name '%s' too long", export_name);
> +                exit(EXIT_FAILURE);
> +            }
>               break;
>           case 'D':
>               export_description = optarg;
> +            if (strlen(export_description) > NBD_MAX_STRING_SIZE) {
> +                error_report("export description '%s' too long",
> +                             export_description);
> +                exit(EXIT_FAILURE);
> +            }
>               break;
>           case 'v':
>               verbose = 1;
> 


-- 
Best regards,
Vladimir