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 <ebl...@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
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
