Re: [PATCH v2 1/2] nbd: Don't send oversize strings

[Vladimir Sementsov-Ogievskiy]

11.10.2019 0:00, 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
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
>   include/block/nbd.h |  1 +
>   block/nbd.c         |  9 +++++++++
>   blockdev-nbd.c      |  5 +++++
>   nbd/client.c        | 16 +++++++++++++---
>   nbd/server.c        | 14 ++++++++++++--
>   qemu-nbd.c          |  9 +++++++++
>   6 files changed, 49 insertions(+), 5 deletions(-)
> 
> 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. */

This comment says, that we restrict export name to 256...

>   #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 813c40d8f067..76eb1dbe04df 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -1621,6 +1621,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) {
> @@ -1638,6 +1642,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;

this is new latest "goto error", you should add g_free(s->x_dirty_bitmap) in 
following "if (ret < 0)"

> +    }
> +
>       s->reconnect_delay = qemu_opt_get_number(opts, "reconnect-delay", 0);
> 
>       ret = 0;
> 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;
> +    }

Hmmm, no, so here we restrict to 4096, but, we will not allow client to request 
more than
256. Seems, to correctly update server-part, we should drop NBD_MAX_NAME_SIZE 
and do the
audit mentioned in the comment above its definition.

> +
>       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..d6e29daced63 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 list name length");

New wording made me go above and read the comment, what functions does. Comment 
is good, but without
it, it sounds like name of the list for me...

>           nbd_send_opt_abort(ioc);
>           return -1;
>       }
> @@ -303,6 +303,11 @@ 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 list description length");
> +            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 +484,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: ");
> @@ -648,6 +657,7 @@ static int nbd_send_meta_query(QIOChannel *ioc, uint32_t 
> opt,
>       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);
>       }

you may assert export_len as well..

> @@ -1009,7 +1019,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 d8d1e6245532..dfbefd5a1ebc 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -375,6 +375,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) {
> @@ -608,6 +609,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) {
> @@ -752,6 +754,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;
>       }
> @@ -900,7 +903,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.
> @@ -926,6 +929,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);
> @@ -1487,7 +1494,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);
> 
> @@ -1513,6 +1520,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);
> @@ -1561,6 +1569,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, 
> uint64_t dev_offset,
>           exp->export_bitmap = bm;
>           exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s",
>                                                        bitmap);
> +        /* See BME_MAX_NAME_SIZE in block/qcow2-bitmap.c */

Hmm. BME_MAX_NAME_SIZE is checked only when creating persistent bitmaps. But 
for non-persistent
name length is actually unlimited. So, we should either limit all bitmap names 
to 1023 (hope,
this will not break existing scenarios) or error out here (or earlier) instead 
of assertion.

We also may want QEMU_BUILD_BUG_ON(NBD_MAX_STRING_SIZE < BME_MAX_NAME_SIZE + 
sizeof("qemu:dirty-bitmap:") - 1)

> +        assert(strlen(exp->export_bitmap_context) <= NBD_MAX_STRING_SIZE);
>       }
> 
>       exp->close = close;
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 9032b6de2ace..55ce69b141f0 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -826,9 +826,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