From 93676c88d7a5cd5971de94f9091eff8e9773b1af Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 13 Nov 2019 20:46:34 -0600 Subject: nbd: Don't send oversize strings 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 Message-Id: <20191114024635.11363-4-eblake@redhat.com> Reviewed-by: Maxim Levitsky Reviewed-by: Vladimir Sementsov-Ogievskiy --- nbd/client.c | 18 +++++++++++++++--- nbd/server.c | 20 +++++++++++++++----- 2 files changed, 30 insertions(+), 8 deletions(-) (limited to 'nbd') diff --git a/nbd/client.c b/nbd/client.c index f673396..ba17310 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 6bbeb98f..24ebc1a 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; -- cgit v1.1