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

[Maxim Levitsky]

On Wed, 2019-11-13 at 20:46 -0600, 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>
> ---
>  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;

Maybe I would split that patch into server side checks,
then client side checks, and then patch that adds bunch
of asserts, which are true after client side checks are added.


Reviewed-by: Maxim Levitsky <address@hidden>

Best regards,
        Maxim Levitsky